Align MaterialX to Color Interop recommendations - #2514
Closed
JGamache-autodesk wants to merge 6 commits into
Closed
JGamache-autodesk wants to merge 6 commits into
JGamache-autodesk wants to merge 6 commits into
Conversation
For more details, see the [Texture asset color space](https://github.com/AcademySoftwareFoundation/ColorInterop/blob/main/Recommendations/01_TextureAssetColorSpaces/TextureAssetColorSpaces.md) recommendations. - All names are now based on color interop short names - Deprecated names are now labeled as such - Unit tests still covers all names even older legacy ones - Introduced lin_ap0, lin_rec2020, and srgb_ap1 color spaces
Contributor
Author
| Declarations of the default color transforms in MaterialX. | ||
| --> | ||
|
|
||
| <!-- Functions that keep the same name in both 1.38 and color interop naming schemes --> |
Contributor
Author
There was a problem hiding this comment.
A lot of movement because I wanted to group NodeDefs by how they were affected by the change.
| <output name="out" type="color4" /> | ||
| </nodedef> | ||
|
|
||
| <nodedef name="ND_lin_ap0_to_lin_rec709_color3" node="lin_ap0_to_lin_rec709" nodegroup="colortransform"> |
Contributor
Author
There was a problem hiding this comment.
Introducing lin_ap0 and lin_rec2020
| <output name="out" type="color4" /> | ||
| </nodedef> | ||
|
|
||
| <!-- Note: not adding lin_ciexyzd65_scene --> |
Contributor
Author
There was a problem hiding this comment.
Can be added if requested.
| <!-- Functions introduced with color interop naming scheme --> | ||
|
|
||
| <implementation name="IMPL_lin_ap1_to_lin_rec709_color3" nodedef="ND_lin_ap1_to_lin_rec709_color3" nodegraph="NG_lin_ap1_to_lin_rec709_color3" /> | ||
| <implementation name="IMPL_acescg_to_lin_rec709_color3" nodedef="ND_acescg_to_lin_rec709_color3" nodegraph="NG_lin_ap1_to_lin_rec709_color3" /> |
Contributor
Author
There was a problem hiding this comment.
Backward compatibility with deprecated NodeDefs done via implementations.
| <input name="surfaceshader" type="surfaceshader" nodename="standard_surface1" /> | ||
| </surfacematerial> | ||
| <!-- acescg becomes lin_ap1 --> | ||
| <acescg_to_lin_rec709 name="deprecated13" type="color3"> |
Contributor
Author
There was a problem hiding this comment.
Testing every single deprecated node to make sure it can be referenced in older documents. Will double up as an upgrade test in 1.40. Should produce 2 spheres of uniform orange color.
| </convert> | ||
| <switch name="switch1" type="color4"> | ||
| <input name="in1" type="color4" value="0.5776, 0.1274, 0.0319, 1" /> | ||
| <input name="in2" type="color4" value="0.5776, 0.1274, 0.0319, 1" colorspace="lin_rec709" /> |
Contributor
Author
There was a problem hiding this comment.
Every single color space name from legacy gamma22 up to latest g22_adobergb_scene is tested in both color3 and color4 variants. Should produce 10 spheres of uniform orange color.
|
|
||
| if (sourceSpace == targetSpace) | ||
| { | ||
| return _document->getNodeDef("ND_dot_" + transform.type.getName()); |
Contributor
Author
There was a problem hiding this comment.
Discovered because we are testing every name, even lin_rec709 and lin_rec709_scene in the unit test.
Member
|
Thanks @JGamache-autodesk, and I'd agree this makes the most sense as a contribution for 1.39.5. |
doug-walker
reviewed
Aug 5, 2025
Contributor
There was a problem hiding this comment.
I confirm that the matrices for Rec.2020, AP0, AP1, and AdobeRGB are correct.
The matrix for P3 D65 is not new in this PR, but I checked that one too and got a slightly different value:
1.224940176281, -0.042056954710, -0.019637554590,
-0.224940176281, 1.042056954710, -0.078636045551,
0.000000000000, -0.000000000000, 1.098273600141
And small tweak to the p3d65 matrix.
…autodesk-forks/MaterialX into adsk/full_color_interop_support
jstone-lucasfilm
pushed a commit
that referenced
this pull request
Aug 28, 2026
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](https://github.com/AcademySoftwareFoundation/ColorInterop/blob/main/Recommendations/01_TextureAssetColorSpaces/TextureAssetColorSpaces.md) that is used in other ASWF projects and OpenUSD. It supersedes PR #2514. Support has been retained for all existing `colorspace` names and `cmlib` nodes. 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: - Adds color spaces and nodegraph implementations for "Linear Rec.2020", "ACES2065-1", "CIE XYZ-D65 - Scene-referred", and "sRGB Encoded AP1". - Since these are public-facing, existing stdlib nodedefs have been retained without changing their signature to the new name. The nodedefs for the new color spaces use the new names. - Improves the accuracy of the matrix converting P3 D65 to Rec.709 primaries. - Adds support for the Color Interop Forum "data" colorspace designation (equivalent to the earlier "none"). - Adds `ColorManagementSystem::isNoOpColorSpace` to manage what colorspaces are no-ops (includes unit test). Note: this will break ABI-compatibility with the current release. - Adds `ColorManagementSystem::getUserFacingName` to convert the colorspace's color interop ID into a name suitable for use in a user interface. - Updates all documentation. Clarified that the rendering space is not guaranteed to be the document's working color space. - Adds new mtlx test files in resources/Materials/TestSuite/stdlib/color_management. All colorspace names (new and old) are tested. I validated the resulting renders against OCIO's built-in CG config for ACES. - Fixes the texture mapping in existing color management tests so that renders may be properly evaluated. Previously, some parts of the tests were not visible in the image because of where they were mapped onto the sphere. - Changes usage of "lin_rec709" to "lin_rec709_scene" in various .cpp modules that call targetColorSpaceOverride.
Member
|
Thanks for this original PR, @JGamache-autodesk, and I believe we can now close it as superseded by #3042 from @doug-walker. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I would recommend we wait for 1.39.5, but if there is a consensus to be bold, I don't see why it could not sneak in 1.39.4.
For more details, see the Texture asset color space recommendations.