-
Notifications
You must be signed in to change notification settings - Fork 207
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Render Pass Menu #6177
Render Pass Menu #6177
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
for plug in self.getPlugs() : | ||
plug.setValue( renderPass ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, as mentioned above I've gone with undoable everywhere in 9f04c49. It might be a little odd in the main menu next to the not-undoable global edit target menu, but it still seems better than having some ways of setting the current render pass being undoable while others are not? Give it a try and let me know if you think otherwise.
Thanks for the updates!
Ooh, yeah, this does feel better - my vote is to go with it. I think that means we don't need the ScriptWindow changes any more? Perhaps we hold those back one minor version just so there's less going into this release at the last minute? |
cb910e8
to
a41d9b2
Compare
Cool, I've included the last commit in the squash and rebase. Ready for merging.
As discussed offline, we'd still need the ScriptWindow changes in order to prevent the |
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.
We often use 14px high icons in these, but some standard icons such as warningSmall.png are only 12px high. This can result in a difference in height when switching icons on some desktop environments, so we set a min height of 14px to ensure consistency when displaying shorter icons. We also apply this to the EditScopePlugValueWidget and _RenderPassPlugValueWidget to maintain the same height consistency when they are used in other parts of the UI.
a41d9b2
to
742eb0d
Compare
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.renderPass
plug. 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.