Resolve geompropvalue names through nested node graphs in HW codegen#2957
Open
ld-kerley wants to merge 2 commits into
Open
Resolve geompropvalue names through nested node graphs in HW codegen#2957ld-kerley wants to merge 2 commits into
ld-kerley wants to merge 2 commits into
Conversation
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) <noreply@anthropic.com>
5 tasks
pablode
reviewed
May 30, 2026
kwokcb
reviewed
Jun 4, 2026
kwokcb
left a comment
kwokcb
left a comment
Contributor
There was a problem hiding this comment.
The "_st" mapping feels a bit strange to me a prone to error my omission.
I'm not sure what is better but that's my only main concern with some minor
additional comments.
|
|
||
| MATERIALX_NAMESPACE_BEGIN | ||
|
|
||
| static const string GEOMPROP = "geomprop"; |
Contributor
There was a problem hiding this comment.
Think you can use GeomProp::CATEGORY
|
|
||
| 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) |
Contributor
There was a problem hiding this comment.
Niklas can speak better than I on threading but I don't think we should add any static methods in code generation. Maybe add this as a method on ShaderNode instead ?
| texcoordStream = mesh->generateTextureCoordinates(positionStream); | ||
| mesh->addStream(texcoordStream); | ||
| } | ||
| mesh->addStreamAlias("i_" + MeshStream::GEOMETRY_PROPERTY_ATTRIBUTE + "_st", texcoordStream->getName()); |
Contributor
There was a problem hiding this comment.
This assumes a assumption on both the code gen and geometry loader side to "know" about adding "_st" postfix alias. Would it be possible to reflect this mapping within code generation so renderers can pull the appropriate given name or alias name ?
| // "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 = | ||
| "<?xml version=\"1.0\"?> \ |
Contributor
There was a problem hiding this comment.
Would it be more maintainable to keep this as a test file.
| 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 |
Contributor
There was a problem hiding this comment.
Is there a more robust check that building GLSL ? I think this is currently required for related HW backends, but would it be better long term to insert a build time MATERIALX_BUILD_GEN_HW define ?
| // compounds resolve to the correct enclosing instance. | ||
| size_t depth = 0; | ||
|
|
||
| while (geomPropInput) |
Contributor
There was a problem hiding this comment.
Checking that this will work if this has to read through a functional nodegraph (i.e. the graph connection is a nodedef implementation). I think it does since shader gen embeds implementation graphs.
pixar-oss
pushed a commit
to PixarAnimationStudios/OpenUSD
that referenced
this pull request
Jul 14, 2026
This is built on top of a corresponding MaterialX PR, that fixes nested geomprop nodes: AcademySoftwareFoundation/MaterialX#2957 Rendering a UsdPreviewSurface network that reads a primvar through an ND_UsdPrimvarReader_* node (for example varname="st" feeding a UsdUVTexture) failed in Hydra Storm with: HdSt_ApplyMaterialXFilter -- Unable to create the Glslfx Shader. MxException: No "geomprop" value found for geompropvalue node "primvar". That crash exposed a few different issues this PR fixes, all in how hdMtlx/hdSt build the document and emit the glslfx. 1. Preserve the topology-affecting primvar name. The topological classification (_IsTopologicalShader) only recognized the native geompropvalue family, so a UsdPrimvarReader varname was stripped from the anonymized network. 2. Recognize primvar-reader nodes generically (hdMtlx). A node is treated as a primvar reader when its implementation nodegraph wraps a geompropvalue or geompropvalueuniform node. New helpers HdMtlxIsPrimvarReaderNodeDef, HdMtlxGetPrimvarNameInputName and HdMtlxGetPrimvarDefaultInputName discover this and the input names that carry the primvar name and fallback (read from the interfacename on the wrapped geompropvalue inputs, for example varname and fallback for UsdPrimvarReader). These drive three call sites that previously only understood the native geompropvalue node by category or by the hardcoded geomprop/default parameter names: - _IsTopologicalShader now treats any primvar-reader node as topological. - _AddMaterialXNode now registers primvar-reader nodes in hdPrimvarNodes so the primvar is declared and bound. - _UpdatePrimvarNodes now extracts the primvar name and fallback from the correct inputs instead of assuming geomprop/default. Detecting by implementation structure rather than by family name keeps this robust for any future node that wraps a geompropvalue. Over-preserving a parameter is always correctness-safe; the only cost is slightly reduced shader reuse for such nodes, matching how the native geompropvalue node is already handled. 3. Emit glslfx-compatible primvar types (hdSt/materialXShaderGen.cpp). The primvar attributes block of the generated glslfx took its type name from the target shader language syntax. On Metal that produced names like float2, which HioGlslfxConfig does not understand (it accepts float, double, int, vec2, vec3, vec4), so the primvar declaration was rejected, st fell back to a default, and texture coordinates were lost (surfaces rendered untextured). The attributes type is now mapped to a glslfx type independent of the target language. This is a Metal-specific defect that became reachable only once a primvar actually reached shader generation. Closes #4091 (Internal change: 2419116)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves the MaterialX part of #2900 - Note a OpenUSD PR is still necessary to fix things when running inside of OpenUSD.
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). HwGeomPropValueNode 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.
Also updated MaterialXRender to allow testing of UsdPrimvarReader for reading texture coordinates "st".
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.
Add a GenShader test covering two levels of nested geompropvalue interface resolution across GLSL/MSL/Slang.