USD: Improved Texture Coordinate Translation (UsdTransform2d) #114821

Charles Wardlaw · 2023-11-14T00:18:43+01:00

Charles Wardlaw commented

2023-11-14 00:18:43 +01:00

UsdTransform2d was previously ignored, and the material read would stop when it found these nodes. This resulted in many surfaces having their textures mapped incorrectly when imported into Blender, and worse, resulted in the loss of that data when round-tripping back out of Blender.

This patch (mostly the work of @makowalski) adds support for the Transform 2D node and also ensures that the wrap modes are similarly written correctly.

Attached are files demonstrating the differences. On import before the patch:

Shader graph after the patch:

Exports are similarly handled.

UsdTransform2d was previously ignored, and the material read would stop when it found these nodes. This resulted in many surfaces having their textures mapped incorrectly when imported into Blender, and worse, resulted in the loss of that data when round-tripping back out of Blender. This patch (mostly the work of @makowalski) adds support for the Transform 2D node and also ensures that the wrap modes are similarly written correctly. Attached are files demonstrating the differences. On import before the patch: ![image](/attachments/47197a13-aab4-4ffe-8f75-151bc59e8722) Shader graph after the patch: ![image](/attachments/b8371b55-97da-4ce3-8460-7945ab3e3b50) Exports are similarly handled.

mapping_nodes.usdz

2.3 MiB

mapping_nodes.blend

3.3 MiB

image.png

637 KiB

image.png

83 KiB

Charles Wardlaw added 3 commits 2023-11-14 00:18:54 +01:00

Part 1/x: getting the mapping nodes to both be read and written. Still dealing with an issue on the write side, with nodes coming in incorrectly. d7144e8e43

Part 3/x: Finished out the Usdtransform2D settings (node was missing the id type) and wrap values. d50a62f4ec

Merge branch 'main' into feature/port_mapping_to_main a134ef33cc

Charles Wardlaw requested review from Michael Kowalski 2023-11-14 00:19:04 +01:00

Charles Wardlaw requested review from Hans Goudey 2023-11-14 00:19:13 +01:00

Jesse Yurkovich requested changes 2023-11-14 06:14:08 +01:00

source/blender/io/usd/intern/usd_reader_material.cc

						
				@ -64,0 +69,4 @@

				static const pxr::TfToken clamp("clamp", pxr::TfToken::Immortal);

				static const pxr::TfToken repeat("repeat", pxr::TfToken::Immortal);

				static const pxr::TfToken wrapS("wrapS", pxr::TfToken::Immortal);

				static const pxr::TfToken wrapT("wrapT", pxr::TfToken::Immortal);

Jesse Yurkovich commented

2023-11-14 04:21:47 +01:00

These wrap mode values don't seem to be used in the reader.

Charles Wardlaw commented

2023-11-17 16:32:18 +01:00

Good catch-- on the port I missed a function. Thanks!

CharlesWardlaw marked this conversation as resolved

source/blender/io/usd/intern/usd_reader_material.cc Outdated

						
				@ -329,0 +367,4 @@

				{

				  bNodeSocket *sock = nodeFindSocket(node, SOCK_IN, identifier);

				  if (!sock) {

				    WM_reportf(RPT_ERROR,

Jesse Yurkovich commented

2023-11-14 04:25:13 +01:00

We shouldn't use WM_reportf any longer. Such reports were changed as part of b684864561 last month. All calls to WM_reportf in this PR will have to change.

We shouldn't use `WM_reportf` any longer. Such reports were changed as part of b6848645614 last month. All calls to WM_reportf in this PR will have to change.

Bastien Montagne commented

2023-11-14 10:33:58 +01:00

In addition, these kind of 'refined' detailed messages are better suited for CLOG output than BKE_report.

Reports should be considered as high-level information that is important to be printed to the user in the UI. More technical details should rather use the logging system, to avoid spamming users with hundreds of messages that they will not read (or even actually see).

This is also something that needs to be addresses in the whole USD IO area of Blender, but would be good to get it for new code at least. :)

In addition, these kind of 'refined' detailed messages are better suited for `CLOG` output than `BKE_report`. Reports should be considered as high-level information that is important to be printed to the user in the UI. More technical details should rather use the logging system, to avoid spamming users with hundreds of messages that they will not read (or even actually see). This is also something that needs to be addresses in the whole USD IO area of Blender, but would be good to get it for new code at least. :)

Charles Wardlaw commented

2023-11-16 00:16:51 +01:00

I'm happy to make any changes you like here, but the new BKE_report thing introduces many intrusive changes that are in my opinion unnecessary.

We're now passing a ReportList pointer through a LOT of functions-- is this the best solution? Was a central logging store considered, where BKE_reportf's first parameter would be an std::string log name or some sort of integer handle that looks up the ReportList from a central place instead? This seems to be how most logging systems work, and wouldn't preclude doing a pre-write lock on a per-log basis to get around threading issues.
If that's not a possibility, why are we passing a pointer as opposed to passing a ReportList structure or class by reference?

@mont29 I'd appreciate a more specific idea on CLOG vs BKE_report-- When we've spammed users in the past, we've corrected the issues to reduce spam by fixing or adding features. I'd rather have people annoyed by printouts than have them unaware of the reason why a file imported incorrectly, but I will defer to your guidance.

I'm happy to make any changes you like here, but the new BKE_report thing introduces many intrusive changes that are in my opinion unnecessary. - We're now passing a ReportList pointer through a LOT of functions-- is this the best solution? Was a central logging store considered, where BKE_reportf's first parameter would be an std::string log name or some sort of integer handle that looks up the ReportList from a central place instead? This seems to be how most logging systems work, and wouldn't preclude doing a pre-write lock on a per-log basis to get around threading issues. - If that's not a possibility, why are we passing a pointer as opposed to passing a ReportList structure or class by reference? @mont29 I'd appreciate a more specific idea on CLOG vs BKE_report-- When we've spammed users in the past, we've corrected the issues to reduce spam by fixing or adding features. I'd rather have people annoyed by printouts than have them unaware of the reason why a file imported incorrectly, but I will defer to your guidance.

CharlesWardlaw marked this conversation as resolved

source/blender/io/usd/intern/usd_reader_material.cc

						
				@ -703,0 +792,4 @@

				      if (pxr::UsdShadeInput scale_input = get_input(usd_shader, usdtokens::scale)) {

				        pxr::VtValue val;

				        if (scale_input.Get(&val) && val.CanCast<pxr::GfVec2f>()) {

				          pxr::GfVec2f scale_val = val.Cast<pxr::GfVec2f>().UncheckedGet<pxr::GfVec2f>();

Jesse Yurkovich commented

2023-11-14 04:32:06 +01:00

As someone new to the USD API, I've noticed there's like 5 different coding conventions when it comes to getting values:

if (Attr.Get(val) && val.IsHolding<T>()) { ... val.Get<T>() }
if (Attr.Get(val) && val.IsHolding<T>()) { ... val.UncheckedGet<T>() }
if (Attr.Get(val) && val.CanCast<T>()) { ... val.Cast<T>().Get<T>() }
if (Attr.Get(val) && val.CanCast<T>()) { ... val.Cast<T>().UncheckedGet<T>() }
if (Attr.Get(val)) { ... val.Get<T>() }

Is it possible to choose just 1 style? In what situations are the Casts necessary? Can UncheckedGet always be used since we verify in some form first?

As someone new to the USD API, I've noticed there's like 5 different coding conventions when it comes to getting values: ``` if (Attr.Get(val) && val.IsHolding<T>()) { ... val.Get<T>() } if (Attr.Get(val) && val.IsHolding<T>()) { ... val.UncheckedGet<T>() } if (Attr.Get(val) && val.CanCast<T>()) { ... val.Cast<T>().Get<T>() } if (Attr.Get(val) && val.CanCast<T>()) { ... val.Cast<T>().UncheckedGet<T>() } if (Attr.Get(val)) { ... val.Get<T>() } ``` Is it possible to choose just 1 style? In what situations are the Casts necessary? Can `UncheckedGet` always be used since we verify in some form first?

Charles Wardlaw commented

2023-11-16 00:31:00 +01:00

God, casts are necessary all over the place. >_<;

One example is something like GfVec3f -- that could also be stored as GfVec3d, GfVec3i, or GfVec3h. Going from float to doubles makes sense, but the other two are less useful / used and don't convert automatically. So, if you want a GfVec3f out but the value is stored as one of the others, you need to cast.

I don't know about one style over the others; I feel like sometimes it works one way, and sometimes it works another way. I'll defer to @makowalski on this.

God, casts are necessary all over the place. >_<; One example is something like GfVec3f -- that could also be stored as GfVec3d, GfVec3i, or GfVec3h. Going from float to doubles makes sense, but the other two are less useful / used and don't convert automatically. So, if you want a GfVec3f out but the value is stored as one of the others, you need to cast. I don't know about one style over the others; I feel like sometimes it works one way, and sometimes it works another way. I'll defer to @makowalski on this.

Michael Kowalski commented

2023-11-21 23:56:19 +01:00

God, casts are necessary all over the place. >_<;

One example is something like GfVec3f -- that could also be stored as GfVec3d, GfVec3i, or GfVec3h. Going from float to doubles makes sense, but the other two are less useful / used and don't convert automatically. So, if you want a GfVec3f out but the value is stored as one of the others, you need to cast.

I don't know about one style over the others; I feel like sometimes it works one way, and sometimes it works another way. I'll defer to @makowalski on this.

I agree casts are often necessary, because in practice one finds that attributes are not always consistently authored, or for backward compatibility when the USD specification has changed. That said, we can perhaps open a separate task to review the existing USD import code and decide on a more consistent convention throughout.

> God, casts are necessary all over the place. >_<; > > One example is something like GfVec3f -- that could also be stored as GfVec3d, GfVec3i, or GfVec3h. Going from float to doubles makes sense, but the other two are less useful / used and don't convert automatically. So, if you want a GfVec3f out but the value is stored as one of the others, you need to cast. > > I don't know about one style over the others; I feel like sometimes it works one way, and sometimes it works another way. I'll defer to @makowalski on this. I agree casts are often necessary, because in practice one finds that attributes are not always consistently authored, or for backward compatibility when the USD specification has changed. That said, we can perhaps open a separate task to review the existing USD import code and decide on a more consistent convention throughout.

CharlesWardlaw marked this conversation as resolved

source/blender/io/usd/intern/usd_writer_material.cc

						
				@ -77,0 +80,4 @@

				static const pxr::TfToken repeat("repeat", pxr::TfToken::Immortal);

				static const pxr::TfToken wrapS("wrapS", pxr::TfToken::Immortal);

				static const pxr::TfToken wrapT("wrapT", pxr::TfToken::Immortal);

				static const pxr::TfToken emissiveColor("emissiveColor", pxr::TfToken::Immortal);

Jesse Yurkovich commented

2023-11-14 04:40:35 +01:00

There is another "emissiveColor" token name defined above already.

CharlesWardlaw marked this conversation as resolved

source/blender/io/usd/intern/usd_writer_material.cc Outdated

						
				@ -82,0 +98,4 @@

				static const pxr::TfToken attribute("attribute", pxr::TfToken::Immortal);

				static const pxr::TfToken bsdf("bsdf", pxr::TfToken::Immortal);

				static const pxr::TfToken closure("closure", pxr::TfToken::Immortal);

				static const pxr::TfToken vector("vector", pxr::TfToken::Immortal);

Jesse Yurkovich commented

2023-11-14 04:43:18 +01:00

These newly added usdtokens don't seem to be used either? Everything from cycles down to vector.

Charles Wardlaw commented

2023-11-17 22:36:52 +01:00

UVMap is used, but I removed the rest.

CharlesWardlaw marked this conversation as resolved

source/blender/io/usd/intern/usd_writer_material.cc Outdated

						
				@ -166,2 +206,2 @@

				      if (!input_link &&

				          ((bNodeSocketValueFloat *)emission_strength_sock->default_value)->value == 0.0f) {

				      if (!emission_strength_sock) {

Jesse Yurkovich commented

2023-11-14 06:13:43 +01:00

This seems like another case where we'd, maybe, want to know we didn't find the socket? In general not being able to find a socket for one of our own nodes is totally an error on our part, nothing the user can do about it, but we don't have a "Retail assert" right now so tracing is the only option... I'm torn between a) silently continuing, b) tracing and continuing, or c) just use a Debug assert for folks running debug and crash with null-ref in retail.

Charles Wardlaw commented

2023-11-16 00:22:30 +01:00

This kind of missing socket stuff occurs either with broken .blend files or, sometimes, when loading stuff between versions and upgrades or downgrades don't work 100%. I don't think normal users who stick to official releases will hit something like this.

The version of Blender we publish through our launcher, however, is constantly running on top of main. We at times ingest bugs that are the result of refactors or other upstream changes, but are not always able to ingest the subsequent fixes as quickly.

I am 200% against allowing the application to get into a crash state knowingly, especially when a simple pointer check can avoid this. I feel it's better for the user to have bad behavior and recover from it than to suddenly be unable to open a file and lose all the data contained within.

I'm happy to add a report print here if you like.

This kind of missing socket stuff occurs either with broken .blend files or, sometimes, when loading stuff between versions and upgrades or downgrades don't work 100%. I don't think normal users who stick to official releases will hit something like this. The version of Blender we publish through our launcher, however, is constantly running on top of main. We at times ingest bugs that are the result of refactors or other upstream changes, but are not always able to ingest the subsequent fixes as quickly. I am 200% against allowing the application to get into a crash state knowingly, especially when a simple pointer check can avoid this. I feel it's better for the user to have bad behavior and recover from it than to suddenly be unable to open a file and lose all the data contained within. I'm happy to add a report print here if you like.

CharlesWardlaw marked this conversation as resolved

source/blender/io/usd/intern/usd_writer_material.cc

						
				@ -382,0 +465,4 @@

				          usdtokens::scale, pxr::SdfValueTypeNames->Float2))

				  {

				    pxr::GfVec2f scale_val(scale[0], scale[1]);

				    if (!scale_input.Set(scale_val)) {

Jesse Yurkovich commented

2023-11-14 05:41:35 +01:00

Is checking for success during .Set really necessary for these and the ones below? Other .Set calls don't seem to check and these aren't completely untrusted inputs that would fail I think.

Is checking for success during `.Set` really necessary for these and the ones below? Other .Set calls don't seem to check and these aren't completely untrusted inputs that would fail I think.

Charles Wardlaw commented

2023-11-17 16:37:57 +01:00

Spoke with Michael and we agree-- we can remove the checks here, as the block is only opened on successful param creation.

CharlesWardlaw marked this conversation as resolved

source/blender/io/usd/intern/usd_writer_material.cc

						
				@ -511,0 +642,4 @@

				static pxr::TfToken get_node_tex_image_wrap(bNode *node)

				{

				  if (node->type != SH_NODE_TEX_IMAGE) {

				    std::cout << "get_node_tex_image_wrap() called with unexpected type.\n";

Jesse Yurkovich commented

2023-11-14 05:15:22 +01:00

Leftover debug trace?

CharlesWardlaw marked this conversation as resolved

Bastien Montagne commented

2023-11-16 10:14:12 +01:00

Let's move the 'report' topic outisde of comments.

`WM_report`

This API is intrinsically not threadsafe, therefore using it in any code called from wmJob execution callback (or any other thread) is a mistake per se.

It usually won't give too bad effects, except when code starts spamming tens (if not hundreds) of reports, like USD I/O code can easily do currently with relatively complex files. So use new system, with BKE_report and passing around reportlists.

And yes, I would love to get this reportlist storage better centralized, but there is no 'universal' USD I/O context available everywhere from USD code currently, at least I could not find anything like that. Could be a good more general refactor to implement at some point though.

CLOG vs. Report

Reports are shown to user, typically as pop-ups + temporary text in the header, in the UI. So there should be a very limited amount of these (ideally only one for a whole USD I/O operation e.g.), synthesizing the issues to report.

Then user can look at the console for the detailed info, generated by CLOG (warnings and errors are always printed out to console, regardless of the --log-level value).

Also not (yet) ideal, I would consider the Blendfile reading code a fairly good example:

Using a dedicated struct to gather user-level important reporting info (BlendFileReadReport).
Generating a limited amount of reports at the end based on these data (file_read_reports_finalize()) (which also uses a mix of reports and clog btw).
Using CLOG in readfile.cc code itself for all kind of detailed immediate info, including reports, warnings and just debugging data.

Let's move the 'report' topic outisde of comments. ## `WM_report` This API is intrinsically not threadsafe, therefore using it in any code called from `wmJob` execution callback (or any other thread) is a mistake _per se_. It usually won't give too bad effects, except when code starts spamming tens (if not hundreds) of reports, like USD I/O code can easily do currently with relatively complex files. So use new system, with `BKE_report` and passing around reportlists. And yes, I would love to get this reportlist storage better centralized, but there is no 'universal' USD I/O context available everywhere from USD code currently, at least I could not find anything like that. Could be a good more general refactor to implement at some point though. ## CLOG vs. Report Reports are shown to user, typically as pop-ups + temporary text in the header, in the UI. So there should be a very limited amount of these (ideally only one for a whole USD I/O operation e.g.), synthesizing the issues to report. Then user can look at the console for the detailed info, generated by CLOG (warnings and errors are always printed out to console, regardless of the `--log-level` value). Also not (yet) ideal, I would consider the Blendfile reading code a fairly good example: * Using a dedicated struct to gather user-level important reporting info (`BlendFileReadReport`). * Generating a limited amount of reports at the end based on these data (`file_read_reports_finalize()`) (which also uses a mix of reports and clog btw). * Using `CLOG` in `readfile.cc` code itself for all kind of detailed immediate info, including reports, warnings and just debugging data.

Charles Wardlaw commented

2023-11-16 22:43:05 +01:00

It usually won't give too bad effects, except when code starts spamming tens (if not hundreds) of reports, like USD I/O code can easily do currently with relatively complex files. So use new system, with BKE_report and passing around reportlists.

I get the issues you were trying to solve for sure-- it's the intrusive requirement to pass around the ReportList that I'm bumping on here.

Why not "register" handles to specific lists and use that handle instead of forcing the modification of every function signature down the stack?

> It usually won't give too bad effects, except when code starts spamming tens (if not hundreds) of reports, like USD I/O code can easily do currently with relatively complex files. So use new system, with `BKE_report` and passing around reportlists. I get the issues you were trying to solve for sure-- it's the intrusive requirement to pass around the ReportList that I'm bumping on here. Why not "register" handles to specific lists and use that handle instead of forcing the modification of every function signature down the stack?

Bastien Montagne commented

2023-11-17 10:30:40 +01:00

It usually won't give too bad effects, except when code starts spamming tens (if not hundreds) of reports, like USD I/O code can easily do currently with relatively complex files. So use new system, with BKE_report and passing around reportlists.

I get the issues you were trying to solve for sure-- it's the intrusive requirement to pass around the ReportList that I'm bumping on here.

Why not "register" handles to specific lists and use that handle instead of forcing the modification of every function signature down the stack?

Not sure what you mean here? The reportlist is stored in USDExportParams/USDImportParams (via the wmJobWorkerStatus *worker_status member), so that it's available as much as possible in USD IO code. Again, afaik there is no universal data shared/accessible to all USD IO code currently.

I Chose the Params structs as they seem to be the most widely shared among the codebase currently, arguably this is not the best place to put this job status data though. I would much rather see something like the USDExporterContext made available to the whole exporter code, and then store the wmJobWorkerStatus data in it (and have something similar for the importer as well).

And I kinda fail to see the point of this discussion in this PR honestly, this is the current system and way to go. You're much welcome to suggest a better system in a dedicated design task obviously.

> > It usually won't give too bad effects, except when code starts spamming tens (if not hundreds) of reports, like USD I/O code can easily do currently with relatively complex files. So use new system, with `BKE_report` and passing around reportlists. > > I get the issues you were trying to solve for sure-- it's the intrusive requirement to pass around the ReportList that I'm bumping on here. > > Why not "register" handles to specific lists and use that handle instead of forcing the modification of every function signature down the stack? Not sure what you mean here? The reportlist is stored in `USDExportParams`/`USDImportParams` (_via_ the `wmJobWorkerStatus *worker_status` member), so that it's available as much as possible in USD IO code. Again, afaik there is no universal data shared/accessible to all USD IO code currently. I Chose the `Params` structs as they seem to be the most widely shared among the codebase currently, arguably this is not the best place to put this job status data though. I would much rather see something like the `USDExporterContext` made available to the whole exporter code, and then store the `wmJobWorkerStatus` data in it (and have something similar for the importer as well). And I kinda fail to see the point of this discussion in this PR honestly, this is the current system and way to go. You're much welcome to suggest a better system in a dedicated design task obviously.

Charles Wardlaw added 3 commits 2023-11-17 22:35:25 +01:00

Merge remote-tracking branch 'origin/main' into feature/port_mapping_to_main 7c9ea4f56d

Fix: Removing unnecessary tokens for now, and adding back the get_image_extension method. b7fe9aa421

Addressed notes on replacing WM_reportf with BKE_reportf.

buildbot/vexp-code-patch-coordinator Build done.

Details

76e5bdb528

Jesse Yurkovich approved these changes 2023-11-21 09:19:59 +01:00

Jesse Yurkovich left a comment

This looks good from my side now. Spot checked functionality with the provided repro as well as the Intel Moore's Lane demo asset and confirmed that we're now getting Mapping nodes added as appropriate.