-
Notifications
You must be signed in to change notification settings - Fork 352
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.
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
Initial implementation of structs in MaterialX #1831
base: main
Are you sure you want to change the base?
Initial implementation of structs in MaterialX #1831
Conversation
Tagging @jstone-lucasfilm @niklasharrysson for discussion - happy to jump on a call if thats easier too. |
@ld-kerley Great to see this work! I like this direction a lot, where structs can remain unfolded in generated code. Some feedback on your questions:
|
@niklasharrysson - thanks for the great feedback - I'll get working on some of those ideas. regarding the name clashing - I don't think the struct name clashes with a reserved word, instead it clashed with a parameter name in some of the the shader code. I had named my struct I don't know if we need to obfuscate the struct names in the generated shader code somehow - add some uuid suffix perhaps? |
Ah I see! Yes, obfuscating the struct type names with a suffix sounds like a good idea then. |
@niklasharrysson I hit a roadblock when adding a randomly generated suffix to the struct names in the generated shader code. It means the struct names are not know when the node definition author is writing the shader source, and they need to declare the type of the inputs to their functions in the shader source. We could opt for a static suffix. Or, we could just not suffix at all, and instead find a way to make the error messages more informative? The most recent push includes the |
@ld-kerley Aha, yes that's a roadblock indeed! :) A codegen added static suffix feels a bit awkward, since as you note, node authors need to be aware of this and append the suffix in all shader code using this data type. Perhaps it's better then to not add a suffix at all during codegen, and instead introduce a naming convention for custom data types, that authors must follow? For example, something like: Would be great to have better error messages if collisions do occur. I'm not sure how though, since it's the target language compiler that finds this. Sounds good to postpone the centralized initialize/deinitialize to another PR. |
@niklasharrysson I really like the Also you suggest using the semantic of the type, does that mean that this example from the current spec Or perhaps this suggestion infers a completely new XMLTag/MaterialX type?
Given this feature never worked before, it doesn't seem too difficult, from a backwards compatibility perspective, to pivot this way? Perhaps @dbsmythe or @jstone-lucasfilm have input here? |
@ld-kerley About validation of a naming convention, I'm not sure we need to do that, since even if we do there is no guarantee that this will help to resolve name conflicts. At least in theory, a node author could use the same name for a variable in the code added for a node implementation, even if both a prefix and suffix is used. So perhaps the best we can do is to provide guidelines for how to name you data types and identifiers. And if there is a conflict anyway, resulting in shader compile errors, it's up to the node author to resolve it. With this in mind perhaps it would be best to just stick with the As for the naming convention to use, the one I gave was just an example, and it would be great to get others feedback on this as well. |
@ld-kerley We've now merged development work on MaterialX 1.39 back to the |
4403f1e
to
b04f9ca
Compare
@jstone-lucasfilm - done - and also pushed my latest. @niklasharrysson - this includes the Logged an issue (#1850) for the common init/de-init for ShaderGeneration. |
9ca2b71
to
9ae8173
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is coming along nicely! I added some suggestions above.
Apart from those there seems to be some build errors caught by CI, which mainly looks like compile warnings that should be fixed.
resources/Materials/TestSuite/experimental/struct/struct_texcoord.mtlx
Outdated
Show resolved
Hide resolved
9ae8173
to
42b3e6d
Compare
I think all the tests are passing now and I addressed the @niklasharrysson notes above - moving the the example scenes to the test suite required a little re-working of the testing framework to add another libraries location to the search path. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes looks great to me. I only added a few extra comments above.
Thanks @ld-kerley!
This is a first pass at updating the specification to reflect the upcoming struct support in PR #1831. A lot of this is lifted from the old 1.38 specification pdf, as that was largely the inspiration for the implementation.
2d5287b
to
ec209d0
Compare
ec209d0
to
1fdcfa9
Compare
Addressing some notes from @jstone-lucasfilm - and also removing the unit testing changes - these will be added at a later date once this is merged. |
…eparate PR to make this easier to review now.
Signed-off-by: Jonathan Stone <[email protected]>
Signed-off-by: Jonathan Stone <[email protected]>
Signed-off-by: Jonathan Stone <[email protected]>
Signed-off-by: Jonathan Stone <[email protected]>
Signed-off-by: Jonathan Stone <[email protected]>
Signed-off-by: Jonathan Stone <[email protected]>
1378e96
to
9801db0
Compare
Signed-off-by: Jonathan Stone <[email protected]>
Signed-off-by: Jonathan Stone <[email protected]>
Signed-off-by: Jonathan Stone <[email protected]>
Signed-off-by: Jonathan Stone <[email protected]>
Signed-off-by: Jonathan Stone <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking really good, thanks @ld-kerley, and I had just a few additional notes on implementation details.
Signed-off-by: Jonathan Stone <[email protected]>
using StructTypeDescStorage = vector<StructTypeDesc>; | ||
StructTypeDescStorage& structTypeStorage() | ||
{ | ||
static StructTypeDescStorage storage; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I hadn't noticed this singleton pattern until now, and this seems like a design decision that we should discuss.
Would this break in situations where MaterialX is leveraged across multiple threads, with each thread performing shader generation on a different document with different usage of structs?
Traditionally we forbid singletons in MaterialX to avoid this class of multi-threading hazards, and if there's another engineering solution that stores this data in a thread-local object such as the GenContext
(which was explicitly designed to handle this category of storage), that would be strongly preferable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This use intentionally parallels the existing pattern for the TypeDesc system here
TypeDescMap& typeMap()
{
static TypeDescMap map;
return map;
}
If we want to refactor the type system to use storage in GenContext
I would propose that we do that as a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, and it sounds we have some existing threading issues to address in our shader generation, independent of this proposal. I'll close out this request for now, but I'm CC'ing @niklasharrysson so that he's in the loop.
This is a follow on from the previous draft PR #1684 on implementing structs.
This implementation maintains the structs in the shader code.
This is still very much a draft and needs a lot of cleanup, but posting so we can discuss if this direction is worth pursuing.
Looking for feedback on :
compoundValue
subclass of theValue
type?texcoord
which is a variable name somewhere in some generated code.loadStructTypeDefs
, having to add it in many places to get everything to work feels a little clunky/error prone, but perhaps this sort of refactor its beyond the scope of this work.