-
Notifications
You must be signed in to change notification settings - Fork 41
PR: ACES 2.0 #130
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 2.0 #130
Conversation
94ddacb
to
c4f7ea4
Compare
c4f7ea4
to
9982dd2
Compare
c84d3e3
to
53dd1a0
Compare
First artifacts are here: https://github.com/AcademySoftwareFoundation/OpenColorIO-Config-ACES/actions/runs/10648206791 I haven't checked anything yet. |
Amazing Thomas - just want to note a few quick things: CG Config:
Studio config:
General:
|
53dd1a0
to
9792005
Compare
1aef3a0
to
573f739
Compare
I added three of the missing display built-in transforms to your spreadsheet (only one still missing). Looking at the color spaces, I'm noticing both "Apple Log" and "AppleLog BT2020", despite the latter not being in the spreadsheet. |
573f739
to
0a55d32
Compare
I implemented support for:
|
@KelSolaar , thanks for adding the ability to have an SDR and HDR version of DisplayP3! Here are some comments looking at the most recent Studio config artifact:
|
I had an action-item from the last meeting to check the ACES v2 config to see if we have all the color spaces from the ACES v1.3 configs. Here are my findings:
Not sure what to do about the displays. On the one hand, they are not very relevant and I'm open to dropping them. On the other hand, I'm just wondering if anyone will complain about missing displays when moving from ACES v1 to v2. |
Hello, Thanks for the thorough review! 1
The second dash is coming from the 2
Yes, good catch! 3
Where did you see this? The only instances I saw are occurring for Display P3 which is "expected" because we use the same builtin for both HDR and SDR. It would be much cleaner to have a dedicated Display P3 HDR builtin as suggested in previous meetings:
I'm happy to make a PR for this on the OCIO side. 4
Yes, I will check what happened for Display P3, maybe the quick filterer I wrote is not "smart" enough or there are yet another problem because we introduced that special case as mentioned earlier. 5
6
I will check, it might not even be in the spreadsheet. 7
If they are not in ACES 2.0, they cannot be in the config at the moment. 8
If you have a list we could talk about it yep! |
@KelSolaar , I looked again at the alias differences I reported above and found a bug in my comparison script. The changes needed are actually very minimal. Please simply add "lin_ap1_scene" to ACEScg. |
@KelSolaar , if you would like to add the additional DisplayP3 HDR built-in, you would make a second call to registry.addBuiltin here. Suggested name: "DISPLAY - CIE-XYZ-D65_to_DisplayP3-HDR", but I'm flexible. |
References AcademySoftwareFoundation/OpenColorIO-Config-ACES#130 Signed-off-by: Thomas Mansencal <thomas.mansencal@gmail.com>
References AcademySoftwareFoundation/OpenColorIO-Config-ACES#130 Signed-off-by: Thomas Mansencal <thomas.mansencal@gmail.com>
OP updated with the latest config artifacts! |
Found a bug: The Rec.2100-PQ Display does not have the view "ACES 2.0 - HDR 4000 nits (P3 D65)", which it is supposed to have. Applies to both the D65 and All versions. |
@KelSolaar Thanks for all of your effort on this (and in general)! The defaults for
This made me wonder which config everyone is testing/referring to? I've mainly been using studio-config-v2.3.0_aces-v2.0_ocio-v2.4.ocio.txt as I believe this is the one we'll be promoting as the "main" config for use/inclusion in DCCs? The Both are valid of course but it could result in confusion if we're looking at different things. |
I think that I finally figured out what is happening in the depth of the Reference config generator and producing unpredictable ordering. I'll do more tests. |
fa5263b
to
7b10a56
Compare
Ok, OP updated, involved deep and dangerous changes to the reference config generator that everything depends on, hopefully, nothing broke. It is extremely painful not being able to test this with confidence. @mjtitchener-fn : You are my eyes here, let me know! :) |
Here's a comparison of the the BEFORE:
AFTER:
|
Thanks for checking, one step forward, i.e., things are ordered properly but two backwards, i.e., there is now duplication (which is why I was grouping the spreadsheet elements in a particular way before when building the internal data). This should be an easy fix and I have ocioview finally running so it should be able to also test now! |
…ing config mapping generation. Signed-off-by: Thomas Mansencal <thomas.mansencal@gmail.com>
7b10a56
to
493ef9e
Compare
@mjtitchener-fn: I did a one liner change that should avoid duplication, cursory check of the internal data and in OCIO view seems to show that things are trending in the correct direction. Let me know, I need to jump on the other job. |
Thanks @KelSolaar, yep this has fixed the duplication problem. sRGB - Display Display P3 - Display Display P3 HDR - Display P3-D65 - Display Rec.1886 Rec.709 - Display Rec.2100-HLG - Display Rec.2100-PQ - Display ST2084-P3-D65 - Display I know it’s hard to have it all ways when setting rules to do to this programitically and users can ultimately select what's right for their workflow. Just thinking out loud about what the best starting points are 🤔 |
@mjtitchener-fn , thanks for testing this! Regarding HLG, the ACES 2 working group only made the 1000 nit P3 flavor available. My impression is that P3 is the preferred default across the different HDR displays since with Rec.2020 one cannot see it yet (except with various exotic displays) and so there is not the ability to confidently master to that yet. I would make the 1000 nit P3 the default for those last three items in your list. The P3-D65 display's view should indeed be P3 but it should also be named 48 nit (even though it is using the same view transform as the 100 nit). I was wondering if we need to create a duplicate view transform with that name in OCIO to be used for the config generator. |
@KelSolaar , per my action item from today's meeting, please make the following changes to the active_views lists. The rest of the configs look mostly ok. With the following exceptions:
Active view list for config: studio-config-v2.3.0_aces-v2.0_ocio-v2.4.ocio The default views will then be as follows:
Active view list for config: studio-config-d60-views-v2.3.0_aces-v2.0_ocio-v2.4.ocio The default views will then be as follows:
Active view list for config: studio-config-all-views-v2.3.0_aces-v2.0_ocio-v2.4.ocio The default views will then be as follows:
Active view list for config: cg-config-v2.3.0_aces-v2.0_ocio-v2.4.ocio The default views will then be as follows:
Active view list for config: reference-config-v2.3.0_aces-v2.0_ocio-v2.4.ocio The default views will then be as follows:
|
Thanks @doug-walker!
>>> [k for k in opencolorio_config_aces.BUILTIN_TRANSFORMS if "48nit" in k]
[]
>>> [k for k in opencolorio_config_aces.BUILTIN_TRANSFORMS if "100nit" in k]
['ACES-OUTPUT - ACES2065-1_to_CIE-XYZ-D65 - SDR-100nit-REC709_2.0', 'ACES-OUTPUT - ACES2065-1_to_CIE-XYZ-D65 - SDR-100nit-P3-D65_2.0', 'ACES-OUTPUT - ACES2065-1_to_CIE-XYZ-D65 - SDR-100nit-REC709-D60-in-REC709-D65_2.0', 'ACES-OUTPUT - ACES2065-1_to_CIE-XYZ-D65 - SDR-100nit-REC709-D60-in-P3-D65_2.0', 'ACES-OUTPUT - ACES2065-1_to_CIE-XYZ-D65 - SDR-100nit-REC709-D60-in-REC2020-D65_2.0', 'ACES-OUTPUT - ACES2065-1_to_CIE-XYZ-D65 - SDR-100nit-P3-D60-in-P3-D65_2.0', 'ACES-OUTPUT - ACES2065-1_to_CIE-XYZ-D65 - SDR-100nit-P3-D60-in-XYZ-E_2.0'] I will work on the active views ordering now. |
Signed-off-by: Thomas Mansencal <thomas.mansencal@gmail.com>
…Views* configs. Signed-off-by: Thomas Mansencal <thomas.mansencal@gmail.com>
Signed-off-by: Thomas Mansencal <thomas.mansencal@gmail.com>
Signed-off-by: Thomas Mansencal <thomas.mansencal@gmail.com>
a217c86
to
b1879b9
Compare
OP updated, the ordering should match for most of the configs but:
I fixed an error introduced whilst doing the Reference config changes for the ordering, this should have sorted most of the descriptions and the AMF components I think. |
Thanks for the update @KelSolaar ! The Reference and Studio configs now look fine to me. Unfortunately, I made an error when I reviewed the CG config. For ACES 1.3 we only included the HDR 1000 nit view and our displays list included "ST2084-P3-D65 - Display". I think we should continue likewise for the ACES 2 configs. You already fixed my error of including the non-1000 nit HDR views, so the active_views list is correct. Please just add the "ST2084-P3-D65 - Display" display to the CG config and we should be good to release. |
Signed-off-by: Thomas Mansencal <thomas.mansencal@gmail.com>
CG config is updated, I added |
Thank you @KelSolaar, all issues now seem to have been addressed. Please proceed with the release!! :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Awesome work Thomas!
Starting a PR for the ACES 2.0 work, I haven't even tried to build the configs but the classifying code is working, so did a solid good pass at updating the spreadsheets:
Current build artifacts can be found here:
Reference
CG
Studio