Autodesk: Optimize MaterialX shader generation in Storm by comparing topologies #3073

erikaharrison-adsk · 2024-05-06T21:53:26Z

Description of Change(s)

Fixes:

MaterialX: Storm fails to reuse GLSL shaders for networks differing only on node names MaterialX: Storm fails to reuse GLSL shaders for networks differing only on node names #2330
Storm shader compilation errors when using MaterialX triplanar projection node. Storm shader compilation errors when using MaterialX triplanarprojection node. #3004
Improve transparency detection and fix generation

Using a topo-trimmed network resulted in all shaders being generated with opaque semantics. The shaders are now created with the right transparent hardware settings.

Also improved transparency detection at the _GetMaterialTag level by adding more checks for known surfaces and delegating to MaterialX when dealing with custom shaders, but without generating the full MaterialX document.

Fixes Issue(s)

I have verified that all unit tests pass with the proposed changes

I have submitted a signed Contributor License Agreement

Fixes: - MaterialX: Storm fails to reuse GLSL shaders for networks differing only on node names PixarAnimationStudios#2330 - Storm shader compilation errors when using MaterialX triplanarprojection node. PixarAnimationStudios#3004 - Improve transparency detection and fix generation Using a topo-trimmed network resulted in all shaders being generated with opaque semantics. The shaders are now created with the right transparent hardware settings. Also improved transparency detection at the _GetMaterialTag level by adding more checks for known surfaces and delegating to MaterialX when dealing with custom shaders, but without generating the full MaterialX document.

JGamache-autodesk · 2024-05-06T22:14:54Z

Primary goal was to only reconstruct the MaterialX document if we are really going all the way to compiling the OpenGL shader. This means we prioritize gathering the Storm requirements from:
1- A previously generated MaterialX::Shader
2- The Sdr registry
3- The NodeDef info that can be found in the MaterialX library
4- A partially reconstructed MaterialX document as last resort (for resolving transparency)

To do this, we reduce the HdMaterialNetwork2 to a topology equivalent skeleton network that will produce the exact same shader code. We generate a hash of this skeleton network and use it to fetch a previously compiled MaterialX shader.

With this we can generate the uniform block required by Storm using the default values found in the shader, modified by the values found in the original network. Some extra work was required to make sure the final HdMaterialNetwork had all the requirements for correctly findings the textures to load.

Testing shows that for the scene submitted in #2330 we now only generate a single OpenGL shader. Time to first frame is 10X faster. Also, a scene with a simple animated material (animated ND_dot_color3 connected to the diffuse_color input of a standard_surface shader), the frame rate went from 9.3 to 23.7 on my machine since we do not regenerate the MaterialX document at each frame. Could probably be improved by caching the fallback values as a VtValue dictionary.

JGamache-autodesk · 2024-05-06T22:18:04Z

pxr/imaging/hdSt/materialXFilter.cpp

    (transmission)

+    (open_pbr_surface)


Added programmatic detection for both OpenPBR and glTf surfaces to be able to get the materialTag without having to peek into the MaterialX NodeGraph implementation.

Ahh, just merged #3069 ... Any quick opinions on how to merge those changes vs these?

This PR also includes detection for glTf textures via a _GetGlTFSurfaceMaterialTag() function that properly differentiate between translucent and masked mode. The merge resolution strategy would be to let this PR fully win over #3069, equivalent to reverting it before merge.

I have opened Pablo's asset with my local build and it renders correctly.

JGamache-autodesk · 2024-05-06T22:19:07Z

pxr/imaging/hdSt/materialXFilter.cpp

+    _topologicalTokens,
+    // This represents living knowledge of the internals of the MaterialX shader generator
+    // for both GLSL and Metal. Such knowledge should reside inside the generator class
+    // provided by MaterialX.


To be submitted to MaterialX for 1.39.

Is the intent here to have bool MaterialXIsNodeTopological(std::string name) or something?

We'd definitely be interested in this code moving into MX, for the record :D.

Yes, the information being dependent on ShaderGen target, it should probably be static bool MaterialX::GlslShaderGenerator::isNodeDefTopological(MaterialX::NodeDefPtr const&)

I do love the fact that we can get all the required info from Sdr and compare via TfTokens without having to use the MaterialX library, but in this case correctness trumps convenience, so this will definitely be added to MaterialX. If performance matters, we can always cache the result since the return value will always be the same for a given nodeId.

JGamache-autodesk · 2024-05-06T22:20:06Z

pxr/imaging/hdSt/materialXFilter.cpp

 {
-    bool hasTransparency = mx::isTransparentSurface(mxElem,
-                               mxContext.getShaderGenerator().getTarget());
+    bool hasTransparency = mxContext.getOptions().hwTransparency;


Should have been correctly set in mxContext .

JGamache-autodesk · 2024-05-06T22:22:09Z

pxr/imaging/hdSt/materialXFilter.cpp

+                    outNode.parameters.insert(param);
+                    // Need an empty asset as well:
+                    outNode.parameters.emplace(TfToken(colorManagedFile.first),
+                                               VtValue{SdfAssetPath{}});


Might be incorrect if there are plans to color-correct color3 values on nodes that are not file based.

We're planning to, in some form, but I think we can update this bit when we get to it.

We actually already support that for MaterialX - there is an existing test case but it looks like the opensource thresholds were not tight enough to catch this. Let me know if you are good with this change to catch color corrected values

const auto colorManagedInput = SdfPath::StripPrefixNamespace(param.first.GetString(), SdfFieldKeys->ColorSpace); // If this parameter indicates a colorspace on a color // managed input, find and add that corresponding input if (colorManagedInput.second) { outNode.parameters.insert(param); // Get the parameter value for the input const TfToken colorInputParam(colorManagedInput.first); const auto colorInputIt = inNode.parameters.find(colorInputParam); if (colorInputIt != inNode.parameters.end()) { VtValue colorInputValue = colorInputIt->second; // Use an empty asset for color managed files if (colorInputValue.IsHolding<SdfAssetPath>()) { colorInputValue = VtValue(SdfAssetPath()); } outNode.parameters.emplace(colorInputParam, colorInputValue); } }

Sadly there will not be a value to be found in the data since it all gets pruned to create a pure "default" shader. But the answer we are looking for can be found in the Sdr registry with code pretty close to this:

const auto colorManagedInput = SdfPath::StripPrefixNamespace(param.first.GetString(), SdfFieldKeys->ColorSpace); // If this parameter indicates a colorspace on a color // managed input, we need to find out if it manages an asset. if (colorManagedInput.second) { outNode.parameters.insert(param); // Get the parameter type for the input const TfToken colorInputParam(colorManagedInput.first); SdrRegistry &sdrRegistry = SdrRegistry::GetInstance(); const SdrShaderNodeConstPtr sdrNode = sdrRegistry.GetShaderNodeByIdentifierAndType(inNode.nodeTypeId, _tokens->mtlx); auto sdrInput = sdrNode->GetShaderInput(colorInputParam); if (sdrInput->GetTypeAsSdfType().first == SdfValueTypeNames->Asset) { // Use an empty asset for color managed files colorInputValue = VtValue(SdfAssetPath()); outNode.parameters.emplace(colorInputParam, colorInputValue); } }

The other solution would be to fix _AddMaterialXNode() in usd\pxr\imaging\hdMtlx\hdMtlx.cpp to change this block:

// Skip Colorspace parameter, this is already captured in the paramData. // Note: Colorspace inputNames are of the form 'colorSpace:inputName' const std::pair<std::string, bool> result = SdfPath::StripPrefixNamespace(mxInputName, SdfFieldKeys->ColorSpace); if (result.second) { continue; }

to:

// Colorspace parameter: create an empty input if it does not already // exist to set colorspace metadata. // Note: Colorspace inputNames are of the form 'colorSpace:inputName' const std::pair<std::string, bool> result = SdfPath::StripPrefixNamespace(mxInputName, SdfFieldKeys->ColorSpace); if (result.second) { mx::InputPtr mxInput = mxNode->addInputFromNodeDef(result .first); mxInput->setColorSpace(HdMtlxConvertToString(paramData.value)); continue; }

Which would allow not having to create a dummy value in the first place.

Actually had to fix this on our side since it impacted our internal unit tests. The following function was added:

// MaterialX requires a value for color-spaced attributes. Fetch the default from Sdr shader: void _AddMaterialXDefaultValue(HdMaterialNode2& node, std::string const& parameterName) { SdrRegistry &sdrRegistry = SdrRegistry::GetInstance(); const SdrShaderNodeConstPtr sdrNode = sdrRegistry.GetShaderNodeByIdentifierAndType( node.nodeTypeId, _tokens->mtlx); if (sdrNode) { SdrShaderPropertyConstPtr sdrInput = sdrNode->GetShaderInput(TfToken(parameterName)); if (sdrInput) { if (sdrInput->IsAssetIdentifier()) { // Replace empty filename with a placeholder ignored by MaterialX shadergen: node.parameters.emplace(TfToken(parameterName), VtValue{SdfAssetPath{"placeholder"}}); } else { node.parameters.emplace(TfToken(parameterName), sdrInput->GetDefaultValueAsSdfType()); } } } }

and was called:

@@ -282,9 +303,8 @@ size_t _BuildEquivalentMaterialNetwork( SdfFieldKeys->ColorSpace); if (colorManagedFile.second) { outNode.parameters.insert(param); - // Need an empty asset as well: - outNode.parameters.emplace(TfToken(colorManagedFile.first), - VtValue{SdfAssetPath{}}); + // We need a value as well: + _AddMaterialXDefaultValue(outNode, colorManagedFile.first); } } }

And this made sure there was a correct value for the MaterialX code.

JGamache-autodesk · 2024-05-06T22:25:56Z

pxr/imaging/hdSt/materialXFilter.cpp

-
-// Replace the filenameInput parameter with a connection to a new texture node
-static void
-_ReplaceFilenameInput(


Moved below at line 1158. Heavily modified to only require the Hydra network.

JGamache-autodesk · 2024-05-06T22:28:22Z

pxr/imaging/hdSt/materialXFilter.cpp

 }

 static std::string const&
-_GetMaterialTag(HdMaterialNode2 const& terminal)
+_GetUsdPreviewSurfaceMaterialTag(HdMaterialNode2 const& terminal)


Using a per nodetype function since generic code does not work all the time. Especially true for UsdPreviewSurface and gfTF since they both support masked mode and this prevents automatic detection from properly deducing the material tag.

JGamache-autodesk · 2024-05-06T22:32:16Z

pxr/imaging/hdSt/materialXShaderGen.cpp

-        }
+        auto const& lineSeparator =
+            (i == block.size() - 1) ? mx::EMPTY_STRING : separator;
+        line += _EmitMxVertexDataLine(block[i], lineSeparator);


Found while testing some assets. Make sure we do not add the commas in the first place because there might be more than one to remove afterwards.

JGamache-autodesk · 2024-05-06T22:32:37Z

pxr/imaging/hdSt/materialXShaderGen.cpp

                "    #else\n"
-                "        %s(0.0),\n"
+                "        %s(0.0)%s\n"


This is where the unremovable extra commas were added.

JGamache-autodesk · 2024-05-06T22:33:59Z

pxr/imaging/hdSt/testenv/testHdStMaterialXShaderGen/baseline/shadergen_UsdPSdefault.out

@@ -70,7 +70,6 @@ float displacement;
 float occlusion;

 // Uniform block: PrivateUniforms
-float u_alphaThreshold = 0.001000;


These were incorrectly detected as transparent by MaterialX. The new code that enforces transparency using the materialTag is correct so I had to fix the baseline.

jesschimein · 2024-05-07T16:49:42Z

Filed as internal issue #USD-9646

jesschimein · 2024-05-07T16:49:44Z

/AzurePipelines run

azure-pipelines · 2024-05-07T16:49:53Z

Azure Pipelines successfully started running 1 pipeline(s).

tcauchois · 2024-05-10T22:46:30Z

pxr/imaging/hdSt/materialXFilter.cpp

+    // Everything boils down to an <image> node. We might have to dig it out of
+    // the nodegraph. Unsure about triplanar that has 3 image nodes. Does Storm
+    // require per-image texture params? How does one specify that using a single
+    // token?


Are you talking about sampler parameters (e.g. wrap mode)? Hydra takes them specified per-texture, and in the texture metadata we're gathering we should have a per-texture entry for everything. If the triplanar node only has one set of params, we can just splat that to all of the referenced textures.

Yes, sample parameters could theoretically vary from one texture to the other. This is not the case with triplanar, but a custom NodeDef/NodeGraph node with multiple texture could try having one texture repeat while the other clamps. This is a bit of a corner case though, so unsure how much energy we should invest into covering it.

I'm happy to leave it until we have a concrete asset we're trying to get working.

klucknav · 2024-05-17T01:04:23Z

pxr/imaging/hdSt/materialXFilter.cpp

+{
+    _AddDefaultMtlxTextureValues(hdTextureParams);
+
+    if (nodeDef->getCategory() == mx::ShaderNode::IMAGE) {


Isn't the category for nodeDef's 'nodeDef' should we be checking the category on the mtlx Node itself?

Agreed. Checking on the node would indeed work. The other option is to use nodeDef->getNodeString() instead. Do you want me to update the PR?

But, unit tests passing, as well as my test scenes rendering correctly, give a strong indication that this code block should be removed.

I agree I think it should be removed. My next question was going to be how is it different from the other _AddDefaultMtlxTextureValues() above, which seems to be the reason the tests were ok.
Am I understanding this function correctly in that the first _AddDefaultMtlxTextureValues() call handles the <image>and <tiledimage> nodes and the rest is intended to handle any other custom texture based nodes?

We are looking for default values and indeed those default are correct for mx::ShaderNode::IMAGE. One more reason to remove the if (nodeDef->getCategory() == mx::ShaderNode::IMAGE) block of code.

The first _AddDefaultMtlxTextureValues() can probably be merged into the second one since it is the only place where it is called.

I just browsed the MaterialX library, and all image nodes used in NodeGraphs are repeat, which mean the defaults are sufficient for all cases I tested.

The only way to insure this code actually does something useful would be to create a custom untiledimage NodeDef that does not have repeat as the default on its embedded image node.

Just to verify my understanding of this function. Since the first _AddDefaultMtlxTextureValues() correctly gets the default values for both of the image based stdlib nodes, is the rest of the function (after that if block we agree should be removed) handling the case that we have a custom texture node that uses different default values for the wrap mode or filterType than the stdlib nodes?

klucknav · 2024-05-17T21:01:53Z

pxr/imaging/hdSt/testenv/testHdStMaterialXShaderGen.cpp

+    }
+
+    auto renderableNode = renderableElem->asA<mx::Node>();
+    if (renderableNode && renderableNode->getCategory() == "UsdPreviewSurface")


Why do we need special handling for UsdPreviewSurface materials? Would we not get the correct result by calling mx::isTransparentSurface() below?

Sadly, mx::isTransparentSurface is only able to handle the most basic cases. It digs inside layers of NodeGraph until it finds either the opacity input of the ND_surface, or the weight input of a transparent Schlick node.

Then is has to traverse back up the connections to the calling node interface to check what was the actual value.

The problem is when there are intermediate nodes in that code path. MaterialX is able to deal with the luminance found in standard_surface that converts the color3 interface opacity input down to the float opacity input of the ND_surface node, but when the graph is more complex, as is the case with PreviewSurface and glTf, it becomes very hard to evaluate the expression in C++ to know or not if the surface is transparent.

In these cases, the mx::isTransparentSurface will always return transparent, and this is why we need a custom test. This is also why I had to fix the output of a few PreviewSurface unit tests because the surface was not transparent anymore.

JGamache-autodesk reviewed May 6, 2024

View reviewed changes

tcauchois reviewed May 10, 2024

View reviewed changes

klucknav reviewed May 17, 2024

View reviewed changes

pixar-oss merged commit c8d22f9 into PixarAnimationStudios:dev Jun 4, 2024
5 checks passed

This was referenced Jul 18, 2024

Incorrect material tag detection for OpenPBR surface #3172

Open

LOOKDEVX-3144 - Fix transmission issues Autodesk/maya-usd#3857

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Autodesk: Optimize MaterialX shader generation in Storm by comparing topologies #3073

Autodesk: Optimize MaterialX shader generation in Storm by comparing topologies #3073

erikaharrison-adsk commented May 6, 2024

JGamache-autodesk commented May 6, 2024 •

edited

Loading

JGamache-autodesk May 6, 2024

tcauchois May 10, 2024

JGamache-autodesk May 13, 2024

JGamache-autodesk May 6, 2024

tcauchois May 10, 2024

tcauchois May 10, 2024

JGamache-autodesk May 13, 2024

JGamache-autodesk May 6, 2024

JGamache-autodesk May 6, 2024

tcauchois May 10, 2024

klucknav May 20, 2024

JGamache-autodesk May 21, 2024

JGamache-autodesk Jun 3, 2024

JGamache-autodesk May 6, 2024

JGamache-autodesk May 6, 2024

JGamache-autodesk May 6, 2024

JGamache-autodesk May 6, 2024

JGamache-autodesk May 6, 2024

jesschimein commented May 7, 2024

jesschimein commented May 7, 2024

azure-pipelines bot commented May 7, 2024

tcauchois May 10, 2024

JGamache-autodesk May 13, 2024

tcauchois May 13, 2024

klucknav May 17, 2024

JGamache-autodesk May 17, 2024

JGamache-autodesk May 17, 2024

klucknav May 17, 2024 •

edited

Loading

JGamache-autodesk May 17, 2024

JGamache-autodesk May 17, 2024

klucknav May 17, 2024

klucknav May 17, 2024

JGamache-autodesk Jun 3, 2024

Autodesk: Optimize MaterialX shader generation in Storm by comparing topologies #3073

Autodesk: Optimize MaterialX shader generation in Storm by comparing topologies #3073

Conversation

erikaharrison-adsk commented May 6, 2024

Description of Change(s)

Fixes Issue(s)

JGamache-autodesk commented May 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jesschimein commented May 7, 2024

jesschimein commented May 7, 2024

azure-pipelines bot commented May 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

klucknav May 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JGamache-autodesk commented May 6, 2024 •

edited

Loading

klucknav May 17, 2024 •

edited

Loading