Align MaterialX to ASWF Color Interop Forum recommendation - #3042
jstone-lucasfilm merged 7 commits into
Conversation
Signed-off-by: Doug Walker <doug.walker@autodesk.com>
| <?xml version="1.0"?> | ||
| <!-- | ||
| Test that a colorspace on an image and on a color4 value work. | ||
| --> |
There was a problem hiding this comment.
This is actually a trimmed-down version of color_management.mtlx. I did a git mv on that file, so I'm surprised this is showing as a new file rather than as a rename of the existing one. The original "color_management" name was effectively meaningless and so I wanted to make it more precise.
I trimmed the test down to just a few values since the new all_colorspace_names.mtlx test is where we now test that all color space names are supported.
Signed-off-by: Doug Walker <doug.walker@autodesk.com>
Signed-off-by: Doug Walker <doug.walker@autodesk.com>
Signed-off-by: Doug Walker <doug.walker@autodesk.com>
Signed-off-by: Doug Walker <doug.walker@autodesk.com>
|
@jstone-lucasfilm, I've made the changes to the nodedefs we discussed in the Nanocolor meeting. I updated the PR description accordingly. |
|
Thanks for this substantial contribution, @doug-walker! I'll defer to you on the correctness of the new matrices, and it's very encouraging that you've validated that the values in From an initial review, I see two potentially important issues to address: The first is the default for The second is the potential cost of OCIO lookups. A few smaller notes:
On your open questions: I agree that the Finally, this work interacts with #3005, which adds a Overall this is an excellent proposal, and let's aim to include it in our v1.39.6 release. |
Signed-off-by: Doug Walker <doug.walker@autodesk.com>
|
Thank you for the careful review @jstone-lucasfilm! Totally agree with your findings, and I think they should all be addressed now. I will plan a separate PR to update names in the example materials, as you suggested. I made a comment in #3005, I hope it was what you had in mind, please let me know if not. |
jstone-lucasfilm
left a comment
There was a problem hiding this comment.
Thanks so much for this contribution, @doug-walker, and this looks good to me!
d23766b
into
AcademySoftwareFoundation:main
This changelist follows up on the color interop alignment in AcademySoftwareFoundation#3042, resolving a regression for documents that use legacy color space names such as `lin_rec709`. With shader generation now targeting `lin_rec709_scene`, every color input in a legacy document was seen as requiring a transform, and the default color management system expressed the equivalence of these names as a pass-through `dot` node rather than omitting the transform. This added a redundant node and renamed uniform for each color input, and in MDL it dropped the uniform qualifier from inputs such as glTF PBR `attenuation_color`, producing shaders that fail to compile. The following specific changes are included: - Add a virtual `ColorManagementSystem::isNoOpTransform` method, with a `DefaultColorManagementSystem` override that recognizes legacy and color interop names for the same color space, and consult it in `ShaderGraph` so that equivalent color spaces introduce no transform nodes. - Preserve the uniform flag of an input when inserting a color or unit transform node, so that MDL declares the published value correctly. - Update `TextureBaker` and the Metal render test to the `lin_rec709_scene` working space, with baked documents now written using color interop names. - Expose `isNoOpTransform` in Python, with unit tests covering the alias logic and the generated GLSL and MDL shaders.
This changelist follows up on the color interop alignment in AcademySoftwareFoundation#3042, resolving a regression for documents that use legacy color space names such as `lin_rec709`. With shader generation now targeting `lin_rec709_scene`, every color input in a legacy document was seen as requiring a transform, and the default color management system expressed the equivalence of these names as a pass-through `dot` node rather than omitting the transform. This added a redundant node and renamed uniform for each color input, and in MDL it dropped the uniform qualifier from inputs such as glTF PBR `attenuation_color`, producing shaders that fail to compile. The following specific changes are included: - Add a virtual `ColorManagementSystem::isNoOpTransform` method, with a `DefaultColorManagementSystem` override that recognizes legacy and color interop names for the same color space, and consult it in `ShaderGraph` so that equivalent color spaces introduce no transform nodes. - Preserve the uniform flag of an input when inserting a color or unit transform node, so that MDL declares the published value correctly. - Update `TextureBaker` and the Metal render test to the `lin_rec709_scene` working space, with baked documents now written using color interop names. - Expose `isNoOpTransform` in Python, with unit tests covering the alias logic and the generated GLSL and MDL shaders.
Following up on review notes from @doug-walker and @meshula, this changelist consolidates the `isNoOpColorSpace` and `isNoOpTransform` methods of `ColorManagementSystem` into a single `isNoOpTransform` query, and clarifies the comments and string literals of the new unit test. Both methods were introduced in the color interop alignment of AcademySoftwareFoundation#3042 and have not yet shipped in a MaterialX release, so a single query covering both equivalent color space names and no-op color spaces is the simpler abstraction for both `ShaderGraph` and color management system implementations. The following specific changes are included: - Fold the no-op color space check into `ColorManagementSystem::isNoOpTransform`, which now returns true for identical names or when either name is a reserved no-op color space, and remove `isNoOpColorSpace` from the base class and Python bindings. - Update the `DefaultColorManagementSystem` and `OcioColorManagementSystem` overrides to call their base implementations, with the OCIO override additionally recognizing color spaces flagged as data in the active config. - Consult `isNoOpTransform` in `DefaultColorManagementSystem::getNodeDef`, so that the pass-through node returned to direct callers of `supportsTransform` and `createNode` follows the same rule as `ShaderGraph`, which omits these transforms entirely. - Collapse the color space checks in `ShaderGraph::populateColorTransformMap` into a single `isNoOpTransform` query. - Return by value from the `remapColorSpace` helper, removing a reference that would dangle if a caller passed a temporary.
This changelist follows up on the color interop alignment in #3042, resolving a regression for documents that use legacy color space names such as `lin_rec709`. With shader generation now targeting `lin_rec709_scene`, every color input in a legacy document was seen as requiring a transform, and the default color management system expressed the equivalence of these names as a pass-through `dot` node rather than omitting the transform. This added a redundant node and renamed uniform for each color input, and in MDL it dropped the uniform qualifier from inputs such as glTF PBR `attenuation_color`, producing shaders that fail to compile. The following specific changes are included: - Add a virtual `ColorManagementSystem::isNoOpTransform` method, with a `DefaultColorManagementSystem` override that recognizes legacy and color interop names for the same color space, and consult it in `ShaderGraph` so that equivalent color spaces introduce no transform nodes. - Preserve the uniform flag of an input when inserting a color or unit transform node, so that MDL declares the published value correctly. - Update `TextureBaker` and the Metal render test to the `lin_rec709_scene` working space, with baked documents now written using color interop names. - Expose `isNoOpTransform` in Python, with unit tests covering the alias logic and the generated GLSL and MDL shaders.
As proposed at the August 11th TSC meeting, this PR brings to MaterialX full support for the ASWF Color Interop Forum recommendation Color Space Encodings for Texture Assets and CG Rendering that is used in other ASWF projects and OpenUSD. It supersedes PR #2514.
Support has been retained for all existing
colorspacenames andcmlibnodes.Per the discussion at the TSC meeting, this PR does not upgrade existing documents when reading them to use the new names, however, that is still recommended as a follow-on step. Converting to the new names would likely simplify loading of MaterialX documents into OpenUSD. As things stand with this PR, the earlier MtlX names persist into the USD representation of the material, where they won't be recognized.
PR contents:
ColorManagementSystem::isNoOpColorSpaceto manage what colorspaces are no-ops (includes unit test). Note: this will break ABI-compatibility with the current release.ColorManagementSystem::getUserFacingNameto convert the colorspace's color interop ID into a name suitable for use in a user interface.Notes:
Open questions:
transformcolornode which potentially could be useful in documents, unlike the existing cmlib nodes.