AcademySoftwareFoundation
Names defined in MaterialX documents displace library names and cause strange behavior
Slightly complex one here. One of our devs was asking a very interesting question while working with MaterialX:
Here’s another strange one about naming: Why won’t it let me name the node angle? It always appends 2. But it lets me name it test just fine. Are there some reserved names or something like that…?
After some pondering, it hit me that all the library definitions reside at the top level of the document, which means the unit def "angle" becomes the equivalent of a reserved keyword.
So, the next question was:
I can already see bugs created by this since the library is usually added after the material document gets loaded, so a node named "ND_standard_surface_surfaceshader" will most probably displace the NodeDef from the library, which will gain an extra digit at the end and causing quite a lot of mischief trying to find node definitions later.
And indeed you can create such documents in a text editor that will completely break MaterialX in strange ways.
Can I suggest restructuring the internal of mx::Document to have a child named "__library__" which would get all those definitions as children. Then we would end up with a single reserved keyword that would have little risk of colliding with user work.
This is a great observation, @JGamache-autodesk, and it's definitely worthwhile to fix this.
Although we could move imported libraries into their own document, this wouldn't necessarily resolve the overlap in namespaces between content documents and library documents, and references to angle would still be ambiguous.
Another option would be to leverage the existing namespace functionality in MaterialX to eliminate this overlap, e.g. assigning imported libraries to a namespace such as std by default. The user could always override this behavior when other namespaces were desired, but this ought to eliminate the namespace overlap between content documents and the standard libraries that are required for shader generation and rendering.
At first glance I like the idea of namespacing libraries.
Currently libraries are "flattened" and the namespace embedded as a prefix for the name (which can also cause name clashes) so maybe it could be as "easy" as adding a namespace to the "standard" libraries, It could even be per library: e.g. "std", "pbr", "bxdf" etc.
That said, I think it can get "tricky" if you start to namespace default type def, geom defs, unit defs etc. I think these really should be reserved keywords and always loaded in first.
Maybe as a short-term validation change if the imported libraries clash with an existing loaded name this should throw an exception as most likely something will break.
