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

[FEATURE]: Update of the colour.XYZ_to_RGB and colour.RGB_to_XYZ definition signatures. #1127

Closed
KelSolaar opened this issue Mar 28, 2023 · 8 comments
Labels
Milestone

Comments

@KelSolaar
Copy link
Member

KelSolaar commented Mar 28, 2023

Description

Introduction

What follows has been triggered by a discussion with Rod Bogart.

colour.XYZ_to_RGB

Let us consider the current signature of the colour.XYZ_to_RGB definition:

def XYZ_to_RGB(
    XYZ: ArrayLike,
    illuminant_XYZ: ArrayLike,
    illuminant_RGB: ArrayLike,
    matrix_XYZ_to_RGB: ArrayLike,
    chromatic_adaptation_transform: Literal["Bianco 2010", "Bianco PC 2010", "Bradford", "CAT02 Brill 2008", "CAT02", "CAT16", "CMCCAT2000", "CMCCAT97", "Fairchild", "Sharp", "Von Kries", "XYZ Scaling"] | str | None = "CAT02",
    cctf_encoding: Callable | None = None,
) -> NDArrayFloat:
    ...

Doing the conversion without chromatic adaptation requires passing the following arguments:

  • illuminant_XYZ
  • illuminant_RGB
  • chromatic_adaptation_transform

When ideally, only matrix_XYZ_to_RGB should be required. One would argue that for a conversion without chromatic adaptation one could simply do RGB = vector_dot(matrix_XYZ_to_RGB, XYZ)

colour.RGB_to_RGB

We also have the colour.RGB_to_RGB definition:

def RGB_to_RGB(
    RGB: ArrayLike,
    input_colourspace: RGB_Colourspace,
    output_colourspace: RGB_Colourspace,
    chromatic_adaptation_transform: Literal["Bianco 2010", "Bianco PC 2010", "Bradford", "CAT02 Brill 2008", "CAT02", "CAT16", "CMCCAT2000", "CMCCAT97", "Fairchild", "Sharp", "Von Kries", "XYZ Scaling"] | str | None = "CAT02",
    apply_cctf_decoding: bool = False,
    apply_cctf_encoding: bool = False,
    **kwargs: Any,
) -> NDArrayFloat:
    ...

Note how it takes directly a colour. RGB_Colourspace as argument and defines whether to use the decoding and encoding cctf.

Enhancement Proposal

Having to pass the 3 arguments when no CAT is desired is not a great design, Rod suggested having a colour.XYZ_to_RGB_no_CAT definition. I would like to see if we can modify the existing definition.

Something as follows seems better and more inline with the colour.RGB_to_RGB definition:

def XYZ_to_RGB(
    XYZ: ArrayLike,
    colourspace: RGB_Colourspace | None = None,
    illuminant: ArrayLike | None = None,
    chromatic_adaptation_transform: Literal["Bianco 2010", "Bianco PC 2010", "Bradford", "CAT02 Brill 2008", "CAT02", "CAT16", "CMCCAT2000", "CMCCAT97", "Fairchild", "Sharp", "Von Kries", "XYZ Scaling"] | str = "CAT02",
    apply_cctf_encoding: bool = False,
    **kwargs,
) -> NDArrayFloat:
    ...

Full Arguments

The usage would be as follows:

XYZ_to_RGB(
    XYZ,
    colour.models.RGB_COLOURSPACE_ARRI_WIDE_GAMUT_4,
    colour.CCS_ILLUMINANTS["cie_2_1931"]["D65"],
    "Bradford",
    True,
)

Compared with:

XYZ_to_RGB(
    XYZ,
    colour.CCS_ILLUMINANTS["cie_2_1931"]["D65"],
    colour.models.RGB_COLOURSPACE_ARRI_WIDE_GAMUT_4.whitepoint,
    colour.models.RGB_COLOURSPACE_ARRI_WIDE_GAMUT_4.matrix_XYZ_to_RGB,
    "Bradford",
    colour.models.RGB_COLOURSPACE_ARRI_WIDE_GAMUT_4.cctf_encoding,
)

No Chromatic Adaptation

If no chromatic adaptation is desired, this should be enough:

XYZ_to_RGB(
    XYZ,
    colour.models.RGB_COLOURSPACE_ARRI_WIDE_GAMUT_4,
)

We could also support all the old arguments through **kwargs if required, they would override the various attributes of the RGB colourspace if passed and relevant, for example:

XYZ_to_RGB(
    XYZ,
    illuminant_XYZ=colour.CCS_ILLUMINANTS["cie_2_1931"]["D65"],
    illuminant_RGB=colour.models.RGB_COLOURSPACE_ARRI_WIDE_GAMUT_4.whitepoint,
    matrix_XYZ_to_RGB=colour.models.RGB_COLOURSPACE_ARRI_WIDE_GAMUT_4.matrix_XYZ_to_RGB,
    chromatic_adaptation_transform="Bradford",
    cctf_encoding=colour.models.RGB_COLOURSPACE_ARRI_WIDE_GAMUT_4.cctf_encoding,
)

This is obviously a breaking change but it can be handled easily with warnings for transition, keen to hear what people think?

@KelSolaar KelSolaar added this to the v0.4.3 milestone Mar 28, 2023
@tjdcs
Copy link
Contributor

tjdcs commented Mar 28, 2023

I have also had many complaints to myself about the XYZ to RGB definition. I mainly handle chromatic adaptation elsewhere in my workflows, so I would only likely utilize the more simple definition. In that case, I'd still prefer to just pass a 3x3 matrix and requisite XYZ. Does the CCTF encoding handle the EOTF? I suppose I'd need some quick options for that too.

In my own workflow I have never liked the RGB color space objects, and generally don't use them. I'd like for any XYZ / RGB APIs to support functioning with some basic assumptions and only inputing the NPM and XYZ values.

@KelSolaar
Copy link
Member Author

Thx @tjdcs!

Those definitions are convenience helpers as you could simply do RGB = vector_dot(matrix_XYZ_to_RGB, XYZ) thus not really require any API for that matter. Getting the chromatic adaptation right is hard if you don't know what you are doing, this is mostly why those definitions exist.

I considered doing something like this also:

def XYZ_to_RGB(
    XYZ: ArrayLike,
    colourspace_or_ matrix_XYZ_to_RGB: RGB_Colourspace | ArrayLike | None = None,
    illuminant: ArrayLike | None = None,
    chromatic_adaptation_transform: Literal["Bianco 2010", "Bianco PC 2010", "Bradford", "CAT02 Brill 2008", "CAT02", "CAT16", "CMCCAT2000", "CMCCAT97", "Fairchild", "Sharp", "Von Kries", "XYZ Scaling"] | str = "CAT02",
    apply_cctf_encoding: bool = False,
    **kwargs,
) -> NDArrayFloat:
    ...

where the first argument could be either the matrix or a RGB_Colourspace instance.

@nick-shaw
Copy link
Contributor

I have to admit, I tend to use the simple vector_dot approach, as I find the API unnecessarily unwieldy here.

@KelSolaar
Copy link
Member Author

Thanks @nick-shaw,

Do you also handle chromatic adaptation separately?

@KelSolaar
Copy link
Member Author

KelSolaar commented Mar 29, 2023

Another potential signature:

def XYZ_to_RGB(
    XYZ: ArrayLike,
    colourspace: RGB_Colourspace | str,
    illuminant: ArrayLike | None = None,
    chromatic_adaptation_transform: Literal["Bianco 2010", "Bianco PC 2010", "Bradford", "CAT02 Brill 2008", "CAT02", "CAT16", "CMCCAT2000", "CMCCAT97", "Fairchild", "Sharp", "Von Kries", "XYZ Scaling"] | str = "CAT02",
    apply_cctf_encoding: bool = False,
) -> NDArrayFloat:
    ...

Usage could then be:

XYZ_to_RGB(XYZ, "ARRI Wide Gamut 4")

We would also modify the colour.RGB_to_RGB definition accordingly:

RGB_to_RGB(RGB, "ACEScg", "ARRI Wide Gamut 4")

@nick-shaw
Copy link
Contributor

Thanks @nick-shaw,

Do you also handle chromatic adaptation separately?

I would probably use the API if chromatic adaptation is needed. But I’m not consistent!

@KelSolaar
Copy link
Member Author

What do you think about the string approach? Seems like this alone warrants the overhaul, that would make it easier to pick-up for people.

@nick-shaw
Copy link
Contributor

Easier is always good if it doesn’t reduce functionality. Seems sensible to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants