OpenColorIO icon indicating copy to clipboard operation
OpenColorIO copied to clipboard

Adjust 'family' description in OpenColorIO.h to match documentation

Open doug-walker opened this issue 3 years ago • 6 comments

In the documentation (both v1 and v2), the family property of a color space is defined as: "Allows for logical grouping of colorspaces within a UI."

And indeed, that is how it seems to be used in practice. For example, in the ACES config, the color spaces {ACES2065-1, ACEScc, ACEScct, ACEScg} are all part of a family "ACES" and all of the various ARRI IDTs are in a family called "Input/ARRI".

However, in the OpenColorIO.h header file there are several places where 'family' has a different description. For example line 1373:

// *ColorSpaces* are specific to a particular image precision (float32,
// uint8, etc.), and the set of ColorSpaces that provide equivalent mappings
// (at different precisions) are referred to as a 'family'.

The proposed correction is to remove that sentence.

And line 1399:

     * Get the ColorSpace group name (used for equality comparisons)
     * This allows no-op transforms between different colorspaces.
     * If an equalityGroup is not defined (an empty string), it will be considered
     * unique (i.e., it will not compare as equal to other ColorSpaces with an
     * empty equality group).  This is often, though not always, set to the
     * same value as 'family'.

The proposed correction is to remove the last sentence ("This is often, ...").

And line 1988:

     *    This may provide higher fidelity than anticipated due to internal
     *    optimizations. For example, if the inputColorSpace and the
     *    outputColorSpace are members of the same family, no conversion
     *    will be applied, even though strictly speaking quantization
     *    should be added.

Note that this latter usage is definitely not consistent with the code -- the family property has no impact on the result of color processing. The proposed change in this case is to replace 'family' with 'equalitygroup' in that sentence.

It seems the definition of 'family' changed during the development of v1 but the header file was not updated. This should now be done as it could easily cause confusion among developers.

doug-walker avatar Nov 28 '20 02:11 doug-walker

@doug-walker could you pls assign this issue to me. Am relatively new to open source. Would love to start with this.

Ansh-Sarkar avatar Dec 03 '20 04:12 Ansh-Sarkar

Done. Thank you @Ansh-Sarkar.

doug-walker avatar Dec 03 '20 06:12 doug-walker

Thank you so much. Will submit a PR with the necessary changes as soon as possible

Ansh-Sarkar avatar Dec 04 '20 03:12 Ansh-Sarkar

@doug-walker have made the necessary changes and modified comments at required places. please review my P.R. #1227 .

Ansh-Sarkar avatar Dec 04 '20 08:12 Ansh-Sarkar

@doug-walker I have made the necessary changes. The CLA has been signed and all the tests have been passed. Please review my P.R. #1230

Ansh-Sarkar avatar Dec 05 '20 10:12 Ansh-Sarkar

This issue is still open. It's a simple one, so a good option for first-time contributors.

doug-walker avatar Apr 06 '22 02:04 doug-walker

Implemented via PR #1635.

doug-walker avatar Nov 16 '22 01:11 doug-walker