Consolidate ShaderGenerator::getImplementation#2604
Merged
jstone-lucasfilm merged 2 commits intoAcademySoftwareFoundation:mainfrom Oct 9, 2025
Merged
Conversation
…rent generators, just specializing where necessary
niklasharrysson
approved these changes
Oct 9, 2025
Contributor
niklasharrysson
left a comment
niklasharrysson
left a comment
There was a problem hiding this comment.
This looks like a great cleanup and reduction of duplicate code. Thanks @ld-kerley
jstone-lucasfilm
approved these changes
Oct 9, 2025
Member
jstone-lucasfilm
left a comment
jstone-lucasfilm
left a comment
There was a problem hiding this comment.
This looks fine to me as well, thanks @ld-kerley!
jstone-lucasfilm
changed the title
ShaderGenerator::getImplementation()ShaderGenerator::getImplementation
jstone-lucasfilm
merged commit 0fd40b1
into
AcademySoftwareFoundation:main
32 checks passed
jstone-lucasfilm
pushed a commit
that referenced
this pull request
Oct 31, 2025
In #2604 two new API calls for the `ShaderGenerator` class were added to allow customization of the creation of `ShaderNodeImplementation` objects for `NodeGraph` and `Implementation` elements respectively. I think the API is cleaner, and more robust if we pass the actual implementation objects, and not the associated `NodeDef`. In some cases these function have to re-acquire the implementation object from the `NodeDef`, but at the call sight we already have it. #2604 has not been included in a MaterialX release yet - so I don't believe we have any problems changing the API.
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.
While thinking about #2603 - and how to refactor to avoid storing the node definition name in the
ShaderNode, I noticed thatgetImplementation()is very similar, and in some cases identical across different shader generators.This PR consolidates the common implementation to the base
ShaderGeneratorclass, and introduces smaller specialization points where necessary. ThegetImplementation()function is leftvirtualfor now, as there may be downstream generators that are overriding it, but in a future breaking change we should probably consider changing that.The GLSL and MSL implementations were actually identical, so I moved that to the HwShaderGenerator common base class.
For #2603 - I think this would all the Imageworks folks to introduce their own specialized
SourceCodenode, without having to replicate a lot of code. This could store the relevant oso data during its initialization, and be queried when emitting the "source".Tagging @krohmerNV - as this touches some MDL code.