Per-script OpenColorIO config, working space and display transform

[johnhaddon]

This implements the majority of the remaining items from #5215. It adds an OpenColorIO tab to the ScriptNode settings, allowing the OCIO config, working space and context to be configured on a per-script basis :

@jedypod, this is probably a good moment for you to check out the latest state of things before it gets locked down into the 1.3 release. Perhaps you could download the build artifact from the CI, or get @danieldresser-ie to furnish you with a test build at IE?

[danieldresser-ie]

Found a single nitpick with the code.

Definitely not really qualified to evaluate the user experience - I'll try to make sure jedypod gets ahold of some sort of build.

[danieldresser-ie]

Not sure if I'm missing something, but this seems unnecessarily complex ... couldn't we just start by discarding all ocio variables in the context, since all of them are either going to be overwritten, or removed?

[johnhaddon]

We could, but it would lead to a lot of unnecessary Context::changedSignal() emissions.

[danieldresser-ie]

Ah, makes sense. Maybe a comment explaining that would be nice - with the way it's implemented, it took me a little bit to convince myself that we would never leave behind some of the previous values.

[johnhaddon]

Added comment in 8a1c7a8.

[jedypod]

Quickly looking over this from a user perspective I think this looks great overall! Super exciting, thank you for putting these changes together! I do have a couple small comments.

Would it make sense to add a tooltip to explain what the WorkingSpace plug does? There are helpful tooltips for the Config and DisplayTransform plugs but not for this one.

I'm worried that the plug name displayTransform is misleading regarding the functionality. It's clear after reading the tooltip that this dropdown specifies the picker / ui display colorspace, but the name of the plug doesn't really indicate this. Maybe something like pickerColorspace instead? There's probably a better name.

I'm also wondering if it's appropriate to use a displayTransform for this? Would it be better to allow arbitrary colorspace selection? This could allow the user to specify a role or colorspace instead of only one of the views. I'm not really sure of this, just pondering.

There's also a weird bug I noticed: When changing the config to the ACES 1.3 Studio Config, the available colorspaces in the Working Space plug stay as though the CG config were selected. Here's a screen recording demonstrating the issue:

WorkingSpace_not_updating_for_StudioConfig_2023-06-21_15-38.mp4

Hope this helps!

[johnhaddon]

Thanks for taking a look @jedypod. Your main comments highlight things I'd been unsure about myself, so I welcome the chance to think about them a little more.

I'm worried that the plug name displayTransform is misleading regarding the functionality. It's clear after reading the tooltip that this dropdown specifies the picker / ui display colorspace, but the name of the plug doesn't really indicate this. Maybe something like pickerColorspace instead?

Yeah, I was worried about that too. But I also worried that something like pickerColorspace was too specific, and might lead someone to think it only applied to the color chooser dialogue. Maybe that's better than leading someone to think it relates to the Viewer though? And the tooltip can document that by picker we mean "all the places you see a colour but that aren't the viewer"? Or maybe uiColorspace?

The other reason I called it displayTransform was because it is a display transform and not a color space. Which brings us to the next question...

I'm also wondering if it's appropriate to use a displayTransform for this? Would it be better to allow arbitrary colorspace selection?

I did think about that, and this was my reasoning for choosing a display transform :

• OpenColorIO is moving away from providing colorspaces for each display/view pair. I don't know if this is stated authoritatively in the docs, but it's what I gleaned from DisplayViewTransforms for output? AcademySoftwareFoundation/OpenColorIO-Config-ACES#82 (comment). So to appropriately account for the display, it seems that a display transform is necessary - a colorspace often won't be available.
• I want to be able to be able to say "If you use the same settings for the UI and the Viewer, you will see the same color in the UI and the Viewer". That only works if we present the same set of options for the UI and the Viewer.

The second point does bring me to a question though : what is the right default for UI settings? Would it be better to avoid the tone-mapping and wotnot for that? Or is it better to have a one-to-one match with the Viewer by default?

Returning to the naming discussion, if we stick with using a display transform, perhaps uiDisplayTransform or pickerDisplayTransform? Any preference?

There's also a weird bug I noticed: When changing the config to the ACES 1.3 Studio Config, the available colorspaces in the Working Space plug stay as though the CG config were selected.

Thanks for spotting that, and for the video. I'll take a look, and I'll sort out the workingSpace tooltip as well...

[johnhaddon]

Would it make sense to add a tooltip to explain what the WorkingSpace plug does?

Added in f5308e7.

There's also a weird bug I noticed: When changing the config to the ACES 1.3 Studio Config, the available colorspaces in the Working Space plug stay as though the CG config were selected.

It turned out that this wasn't a bug - I had deliberately filtered the menu to only show colorspaces in the working-space category, and the set of those is the same for the CG and Studio configs. You can run this in the PythonEditor if you want to see everything :

Gaffer.Metadata.registerValue( GafferImage.OpenColorIOConfigPlug, "workingSpace", "openColorIO:categories", "" )

Do you think that should be the default? I was trying to avoid giving folks enough rope to shoot themselves in the foot, but maybe it's too restrictive in practice?

[jedypod]

Do you think that should be the default? I was trying to avoid giving folks enough rope to shoot themselves in the foot, but maybe it's too restrictive in practice?

Now that I understand about the working-space colorspace category, I do think this makes sense as it is. I'm good with this!

[jedypod]

if we stick with using a display transform, perhaps uiDisplayTransform or pickerDisplayTransform? Any preference?

With your explanations I do think it makes sense to use a view transform instead of a colorspace for the UI. I like uiDisplayTransform or uiViewTransform as a name. I think that's pretty clear and indicates the right idea. I agree that pickerDisplayTransform is not really conveying the right idea.

what is the right default for UI settings? Would it be better to avoid the tone-mapping and wotnot for that? Or is it better to have a one-to-one match with the Viewer by default?

I would lean towards defaulting to the default view transform in the selected OCIO config: that is, the first item in the list of Active Views. By default this would give a match between what we see in the viewer and what we see in the UI, but it will still be configurable if a different behavior is desired by the user.

[jedypod]

I did a little more testing, and have a couple more little questions. (Sorry I didn't catch these before).

Working Space Label Not Updating

The first thing is very minor, and might not be a bug but i figured i would flag it anyway:

• when changing the ocio config, the label of the working space is not updating. If the working space is set to scene_linear role, and in one config (ACES 1.0.3) the colorspace that role points to is named ACES - ACEScg, and in another ocio config (ACES Studio Config), the aliased colorspace of the same role name is ACEScg, the label doesn't change. Here's a screen recording of the behavior if it helps explain:

gaffer-workingspace_label_not_updating.mp4

DisplayTransform Behavior

I noticed that the default state of the DisplayTransform node is to do nothing. I'm not sure if there's a good Gaffery reason for this behavior, so please disregard if so.

When creating a new DisplayTransform node, both display and view plugs are set to "None". By testing, I guess this plug value bypasses the processing of the node. The user needs to manually set the display and view as needed.

Is there a good reason not to have the default plug values match the default display and view as specified by the OCIO config?

And is there a good reason for adding the None option, which is an extra item that is not necessarily defined as part of the OCIO config? (The user could just disable the node to bypass its processing right?) Does it make sense to have an Automatic (view/display) entry, instead of None?

Just curious about that one! Again here's a screen recording showing the behavior.

gaffer-displayTransform_behavior.mp4

I'm not totally confident this is in-scope for this specific pull request, so apologies if I got that wrong!

[johnhaddon]

when changing the ocio config, the label of the working space is not updating

Well spotted - thanks for catching that. It was a more general problem in our presets widget - fixed in 06fbefc.

I noticed that the default state of the DisplayTransform node is to do nothing...Is there a good reason not to have the default plug values match the default display and view as specified by the OCIO config?

I suppose the general philosophy is to give nodes do-nothing defaults if we don't think we can read the user's mind sufficiently to choose a do-something default that's likely to be a better starting point. I'm not sure that really applies here though, so I've done as you suggested in e18eaca.

<pedantry>We're not actually changing the default values (they're still ""), because at the point the node is created we don't know what contexts it will be used in (and there could be many). Instead, we're just interpreting the default differently, to mean "read the default from the current ocio config (which is defined by Gaffer's current context).<pedantry>

[johnhaddon]

I like uiDisplayTransform or uiViewTransform as a name.

I've relabelled the plug as "UI Display Transform" in the Settings window in 2dcc754. I've actually kept the original name (as used in the API) though, as theoretically the OpenColorIOConfigPlug could be used elsewhere, where the display transform might be used for something else. This also lets us change our mind about the label later without worrying about breaking API compatibility. Hopefully that seems reasonable.

[johnhaddon]

@danieldresser-ie, I think that's all feedback covered now, so if you could cast your eye over the new commits that would be great.

[danieldresser-ie]

All looks fine to me