Resolve geompropvalue names through nested node graphs in HW codegen#2957
Resolve geompropvalue names through nested node graphs in HW codegen#2957ld-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>
kwokcb
left a comment
kwokcb
left a comment
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"; |
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) |
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()); |
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\"?> \ |
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 |
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) |
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.
3 participants
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.