OCIO/ContextTracker fixes #6080
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
4 tasks
- Fix update on context changes. Previously we were connecting to `self.context().changedSignal()`, but that is never triggered now contexts are delivered by a ContextTracker. We now override `_valuesDependOnContext()` instead. - Don't change plug values when they become invalid. Instead enter an error state. Now that the ImageView context potentially changes frequently with focus, I don't think it's appropriate to be changing plug values willy nilly. - Instead, use the "" default value to mean "the default view and display for this context". This allows the default value to work appropriately in _any_ context. This required a small change in View.cpp. I've also fixed the menu to show the available views for the default display when the current state is invalid - previously it showed the displays but no views. It's now easier to get into a valid state again. And the menu no longer computes the plug value in the foreground - instead using values we got from `_updateFromValues()`.
I've put this in a separate commit, since on GafferHQ#5215 it was suggested that "Default" was a very confusing label for everyone. I'm hoping that the reasoning behind the previous commit and the addition of parenthesis with the current resolved value might change that opinion.
Changing "" to be a transform rather than "no transform" was causing problems all over the unit tests. So we now use "__default__" to denote the default OCIO transform instead.
|
I've pushed two fixups that should hopefully take care of the test failures. And then a final commit which removes the "Default" wording from the button text, and hopefully clarifies the wording in the menu. I'm feeling better about this now, but definitely open to suggestions for alternative wording still. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This attempts to improve the DisplayTransformPlugValueWidget for the new ContextTracker-based world, where contexts may change more frequently than before. This affects the display transform menu in the Viewer, and the OCIO section in the Settings window. I'm not sure how successful I have been - I'm definitely open to opinions on other ways of approaching it.
@jedypod, your input would be valuable here. What I've ended up doing is allowing a "Default" value for display transforms, which adapts dynamically to whatever the default is in the current OCIO config. A bit like how "Automatic" on the ImageReader adapts based on the file type. In Gaffer 1.5, the OCIO config used in the Viewer can be even more dynamic than it already is, and can theoretically change based on what you're node you have focussed. So I felt that a dynamic default had some value - better than either errors or changing things behind people's backs. I've included the dynamically resolved name in the label, so hopefully it's clear that it's not a mystery transform called "Default", but a mode which resolves to a real transform. But I know on #5215 you made a good case against having "Default", so I'd like your take on this latest iteration...