Support concurrent node definition in the same document#2883
Support concurrent node definition in the same document#2883AdrienHerubel wants to merge 1 commit into
Conversation
|
Can the requirement to have identical nodedef names with different versions be explained? Feels like this should not be required as the existing mechanisms will support multiple versions. I put this into Slack as reply to the question on identifiers ( For nodedef versioning, seems like what's in the PR breaks the unique name requirement for nodedefs (and any node in general) in the spec. Unsure why the same names are being used ? (*) -- with additional patching to try and auto create unique names, which is also not something defined in the spec, and looks like it could break USD shader registration. Custom Node Declaration NodeDef Elements Each custom node must be explicitly declared with a element, with child , and elements specifying the expected names and types of the node’s ports. (*) If this is to try and make things work with the existing shader translation logic, then I'd suggest that that logic be extended to support versions. As a further note, standard surface already has 2 versions and both can be instantiated without any addition logic changes. There is a USD/MTLX Slack thread with a note on this as well. |
| <!-- OpenPBR Surface v1.1.1 --> | ||
| <!-- ============================================================ --> | ||
|
|
||
| <nodedef name="ND_open_pbr_surface_surfaceshader" node="open_pbr_surface" nodegroup="pbr" version="1.1.1" isdefaultversion="false" |
There was a problem hiding this comment.
I think we just want to call this ND_open_pbr_surface_surfaceshader_111 or maybe ND_open_pbr_surface_surfaceshader_1_1_1
So we have unique node def names - as we were discussing at the TSC - my hope would be if you do that - then you wouldn't need any changes in MaterialXCore or MaterialXFormat at all - this really boils down to a new version of the node, and some code changes on the application side (MaterialXGraphEditor)
There was a problem hiding this comment.
I'd vote for that. If we follow what was done for std surface than it's ND_open_pbr_surface_surfaceshader_111, but not sure if this should be the recommended "convention" -- I don't recall the discussion anymore on why we chose this.
There was a problem hiding this comment.
I'll revisit the PR to use different nodedef names as discussed at the TSC. Though for backward compatibility with USD where we explicitly use a nodedef name and no nodedef versioning, should we not keep the current name for the 1.1.1 version to maintain the look ?
There was a problem hiding this comment.
Thats a really interesting point Adrien - and doesn't follow the existing MaterialX convention (although we do only have one data point in standard surface - as far as I'm aware).
I think its a good idea to leave the existing nodedef named as it was - and instead add ND_open_pbr_surface_surfaceshader_1_2_0 - I think I prefer using _ to replace the periods in the version number rather than 120 because it feels less ambiguous.
I think this is how Pixar version their shaders internally too, starting with Taco and then creating Taco_2.
Actually the UsdPreviewSurface texture node in MaterialX got a new version too - and it followed that pattern too.
<nodedef name="ND_UsdUVTexture" node="UsdUVTexture" nodegroup="texture2d" version="2.2" inherit="ND_UsdUVTexture_23">
and then
<nodedef name="ND_UsdUVTexture_23" node="UsdUVTexture" nodegroup="texture2d" version="2.3" isdefaultversion="true">
3 participants
This PR adds support for having nodes pointing at multiple versions of the same node definition using different node graphs in the same document. This also adds the draft version of OpenPBR 1.2 as a practical example.