blender/blender
148
473
Fork 695
Code Issues 6.7k Pull Requests 897 Projects 18 Wiki Activity

USD: Add MaterialX shader export #122575

Merged
Brecht Van Lommel merged 7 commits from wave/blender_wave_Apple:dhruv/usd-materialx into blender-v4.2-release 2024-06-05 20:43:57 +02:00
Conversation 25 Commits 7 Files Changed 15 +435 -62
Michael B Johnson commented 2024-05-31 20:18:59 +02:00
Member
Copy Link

This change adds the ability to export MaterialX networks into the resulting
USD layer.

Details:

A new export option has been added to the USD export to enable MaterialX
export. It is off by default currently due to reasons in the caveats
section.

When enabled, it exports the MaterialX shading network alongside the
UsdPreviewSurface network, on the same USD Material. This allows the same
material to be used by renderers that don't support MaterialX, using the
USDPreviewSurface as a fallback. This is similar to setups in other DCC
packages, and matches the format we've used in our Reality Composer Pro
asset library.

It uses the existing MaterialX framework used to generate MaterialX
documents for rendering, to act as the basis for the USD graph. In this
process it also re-uses the existing texture export code as well if provided
and necessary.

Once the MaterialX document is created, use usdMtlx to generate a USD
shading network. Unfortunately, usdMtlx generates a graph that is unlike
what other DCCs that support MaterialX-embedded-in-USD generates. It
generates several extra prim hierarchies, and externalizes all shader
inputs, making them difficult to edit in other MaterialX graph editors.

To workaround this, generate the MaterialX shading network onto a
temporary stage, where we then run various pre-processing steps to prevent
prim collisions and to reflow the paths once they're converted.

The PrimSpecs are then copied over to their new path. The resulting prim
hierarchy matches what many artists we've worked with prefer to work with.

Caveats:

The Export MaterialX check is off by default. When using the Principled
BSDF, the resulting graph is very usable. However, when using some of the
other BSDFs, the shading networks generated by the existing MaterialX
framework in Blender generate some shading graphs that are difficult for
usdview and other DCC's to understand. The graph is still correct, but
because we're trying to prioritize compatibility, the default is off.

In future PRs we can aim to make the graphs for those other BSDFs play
better with other DCCs.

Other Implementation Details:

As part of this commit we've also done the following:

  • Place some of the materialx graphs inside a passthrough nodegraph to
    avoid node conflicts.
  • Better handle some shader output types , and better handle some
    conflict cases.
  • Moved the ExportTextureFunction to materials.h due to some difficult
    to resolve header ordering issues. This has no effect on any runtime code.
  • There is a test for the MaterialX export that does some basic checking to
    make sure we get an export out the other end that matches our expectations

Authored by Apple: Dhruv Govil

This PR is based on an earlier implementation by Brecht van Lommel , as well
as Brian Savery and his teams' work at AMD to implement the general
MaterialX framework within Blender.

This change adds the ability to export MaterialX networks into the resulting USD layer. Details: A new export option has been added to the USD export to enable MaterialX export. It is off by default currently due to reasons in the caveats section. When enabled, it exports the MaterialX shading network alongside the UsdPreviewSurface network, on the same USD Material. This allows the same material to be used by renderers that don't support MaterialX, using the USDPreviewSurface as a fallback. This is similar to setups in other DCC packages, and matches the format we've used in our Reality Composer Pro asset library. It uses the existing MaterialX framework used to generate MaterialX documents for rendering, to act as the basis for the USD graph. In this process it also re-uses the existing texture export code as well if provided and necessary. Once the MaterialX document is created, use usdMtlx to generate a USD shading network. Unfortunately, usdMtlx generates a graph that is unlike what other DCCs that support MaterialX-embedded-in-USD generates. It generates several extra prim hierarchies, and externalizes all shader inputs, making them difficult to edit in other MaterialX graph editors. To workaround this, generate the MaterialX shading network onto a temporary stage, where we then run various pre-processing steps to prevent prim collisions and to reflow the paths once they're converted. The PrimSpecs are then copied over to their new path. The resulting prim hierarchy matches what many artists we've worked with prefer to work with. Caveats: The Export MaterialX check is off by default. When using the Principled BSDF, the resulting graph is very usable. However, when using some of the other BSDFs, the shading networks generated by the existing MaterialX framework in Blender generate some shading graphs that are difficult for usdview and other DCC's to understand. The graph is still correct, but because we're trying to prioritize compatibility, the default is off. In future PRs we can aim to make the graphs for those other BSDFs play better with other DCCs. Other Implementation Details: As part of this commit we've also done the following: * Place some of the materialx graphs inside a passthrough nodegraph to avoid node conflicts. * Better handle some shader output types , and better handle some conflict cases. * Moved the ExportTextureFunction to materials.h due to some difficult to resolve header ordering issues. This has no effect on any runtime code. * There is a test for the MaterialX export that does some basic checking to make sure we get an export out the other end that matches our expectations Authored by Apple: Dhruv Govil This PR is based on an earlier implementation by Brecht van Lommel , as well as Brian Savery and his teams' work at AMD to implement the general MaterialX framework within Blender.
❤️ 9
Michael B Johnson added the
Interest
Pipeline, Assets & IO
Interest
USD
 labels 2024-05-31 20:18:59 +02:00
Jesse Yurkovich was assigned by Michael B Johnson 2024-05-31 20:19:00 +02:00
Michael Kowalski was assigned by Michael B Johnson 2024-05-31 20:19:00 +02:00
Michael B Johnson requested review from Jesse Yurkovich 2024-05-31 20:20:14 +02:00
Michael B Johnson requested review from Michael Kowalski 2024-05-31 20:20:14 +02:00
Jesse Yurkovich was unassigned by Michael B Johnson 2024-05-31 20:20:56 +02:00
Michael Kowalski was unassigned by Michael B Johnson 2024-05-31 20:21:06 +02:00
Michael B Johnson force-pushed dhruv/usd-materialx from 58c5b542ac to 3ac2827fd6 2024-06-01 01:22:22 +02:00 Compare
Brecht Van Lommel reviewed 2024-06-03 11:35:00 +02:00
source/blender/editors/io/io_usd.cc Outdated
@ -544,3 +548,3 @@
"generate_preview_surface",
true,
"To USD Preview Surface",
"Export USD Preview Surface",
Brecht Van Lommel commented 2024-06-03 10:42:07 +02:00
Owner
Copy Link

Leave out "Export" from label name, for consistency with other properties.

Leave out "Export" from label name, for consistency with other properties.
brecht marked this conversation as resolved
source/blender/io/usd/intern/usd_writer_material.cc Outdated
@ -39,0 +45,4 @@
#endif
#include <iostream>
#include <unordered_map>
Brecht Van Lommel commented 2024-06-03 10:43:15 +02:00
Owner
Copy Link

Generally BLI_map.hh should be used instead.

Generally `BLI_map.hh` should be used instead.
brecht marked this conversation as resolved
source/blender/io/usd/intern/usd_writer_material.cc Outdated
@ -1088,0 +1247,4 @@
* and modify it to match the final target location */
for (auto &temp_material_output : temp_material.GetOutputs()) {
pxr::SdfPathVector output_paths;
;
Brecht Van Lommel commented 2024-06-03 10:46:50 +02:00
Owner
Copy Link

Stray ;, here and various later instances in this function.

Stray `;`, here and various later instances in this function.
brecht marked this conversation as resolved
source/blender/nodes/shader/materialx/material.cc Outdated
@ -59,3 +62,4 @@
if (material->use_nodes) {
material->nodetree->ensure_topology_cache();
bNode *output_node = ntreeShaderOutputNode(material->nodetree, SHD_OUTPUT_ALL);
// Add a nodegraph for the material
Brecht Van Lommel commented 2024-06-03 10:48:50 +02:00
Owner
Copy Link

Code style: use /* */

Code style: use `/* */`
brecht marked this conversation as resolved
source/blender/nodes/shader/materialx/node_item.cc Outdated
@ -766,2 +766,3 @@
NodeItem res = empty();
res.node = graph_->addNode(category, MaterialX::EMPTY_STRING, type_str);
// Surfaceshader nodes and materials are added directly to the document, otherwise to the
// nodegraph
Brecht Van Lommel commented 2024-06-03 10:54:30 +02:00
Owner
Copy Link

Code style: use /* */

Code style: use `/* */`
brecht marked this conversation as resolved
Brecht Van Lommel commented 2024-06-03 11:43:39 +02:00
Owner
Copy Link

@blender-bot build

@blender-bot build
Brecht Van Lommel reviewed 2024-06-03 12:10:32 +02:00
source/blender/nodes/shader/materialx/material.h Outdated
@ -13,8 +13,11 @@ class ExportImageFunction;
namespace blender::nodes::materialx {
using ExportImageFunction = std::function<std::string(Main *, Scene *, Image *, ImageUser *)>;
Brecht Van Lommel commented 2024-06-03 12:10:32 +02:00
Owner
Copy Link

Getting a build error here, needed to add:

#include <functional>
#include <string>

struct Depsgraph;
struct Image;
struct ImageUser;
struct Main;
struct Material;
struct Scene;

(It's because I have unity build disabled, which catches some missing header includes)

Getting a build error here, needed to add: ``` #include <functional> #include <string> struct Depsgraph; struct Image; struct ImageUser; struct Main; struct Material; struct Scene; ``` (It's because I have unity build disabled, which catches some missing header includes)
brecht marked this conversation as resolved
Brecht Van Lommel commented 2024-06-03 14:24:58 +02:00
Owner
Copy Link

Can you add this code to make it so that it also works for our Hydra integration?
https://projects.blender.org/brecht/paste/src/branch/main/usd_hydra_materialx.patch

That way we can run the Storm render tests, enabled through WITH_GPU_RENDER_TESTS. These test rendering both through Hydra directly, and through loading an exported USD file in Hydra. There are some known differences (e.g. #122651), but we can at least manually check then if the results are as expected.

In testing this, it seems this is breaking Hydra MaterialX export, and USD also does not appear to be using the MaterialX network. Reverting the changes made in source/blender/nodes/shader/materialx/material.cc appears to fix both.

The issue can be reproduced as follows:

  • Load attached blend file
  • Enable Hydra Storm add-on in preferences
  • Render
Warning (secondary thread): in ~_DeferredDiagnostics at line 78 of /src/build_linux/deps/build/usd/src/external_usd/pxr/usd/sdf/path.cpp -- Invalid prim name 'MAMaterial.001'
Coding Error (secondary thread): in _IsValidPathForCreatingPrim at line 3616 of /src/build_linux/deps/build/usd/src/external_usd/pxr/usd/usd/stage.cpp -- Path must be an absolute path: <>
Warning (secondary thread): in ReadMaterials at line 2378 of /src/build_linux/deps/build/usd/src/external_usd/pxr/usd/usdMtlx/reader.cpp -- Failed to create material 'MAMaterial.001'
Coding Error (secondary thread): in _ComputeNamedOutputSources at line 552 of /src/build_linux/deps/build/usd/src/external_usd/pxr/usd/usdShade/material.cpp -- Failed verification: ' universalOutput '

To test with USD export:

  • Enable Developer Extras in preferences
  • Change export method from Hydra to USD in Hydra Debug panel

If you want to run the full tests, you need to merge with main to get the latest test reference images.

Can you add this code to make it so that it also works for our Hydra integration? https://projects.blender.org/brecht/paste/src/branch/main/usd_hydra_materialx.patch That way we can run the Storm render tests, enabled through `WITH_GPU_RENDER_TESTS`. These test rendering both through Hydra directly, and through loading an exported USD file in Hydra. There are some known differences (e.g. #122651), but we can at least manually check then if the results are as expected. In testing this, it seems this is breaking Hydra MaterialX export, and USD also does not appear to be using the MaterialX network. Reverting the changes made in `source/blender/nodes/shader/materialx/material.cc` appears to fix both. The issue can be reproduced as follows: * Load attached blend file * Enable Hydra Storm add-on in preferences * Render ``` Warning (secondary thread): in ~_DeferredDiagnostics at line 78 of /src/build_linux/deps/build/usd/src/external_usd/pxr/usd/sdf/path.cpp -- Invalid prim name 'MAMaterial.001' Coding Error (secondary thread): in _IsValidPathForCreatingPrim at line 3616 of /src/build_linux/deps/build/usd/src/external_usd/pxr/usd/usd/stage.cpp -- Path must be an absolute path: <> Warning (secondary thread): in ReadMaterials at line 2378 of /src/build_linux/deps/build/usd/src/external_usd/pxr/usd/usdMtlx/reader.cpp -- Failed to create material 'MAMaterial.001' Coding Error (secondary thread): in _ComputeNamedOutputSources at line 552 of /src/build_linux/deps/build/usd/src/external_usd/pxr/usd/usdShade/material.cpp -- Failed verification: ' universalOutput ' ``` To test with USD export: * Enable Developer Extras in preferences * Change export method from Hydra to USD in Hydra Debug panel If you want to run the full tests, you need to merge with main to get the latest test reference images.
diffuse_hydra.blend
614 KiB
Brecht Van Lommel commented 2024-06-03 14:25:08 +02:00
Owner
Copy Link

I edited the description to make it ready as a commit message, and added the "Authored by Apple" from the commit which I guess was unintentionally left out.

I edited the description to make it ready as a commit message, and added the "Authored by Apple" from the commit which I guess was unintentionally left out.
Dhruv Govil commented 2024-06-03 19:34:41 +02:00
First-time contributor
Copy Link

Thanks for the notes, @brecht . I'll get those sorted.

Thanks for the notes, @brecht . I'll get those sorted.
Jesse Yurkovich reviewed 2024-06-03 19:41:42 +02:00
Jesse Yurkovich left a comment
Member
Copy Link

Still reviewing a bit but the biggest issue I've run into is that I can't get textures to be exported (both generated textures or ones with paths). I'm testing with "Export USD Preview Surface" disabled and only keeping "Export MaterialX Network" enabled. The material network is basically Image Texture -> Diffuse BSDF -> Output.

Still reviewing a bit but the biggest issue I've run into is that I can't get textures to be exported (both generated textures or ones with paths). I'm testing with "Export USD Preview Surface" _disabled_ and only keeping "Export MaterialX Network" enabled. The material network is basically Image Texture -> Diffuse BSDF -> Output.
source/blender/io/usd/intern/usd_writer_material.cc Outdated
@ -39,0 +44,4 @@
# include <pxr/usd/usdMtlx/utils.h>
#endif
#include <iostream>
Jesse Yurkovich commented 2024-06-03 19:38:36 +02:00
Member
Copy Link

iostream is also unnecessary and can be removed.

iostream is also unnecessary and can be removed.
brecht marked this conversation as resolved
source/blender/io/usd/intern/usd_writer_material.cc Outdated
@ -1088,0 +1174,4 @@
* This does mean that we need to pre-process the resulting graph so that there are no
* name conflicts.
* So we first gather all the existing names in this namespace to avoid that.*/
std::set<std::string> used_names;
Jesse Yurkovich commented 2024-06-03 19:26:54 +02:00
Member
Copy Link

Use blender::Set (like the blender::Map feedback for the unordered_map)

Use `blender::Set` (like the `blender::Map` feedback for the unordered_map)
brecht marked this conversation as resolved
Dhruv Govil commented 2024-06-03 20:40:49 +02:00
First-time contributor
Copy Link

@brecht I'm looking into if there's a workaround for the EDF failure , but in the meantime I did also file an issue with Pixar because it is a problem on their end https://github.com/PixarAnimationStudios/OpenUSD/issues/3105

This only happens when using the Diffuse BSDF because it has to surface the node graph up from inside an ND_Surface node.

I checked against the latest commit (3a2ea7828e) in main and am seeing the same errors there. I therefore believe this is an issue that pre-dates this PR, and falls into the caveats+future work I had mentioned in my original description.

@brecht I'm looking into if there's a workaround for the EDF failure , but in the meantime I did also file an issue with Pixar because it is a problem on their end https://github.com/PixarAnimationStudios/OpenUSD/issues/3105 This only happens when using the Diffuse BSDF because it has to surface the node graph up from inside an ND_Surface node. I checked against the latest commit (3a2ea7828e96ec92106ac6b332064f3dddd368a5) in `main` and am seeing the same errors there. I therefore believe this is an issue that pre-dates this PR, and falls into the caveats+future work I had mentioned in my original description.
Dhruv Govil commented 2024-06-03 21:10:53 +02:00
First-time contributor
Copy Link

@deadpin would you have a file I can test with? Preferably one with an embedded texture?
It was working for me in my simple case, but having more test cases would be really valuable so I can iron out the kinks.

I've addressed the rest of yours and Brecht's notes and will get an update pushed as soon as I can validate the texture export against your file.

@deadpin would you have a file I can test with? Preferably one with an embedded texture? It was working for me in my simple case, but having more test cases would be really valuable so I can iron out the kinks. I've addressed the rest of yours and Brecht's notes and will get an update pushed as soon as I can validate the texture export against your file.
Jesse Yurkovich commented 2024-06-03 21:29:40 +02:00
Member
Copy Link

@DhruvGovil Attached here.

@DhruvGovil Attached here.
matx-repro.blend
918 KiB
Dhruv Govil commented 2024-06-03 23:02:33 +02:00
First-time contributor
Copy Link

Thanks, I can reproduce that it doesn't export textures if USD Preview Surface is off. Should be a quick fix.

Thanks, I can reproduce that it doesn't export textures if USD Preview Surface is off. Should be a quick fix.
Dhruv Govil commented 2024-06-04 00:36:32 +02:00
First-time contributor
Copy Link

Okay, I believe I've hit up all the notes above. @wave will upload the changes as soon as he can.

@brecht , I've also uploaded a patch to USD that will fix the issue you were reporting above. https://github.com/PixarAnimationStudios/OpenUSD/pull/3106

I'm not sure when this will get merged into mainline USD, but perhaps it would be good to merge the change on Blender's vendor version of USD?

Okay, I believe I've hit up all the notes above. @wave will upload the changes as soon as he can. @brecht , I've also uploaded a patch to USD that will fix the issue you were reporting above. https://github.com/PixarAnimationStudios/OpenUSD/pull/3106 I'm not sure when this will get merged into mainline USD, but perhaps it would be good to merge the change on Blender's vendor version of USD?
Brecht Van Lommel commented 2024-06-04 14:06:28 +02:00
Owner
Copy Link

Is the idea that Blender would still write out USD files that can't be rendered by the current version of Hydra Storm, and they would only start working in a future USD version? If so, that doesn't seem ideal.

The main purpose of Hydra Storm in Blender right now is more to verify what the results will be when others import the data, including quirks and bugs. We could apply the PR ourselves if we have no alternative.

Could we work around it by adding an EDF with color zero when it's not there, which I guess is what makes Principled BSDF work? Maybe there are some downsides regarding performance (would affect Principled BSDF too though) or messy node graphs.

Is the idea that Blender would still write out USD files that can't be rendered by the current version of Hydra Storm, and they would only start working in a future USD version? If so, that doesn't seem ideal. The main purpose of Hydra Storm in Blender right now is more to verify what the results will be when others import the data, including quirks and bugs. We could apply the PR ourselves if we have no alternative. Could we work around it by adding an EDF with color zero when it's not there, which I guess is what makes Principled BSDF work? Maybe there are some downsides regarding performance (would affect Principled BSDF too though) or messy node graphs.
Brecht Van Lommel commented 2024-06-04 14:11:25 +02:00
Owner
Copy Link

Also to be clear, the Hydra MaterialX code path with diffuse BSDF is working in main for me, on Linux with OpenGL. But maybe not with macOS and Metal, I remember some extra issues there.

Also to be clear, the Hydra MaterialX code path with diffuse BSDF is working in main for me, on Linux with OpenGL. But maybe not with macOS and Metal, I remember some extra issues there.
Michael Kowalski commented 2024-06-04 17:00:41 +02:00
Member
Copy Link

I can only do a partial review (due to time constraints), but the changes to the existing files (e.g., usd_writer_material.cc, etc.) look good to me. I tested exporting MaterialX to Houdini and that seemed to work as expected.

@DhruvGovil I did notice that texture coordinates must be named st and that the texture coordinate name provided in Blender's UV Map node is ignored. If I recall, this is a limitation in MaterialX support in USD. Is that correct?

I can only do a partial review (due to time constraints), but the changes to the existing files (e.g., `usd_writer_material.cc`, etc.) look good to me. I tested exporting MaterialX to Houdini and that seemed to work as expected. @DhruvGovil I did notice that texture coordinates must be named `st` and that the texture coordinate name provided in Blender's `UV Map` node is ignored. If I recall, this is a limitation in MaterialX support in USD. Is that correct?
Dhruv Govil commented 2024-06-04 17:52:56 +02:00
First-time contributor
Copy Link

@makowalski Yes that's a limitation of USD's MaterialX translation currently. Is there a place where you resolve that name? It doesn't seem to make it into the MaterialX graph itself, so it's lost by the time it gets to my code that writes it out. Perhaps we could find a way to persist it through though. If you have a test file as well, that would be useful.

@brecht Apologies, I got confused between two sets of stacked errors in your example file.

The errors you mentioned above should be fixed now as soon as Wave can upload it. The issue there was that the Material name was being derived from Blender's name, which aren't always valid identifiers in USD. I now pre-convert them to a valid identifier. Which is why you don't see them prior to this PR.

However that still leaves the EDF issue in the resulting USD file with an EDF type, when using an ND_Surface node. Unfortunately that would require a patch in USD itself (as linked) because it just doesn't understand what an EDF is. Even connecting it to something would fail because it's looking up the shader registry within itself, and fails before it even parses the graph connections.

These are the ones I was also hitting on main as well, so pre-date this PR. If you're not hitting them on Linux, then I suspect the Pixar codepath only has the issue in the Metal path (though it does happen in the shared shader gen section so I am suspect). Those errors would look like this when a diffuse BSDF is loaded in Storm:

Warning: in _ValidateCompilation at line 229 of /Users/dhruvgovil/Projects/usd/pxr/imaging/hdSt/glslProgram.cpp -- Failed to compile shader (FRAGMENT_SHADER): program_source:2394:5: error: unknown type name 'EDF'
    EDF edf;
    ^

ERROR: Usdview encountered an error while rendering.
	Error in '&pxrInternal_v0_24__pxrReserved__::HdSt_DrawBatch::_GetDrawingProgram' at line 408 in file /Users/dhruvgovil/Projects/usd/pxr/imaging/hdSt/drawBatch.cpp : 'Failed to compile shader for prim /root/Plane/Plane.'
	Error in '&pxrInternal_v0_24__pxrReserved__::HdSt_DrawBatch::_GetDrawingProgram' at line 408 in file /Users/dhruvgovil/Projects/usd/pxr/imaging/hdSt/drawBatch.cpp : 'Failed to compile shader for prim /root/Sphere/Sphere.'

Unfortunately, the state of USD+MaterialX is a bit raw at the moment, and my team is working with both projects + several DCC developers to fix them.

I think if you're not seeing the EDF issue on Linux, it's probably fine to not apply the USD patch mentioned above, and I can then work with Pixar to land it ASAP (earliest would likely be USD 24.8 at SIGGRAPH). My opinion would be that the EDF issue shouldn't be a blocker for this PR, if everything else looks alright (though of course I defer to you on that) since it'll be fixed in the upstream USD soon.

@makowalski Yes that's a limitation of USD's MaterialX translation currently. Is there a place where you resolve that name? It doesn't seem to make it into the MaterialX graph itself, so it's lost by the time it gets to my code that writes it out. Perhaps we could find a way to persist it through though. If you have a test file as well, that would be useful. @brecht Apologies, I got confused between two sets of stacked errors in your example file. The errors you mentioned above should be fixed now as soon as Wave can upload it. The issue there was that the Material name was being derived from Blender's name, which aren't always valid identifiers in USD. I now pre-convert them to a valid identifier. Which is why you don't see them prior to this PR. However that still leaves the EDF issue in the resulting USD file with an EDF type, when using an ND_Surface node. Unfortunately that would require a patch in USD itself (as linked) because it just doesn't understand what an EDF is. Even connecting it to something would fail because it's looking up the shader registry within itself, and fails before it even parses the graph connections. These are the ones I was also hitting on `main` as well, so pre-date this PR. If you're not hitting them on Linux, then I suspect the Pixar codepath only has the issue in the Metal path (though it does happen in the shared shader gen section so I am suspect). Those errors would look like this when a diffuse BSDF is loaded in Storm: ``` Warning: in _ValidateCompilation at line 229 of /Users/dhruvgovil/Projects/usd/pxr/imaging/hdSt/glslProgram.cpp -- Failed to compile shader (FRAGMENT_SHADER): program_source:2394:5: error: unknown type name 'EDF' EDF edf; ^ ERROR: Usdview encountered an error while rendering. Error in '&pxrInternal_v0_24__pxrReserved__::HdSt_DrawBatch::_GetDrawingProgram' at line 408 in file /Users/dhruvgovil/Projects/usd/pxr/imaging/hdSt/drawBatch.cpp : 'Failed to compile shader for prim /root/Plane/Plane.' Error in '&pxrInternal_v0_24__pxrReserved__::HdSt_DrawBatch::_GetDrawingProgram' at line 408 in file /Users/dhruvgovil/Projects/usd/pxr/imaging/hdSt/drawBatch.cpp : 'Failed to compile shader for prim /root/Sphere/Sphere.' ``` Unfortunately, the state of USD+MaterialX is a bit raw at the moment, and my team is working with both projects + several DCC developers to fix them. I think if you're not seeing the EDF issue on Linux, it's probably fine to not apply the USD patch mentioned above, and I can then work with Pixar to land it ASAP (earliest would likely be USD 24.8 at SIGGRAPH). My opinion would be that the EDF issue shouldn't be a blocker for this PR, if everything else looks alright (though of course I defer to you on that) since it'll be fixed in the upstream USD soon.
Michael Kowalski commented 2024-06-04 18:08:42 +02:00
Member
Copy Link

@makowalski Yes that's a limitation of USD's MaterialX translation currently. Is there a place where you resolve that name? It doesn't seem to make it into the MaterialX graph itself, so it's lost by the time it gets to my code that writes it out. Perhaps we could find a way to persist it through though.

For UsdPreviewSurface we find a UV Map node connected to the Image Texture node and look up the name. If there is no UV Map node, we use the name of the active UV map on the mesh as a guess. However, in my opinion, solving this for your MaterialX export is not a high priority. I would be fine with the assumption that the maps need to be named st in the initial commit.

> @makowalski Yes that's a limitation of USD's MaterialX translation currently. Is there a place where you resolve that name? It doesn't seem to make it into the MaterialX graph itself, so it's lost by the time it gets to my code that writes it out. Perhaps we could find a way to persist it through though. For `UsdPreviewSurface` we find a `UV Map` node connected to the `Image Texture` node and look up the name. If there is no `UV Map` node, we use the name of the active UV map on the mesh as a guess. However, in my opinion, solving this for your MaterialX export is not a high priority. I would be fine with the assumption that the maps need to be named `st` in the initial commit.
👍 1
Dhruv Govil commented 2024-06-04 18:16:43 +02:00
First-time contributor
Copy Link

Ah that's good to know. Thinking about it now, the changes could in theory be completely on the MaterialX graph side by explicitly authoring geompropvalue nodes that have a stringed lookup of the primvar, which (like you said) probably lends itself to being a future PR since I would like to keep graph changes here to a minimum.

Ah that's good to know. Thinking about it now, the changes could in theory be completely on the MaterialX graph side by explicitly authoring geompropvalue nodes that have a stringed lookup of the primvar, which (like you said) probably lends itself to being a future PR since I would like to keep graph changes here to a minimum.
👍 1
Brecht Van Lommel commented 2024-06-04 19:00:58 +02:00
Owner
Copy Link

@DhruvGovil Thanks for the explanation. I have added the patch to our USD build in #122725.

So then we should be all good with the updated version of this PR.

@DhruvGovil Thanks for the explanation. I have added the patch to our USD build in #122725. So then we should be all good with the updated version of this PR.
Brecht Van Lommel changed target branch from main to blender-v4.2-release 2024-06-05 16:30:14 +02:00
Michael B Johnson force-pushed dhruv/usd-materialx from 3ac2827fd6 to 488efe82e0 2024-06-05 18:24:39 +02:00 Compare
Dhruv Govil commented 2024-06-05 18:26:31 +02:00
First-time contributor
Copy Link

@brecht Okay, latest changes are pushed! Apologies for the delay getting them up.
I've included your patch as requested as well in this change.

@brecht Okay, latest changes are pushed! Apologies for the delay getting them up. I've included your patch as requested as well in this change.
Michael Kowalski approved these changes 2024-06-05 18:34:50 +02:00
Michael Kowalski left a comment
Member
Copy Link

As I mentioned earlier, I can only do a limited review, but the changes on the whole look good to me. With that caveat, I am approving this PR, so as not to block the commit once @deadpin and @brecht approve the latest changes.

As I mentioned earlier, I can only do a limited review, but the changes on the whole look good to me. With that caveat, I am approving this PR, so as not to block the commit once @deadpin and @brecht approve the latest changes.
👍 1
Dhruv Govil commented 2024-06-05 18:55:21 +02:00
First-time contributor
Copy Link

Thanks, @makowalski

Thanks, @makowalski
👍 1
Brecht Van Lommel added 1 commit 2024-06-05 19:52:52 +02:00
Merge branch 'blender-v4.2-release' into HEAD
Some checks failed
buildbot/vexp-code-patch-darwin-x86_64 Build done.
Details
buildbot/vexp-code-patch-windows-amd64 Build done.
Details
buildbot/vexp-code-patch-lint Build done.
Details
buildbot/vexp-code-patch-darwin-arm64 Build done.
Details
buildbot/vexp-code-patch-coordinator Build done.
Details
buildbot/vexp-code-patch-linux-x86_64 Build done.
Details
6cf1cdadf5
Brecht Van Lommel commented 2024-06-05 19:53:09 +02:00
Owner
Copy Link

@blender-bot build

@blender-bot build
Brecht Van Lommel commented 2024-06-05 20:09:21 +02:00
Owner
Copy Link

Testing this with our Storm render test, the Hydra regression is fixed.

For the USD path, the two main difference with Hydra are:

  • Missing dome light, to be handled by (#122651)
  • Textures are missing in many tests, I guess due to issues with texture coordinates. (#122800)
Testing this with our Storm render test, the Hydra regression is fixed. For the USD path, the two main difference with Hydra are: * Missing dome light, to be handled by (#122651) * Textures are missing in many tests, I guess due to issues with texture coordinates. (#122800)
Brecht Van Lommel commented 2024-06-05 20:10:33 +02:00
Owner
Copy Link

Probably best to merge as is an deal with texture coordinate issues afterwards.

Probably best to merge as is an deal with texture coordinate issues afterwards.
Brecht Van Lommel commented 2024-06-05 20:11:35 +02:00
Owner
Copy Link

@blender-bot build +gpu linux
@blender-bot build macos-arm64

@blender-bot build +gpu linux @blender-bot build macos-arm64
Brecht Van Lommel commented 2024-06-05 20:22:44 +02:00
Owner
Copy Link

@blender-bot build macos-arm64

@blender-bot build macos-arm64
Brecht Van Lommel approved these changes 2024-06-05 20:24:18 +02:00
Brecht Van Lommel merged commit f913fb6159 into blender-v4.2-release 2024-06-05 20:43:57 +02:00
Brecht Van Lommel deleted branch dhruv/usd-materialx 2024-06-05 20:43:59 +02:00
Brecht Van Lommel commented 2024-06-05 20:48:01 +02:00
Owner
Copy Link

I created a follow up issue for the texture coordinates in #122800.

I created a follow up issue for the texture coordinates in #122800.
Jesse Yurkovich approved these changes 2024-06-05 20:49:12 +02:00
Jesse Yurkovich left a comment
Member
Copy Link

Looks alright from my side too. I tested with the materials I had on hand, not many worked due to existing MaterialX conversion known issues, but nothing crashed Blender or usdview. Seems ok enough to continue from this point forward.

Looks alright from my side too. I tested with the materials I had on hand, not many worked due to existing MaterialX conversion known issues, but nothing crashed Blender or `usdview`. Seems ok enough to continue from this point forward.
Dhruv Govil commented 2024-06-05 21:34:42 +02:00
First-time contributor
Copy Link

Awesome, thanks @brecht and @deadpin ! Much appreciated :-)

Awesome, thanks @brecht and @deadpin ! Much appreciated :-)
Sign in to join this conversation.
Reviewers
No reviewers
Michael Kowalski
Brecht Van Lommel
Jesse Yurkovich
Labels
Clear labels   
Interest
Alembic

  
Interest
Animation & Rigging

  
Interest
Asset System
Asset datablocks, libraries, browser and shelf

  
Interest
Audio

  
Interest
Automated Testing

  
Interest
Blender Asset Bundle

  
Interest
BlendFile

  
Interest
Collada

  
Interest
Compatibility
Backward and forward compatibility

  
Interest
Compositing

  
Interest
Core

  
Interest
Cycles

  
Interest
Dependency Graph

  
Interest
Development Management

  
Interest
EEVEE

  
Interest
Freestyle

  
Interest
Geometry Nodes

  
Interest
Grease Pencil

  
Interest
ID Management

  
Interest
Images & Movies

  
Interest
Import Export

  
Interest
Line Art

  
Interest
Masking

  
Interest
Metal

  
Interest
Modeling

  
Interest
Modifiers

  
Interest
Motion Tracking

  
Interest
Nodes & Physics

  
Interest
OpenGL

  
Interest
Overlay

  
Interest
Overrides

  
Interest
Performance

  
Interest
Physics

  
Interest
Pipeline, Assets & IO

  
Interest
Platforms, Builds & Tests

  
Interest
Python API

  
Interest
Render & Cycles

  
Interest
Render Pipeline

  
Interest
Sculpt, Paint & Texture

  
Interest
Text Editor

  
Interest
Translations

  
Interest
Triaging

  
Interest
Undo

  
Interest
USD

  
Interest
User Interface

  
Interest
UV Editing

  
Interest
VFX & Video

  
Interest
Video Sequencer

  
Interest
Viewport & EEVEE

  
Interest
Virtual Reality

  
Interest
Vulkan

  
Interest
Wayland
Wayland windowing on Unix

  
Interest
Workbench

  
Interest: X11

Xorg/X11 windowing

  
Legacy
Asset Browser Project

Archived

  
Legacy
Blender 2.8 Project

Archived

  
Legacy
Milestone 1: Basic, Local Asset Browser

Archived

  
Legacy
OpenGL Error

Archived

  
Meta
Good First Issue

  
Meta
Papercut

  
Meta
Retrospective

  
Meta
Security
Related to security, https://wiki.blender.org/wiki/Process/Vulnerability_Reports

  
Module
Animation & Rigging

  
Module
Core

  
Module
Development Management

  
Module
Grease Pencil

  
Module
Modeling

  
Module
Nodes & Physics

  
Module
Pipeline, Assets & IO

  
Module
Platforms, Builds & Tests

  
Module
Python API

  
Module
Render & Cycles

  
Module
Sculpt, Paint & Texture

  
Module
Triaging

  
Module
User Interface

  
Module
VFX & Video

  
Module
Viewport & EEVEE

  
Platform
FreeBSD

  
Platform
Linux

  
Platform
macOS

  
Platform
Windows

  
Severity
High

  
Severity
Low

  
Severity
Normal

  
Severity
Unbreak Now!

  
Status
Archived

  
Status
Confirmed

  
Status
Duplicate

  
Status
Needs Info from Developers

  
Status
Needs Information from User

  
Status
Needs Triage

  
Status
Resolved

  
Type
Bug

  
Type
Design

  
Type
Known Issue

  
Type
Patch

Archived

  
Type
Report

Archived

  
Type
To Do

No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Viewport & EEVEE
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Module
Viewport & EEVEE
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Severity
High
Severity
Low
Severity
Normal
Severity
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
5 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#122575
No description provided.
Delete Branch "wave/blender_wave_Apple:dhruv/usd-materialx"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?