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: ocioview - Chromaticities Inspector #1914

Conversation

KelSolaar
Copy link
Contributor

A temporary PR to park and discuss about the work on the Chromaticity Inspector.

image

Most of the base pieces are here but there are a few questions, the principal one being:

In order to plot chromaticities with colours, we need to be able to know which RGB colourspace we are coming from to do: $RGB \rightarrow CIE\ XYZ \rightarrow CIE\ xy(Y)$
The issue is OCIO cannot really do that transformation, Colour can do it but this assumes that there is a matching RGB colourspace builtin (or a corresponding one added). If only we had primaries and whitepoints... :)

I'm assuming ACES2065-1 in the above image. Suffice to say that we are interested in both the chromaticities of the original image and that of the transformed one.

Food for thoughts!

Paging @remia, @michdolan and @doug-walker!

@KelSolaar
Copy link
Contributor Author

We discussed about using the CIE XYZ Interchange space with @doug-walker et al. this morning. This would work but would not give the final encoded chromaticities. That might be an acceptable compromise.

@KelSolaar
Copy link
Contributor Author

Having put quite a bit of thoughts into that one here are some suggestions:

  1. Output Transform Passthrough: Convert from Input Color Space to CIE XYZ D65 Interchange and display chromaticities.
  2. Output Transform Active: Convert from Input Color Space to Output Transform to CIE XYZ D65 Interchange and display chromaticities.

Now some questions for @michdolan:

The current MessageRouter implementation can return a processor that could handle doing 2., partially. I would need to add the conversion to CIE XYZ D65 Interchange which requires knowledge of what is the Output Transform from the ImageViewer and I don't see a clean API to find that out. I cannot also find a way to get the Input Color Space from the ImageViewer which would be required to do 1.

@remia
Copy link
Collaborator

remia commented Dec 6, 2023

Looks good @KelSolaar (and nice choice of books)! I assume you are going to add settings to choose the chromaticity space to use, switch between 2D / 3D representation, optional gamut triangle (and maybe more) later?

@KelSolaar KelSolaar force-pushed the feature/ocioview-chromaticity-inspector branch from 6223661 to c9278c5 Compare January 20, 2024 09:06
@KelSolaar
Copy link
Contributor Author

Looks good @KelSolaar (and nice choice of books)! I assume you are going to add settings to choose the chromaticity space to use, switch between 2D / 3D representation, optional gamut triangle (and maybe more) later?

Yes, that is the plan! I haven't had too much time to look into it but was slowly getting back to that. Might be worth a discussion as per my previous comment about the new API needs I have. I could implement them from that PR too.

@michdolan : Any thoughts?

@michdolan
Copy link
Collaborator

michdolan commented Apr 17, 2024

Sorry for the long delay in replying. This looks awesome @KelSolaar !

I think for getting the needed data (input color space, output transform) we could wrap the processor being passed to the message router in a data class that contains info about the processor's construction. That would be useful elsewhere too.

@michdolan
Copy link
Collaborator

michdolan commented Apr 18, 2024

In #1966 I added a new ProcessorContext interface that is passed through the message router with the processor, which will give you the input color space name, output transform item type, output transform item name, and the transform direction.

ProcessorContext(input_color_space='ACES - ACES2065-1', transform_item_type=<class 'PyOpenColorIO.ColorSpace'>, transform_item_name='Input - Generic - sRGB - Texture', transform_direction=<TransformDirection.TRANSFORM_DIR_FORWARD: 0>)

@KelSolaar KelSolaar force-pushed the feature/ocioview-chromaticity-inspector branch 8 times, most recently from bbce544 to c41dbbe Compare May 13, 2024 09:55
@KelSolaar
Copy link
Contributor Author

Did some good progress over the weekend and tonight: https://www.youtube.com/watch?v=x5wqywe5mSA

I spent quite a lot of time finding out what would be a useful UX for the inspector and went with 3 layers of transformation, the benefit is that it allows to show the fully processed chromaticities compared to my suggestion above: #1914 (comment)

  1. Apply current active processor
  2. Convert from chromaticities input space to "CIE-XYZ-D65" interchange
  3. Convert from "CIE-XYZ-D65" to "VisualRGBScatter3D" working space

The user HAS to choose the chromaticities input space, it might looks cumbersome but the benefit is that one could load the ACES 1.0 config, define the ROLE_INTERCHANGE_DISPLAY, apply the sRGB ODT, use the Linear sRGB as chromaticities input space and see the final image chromaticities.

I implemented some new convenient functions in Colour to help the CIE 1960 UCS and CIE 1976 UCS Chromaticity Diagrams.

@KelSolaar KelSolaar force-pushed the feature/ocioview-chromaticity-inspector branch 3 times, most recently from 4306616 to 247939b Compare May 18, 2024 03:59
@KelSolaar KelSolaar requested review from remia and michdolan May 18, 2024 04:00
@KelSolaar KelSolaar marked this pull request as ready for review May 18, 2024 04:00
@KelSolaar
Copy link
Contributor Author

I think that this is ready for a first review, at least to discuss about it :)

@KelSolaar
Copy link
Contributor Author

I was also thinking that adding a RGB mode should not be too hard but maybe a dedicated viewer is better. When I have some cycles, I would also like to add a RGB parade and possibly other type of scopes:

image
image

@KelSolaar KelSolaar force-pushed the feature/ocioview-chromaticity-inspector branch from 247939b to 6209abd Compare May 18, 2024 06:22
@KelSolaar KelSolaar changed the title PR: ocioview - Chromaticity Inspector PR: ocioview - Chromaticities Inspector May 18, 2024
Copy link
Collaborator

@michdolan michdolan left a comment

Choose a reason for hiding this comment

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

Amazing work @KelSolaar !

src/apps/ocioview/ocioview/utils.py Outdated Show resolved Hide resolved
src/apps/ocioview/ocioview/utils.py Outdated Show resolved Hide resolved
src/apps/ocioview/ocioview/utils.py Outdated Show resolved Hide resolved
src/apps/ocioview/requirements.txt Show resolved Hide resolved
Copy link

linux-foundation-easycla bot commented May 21, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@KelSolaar KelSolaar force-pushed the feature/ocioview-chromaticity-inspector branch from 54166fc to e9bdfba Compare May 21, 2024 08:52
@KelSolaar
Copy link
Contributor Author

I committed the suggestion but seems like EasyCLA is being a prick... Do we have a process for that or does it look like I will need to rebase/reword the commits?

@michdolan
Copy link
Collaborator

Sorry about the EasyCLA issue... You may need to amend the commit and force push again.

@KelSolaar KelSolaar force-pushed the feature/ocioview-chromaticity-inspector branch from e9bdfba to e971625 Compare May 23, 2024 08:00
@KelSolaar
Copy link
Contributor Author

Trimmed yourself from the commits @michdolan!

@remia
Copy link
Collaborator

remia commented May 26, 2024

Great work @KelSolaar!

Convert from "CIE-XYZ-D65" to "VisualRGBScatter3D" working space

Am I understanding correctly that this transformation is currently a no-op as the latter is also XYZ D65?

The user HAS to choose the chromaticities input space, it might looks cumbersome but the benefit is that one could load the ACES 1.0 config, define the ROLE_INTERCHANGE_DISPLAY, apply the sRGB ODT, use the Linear sRGB as chromaticities input space and see the final image chromaticities.

When applying the sRGB ODT, shouldn't the Chromaticities input space be Display sRGB in that case (sRGB EOTF encoded instead of Linear)?

@KelSolaar
Copy link
Contributor Author

Am I understanding correctly that this transformation is currently a no-op as the latter is also XYZ D65?

As of right now, indeed, so we can optimise it but I left it as a test case for now, allows verifying that any working space produce the same stuff.

When applying the sRGB ODT, shouldn't the Chromaticities input space be Display sRGB in that case (sRGB EOTF encoded instead of Linear)?

Depends if one wants to see the encoded or linear chromaticities, both would be valid options, e.g., you want to ignore the EOTF inverse effect.

@KelSolaar KelSolaar force-pushed the feature/ocioview-chromaticity-inspector branch from e971625 to d7602f3 Compare June 8, 2024 22:26
@KelSolaar
Copy link
Contributor Author

Merging this one down!

@KelSolaar KelSolaar merged commit 55cfe59 into AcademySoftwareFoundation:main Jun 8, 2024
25 checks passed
@mmdanggg2
Copy link

Merged code is broken, you didn't update the color_space_to_rgb_colourspace name in chromaticities_inspector.py

@KelSolaar
Copy link
Contributor Author

Thanks @mmdanggg2! I thought I had checked that the branch was fully working after I rebased but looks like I did not. This should be fixed in #1985 whenever it is merged.

@KelSolaar
Copy link
Contributor Author

@mmdanggg2 : This should be good now!

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.

4 participants