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

LightEditor : Fix missing icons for groups #5932

Merged
merged 1 commit into from
Jul 9, 2024

Conversation

ericmehl
Copy link
Collaborator

@ericmehl ericmehl commented Jul 1, 2024

This fixes a regression introduced in 1.4.8.0 when we added light blockers and filters to the Light Editor. In 85d93d3, I hid the mute / solo icon for anything except lights, including groups. But we can mute and solo groups, so we should see the icon for that.

Additionally, the behavior for toggling was different. We allowed toggling a group's mute / solo status if it was an ancestor of at least one light. This meant you could toggle a group if it had a child light, but not see a visual indication that anything had changed.

There are two options for matching up the behavior to the UI and also showing mute / solo icons on groups :

  1. Show the icon for anything except light filters, and allow toggling on anything except light filters.
  2. Show the icon for lights and locations that are ancestors for lights. Similar for toggling behavior.

I went with option 1 in this PR, on the reasoning that it's simpler for the user to understand. It does mean that you can toggle a group containing only light filters / blockers, which may be somewhat counter-intuitive. But groups containing light filters and those containing lights are not different in function or concept, so this seems like a reasonable choice to me.

Happy to discuss further, especially as it may relate to the Light Editor + Pass Editor unification @murraystevenson is working on.

Checklist

  • I have read the contribution guidelines.
  • I have updated the documentation, if applicable.
  • I have tested my change(s) in the test suite, and added new test cases where necessary.
  • My code follows the Gaffer project's prevailing coding style and conventions.

@johnhaddon
Copy link
Member

There are two options for matching up the behavior to the UI and also showing mute / solo icons on groups

On further reflection (sorry I didn't come up with this yesterday), I think there's a third option. The main principle of the LightEditor is that it shows you the ground truth of the scene data. Since it's possible for the mute attribute to be authored on light filters, I think it's misleading not to show it if it's there. So I'd argue that there should be no special cases at all for how we display the attribute. What that means for editing I'm not sure. Seems like a good topic for today's catch up...

@ericmehl ericmehl changed the base branch from main to 1.4_maintenance July 2, 2024 21:18
@ericmehl
Copy link
Collaborator Author

ericmehl commented Jul 2, 2024

In our call today, we decided that it's better to show the mute and solo icons for all locations, including blockers, to represent the true state of the attribute / set membership. We also decided to allow toggling through the Light Editor to eliminate the special cases and to keep the interaction and presentation equal.

My latest push does those two things, and fixes a bug that was present before but we weren't hitting because of the mute disabling. We were blindly returning the filteredLights plug as the attribute source on a LightFilter node for any attribute queried. That led to the very confusing behavior of showing the filteredLights plug when editing the mute column for a light blocker.

In e6fabdf I check to make sure the attribute is filteredLights before returning the plug for it.

Ready for review!

In the process of hiding mute and solo icons for light blockers, we also
mistakenly hid them for groups. On further consideration, we also want
to show the icons for light blockers, which will eventually get mute and
solo capabilities. This restores the icons for all locations in the
Light Editor and also fixes an error causing the `filteredLights` plug
to be returned when querying any attribute from a `LightFilter`.
@johnhaddon johnhaddon merged commit 02447c7 into GafferHQ:1.4_maintenance Jul 9, 2024
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.

2 participants