Skip to content
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

Merged
merged 12 commits into from
Jan 17, 2025
Merged

Conversation

murraystevenson
Copy link
Contributor

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.

renderPassMenu

@murraystevenson murraystevenson self-assigned this Dec 10, 2024
@tomc-cinesite
Copy link
Contributor

This looks awesome @murraystevenson - thanks for the extra work to disambiguate adaptor-disabled passes, this will be a huge confusion saver for us.

Copy link
Member

@johnhaddon johnhaddon left a 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

python/GafferUI/ScriptWindow.py Outdated Show resolved Hide resolved
python/GafferSceneUI/RenderPassEditor.py Outdated Show resolved Hide resolved
src/GafferSceneUI/ScriptNodeAlgo.cpp Outdated Show resolved Hide resolved
src/GafferSceneUI/ScriptNodeAlgo.cpp Outdated Show resolved Hide resolved
Changes.md Outdated Show resolved Hide resolved
python/GafferSceneUI/RenderPassEditor.py Outdated Show resolved Hide resolved
python/GafferSceneUI/RenderPassEditor.py Outdated Show resolved Hide resolved
Changes.md Outdated Show resolved Hide resolved
src/GafferSceneUIModule/RenderPassEditorBinding.cpp Outdated Show resolved Hide resolved
Comment on lines 1163 to 1164
for plug in self.getPlugs() :
plug.setValue( renderPass )
Copy link
Member

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?

Copy link
Member

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.

Copy link
Contributor Author

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.

@johnhaddon
Copy link
Member

Thanks for the updates!

after our brief musing this morning about Settings nodes in Widgets I've added a commit cb910e8 on the end that gives it a cheeky try. Overlooking any oddities of putting an Editor Settings node in a PlugValueWidget in order to reuse the existing BackgroundTask cancellation workaround, this does feel like the better approach and does address this case and the need to pass in the adapted globals plug, keeping the Widget far more self-contained.

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?

@murraystevenson
Copy link
Contributor Author

Ooh, yeah, this does feel better - my vote is to go with it.

Cool, I've included the last commit in the squash and rebase. Ready for merging.

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?

As discussed offline, we'd still need the ScriptWindow changes in order to prevent the renderPass plug acquired by the RenderPassChooserWidget being the first plug created in the script variables ahead of the plugs created in project.py so I've left it in.

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.
@johnhaddon johnhaddon merged commit 867c9cc into GafferHQ:1.5_maintenance Jan 17, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants