Render Pass Menu #6177
Render Pass Menu #6177
Conversation
Rather than brute-forcing this via `connectFront`, we now ensure that we hold a strong reference to ScriptWindows created via `acquire()` when we have connections to an application's `scripts` plug childAdded and removed signals.
This will allow them to be reused elsewhere, such as in MenuButtons without causing them to become overly large.
Our default style stands out as a bit bright against the dark MenuBar background. While only necessary for the RenderPassChooserWidget, this has been applied generally so any future MenuButtons also get the same treatment.
Pipelines can register render adaptors to `client = "RenderPassWedge"` that disable render passes. We want to display the state of these to the user and distinguish them from regular user-disabled render passes.
As render passes could have been deleted by a render adaptor, we build our RenderPassPaths from a PathMatcher generated with the render adaptors disabled and then later test to see whether the render pass still exists with adaptors enabled. From the end user's perspective, there's no functional difference between render passes deleted or disabled by a render adaptor, so we present both as automatically disabled.
Whitespace fix
|
This looks awesome @murraystevenson - thanks for the extra work to disambiguate adaptor-disabled passes, this will be a huge confusion saver for us. |
There was a problem hiding this comment.
Thanks Murray!
This is a nice addition, and folks seem very pleased about the new disabled-by-adaptor display state.
the interplay between the widgets and their respective plugs on the ScriptNode and CompoundEditor still irks me a little, so suggestions are very welcome.
I think the general approach is decent here given all the constraints, but I've made some suggestions for shuffling the code around in a way that hopefully encapsulates things in one place a little better.
I've made a whole load of other comments too - hopefully some of them are useful at least....
Cheers...
John
| def connect( cls, applicationRoot ) : | ||
|
|
||
| applicationRoot["scripts"].childAddedSignal().connectFront( ScriptWindow.__scriptAdded ) | ||
| applicationRoot["scripts"].childRemovedSignal().connect( ScriptWindow.__staticScriptRemoved ) | ||
| ScriptWindow.__childAddedConnection = applicationRoot["scripts"].childAddedSignal().connect( ScriptWindow.__scriptAdded ) | ||
| ScriptWindow.__childRemovedConnection = applicationRoot["scripts"].childRemovedSignal().connect( ScriptWindow.__staticScriptRemoved ) | ||
|
|
||
| @staticmethod | ||
| def __connected() : | ||
|
|
||
| return ScriptWindow.__childAddedConnection is not None and ScriptWindow.__childRemovedConnection is not None and ScriptWindow.__childAddedConnection.connected() and ScriptWindow.__childRemovedConnection.connected() |
There was a problem hiding this comment.
There can be multiple applications - each with their own ApplicationRoot - running in a single process, and connect() may not have been called for all of them. So I think we need to track whether or not we've connected to each specific application, rather than a generic "we have connected to something". Perhaps we can store the connections as applicationRoot.__childAddedConnection etc?
|
|
||
| currentRenderPass = renderPassPlug["value"].getValue() | ||
| renderPassPlug["value"].setValue( selectedPassNames[0] if selectedPassNames[0] != currentRenderPass else "" ) | ||
| GafferSceneUI.ScriptNodeAlgo.setCurrentRenderPass( |
There was a problem hiding this comment.
Should this be wrapped in an UndoScope? It wasn't before, and it does kindof feel like "UI state" that you wouldn't expect to be undoable, but it is actually editing a plug in the script, which generally speaking should be undoable.
| for( NameValuePlug::Iterator it( script->variablesPlug() ); !it.done(); ++it ) | ||
| { | ||
| if( (*it)->getName() == "renderPass" ) |
There was a problem hiding this comment.
Can we just use script->variablesPlug()->getChild<NameValuePlug>( "renderPass" ) here instead of doing our own search?
|
|
||
| void ScriptNodeAlgo::setCurrentRenderPass( Gaffer::ScriptNode *script, std::string renderPass ) | ||
| { | ||
| if( auto renderPassPlug = acquireRenderPassPlug( script ) ) |
There was a problem hiding this comment.
Is this check redundant? Doesn't acquireRenderPassPlug() always return non-null when createIfMissing = true?
| @@ -11,6 +11,7 @@ Features | |||
| - Inference : Loads ONNX models and performance inference using an array of input tensors. | |||
| - ImageToTensor : Converts images to tensors for use with the Inference node. | |||
| - TensorToImage : Converts tensors back to images following inference. | |||
| - RenderPassChooserWidget : Added a "Render Pass" menu to the Menu Bar that can be used to choose the current render pass from those available from the scene output from the focus node. | |||
There was a problem hiding this comment.
RenderPassChooserWidget
I wonder if this is a bit too API-ey to use as a category here. Maybe just "Menu Bar"? We could always add an API entry to mention the RenderPassChooserWidget.
from those available from the scene output from the focus node.
Quite a lot of froms! Maybe just "choose the current render pass from those provided by the focus node"?
| "/Display Grouped", | ||
| { | ||
| "checkBox" : self.__displayGrouped, | ||
| "command" : lambda checked : self.__setDisplayGrouped( checked ), |
There was a problem hiding this comment.
I think this is creating a circular reference. I've seen some of the dreaded "C++ object already deleted" warnings during testing, and this seems like the most likely explanation.
| "/Hide Disabled", | ||
| { | ||
| "checkBox" : self.__hideDisabled, | ||
| "command" : lambda checked : self.__setHideDisabled( checked ), |
There was a problem hiding this comment.
As above - I think we need a WeakMethod here rather than a lambda.
Changes.md
Outdated
| @@ -20,6 +20,7 @@ Improvements | |||
| - PlugLayout : | |||
| - A warning widget is now displayed when an invalid custom widget is registered. | |||
| - `layout:customWidget:<name>:width` and `layout:customWidget:<name>:minimumWidth` metadata registrations are now supported for custom widgets. | |||
| - RenderPassEditor / RenderPassChooserWidget : Render passes disabled by render adaptors registered to `client = "RenderPassWedge"` are now shown as disabled. To differentiate these from user disabled render passes, an orange dot is shown in the corner of the disabled icon and the tooltip describes them as automatically disabled. | |||
There was a problem hiding this comment.
Maybe we don't need to mention the RenderPassChooserWidget here, since it is in Features?
| const bool disableAdaptors = true; | ||
| scopedContext.set<bool>( g_disableAdaptorsContextName, &disableAdaptors ); |
There was a problem hiding this comment.
Could I make a case for having an enableAdaptors context variable instead?
- We generally try to avoid inverted "true means off" semantics.
- I think adaptors being off by default when using
RenderPassPath::context()directly probably makes the most sense - because RenderPasses provide the "recipe" for adaptors, we want to see that recipe rather than the end result in the RenderPassEditor. If I made an adaptor that monkied withrenderPass:inclusions, I don't think that should be reflected in the RenderPassEditor, but I think it would be currently.
There was a problem hiding this comment.
I also wonder if the context monkeying might be better off in the RenderPassNameColumn, so RenderPassPath is a bit conceptually purer, and the adaptor juggling is in higher-level parts of the UI. I suppose RenderPassPath is already a bit UI-focussed what with the grouping options, but it still feels like maybe it'd be nice to keep it closer to just a raw Path-based view onto a single scene.
| for plug in self.getPlugs() : | ||
| plug.setValue( renderPass ) |
There was a problem hiding this comment.
This definitely feels like it should be undoable when we're editing in the Settings window. It does feel a bit less like it should be undoable when editing in the main menu though. Should we make it undoable everywhere, or fudge it?
There was a problem hiding this comment.
I suppose the other precedent here is that ScriptNode.frame plug, which gets edited by the Timeline without creating an undo entry. But that's somewhat hidden, whereas in this case we're clearly making a direct edit to a plug on the ScriptNode.
This adds a widget for selecting the current render pass to the Menu Bar. This is added as a customWidget registration that hosts a PlugValueWidget for the
script.variables.renderPassplug. Of the few approaches that we've tried, this does seem the best, though the interplay between the widgets and their respective plugs on the ScriptNode and CompoundEditor still irks me a little, so suggestions are very welcome.Given the introduction of #6030, it felt necessary to preview to users that the render passes they are interacting with in the menu may in fact be later disabled or deleted by a render adaptor. We present these to the user by overlaying an orange dot on the regular "disabled" render pass icon, and introducing some tooltips to disambiguate render passes automatically disabled by a render adaptor from those manually disabled in the scene. It made sense to adjust the RenderPassEditor's display of disabled render passes at the same time to keep the presentation in sync.