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

[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]

Thanks! I think it might also be worth renaming the collect: variables to be a bit more descriptive, in case we might have a leak in future. collect:value wouldn't be that helpful in a context annotation, but __lightLinkingEditorCollect:value would be a pretty effective smoking gun.

[murraystevenson]

Good call, renamed to __lightLinkingEditorCollect:value/index in 7645f93.

[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]

HierarchyView, AttributeEditor : The set filter menu now only shows the "Cameras", "Coordinate Systems", "Lights", and "Light Filters" menu items when their equivalent sets are present in the scene.

I'm not sure this is an improvement particularly - muscle-memory-wise it's nice for that set to remain the same all the time. Could we just filter that list by excludedSets but not availableSets?

[murraystevenson]

Cool, I've adjusted the filtering and removed the changelog entry in dbb1eb6.

[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.

[johnhaddon]

Thanks Murray, this is feeling much easier and more obvious to use than the first version. Couple of minor remaining things inline, but the finish line is in sight...

[johnhaddon]

I'm a little wary of per-path processing on the Python side - might be a little slow for large selections? It's pretty easy to select every object with drag-select in the Viewer.

I initially thought that SetAlgo::evaluateSetExpression() might be a good way of doing this all on the C++ side, but the problem there is getting the selection into it in a performant way. We could drop down to SetExpressionAlgo::evaluateSetExpression(), but then we're on the hook for repeating what SetAlgo would otherwise do for us. I wonder if we should have PathMatcher::containing() and PathMatcher::in() methods to go with PathMatcher::intersection()?

Out of scope for this PR probably, but might be worth it as a future improvement?

[murraystevenson]

I wonder if we should have PathMatcher::containing() and PathMatcher::in() methods to go with PathMatcher::intersection()?

Agreed, it would be useful to have these. At the time, I did try an reach for something to avoid this per-path in Python stuff but came up short. I've put up a Cortex PR for discussion ImageEngine/cortex#1550.

[johnhaddon]

Thanks! I think it might also be worth renaming the collect: variables to be a bit more descriptive, in case we might have a leak in future. collect:value wouldn't be that helpful in a context annotation, but __lightLinkingEditorCollect:value would be a pretty effective smoking gun.

[johnhaddon]

HierarchyView, AttributeEditor : The set filter menu now only shows the "Cameras", "Coordinate Systems", "Lights", and "Light Filters" menu items when their equivalent sets are present in the scene.

I'm not sure this is an improvement particularly - muscle-memory-wise it's nice for that set to remain the same all the time. Could we just filter that list by excludedSets but not availableSets?