Skip to content

Add an option to toggle eliding constant nodes #2521

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add an option to toggle eliding constant nodes #2521

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
315c25b
Add an option to toggle eliding constant nodes
JGamache-autodesk Aug 13, 2025
db8c18d
Merge branch 'main' into gamaj/allow_keeping_constant_nodes_unelided
jstone-lucasfilm Aug 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions source/MaterialXGenShader/GenOptions.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ class MX_GENSHADER_API GenOptions
addUpstreamDependencies(true),
libraryPrefix("libraries"),
emitColorTransforms(true),
elideConstantNodes(true),
hwTransparency(false),
hwSpecularEnvironmentMethod(SPECULAR_ENVIRONMENT_FIS),
hwDirectionalAlbedoMethod(DIRECTIONAL_ALBEDO_ANALYTIC),
Expand Down Expand Up @@ -134,6 +135,9 @@ class MX_GENSHADER_API GenOptions
/// system is defined. Defaults to true.
bool emitColorTransforms;

/// Enable eliding constant nodes. Defaults to true.
bool elideConstantNodes;

/// Sets if transparency is needed or not for HW shaders.
/// If a surface shader has potential of being transparent
/// this must be set to true, otherwise no transparency
Expand Down
2 changes: 1 addition & 1 deletion source/MaterialXGenShader/ShaderNode.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@ ShaderNodePtr ShaderNode::create(const ShaderGraph* parent, const string& name,
newNode->_classification = Classification::VDF | Classification::CLOSURE;
}
// Second, check for specific nodes types
else if (nodeDef.getNodeString() == CONSTANT)
else if (nodeDef.getNodeString() == CONSTANT && context.getOptions().elideConstantNodes)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@JGamache-autodesk JGamache-autodesk Aug 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
newNode->_classification = Classification::TEXTURE | Classification::CONSTANT;
}
Expand Down
Loading