Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PR: ACES 1.x Varnishing #132

Merged

Conversation

KelSolaar
Copy link
Contributor

This PR updates various aliases and names according to VWG discussions and also sets the Un-tone-mapped view transform first in the list of view transforms for OCIO profiles <=2.3.

Signed-off-by: Thomas Mansencal <thomas.mansencal@gmail.com>
…ew transforms list for *OpenColorIO* profile lesser or equal to 2.3.

Signed-off-by: Thomas Mansencal <thomas.mansencal@gmail.com>
…rop Forum* recommendations.

Signed-off-by: Thomas Mansencal <thomas.mansencal@gmail.com>
@KelSolaar
Copy link
Contributor Author

@doug-walker: This should also include the new names from #127.

@doug-walker
Copy link
Contributor

Thanks Thomas, I've reviewed the CG and Studio artifacts and will suggest the following changes:

  1. Please change "CIE-XYZ-D65 - Scene-referred" to "CIE XYZ-D65 - Scene-referred" (note the dash to space).

  2. Please add the following aliases:
    g18_rec709_scene
    g22_rec709_scene
    g22_ap1_scene
    lin_ciexyzd65_display
    cie_xyz_d65_display

  3. Please remove the following aliases:
    CIE-XYZ-D65 - Display-referred
    cie_xyz_d65_display_referred

  4. The alias "cie_xyz_d65" was in the previous config and removed from the new one. I think this is probably the correct thing to do since it is now ambiguous, but wanted to mention it in case others feel we need to continue to support it.

  5. The Un-tone-mapped View Transform moved for the earlier CG configs but not the Studio configs. If it's easier, you could make it always first regardless of version or config.

Signed-off-by: Thomas Mansencal <thomas.mansencal@gmail.com>
@KelSolaar
Copy link
Contributor Author

Should not be too far now! Sorry about 5., completely forgot to add the code to the Studio Config.

carolalynn
carolalynn previously approved these changes Sep 13, 2024
Copy link

@carolalynn carolalynn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are looking great, Thomas! I don't think I have any additional notes from what Doug mentioned.

@doug-walker
Copy link
Contributor

I'm wondering if the ordering of the Utility family could be improved. Currently it is this:

ITU/Camera Rec.709
Linear P3-D65
Linear Rec.2020
Linear Rec.709 (sRGB)
CIE XYZ-D65 - Scene-referred
Gamma 2.2 Encoded AdobeRGB
Gamma 2.2 Encoded AP1
Gamma 2.4 Encoded Rec.709
Linear AdobeRGB
sRGB Encoded AP1
sRGB Encoded P3-D65
Gamma 1.8 Encoded Rec.709
Gamma 2.2 Encoded Rec.709
sRGB Encoded Rec.709 (sRGB)
Raw
ST-2084 - Curve
Rec.1886 - Curve
ITU/Rec.709 - Curve
sRGB - Curve

Perhaps an ordering along these lines would be better? Any opinions?

sRGB Encoded Rec.709 (sRGB)
Gamma 1.8 Encoded Rec.709
Gamma 2.2 Encoded Rec.709
Gamma 2.4 Encoded Rec.709
ITU/Camera Rec.709
sRGB Encoded P3-D65
Gamma 2.2 Encoded AdobeRGB
sRGB Encoded AP1
Gamma 2.2 Encoded AP1
Linear Rec.709 (sRGB)
Linear P3-D65
Linear AdobeRGB
Linear Rec.2020
CIE XYZ-D65 - Scene-referred
Raw
sRGB - Curve
Rec.1886 - Curve
ITU/Rec.709 - Curve
ST-2084 - Curve

@doug-walker
Copy link
Contributor

Items 1-5 above have been addressed!

Here's a few other observations. I don't think they require action, but are probably worth noting.

  1. The versioning of the new configs is as follows:
    studio-config-v2.1.0_aces-v1.3_ocio-v2.1.ocio
    studio-config-v2.1.0_aces-v1.3_ocio-v2.2.ocio
    studio-config-v2.2.0_aces-v1.3_ocio-v2.3.ocio
    studio-config-v2.2.0_aces-v1.3_ocio-v2.4.ocio

Note that the color space version is sometimes 2.1.0 and others 2.2.0, even though the contents of all of these is basically the same in terms of the updated naming of existing color spaces. One could argue that the ones that are 2.1.0 are correct in the sense that the previous version of those configs was 2.0.0. Alternatively, one could argue that it's simpler for people if "color space version 2.1" always means basically the same thing, even if that means skipping a version. I have no objections to the current scheme, but wanted to mention it.

  1. In this latest drop, the "CIE-XYZ-D65" alias was removed. This was a color space name (not an alias) in the previous config releases. However, it was in the inactive_colorspaces list, and so it does seem reasonable that we could remove it without even keeping an alias. I have no objections since it has become too ambiguous, but wanted to mention it.

Signed-off-by: Thomas Mansencal <thomas.mansencal@gmail.com>
@KelSolaar
Copy link
Contributor Author

7. the "CIE-XYZ-D65" alias was removed

Yes, this was also done for disambiguation!

Colorspace(s) and NamedTransform(s) should be ordered as per-above suggestion!

Copy link
Contributor

@doug-walker doug-walker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the changes Thomas, these look great!

There is one more small naming issue currently being discussed, so I'm not sure if you prefer to merge this now or wait, but I'll approve now to give you the flexibility.

@KelSolaar
Copy link
Contributor Author

Let's wait for the naming issue resolution.

@KelSolaar
Copy link
Contributor Author

Merging for now because AcademySoftwareFoundation/OpenColorIO#2039 is up and ready!

@KelSolaar KelSolaar merged commit 9c764b4 into AcademySoftwareFoundation:main Sep 17, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants