Light Linking Editor

[murraystevenson]

This adds a new editor for inspecting and editing light links in a scene. It displays lights and sets containing lights in separate tabs on the left, and objects and light filters in separate tabs on the right. The lights and sets can be optionally filtered to only display those linked to or filtered by the selected objects and light filters, this occurs in the background to keep the UI responsive in heavy scenes with large selections of objects and light filters. This would be another beneficiary of a PathListingWidget with an embedded BusyWidget, once we figure out where to put one...

[johnhaddon]

Thanks Murray! I think this is going to be jolly handy for a lot of folks.

I've made a few comments on the code inline - hopefully they are all pretty straightforward. But I think what I want to talk about most is my own first awkward attempt at using the editor interactively. I gave myself the job of adding a couple of special lights to the eyes of the robot, and my process went like this...

• Switch to the linking editor. I can see everything there. Cool.
• Select the eyes using the Viewer. I can see the objects selected and expanded in the Objects tab. So far so good.
• What next?
  • I can see the light I want, but if I select/drag it, I'll lose my visual reference in the Objects tab because the selection is gone. I also need to drag it to two different cells, but don't want to have to repeat myself.
  • I could edit the linkedLights cells directly instead, but they're not selected - the name column is. How to convert the selection to the cells I want without doing it manually?

Was I holding it wrong? This may be frustrating given our prior conversations, but I'm kindof wondering if we should allow selections on the left and the right to peacefully coexist. And then have buttons for "link selected lights to selected objects" and so on...

Cheers...
John

[johnhaddon]

I assume this is so the PathListingWidgets don't have "ghost" selections for objects they are not showing? If so, it seems like this isn't watertight because in __transferSelectionFromScriptNode() we can introduce ghosts. If that's right, what's the benefit in having it be clean only some of the time?

Here's one odd interaction that stems from this :

1. Left-click to select a light in the Lights tab.
2. Ctrl+left click to select an object in the Objects tab.
3. The light remains selected in the scene, but not in the Lights tab.

[murraystevenson]

Yep, this was an overzealous and blunt attempt to address the "ghost" selection concerns, but but as you point out, it's not super useful. I've allowed Ctrl-click to accumulate selection between the different PathListingWidgets in 0fc7370.

[johnhaddon]

I might have got the wrong end of the stick here, but it feels like the selection handling is more complex than it needs to be. If we're saying that the ScriptNodeAlgo selection is the source of truth, then shouldn't __transferSelectionFromScriptNode() be sufficient to synchronize the other editors when any editor calls ScriptNodeAlgo.setSelectedPaths()? At the moment we're blocking the connection for that update and then doing a manual update ourselves.

[murraystevenson]

Yeah, this could stand to be a bit simpler. What we get out of this currently is scrollToFirst = False when the selection change originates from one of the editor's pathListingWidgets, but more importantly, blocking the selection filtering update when selection changes in the lights pathListing (so if you're only displaying lights linked to a selected location, selecting one of those lights doesn't update the filter and immediately change the lights being shown). I'll have a think about how to restructure this as it's likely useful to prevent that same update when one of those filtered lights are selected in the viewer.

[murraystevenson]

I've improved the selection syncing a bit in 2fe2b2a. This allows selection in the various pathListingWidgets to be treated more independently, where the scriptNode selection is partitioned between each, and then combined when selection needs to be synched back to the scriptNode. This removes a bit of the state stored for the onlyLinkedToSelection filter, and allows objects to be selected without clearing the lights selection, and vice-versa. The objects and lightFilters selections still clear each other, but that seemed reasonable as they're on different tabs and aren't interacted with concurrently.

It would be nice for this to go further and have __selectionChanged() only call GafferSceneUI.ScriptNodeAlgo.setSelectedPaths() without the blocked connection, but that runs into issues when cells are selected in columns other than the name column. In that case, we'd sync selection with the ScriptNode, which would then clear the other column selection in __transferSelectionFromScriptNode().

[murraystevenson]

Thanks for the input! I've added some link/unlink buttons in b363c14, with some SetExpressionAlgo supporting functionality in 5c05129, and a couple of fixes I spotted along the way in f920a22 and d63a267.

It'd be worth you having a bit of a play with this so we can chat about it when I'm online tomorrow, might still be worth making selections on the left and right sides coexist more peacefully. Currently selection on both sides is possible with the Ctrl-click gambit but it is a little bit inelegant, maybe there is a world where both sides' selection could just accumulate?

I've left the "Append" link/unlink button modes deliberately simple for now, we could potentially attempt to avoid creating redundant edits for locations where the lights to be linked are already included via another set in the incoming set expression, but that starts venturing into mind reading territory or results in edits that may only be valid for the current context.

The status bar and button tooltip wording might do with a bit of refinement, and there's still a weird visual quirk still to be tracked down tomorrow with the "mode" and "attribute" PlugValueWidget widths (if you switch the LightLinkingEditor's local Edit Target to something other than following the Global Edit Target, those widgets truncate to what looks like a width inclusive of their label width, while the original width they're created at appears exclusive of their label width. The issue goes away when these widgets have no label).

[johnhaddon]

Thanks for the update Murray - I've made a couple of drive by comments as I started looking at this again, but have hit home time before getting to the end. Hopefully they're somewhat useful in the meantime...

[murraystevenson]

there's still a weird visual quirk still to be tracked down tomorrow with the "mode" and "attribute" PlugValueWidget widths (if you switch the LightLinkingEditor's local Edit Target to something other than following the Global Edit Target, those widgets truncate to what looks like a width inclusive of their label width, while the original width they're created at appears exclusive of their label width. The issue goes away when these widgets have no label).

Tracked this one down to PlugLayout inconsistently applying "width" metadata to PlugValueWidgets with labels, setting width on the PlugValueWidget when first created, but later updates set width on the parent PlugWidget. A fix and some tests added in 73acd9f.

[johnhaddon]

Thanks Murray - the buttons definitely feel like what folks will be wanting from this, and I think the way you've presented it as just two buttons and mode/target makes a lot of sense - better than lots of hard-to-distinguish buttons. Few minor comments inline, but I think we're very close - feel free to squash everything down as you go...

[johnhaddon]

I think there's a path out to __adaptedIn that bypasses this :

In case it's useful, here's my cheeky little script for viewing the editor settings as a node graph :

import functools

import GafferUI

def __popupMenu( menuDefinition, plugValueWidget ) :


	settings = plugValueWidget.getPlug().ancestor( GafferUI.Editor.Settings )
	if settings is None :
		return

	menuDefinition.append(
		"/Show Settings Graph...",
		{
			"command" : functools.partial( plugValueWidget.scriptNode().addChild, settings )
		}
	)

GafferUI.PlugValueWidget.popupMenuSignal().connect( __popupMenu )

[murraystevenson]

Thanks, I've plugged the leak in 77a6b9f. That commit also fixes an issue where the object and lightFilter existence queries that enable the Collect were querying existence fromthe filtered objects and lightFilters scenes rather than the unfiltered ones, so filtering those pathListingWidgets could affect the lights displayed by the onlyLinkedToSelection filter if the currently selected objects or lightFilters were removed by the filter.

I also adjusted the priority of objects vs lightFilters in 6ce871e, otherwise a location that was in both the objects and light filters path listings (such as the ancestor of both an object and a light filter) would be considered a light filter for the purposes of the onlyLinkedToSelection filtering.

[johnhaddon]

I'm getting some poor vertical alignment between the labels and the dropdown menus :

[murraystevenson]

I'd used some spacers to keep the footer row heights consistent between both sides of the SplitContainer. These were slightly too tall so I've cut them down to size in a9943ff.

[johnhaddon]

Not the most important thing, but it's a bit weird that we include the Lights and Light Filter sets in the filter menu, when we know we've pruned those out :

[murraystevenson]

I've given this a go by adding some excludedSetNames metadata to SceneEditor._SetFilterPlugValueWidget in e51cdb2. We can't filter the menu items entirely by the available set names, as DeleteSets deliberately prevents deletion of __cameras, __lights and __lightFilters, but an alternative would be to omit empty sets from the list...

[johnhaddon]

I'm not 100% convinced by the usage of the + and - icons here. Everywhere else I can think of them being below a PathListingWidget, they add and remove items from the listing itself. I know you tried a bunch of icons ideas already though. Did they include anything based on 🔗 ?

[murraystevenson]

It's been a bit challenging to convey distinct behaviour in 16 pixels while still making it feel like a button, but I've tried a combo of 🔗 with +/- in faf9f82, which seems to work ok?

[johnhaddon]

Might be worth specifying "labelPlugValueWidget:showValueChangedIndicator" : False to get rid of the green dots from these?

[murraystevenson]

Good idea, added in 056b763.

[murraystevenson]

Comments addressed inline, I've also pushed 68754f2 and af3396a which improve the editing behaviour a little. We now check for any warnings up-front and only edit in situations where there aren't any warnings, rather than partially editing and warning on the rest. This better matches the edit behaviour of InspectorColumn when multiple cells are selected, where we only show the PlugPopup if there are no edit warnings.