Add an option to toggle eliding constant nodes - #2521
Conversation
Editing this node in a real time MaterialX editing software leads to constant shader recompilation because the value gets burned inside the shader text. On complex shading models, this has a noticeable perfomance impact. Add an option to keep these as regular nodes that do not require recompiling the shader.
| } | ||
| // Second, check for specific nodes types | ||
| else if (nodeDef.getNodeString() == CONSTANT) | ||
| else if (nodeDef.getNodeString() == CONSTANT && context.getOptions().elideConstantNodes) |
There was a problem hiding this comment.
Instead of checking the option here - I think it's clearer to apply the same logic inside ShaderGraph::optimize() when the constant nodes are actually being elided here.
I know we only use the CONSTANT classification for this at the moment, but if we ever used it for something else this would need to be refactored.
There was a problem hiding this comment.
I was going to say it was a matter of taste, but you are actually correct since non-elided ND_constant_filename nodes will become an issue in GLSL, which means we need to change the condition in ShaderGraph::optimize() from
else if (node->hasClassification(ShaderNode::Classification::DOT))to
else if (node->hasClassification(ShaderNode::Classification::DOT) ||
node->hasClassification(ShaderNode::Classification::CONSTANT))Mentioning this also opens another can of worms, because that is a vital elision only when the filename is replaced by a sampler, and only if the shading language does not allow passing samplers as function parameters. But that is a discussion for another time.
So my recommendation is to wait until I have completed @ld-kerley 's request and added proper elision at least for GLSL.
ld-kerley
left a comment
ld-kerley
left a comment
There was a problem hiding this comment.
Looks good to me - thanks @JGamache-autodesk
jstone-lucasfilm
left a comment
jstone-lucasfilm
left a comment
There was a problem hiding this comment.
This looks like a great proposal to me, @JGamache-autodesk, and I had just one note about mixed code styles that should be resolved.
| bypass(node, 0); | ||
| ++numEdits; | ||
| bool canElide = context.getOptions().elideConstantNodes; | ||
| if (!canElide) { |
There was a problem hiding this comment.
I see some mixing of code styles here, and make sure to align all of your new code to the Allman-style braces of MaterialX before we merge this.
44932c1
into
AcademySoftwareFoundation:main
3 participants
Editing this node in a real time MaterialX editing software leads to constant shader recompilation because the value gets burned inside the shader text. On complex shading models, this has a noticeable perfomance impact.
Add an option to keep these as regular nodes that do not require recompiling the shader.