[python][feature-request]`GradingPrimary` change set GradingStyle behavior

[MrLixm]

Hello,

When looking at the GradingPrimary class, I have a hard time understanding why do we need to specify the GradingStyle in the __init__ of the class ? I do understand it's kind of needed for its validate() method, but then if we ask the GradingStyle in the validate method, why asking it at __init__ ? Furthermore we can't get back this value GradingStyle value from the instance so we have to track it before in the code. Which is annoying when we then need in GradingPrimaryTransform to specify the GradingStyle AND a GradingPrimary instance which already specify a GradingStyle.

Consider the following snippet :

gp_lin = ocio.GradingPrimary(ocio.GRADING_LIN)
gp_lin.exposure = 0.5  #need GradingRGBM but let's ignore for the example

gp_log = ocio.GradingPrimary(ocio.GRADING_LOG)
gp_log.brightness = 0.5

gp_list = (gp_lin , gp_log)
for gp in gp_list:
    gp_tsfm = ocio.GradingPrimaryTransform(
            gp,
            gp.?????,  # we can't know which style to use
            False,
        )
    ...

So in my opinion, here is 2 suggestions for how I would see the class behave :

1. Style agnostic

you can check compatibility with validate() the user determine at any moment which style the GradingPrimary will be used with,

gp1 = ocio.GradingPrimary()
gp1.exposure = 0.5

gp2 = ocio.GradingPrimary()
gp2.brightness = 0.5
gp2.validate(ocio.GRADING_LOG)  # pass fine
gp2.exposure = 0.5
gp2.validate(ocio.GRADING_LOG)  # raise a warning/error ?

# the user determine at any moment which style the GradingPrimary will be used with
gp_list = (
    (gp1, ocio.GRADING_LIN),
    (gp2, ocio.GRADING_LOG)
)
for gpdata in gp_list:
    gp_tsfm = ocio.GradingPrimaryTransform(
            gpdata[0],
            gpdata[1],
            False,
        )
    ...

2. Style bound to instance, flexible

gp1 = ocio.GradingPrimary(ocio.GRADING_LIN)
gp1.exposure = 0.5

gp2 = ocio.GradingPrimary(ocio.GRADING_LOG)
gp2.brightness = 0.5
gp2.validate()  # pass fine
gp2.exposure = 0.5
gp2.validate()  # raise a warning/error ?

gp_list = (gp1,gp2)
for gp in gp_list:
    gp_tsfm = ocio.GradingPrimaryTransform(
            gp,
            gp.gradingStyle,  # new attribute
            False,
        )
    ...

2.2. Style bound to instance, strict

Remove the validate() method and perform check on attribute set.

gp1 = ocio.GradingPrimary(ocio.GRADING_LIN)
gp1.exposure = 0.5

gp2 = ocio.GradingPrimary(ocio.GRADING_LOG)
gp2.brightness = 0.5
gp2.exposure = 0.5  # will raise error
gp2.contrast = 0.0  # will raise error

gp_list = (gp1,gp2)
for gp in gp_list:
    gp_tsfm = ocio.GradingPrimaryTransform(
            gp,
            gp.gradingStyle,  # new attribute
            False,
        )
    ...

I don't know if this makes sense or is too subjective.

Cheers. Liam.

[remia]

I do understand it's kind of needed for its validate() method, but then if we ask the GradingStyle in the validate method, why asking it at init ?

It seems the only use of specifying the style in the constructor is to initialise the pivot parameter.

I agree this may be an awkward interface to use, I think the reason for the style not being part of GradingPrimary is that this structure is available as a DynamicProperty for GPU shader uniforms. It wouldn't make sense to have the style here as it would most likely mean modification to the shader code itself when changed. That being said I didn't use the grading stack yet so maybe someone more knowledgeable can confirm.

[zachlewis]

If you create the GradingPrimaryTransform first, you can snatch its GradingPrimary, which will have been initialized with default values appropriate for the style.

>>> gp_tsfm1 = ocio.GradingPrimaryTransform(ocio.GRADING_LOG)
>>> gp_tsfm2 = ocio.GradingPrimaryTransform(ocio.GRADING_LIN)
>>> gp1, gp2 = gp_tsfm1.getValue(), gp_tsfm2.getValue()
>>> type(gp1)
PyOpenColorIO.GradingPrimary
>>> gp1.pivot == gp2.pivot
False

This applies for GradingTone too.

>>> ttlog, ttlin, ttvid = [ocio.GradingToneTransform(i) for i in [ocio.GRADING_LOG, ocio.GRADING_LIN, ocio.GRADING_VIDEO]]
>>> _ = [print(i.getValue()) for i in [ttlog, ttlin, ttvid]]
<blacks=<red=1 green=1 blue=1 master=1 start=0.4 width=0.4> shadows=<red=1 green=1 blue=1 master=1 start=0.5 width=0> midtones=<red=1 green=1 blue=1 master=1 start=0.4 width=0.6> highlights=<red=1 green=1 blue=1 master=1 start=0.3 width=1> whites=<red=1 green=1 blue=1 master=1 start=0.4 width=0.5> s_contrast=1>
<blacks=<red=1 green=1 blue=1 master=1 start=0 width=4> shadows=<red=1 green=1 blue=1 master=1 start=2 width=-7> midtones=<red=1 green=1 blue=1 master=1 start=0 width=8> highlights=<red=1 green=1 blue=1 master=1 start=-2 width=9> whites=<red=1 green=1 blue=1 master=1 start=0 width=8> s_contrast=1>
<blacks=<red=1 green=1 blue=1 master=1 start=0.4 width=0.4> shadows=<red=1 green=1 blue=1 master=1 start=0.6 width=0> midtones=<red=1 green=1 blue=1 master=1 start=0.4 width=0.7> highlights=<red=1 green=1 blue=1 master=1 start=0.2 width=1> whites=<red=1 green=1 blue=1 master=1 start=0.5 width=0.5> s_contrast=1>

[MrLixm]

Thanks for the replies, With all of that, wouldn't proposition 1. be a good choice then ?

If the user wants convenience he creates directly the GradingPrimaryTransform as demonstrated by Zach, then the GradingPrimary class in itself is really just a dataclass to hold values that are totally separated from the GradingStyle. To check compatibility we can just run validate(GRADING_LIN). Of course this would mean that with inattention, you could use an wrong pivot value with GRADING_LOG that needs to be documented. But at least the user will be aware that LOG pivot != LIN pivot which is not documented currently ?

But this also have a relation with my other issue #1643 where I don't know why there is 2 contrast formula with 2 different pivot domain range ?

Anyway, the GradingPrimaryTransform is great to know will check if it's fit with my workflow.

Cheers.