-
Notifications
You must be signed in to change notification settings - Fork 206
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
LightEditor support for light filters #5895
LightEditor support for light filters #5895
Conversation
509c231
to
b6d701c
Compare
I added a couple of fixes :
I kept the first two as fixups instead of squashing into the original so they can be discussed as desirable or not. Maybe there are better solutions? |
76d17e0
to
88505dd
Compare
Rebased to clean up the I also improved the naming of the scene processors created to do the edits and added the summary registration for Edit Scopes. I think that should be it for my commits for now, so ready for a review. |
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 Eric, this is going to be really useful! A couple of thoughts inline.
GafferSceneUI.LightEditor.registerShaderParameter( | ||
"ai:light", paramName, attributeName, sectionName | ||
) | ||
|
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.
What do you think about including the filteredLights
attribute on a blocker as a column? Seems like it would be very useful to be able to quickly see the lights the blocker is affecting.
I could then see people wanting to be able to right click on this and "Select Affected Objects" similarly to what we've just added to the Render Pass Editor but maybe I can take that bit on as part of the great LightEditor/RenderPassEditor refactor...
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.
Yes, that's a nice attribute to include for sure. I added that in 583ca50 and 5f0036f.
Adding "Select Affected Objects" seems nice too. I added that to keep parity with the Pass Editor (and you had already done the hard part). It was a pretty straightforward repurposing of the Pass Editor code so maybe it won't be too much trouble to unify the two. That's done in 844837e
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! I think we'll just need to add "filteredLights" to the attribute registry over in EditScopeAlgo, so this can still be edited in an EditScope when there's no upstream attribute.
There'd also be an argument for allowing this to be edited using the new SetExpressionPlugValueWidget, but I think we can handle that in a separate PR as it'll require a little additional metadata and AttributeTweaks plumbing along the lines of what was done for OptionTweaks over here: 4bc8ef6
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.
Right, that explains why I needed to set the value in the unit test. I added that registration, and removed the manual value setting in the unit test to verify the registration worked. Those are in bdf665d
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.
There'd also be an argument for allowing this to be edited using the new SetExpressionPlugValueWidget
I looked at adding that too, but I realized that it may not be as simple. If you're editing the plug on the blocker itself - not a tweak or Edit Scope - the SetExpressionPlugValueWidget
will only be somewhat useful, and might even be misleading. Since there is no input to a light blocker, the widget doesn't add anything useful by way of autocomplete or browsing existing sets.
It would be useful for modifying filteredLights
in a tweak plug / Edit Scope though. Maybe we could manage a conditional where we decide which widget to use based on the source plug?
88505dd
to
e6cdc08
Compare
Thanks @murraystevenson! Those are all nice additional points. I noted the changes in the comments, ready for a fresh look. |
e6cdc08
to
df6bcac
Compare
My latest push is a rebase onto |
df6bcac
to
bdf665d
Compare
Thanks for the additional review @murraystevenson! I squashed down the fixups from Friday and added a couple new ones. Besides the inline comment, 2f2427b fixes the test failure. Should be good to go assuming CI passes. |
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 Eric! This looks good to squash and merge.
bdf665d
to
b22623d
Compare
Great! Squashed and merged |
This adds support for editing light filters in the Light Editor. This includes blockers, which are distinct scene locations apart from lights and filters applied to lights using
ShaderAssignment
.Checklist