Initial implementation of structs in MaterialX

[ld-kerley]

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 :

• Value tokenization, currently it happens during shader generation, but perhaps there a cleaner place to put it in the document parsing, creating some sort of compoundValue subclass of the Value type?
• What to do if a struct name clashes with a variable name in the generated shader code, this happened in my testing when I had named my new struct texcoord which is a variable name somewhere in some generated code.
• If we need to find a more elegant way to call 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.
• Do we need/want to support recursively nested structs? I don't believe the code as it's written will do this, but I think it would be possible.

[ld-kerley]

Tagging @jstone-lucasfilm @niklasharrysson for discussion - happy to jump on a call if thats easier too.

[niklasharrysson]

@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:

• About value tokenization: I think it would be great to introduce a CompoundValue (or AggregateValue) class, as a subclass of Value, in order to do the value parsing in one central place, instead of having to deal with the valuestring parsing in many places. This would also let us validate struct values during document validation, and not only during code generation.

• For name clashes I'm surprised this doesn't work out of the box. There is a call to registerReservedWords inside registerTypeSyntax, that should handle this. So try tracing from there and see why the struct name is not registered as a reserved word. https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/source/MaterialXGenShader/Syntax.cpp#L55

• For loading in struct types from the document, perhaps we could add initialize/deinitialize methods to ShaderGenerator, to handle such things in a central place. We already have other similar methods that need to be called to initialize generation, for example the registerShaderMetadata call. So all such calls could be collected into a single init method.

• For nested structs, it looks like the approach you have take should be able to support this. So that would be nice. That's one of the pros of not unfolding the structs, as that would have been more messy I think.

[ld-kerley]

@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 texcoord, in my example, and I was getting shader compile error because of the GLSL code included from this file. the mx_image_float() takes a vec2 texcoord parameter.

I don't know if we need to obfuscate the struct names in the generated shader code somehow - add some uuid suffix perhaps?

[niklasharrysson]

Ah I see! Yes, obfuscating the struct type names with a suffix sounds like a good idea then.

[ld-kerley]

@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. texcoord -> texcoord_typedef, or something, and then node authors would be able to use the less obvious texcoord_typedef type name in their shader code.

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 AggregateValue update, and I was considering postponing the centralized initialize/deinitialize refactor for the ShaderGenerator to a separate PR, just to keep this one manageable.

[niklasharrysson]

@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: A custom type name should have a suffix that declare the semantic of the type. Example: texcoord_struct In addition, if the type is declared outside of stdlib, it should have a namespace prefix to minimize name collisions. Example: pxr_texcoord_struct, maya_light_struct, etc.

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.

[ld-kerley]

@niklasharrysson I really like the _struct suffix idea. We can add a step in the document validation to check for it. Do we want to block a render, ie. fail code gen, if we don't have the suffix? I can imagine someone could very well get lucky and not clash. Do we have other examples (I think we do because I've hit them) where a document could be invalid, but still render exactly as intended?

Also you suggest using the semantic of the type, does that mean that this example from the current spec <typedef name="spectrum" semantic="color"/> should be re-written to <typedef name="spectrum_color" semantic="color"/> ?

Or perhaps this suggestion infers a completely new XMLTag/MaterialX type? <structdef> ? and we would leave <typedef> alone as its written in the new spec (with no <member> child tag). That way the new concept can more cleanly have its own set of rules?

<structdef name="texcoord_struct">
  <member name="ss" type="float" value="0.5"/>
  <member name="tt" type="float" value="0.5"/>
</structdef>

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?

[niklasharrysson]

@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 typedef element, even for structs. It feels more consistent since after all it's a type you're defining.

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.

[jstone-lucasfilm]

@ld-kerley We've now merged development work on MaterialX 1.39 back to the main branch of MaterialX, in preparation for the final weeks of development on the 1.39.0 release. When you have a chance, could you retarget this pull request back to the main branch as well?

[ld-kerley]

@jstone-lucasfilm - done - and also pushed my latest.

@niklasharrysson - this includes the _struct suffix, to the example only - no validation. And also now should allow for structs-of-structs - including in the GLSL/MSL rendering path.

Logged an issue (#1850) for the common init/de-init for ShaderGeneration.

[ld-kerley]

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.

[ld-kerley]

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.

[jstone-lucasfilm]

@ld-kerley This change looks ready to merge, but it would benefit from a new description, since it's no longer a draft seeking feedback from the team! Would you like to write up a new description before we merge this?