Improved support for conditional enabling of nodes #6064
Merged
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
There was a problem hiding this comment.
Thanks! I've given this a run through and it does appear to cover the desired use-case. Might be a little tricky for users to understand the distinction of the context yellow strikethrough on first encounter, but it is a logical use of the colour...
There's one thought inline about improving the PatternMatch UI metadata, though I'd be fine with seeing this merged either way...
| """, | ||
|
|
||
| "nodeGadget:type", "GafferUI::AuxiliaryNodeGadget", | ||
| "auxiliaryNodeGadget:label", "*", |
There was a problem hiding this comment.
Should we register "nodeGadget:shape", "oval" for consistency with the other Auxiliary nodes in case a user switches this over to the StandardNodeGadget?
It'd probably be worth hiding the "string" and "pattern" nodules for that situation as well?
Black for nodes that are always disabled. Yellow for nodes that are disabled but in a potentiall context-sensitive way.
This consolidates it into the same file that creates the equivalent popup menu item, and will allow us to modify the behaviour for both shortcut and menu in a future commit. It also fixes a couple of small bugs as documented in Changes.md.
I'm not a huge fan of this, which is why I've split it into a separate commit from the rest. It's needed to integrate a custom "ContextEnabledEditScope" concept with Gaffer's new ContextTracker workflow. I'm not a huge fan of the concept, because it encourages serial chains of nodes, where often only one is enabled in a given context. This could often be more efficiently - and readably - represented by multiple input branches to a NameSwitch. And there are many different potential "procedural node enablers" and we might want to apply them to any node type. So just combining a PatternMatch (say) with an EditScope on an ad-hoc basis is more flexible than a fixed ContextEnabledEditScope. Nevertheless, the idea has caught on in certain circles, and we need to do our best to support it. The existing custom "ContextEnabledEditScope" operates with an additional internal switch which bypasses the processing for the node when the context doesn't match. But Gaffer can't reason about this bypass at all because it doesn't use the standard `Node.enabled` plug. So such nodes do not display a strike-through in the GraphEditor, and don't show as disabled in the EditScope menu either. So, the plan is to replace the internal switch with an internal PatternMatch node, driving the `enabled` plug directly. With this, Gaffer already correctly displays the strike-through when the node is disabled due to context. But we need some additional special cases to improve the useability, and that is what this commit does : - StandardNodeGadget : Add additional support for recognising when a node is disabled statically rather than dynamically. This consists of recognising the case where the `enabled` plug is driven by a node such a PatternMatch, and representing the node as statically disabled when the PatternMatch itself is disabled. - GraphEditor : Add support for enabling/disabling the driving node when the `enabled` plug has an input connection. With this, the custom ContextEnabledEditScope has the following behaviour : - When it is disabled due to the context not matching, it is drawn with a yellow "context sensitive" strike-through. - In that state, the user may press "D", which will disable the connected PatternMatch node, turning the strike-through black - denoting "always disabled". Pressing "D" again will return to the yellow strike-through by enabling the PatternMatch node again. - When the node is enabled due to the context matching is displays as usual. And "D" will again give it a black strike-through by disabling the PatternMatch node.
Bringing it into line with other AuxiliaryNodeGadget setups.
johnhaddon
force-pushed
the
patternMatch
branch
from
5de16f9
to
91f02c8
Compare
Done in 91f02c8. Merging... |
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 adds a PatternMatch node which is useful as an enabler of other nodes, particularly by matching against the current context. It also customises the GraphEditor logic to provide improved feedback on such setups using color-coded strike-throughs, and to improve interaction with them in the D shortcut by "disabling the enabler". More thorough details in the final commit message.