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

Inconsistent and possibly incorrect ACEStransformID for ARRI LogC4 #138

Closed
KelSolaar opened this issue Aug 19, 2023 · 6 comments
Closed

Comments

@KelSolaar
Copy link
Contributor

Hello,

There is an issue with the ARRI LogC 4 ACEStransformID:

aces-dev - LogC4

https://github.com/ampas/aces-dev/blob/dev/transforms/ctl/idt/vendorSupplied/arri/alexa/v4/IDT.ARRI.ARRI-LogC4.ctl#L1

urn:ampas:aces:transformId:v1.5:IDT.ARRI.ARRI-LogC4.a1.v1

aces-dev - LogC3

https://github.com/ampas/aces-dev/blob/dev/transforms/ctl/idt/vendorSupplied/arri/alexa/v3/EI800/IDT.ARRI.Alexa-v3-logC-EI800.ctl#L2

urn:ampas:aces:transformId:v1.5:IDT.ARRI.Alexa-v3-logC-EI800.a1.v2

OpenColorIO-Config-ACES - LogC4

https://github.com/AcademySoftwareFoundation/OpenColorIO-Config-ACES/blob/main/opencolorio_config_aces/clf/transforms/arri/generate.py#L345

urn:ampas:aces:transformId:v1.5:IDT.ARRI.LogC4.a1.v1

One of the aces-dev IDT or OpenColorIO-Config-ACES CLF is incorrect. I think that the aces-dev IDT is inconsistent but no strong opinion as to what should be done here.

That being said we are trying to release a new OpenColorIO version along side new builtin configs so it would be awesome to get a resolution as soon as possible.

@scoopxyz, @doug-walker, @aforsythe for VIS.

@KelSolaar KelSolaar changed the title Inconsistent and possibly incorrect ACEStransformID for ARRI LogC4 Inconsistent and possibly incorrect ACEStransformID for ARRI LogC4 Aug 19, 2023
@KelSolaar
Copy link
Contributor Author

@doug-walker and I (and Carol) decided to change it on the OCIO side as it is easier and gets us moving.

@scoopxyz
Copy link
Contributor

scoopxyz commented Sep 15, 2023

Copying my response from the Colour-Science slack thread:

I had to go back and look at why we chose the present form. If I look at the LogC3 EI 800 IDT that is:
IDT.ARRI.Alexa-v3-logC-EI800.a1.v2

And the present LogC4 is:
IDT.ARRI.ARRI-LogC4.a1.v1

This is something like <Class>.<Vendor>.<ColorSpaceName>.<ACESVersion>.<Version>

So "ARRI LogC4" is the official name of the color space, with its component parts being "ARRI Wide Gamut 4" and "ARRI LogC4 Curve". This information is also mirrored in the white paper. So I would actually vote to have the aces-dev TransformID remain as-is. Despite the quasi-ugliness of having the vendor name appear twice.

I'm open to be convinced in another direction, but that is my reasoning.

As to why its different in the OpenColorIO Config, this was based on the observation that it already takes "creative" liberty in the ACES ID. For example between...
...
Which influenced the OCIO config discussion, in addition to the conversation with Doug in the PR: AcademySoftwareFoundation/OpenColorIO-Config-ACES#54

I can repeat this in the OCIO slack as well.

@scoopxyz
Copy link
Contributor

scoopxyz commented Sep 15, 2023

I take back what was said about the other discrepancy, it was referring to another CTL file which I didn't see on my first glance.

So to summarize this specific issue, it seems like it is different because it was different "on purpose" from feedback in the PR. I would still vote to adopt the name as-is in the aces-dev repo, since we're talking about an identification string and not a speaking name, I don't think the aesthetics of the string are of much concern. If there's another angle I'm not seeing please let me know.

@doug-walker
Copy link

Thanks for looking into this Sean! Thomas changed the ADESTransformID in the OCIO config to match what is in aces-dev, so they are both urn:ampas:aces:transformId:v1.5:IDT.ARRI.ARRI-LogC4.a1.v1. I confirmed that this is what is in the latest built-in OCIO configs.

I think this issue could be closed now.

@KelSolaar
Copy link
Contributor Author

Thanks, let's close that one as resolved!

@KelSolaar
Copy link
Contributor Author

(I will indicate that we changed the ACEStransformID in the config release notes.)

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

No branches or pull requests

3 participants