From f81bc2d1ca758b062f4771e13422788fd51eec15 Mon Sep 17 00:00:00 2001 From: Lee Kerley Date: Wed, 13 May 2026 10:18:06 -0700 Subject: [PATCH 1/2] Resolve geompropvalue names through nested node graphs in HW codegen A geompropvalue node wrapped in a node graph (e.g. UsdPrimvarReader) receives the name of the geometric property through the graph interface rather than as a local value. The hardware generators only inspected the node's local "geomprop" input, so generation threw "No 'geomprop' parameter found" whenever the name was supplied from an enclosing graph. Track the active compound instance on a stack in GenContext (pushed via the new ScopedSetCompoundInstanceNode RAII guard from CompoundNode during createVariables / emitFunctionDefinition / emitFunctionCall). HwGeomProp- ValueNode now climbs that stack in tandem with each graph boundary it crosses, so the property name resolves correctly across one or more levels of nested compounds. Add Mesh stream aliasing so getStream() can map the bound "i_geomprop_st" attribute onto the existing UV0 texture-coordinate stream; the glTF and OBJ loaders register this alias so the USD "st" primvar binds to mesh UVs. Also move the file-local "geomprop" string constant out of HwImplementation and into HwGeomPropValueNode, and add a GenShader test covering two levels of nested geompropvalue interface resolution across GLSL/MSL/Slang. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../usd_preview_surface_brass_tiled.mtlx | 21 +++++ source/MaterialXGenHw/HwImplementation.cpp | 1 - source/MaterialXGenHw/HwImplementation.h | 1 - .../Nodes/HwGeomPropValueNode.cpp | 78 ++++++++++++++-- source/MaterialXGenShader/GenContext.h | 45 +++++++++ .../MaterialXGenShader/Nodes/CompoundNode.cpp | 7 +- source/MaterialXRender/CgltfLoader.cpp | 1 + source/MaterialXRender/Mesh.h | 23 ++++- source/MaterialXRender/TinyObjLoader.cpp | 1 + .../MaterialXGenShader/GenShader.cpp | 91 +++++++++++++++++++ 10 files changed, 256 insertions(+), 13 deletions(-) diff --git a/resources/Materials/Examples/UsdPreviewSurface/usd_preview_surface_brass_tiled.mtlx b/resources/Materials/Examples/UsdPreviewSurface/usd_preview_surface_brass_tiled.mtlx index 85f6f3e139..a250136dab 100644 --- a/resources/Materials/Examples/UsdPreviewSurface/usd_preview_surface_brass_tiled.mtlx +++ b/resources/Materials/Examples/UsdPreviewSurface/usd_preview_surface_brass_tiled.mtlx @@ -1,5 +1,26 @@ + + + + + + + + + + + + + + + + + + + + + diff --git a/source/MaterialXGenHw/HwImplementation.cpp b/source/MaterialXGenHw/HwImplementation.cpp index 8c572eb0b5..830f55eaa2 100644 --- a/source/MaterialXGenHw/HwImplementation.cpp +++ b/source/MaterialXGenHw/HwImplementation.cpp @@ -10,7 +10,6 @@ MATERIALX_NAMESPACE_BEGIN const string HwImplementation::SPACE = "space"; const string HwImplementation::INDEX = "index"; -const string HwImplementation::GEOMPROP = "geomprop"; namespace { diff --git a/source/MaterialXGenHw/HwImplementation.h b/source/MaterialXGenHw/HwImplementation.h index 66c20f7c18..802fd1e355 100644 --- a/source/MaterialXGenHw/HwImplementation.h +++ b/source/MaterialXGenHw/HwImplementation.h @@ -37,7 +37,6 @@ class MX_GENHW_API HwImplementation : public ShaderNodeImpl /// Internal string constants static const string SPACE; static const string INDEX; - static const string GEOMPROP; }; MATERIALX_NAMESPACE_END diff --git a/source/MaterialXGenHw/Nodes/HwGeomPropValueNode.cpp b/source/MaterialXGenHw/Nodes/HwGeomPropValueNode.cpp index 9fb78fe7d5..4bd11647de 100644 --- a/source/MaterialXGenHw/Nodes/HwGeomPropValueNode.cpp +++ b/source/MaterialXGenHw/Nodes/HwGeomPropValueNode.cpp @@ -13,17 +13,77 @@ MATERIALX_NAMESPACE_BEGIN +static const string GEOMPROP = "geomprop"; + ShaderNodeImplPtr HwGeomPropValueNode::create() { return std::make_shared(); } -void HwGeomPropValueNode::createVariables(const ShaderNode& node, GenContext&, Shader& shader) const +// Find the ShaderInput that contains the actual name of the geomprop to read +static const ShaderInput* getGeomPropValueInput(const ShaderNode& node, const GenContext& context) { + // We should always have the "geomprop" input locally on this node const ShaderInput* geomPropInput = node.getInput(GEOMPROP); + if (!geomPropInput) + { + throw ExceptionShaderGenError("No 'geomprop' input found on geompropvalue node '" + node.getName() + "'."); + } + + // Depth into the compound instance stack, measured from the innermost + // instance. Incremented each time we cross a graph boundary so that nested + // compounds resolve to the correct enclosing instance. + size_t depth = 0; + + while (geomPropInput) + { + // If it has a local value set, then we've found the ShaderInput and can return that + ConstValuePtr value = geomPropInput->getValue(); + if (value) + { + return geomPropInput; + } + + // If we don't have a local value, then we may have an incoming connection from a containing NodeGraph + // Note : the "geomprop" input on the base node is "uniform" so it cannot be driven by the output from + // another node. + const ShaderOutput* upstreamPort = geomPropInput->getConnection(); + if (!upstreamPort) + { + return nullptr; + } + + const ShaderNode* upstreamNode = upstreamPort->getNode(); + if (!upstreamNode || !upstreamNode->isAGraph()) + { + return nullptr; + } + + // We've reached a graph input socket. Step out to the matching compound + // instance, walking the instance stack outward in tandem with the graph + // boundary we just crossed, so that nested compounds resolve to the + // correct instance rather than always the innermost one. + const ShaderNode* compoundInstance = context.getCompoundInstanceNode(depth++); + if (!compoundInstance) + { + return nullptr; + } + + // Consider the input on the compound instance to be the new potential + // source for the value of "geomprop". The next iteration either finds a + // value here or continues climbing. + geomPropInput = compoundInstance->getInput(upstreamPort->getName()); + } + + return nullptr; +} + +void HwGeomPropValueNode::createVariables(const ShaderNode& node, GenContext& context, Shader& shader) const +{ + const ShaderInput* geomPropInput = getGeomPropValueInput(node, context); if (!geomPropInput || !geomPropInput->getValue()) { - throw ExceptionShaderGenError("No 'geomprop' parameter found on geompropvalue node '" + node.getName() + "'. Don't know what property to bind"); + throw ExceptionShaderGenError("No 'geomprop' value found for geompropvalue node '" + node.getName() + "'. Don't know what property to bind"); } const string geomProp = geomPropInput->getValue()->getValueString(); const ShaderOutput* output = node.getOutput(); @@ -39,10 +99,10 @@ void HwGeomPropValueNode::emitFunctionCall(const ShaderNode& node, GenContext& c { const HwShaderGenerator& shadergen = static_cast(context.getShaderGenerator()); - const ShaderInput* geomPropInput = node.getInput(GEOMPROP); + const ShaderInput* geomPropInput = getGeomPropValueInput(node, context); if (!geomPropInput) { - throw ExceptionShaderGenError("No 'geomprop' parameter found on geompropvalue node '" + node.getName() + "'. Don't know what property to bind"); + throw ExceptionShaderGenError("No 'geomprop' value found for geompropvalue node '" + node.getName() + "'. Don't know what property to bind"); } const string geomname = geomPropInput->getValue()->getValueString(); const string variable = HW::T_IN_GEOMPROP + "_" + geomname; @@ -76,12 +136,12 @@ ShaderNodeImplPtr HwGeomPropValueNodeAsUniform::create() return std::make_shared(); } -void HwGeomPropValueNodeAsUniform::createVariables(const ShaderNode& node, GenContext&, Shader& shader) const +void HwGeomPropValueNodeAsUniform::createVariables(const ShaderNode& node, GenContext& context, Shader& shader) const { - const ShaderInput* geomPropInput = node.getInput(GEOMPROP); + const ShaderInput* geomPropInput = getGeomPropValueInput(node, context); if (!geomPropInput || !geomPropInput->getValue()) { - throw ExceptionShaderGenError("No 'geomprop' parameter found on geompropvalue node '" + node.getName() + "'. Don't know what property to bind"); + throw ExceptionShaderGenError("No 'geomprop' value found for geompropvalue node '" + node.getName() + "'. Don't know what property to bind"); } const string geomProp = geomPropInput->getValue()->getValueString(); ShaderStage& ps = shader.getStage(Stage::PIXEL); @@ -94,10 +154,10 @@ void HwGeomPropValueNodeAsUniform::emitFunctionCall(const ShaderNode& node, GenC DEFINE_SHADER_STAGE(stage, Stage::PIXEL) { const ShaderGenerator& shadergen = context.getShaderGenerator(); - const ShaderInput* geomPropInput = node.getInput(GEOMPROP); + const ShaderInput* geomPropInput = getGeomPropValueInput(node, context); if (!geomPropInput) { - throw ExceptionShaderGenError("No 'geomprop' parameter found on geompropvalue node '" + node.getName() + "'. Don't know what property to bind"); + throw ExceptionShaderGenError("No 'geomprop' value found for geompropvalue node '" + node.getName() + "'. Don't know what property to bind"); } const string attrName = geomPropInput->getValue()->getValueString(); shadergen.emitLineBegin(stage); diff --git a/source/MaterialXGenShader/GenContext.h b/source/MaterialXGenShader/GenContext.h index 0e5e87804b..d8061ece99 100644 --- a/source/MaterialXGenShader/GenContext.h +++ b/source/MaterialXGenShader/GenContext.h @@ -127,6 +127,27 @@ class MX_GENSHADER_API GenContext return _parentNodes; } + /// Push a compound instance node onto the stack. + void pushCompoundInstanceNode(const ShaderNode* node) + { + _compoundInstanceNodes.push_back(node); + } + + /// Pop the current compound instance node from the stack. + void popCompoundInstanceNode() + { + _compoundInstanceNodes.pop_back(); + } + + /// Return the compound instance node at the given depth from the + /// innermost instance (depth 0), or nullptr if the depth is out of range. + const ShaderNode* getCompoundInstanceNode(size_t depth = 0) const + { + return depth < _compoundInstanceNodes.size() + ? _compoundInstanceNodes[_compoundInstanceNodes.size() - 1 - depth] + : nullptr; + } + /// Add user data to the context to make it /// available during shader generator. void pushUserData(const string& name, GenUserDataPtr data) @@ -218,6 +239,7 @@ class MX_GENSHADER_API GenContext std::unordered_map _outputSuffix; vector _parentNodes; + vector _compoundInstanceNodes; ApplicationVariableHandler _applicationVariableHandler; }; @@ -237,6 +259,29 @@ class MX_GENSHADER_API ScopedSetVariableName string _oldName; }; +/// A RAII class for tracking the active compound instance node on the +/// GenContext stack, ensuring the node is popped even if shader generation +/// throws while the compound is being processed. +class MX_GENSHADER_API ScopedSetCompoundInstanceNode +{ + public: + /// Constructor pushing a compound instance node onto the context stack. + ScopedSetCompoundInstanceNode(GenContext& context, const ShaderNode* node) : + _context(context) + { + _context.pushCompoundInstanceNode(node); + } + + /// Destructor popping the compound instance node from the context stack. + ~ScopedSetCompoundInstanceNode() + { + _context.popCompoundInstanceNode(); + } + + private: + GenContext& _context; +}; + MATERIALX_NAMESPACE_END #endif // MATERIALX_GENCONTEXT_H diff --git a/source/MaterialXGenShader/Nodes/CompoundNode.cpp b/source/MaterialXGenShader/Nodes/CompoundNode.cpp index f25dbbdf7b..a9ab601818 100644 --- a/source/MaterialXGenShader/Nodes/CompoundNode.cpp +++ b/source/MaterialXGenShader/Nodes/CompoundNode.cpp @@ -56,8 +56,10 @@ void CompoundNode::initialize(const InterfaceElement& element, GenContext& conte _hash = std::hash{}(_functionName); } -void CompoundNode::createVariables(const ShaderNode&, GenContext& context, Shader& shader) const +void CompoundNode::createVariables(const ShaderNode& node, GenContext& context, Shader& shader) const { + ScopedSetCompoundInstanceNode scopedInstance(context, &node); + // Gather shader inputs from all child nodes for (const ShaderNode* childNode : _rootGraph->getNodes()) { @@ -74,6 +76,8 @@ void CompoundNode::emitFunctionDefinition(const ShaderNode& node, GenContext& co { const ShaderGenerator& shadergen = context.getShaderGenerator(); + ScopedSetCompoundInstanceNode scopedInstance(context, &node); + // Emit functions for all child nodes shadergen.emitFunctionDefinitions(*_rootGraph, context, stage); @@ -164,6 +168,7 @@ void CompoundNode::emitFunctionCall(const ShaderNode& node, GenContext& context, DEFINE_SHADER_STAGE(stage, Stage::VERTEX) { // Emit function calls for all child nodes to the vertex shader stage + ScopedSetCompoundInstanceNode scopedInstance(context, &node); shadergen.emitFunctionCalls(*_rootGraph, context, stage); } diff --git a/source/MaterialXRender/CgltfLoader.cpp b/source/MaterialXRender/CgltfLoader.cpp index 062c811b66..6a733ea229 100644 --- a/source/MaterialXRender/CgltfLoader.cpp +++ b/source/MaterialXRender/CgltfLoader.cpp @@ -491,6 +491,7 @@ bool CgltfLoader::load(const FilePath& filePath, MeshList& meshList, bool texcoo texcoordStream = mesh->generateTextureCoordinates(positionStream); mesh->addStream(texcoordStream); } + mesh->addStreamAlias("i_" + MeshStream::GEOMETRY_PROPERTY_ATTRIBUTE + "_st", texcoordStream->getName()); if (!vec4TangentStream) { MeshStreamPtr tangentStream = mesh->generateTangents(positionStream, normalStream, texcoordStream); diff --git a/source/MaterialXRender/Mesh.h b/source/MaterialXRender/Mesh.h index e0fa3d7001..9ab6c01293 100644 --- a/source/MaterialXRender/Mesh.h +++ b/source/MaterialXRender/Mesh.h @@ -273,7 +273,8 @@ class MX_RENDER_API Mesh return _sourceUri; } - /// Get a mesh stream by name + /// Get a mesh stream by name. If no stream matches directly, the name is + /// resolved through any registered stream aliases. /// @param name Name of stream /// @return Reference to a mesh stream if found MeshStreamPtr getStream(const string& name) const @@ -285,9 +286,28 @@ class MX_RENDER_API Mesh return stream; } } + auto it = _streamAliases.find(name); + if (it != _streamAliases.end()) + { + for (const auto& stream : _streams) + { + if (stream->getName() == it->second) + { + return stream; + } + } + } return MeshStreamPtr(); } + /// Register an alias so that getStream(alias) resolves to the stream + /// named target. Used to expose a single underlying stream under multiple + /// shader-input names (e.g. mapping the USD "st" primvar to UV0). + void addStreamAlias(const string& alias, const string& target) + { + _streamAliases[alias] = target; + } + /// Get a mesh stream by type and index /// @param type Type of stream /// @param index Index of stream @@ -440,6 +460,7 @@ class MX_RENDER_API Mesh float _sphereRadius; MeshStreamList _streams; + std::unordered_map _streamAliases; size_t _vertexCount; vector _partitions; }; diff --git a/source/MaterialXRender/TinyObjLoader.cpp b/source/MaterialXRender/TinyObjLoader.cpp index b01af1ebf9..2310b05727 100644 --- a/source/MaterialXRender/TinyObjLoader.cpp +++ b/source/MaterialXRender/TinyObjLoader.cpp @@ -174,6 +174,7 @@ bool TinyObjLoader::load(const FilePath& filePath, MeshList& meshList, bool texc texcoordStream = mesh->generateTextureCoordinates(positionStream); } mesh->addStream(texcoordStream); + mesh->addStreamAlias("i_" + MeshStream::GEOMETRY_PROPERTY_ATTRIBUTE + "_st", texcoordStream->getName()); MeshStreamPtr tangentStream = mesh->generateTangents(positionStream, normalStream, texcoordStream); if (tangentStream) diff --git a/source/MaterialXTest/MaterialXGenShader/GenShader.cpp b/source/MaterialXTest/MaterialXGenShader/GenShader.cpp index c4381ba7f8..bec76de032 100644 --- a/source/MaterialXTest/MaterialXGenShader/GenShader.cpp +++ b/source/MaterialXTest/MaterialXGenShader/GenShader.cpp @@ -452,3 +452,94 @@ TEST_CASE("GenShader: Track Application Variables", "[genshader]") } #endif } + +// A geompropvalue node wrapped inside a nodegraph (e.g. UsdPrimvarReader) receives +// the name of the geometric property to read through the graph interface rather than +// as a local value. This verifies that hardware shader generation resolves that name +// across more than one level of nested compounds, so the geometric stream is bound. +TEST_CASE("GenShader: Nested geompropvalue interface", "[genshader]") +{ + // "outer_primvar" wraps "inner_primvar", which wraps a geompropvalue node. The + // "geomprop" name is threaded down through both graph interfaces from the top + // level value "st", so resolution must climb two compound instances to find it. + std::string testDocumentString = + " \ + \ + \ + \ + \ + \ + \ + \ + \ + \ + \ + \ + \ + \ + \ + \ + \ + \ + \ + \ + \ + \ + \ + \ + \ + \ + \ + \ + "; + + mx::FileSearchPath searchPath = mx::getDefaultDataSearchPath(); + mx::DocumentPtr libraries = mx::createDocument(); + mx::loadLibraries({ "libraries" }, searchPath, libraries); + + mx::DocumentPtr testDoc = mx::createDocument(); + mx::readFromXmlString(testDoc, testDocumentString); + testDoc->setDataLibrary(libraries); + + mx::NodeGraphPtr graph = testDoc->getNodeGraph("NG_main"); + REQUIRE(graph); + mx::OutputPtr output = graph->getOutput("out"); + REQUIRE(output); + + // The geomprop name "st" resolves through both graph interfaces to the bound + // vertex attribute "i_geomprop_st" only when nested compounds are climbed in + // tandem with the instance stack. + const std::string expectedAttribute = mx::HW::IN_GEOMPROP + "_st"; + + // Only the hardware backends use HwGeomPropValueNode; OSL and MDL do not. +#ifdef MATERIALX_BUILD_GEN_GLSL + { + mx::GenContext context(mx::GlslShaderGenerator::create()); + context.registerSourceCodeSearchPath(searchPath); + mx::ShaderPtr shader = context.getShaderGenerator().generate("NestedPrimvar", output, context); + REQUIRE(shader); + const std::string source = shader->getSourceCode(mx::Stage::VERTEX) + shader->getSourceCode(mx::Stage::PIXEL); + CHECK(source.find(expectedAttribute) != std::string::npos); + } +#endif +#ifdef MATERIALX_BUILD_GEN_MSL + { + mx::GenContext context(mx::MslShaderGenerator::create()); + context.registerSourceCodeSearchPath(searchPath); + mx::ShaderPtr shader = context.getShaderGenerator().generate("NestedPrimvar", output, context); + REQUIRE(shader); + const std::string source = shader->getSourceCode(mx::Stage::VERTEX) + shader->getSourceCode(mx::Stage::PIXEL); + CHECK(source.find(expectedAttribute) != std::string::npos); + } +#endif +#ifdef MATERIALX_BUILD_GEN_SLANG + { + mx::GenContext context(mx::SlangShaderGenerator::create()); + context.registerSourceCodeSearchPath(searchPath); + mx::ShaderPtr shader = context.getShaderGenerator().generate("NestedPrimvar", output, context); + REQUIRE(shader); + const std::string source = shader->getSourceCode(mx::Stage::VERTEX) + shader->getSourceCode(mx::Stage::PIXEL); + CHECK(source.find(expectedAttribute) != std::string::npos); + } +#endif +} From 272ef02569b2447f8a38ffadc2ff1663ac369a92 Mon Sep 17 00:00:00 2001 From: Lee Kerley Date: Wed, 3 Jun 2026 15:53:46 -0700 Subject: [PATCH 2/2] moving scoped guard --- source/MaterialXGenShader/Nodes/CompoundNode.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/MaterialXGenShader/Nodes/CompoundNode.cpp b/source/MaterialXGenShader/Nodes/CompoundNode.cpp index a9ab601818..68a8f9a97e 100644 --- a/source/MaterialXGenShader/Nodes/CompoundNode.cpp +++ b/source/MaterialXGenShader/Nodes/CompoundNode.cpp @@ -164,11 +164,11 @@ void CompoundNode::emitFunctionCall(const ShaderNode& node, GenContext& context, MX_TRACE_SCOPE(Tracing::Category::ShaderGen, _functionName.c_str()); const ShaderGenerator& shadergen = context.getShaderGenerator(); + ScopedSetCompoundInstanceNode scopedInstance(context, &node); DEFINE_SHADER_STAGE(stage, Stage::VERTEX) { // Emit function calls for all child nodes to the vertex shader stage - ScopedSetCompoundInstanceNode scopedInstance(context, &node); shadergen.emitFunctionCalls(*_rootGraph, context, stage); }