Refine dot node elision and remove unused code #1522
Conversation
| { | ||
| bypass(context, node, 0); | ||
| ++numEdits; | ||
| } | ||
| } | ||
| else if (node->hasClassification(ShaderNode::Classification::IFELSE)) |
There was a problem hiding this comment.
This used to target a node called "compare" which was removed in 1.37.
| ++numEdits; | ||
| } | ||
| } | ||
| else if (node->hasClassification(ShaderNode::Classification::SWITCH)) |
There was a problem hiding this comment.
This worked in 1.37 where the "which" input was a "parameter" instead of an "input". Since this input is not marked as "uniform", it should not be elided.
There was a problem hiding this comment.
I think this should be restored as it checks to see if indeed a uniform unconnected value. I have log an new issue and can discuss there, but now new shaders will always add more code in. (#1526)
| ShaderInput* in = node->getInput("in"); | ||
| if (in->getChannels().empty()) | ||
| if (in->getChannels().empty() && in->getType() == Type::FILENAME) |
There was a problem hiding this comment.
Still solves the issue with extra samplers, but preserves the distinction between "dot" nodes and "constant" nodes.
|
This looks very promising, thanks @JGamache-autodesk. Would it be reasonable to consider this proposal for after today's release of MaterialX 1.38.8, so that we have sufficient time to think through all of the potential edge cases? |
|
Hi @jstone-lucasfilm. I would consider the automatic elision of |
|
Don't worry about simplifying the changelist, @JGamache-autodesk, and let's start testing the proposal as it currently stands. Thoughts from @kwokcb would be appreciated as well! |
|
Dot nodes were elided in #1152 to prevent creating extra samplers in rasterized shadergen. |
jstone-lucasfilm
changed the title
| <tiledimage name="image_color" type="color3"> | ||
| <input name="file" type="filename" value="wood_color.jpg" colorspace="srgb_texture" /> | ||
| <input name="file" type="filename" nodename="color_filename" colorspace="srgb_texture" /> |
There was a problem hiding this comment.
Sufficient to trigger the issue fixed in #1152. This will not generate valid GLSL code unless dot elision is active for filenames.
There was a problem hiding this comment.
Thanks for the fix, @JGamache-autodesk! Once all tests have passed, I'll move forward and include this in the release.
jstone-lucasfilm
merged commit f69e4c2
into
AcademySoftwareFoundation:main
|
Just a comment -- after the fact now since this is merged. Since it is possible to not perform an upgrade on a document, Thus, I'm unsure what it means to remove support for older nodes. if someone loads in a document and tries to code-gen. Could be things will not work at all, but just wanted to add this note. |
| { | ||
| newNode->_classification = Classification::TEXTURE | Classification::CONDITIONAL | Classification::IFELSE; | ||
| } | ||
| else if (nodeDef.getNodeString() == SWITCH) |
There was a problem hiding this comment.
I'm not sure way it means to remove the texture classification here. I think nodes like try to perform filtered sample upstream will no longer work. But it could be that it does not work anyways.
The MaterialX topological node list was in need of an update:
For MaterialX 1.38.7:
"dot" nodes are now topological.
Via AcademySoftwareFoundation/MaterialX#1152
For upcoming MaterialX 1.38.8:
"dot" node no longer elided (except for filenames)
"switch" node no longer elided
Via AcademySoftwareFoundation/MaterialX#1522
…tion#1522) Dot nodes were elided in AcademySoftwareFoundation#1152 to prevent creating extra samplers in rasterized shadergen. This preserves the change written by @kwokcb while keeping the distinction between a topological constant node and a non-topological dot node. In scenarios where you want to feed a single color to multiple shader nodes in a complex material, using a constant node will force recompilation every time the color is tweaked, we want users to be able to use a dot node to feed the graph, which should allow tweaking the color without having to recompile the shader. Also allows re-using the shader code for material instances differing on this input color.
No description provided.