More light handles #5483

ericmehl · 2023-09-29T15:06:38Z

This is the start of the remaining light tool handles that will cover sphere / point lights, cylinder lights and disk lights.

It's a work in progress currently but could probably benefit from a look at the last commit in particular where I did a major refactoring to make LightToolHandle useful for all light types and refined the public interface.

Based in interactive testing, all is working fine so this is meant to be a purely architectural change at this point, with the new lights coming either later in this PR lifetime or separately if that works better.

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

Thanks Eric! I haven't by any means done a detailed review, but I've tried to get the gist of the LightToolHandle refactor, and made a few comments that occurred to me. If I've understood it correctly, then it all seems broadly positive, although we might also want to get @danieldresser-ie to cast an eye over it since he reviewed the original more thoroughly than me. Perhaps it makes sense to do that once you've got the new handles added as well?

johnhaddon · 2023-10-02T09:59:11Z

src/GafferSceneUI/LightTool.cpp

@@ -104,6 +104,8 @@ using namespace GafferSceneUI::Private;
 namespace
 {

+const std::string g_lightAttributePattern = "*light";


I think "light *:light" (used with matchMultiple()) would be safer - it seems like other attributes could fairly easily end in light. One obvious way is via the custom attributeSuffix that you get to define when assigning an Arnold gobo.

Ah yes, that would be a problem. Fixed in ba2589c

johnhaddon · 2023-10-02T10:14:53Z

src/GafferSceneUI/LightTool.cpp

+		// May be overriden to update internal state based on the `scenePath`
+		virtual void setScenePathInternal( ScenePathPtr scenePath )


The function signature and naming here seems misleading - the function isn't responsible for setting the scene path, and scenePath is already available from getScenePath(). Perhaps updateFromScenePath() or scenePathChanged() would be more accurate?

Right, fixed in aee8ea2, but also modifed in 82e3677.

johnhaddon · 2023-10-02T10:16:41Z

src/GafferSceneUI/LightTool.cpp

 		}

-		Plug *editScope() const
+		ScenePath *getScenePath() const


Should this be returning const ScenePath *, so nobody can mess with it? I wonder if get/setHandlePath() would be a better name - that seems to add a bit more information about the purpose of the path without being any longer. The fact that the corresponding member data includes "handle" suggests that it might have been a useful distinction to you already?

In 82e3677 I did a broader reworking that I think is necessary. Here's my reasoning :

Returning const ScenePath * does sound good, but the return value of that method was being used to construct a ParameterInspector. ParameterInspector wants a const reference to the ScenePlugPtr which can't be implicitly created from a `const ScenePath *

I'm not actually sure why this was both compiling and not crashing before.

I think a better solution is in 82e3677 where I have LightToolHandle own the pointer to the ScenePlug in m_scene, hold a non-owning pointer the context and hold the ScenePlug::PathPlug. All separately rather than in a ScenePath object.

I think that's the most correct way to handle the ScenePlug and keep ParameterInspector working right. It also has a side benefit of being a bit more direct with the context and handle path, rather than getting it from a ScenePath every time.

I also changed the name to updateHandlePath(). I'm not 100% sold on that name because it kind of wraps up a ScenePlug, Context and ScenePlug::Path all into the concept of a "path", but maybe the precedent of ScenePath class justifies that?

johnhaddon · 2023-10-02T10:18:46Z

src/GafferSceneUI/LightTool.cpp

+		// Derived classes can optionally extend behavior by first calling the base class method
+		// and performing more tests if needed to determine visibility for the `scenePath` in the
+		// current context.


This documentation seems inaccurate, since visible() isn't virtual.. Perhaps the public visible() function should be documented for public consumption, and the documentation about derived class overrides should go on the visibleInternal() method?

I tried making things a bit clearer in c12d27e and 98d314c

johnhaddon · 2023-10-02T10:22:11Z

src/GafferSceneUI/LightTool.cpp

+		// May be overriden to clean up internal state after a drag.
+		virtual bool handleDragEndInternal()
+		{
+			return false;


A whole load of these virtual functions intended to be overridden are in the private section, but I'd expect them to be in the protected section.

I think the class as a whole could benefit from some documentation about which bits of the API are intended for who. I think it goes like this :

The public methods are largely for consumption by LightTool, and are used by it to coordinate event handling and drawing for all of the individual handles, without needing to know any implementation details for specific handle types. They're following the "non-virtual interface" pattern.

The protected virtual methods (and the private ones I think should be protected) are for derived classes to use in customising the behaviour of specific handle types.

Is that right?

By way of documenting our offline conversation, I was originally making these private in the vein discussed here : http://www.gotw.ca/publications/mill18.htm where implementation, even for virtual methods, are encouraged to be kept private unless they need to be called from a derived class, in which case they would be protected.

That doesn't fit with the pattern in other classes though, and could also cause problems should we ever want to bind them in Python. So I've made them protected in 98d314c

johnhaddon · 2023-10-02T10:30:25Z

src/GafferSceneUI/LightTool.cpp

+
+		void setScenePathInternal( ScenePathPtr scenePath ) override
+		{
+			ConstCompoundObjectPtr attributes = getScenePath()->getScene()->fullAttributes( getScenePath()->names() );


I think we need to be scoping a context here? Might be worth checking the other places we access the scene too - maybe worth updating your test scene to include a script-level context variable, and reference it from the graph in such a way that errors are thrown if it's not scoped?

We don't need to scope a path for fullAttributes(), do we?

Not a path, no, because that is dealt with inside fullAttributes(). I was thinking about other variables that might be in the context...

Right, I see now. Though it was motivated by setting the path, I am scoping a context at https://github.com/ericmehl/gaffer/blob/98d314c1bbc7aaa560a05047f1f80757f312d7be/src/GafferSceneUI/LightTool.cpp#L2620 which covers the updateHandlePath() and subsequent handlePathChanged() call.

But I also added a test to make sure that doesn't get lost.

johnhaddon · 2023-10-02T10:35:41Z

src/GafferSceneUI/LightTool.cpp

+			/// \todo Template the value so it can be other things than `float`?
+			float value;


I'm afraid I haven't followed through the code far enough to know if the value here always corresponds to inspection->value(). But if it doesn't, I think there needs to be some documentation clarifying why not, and if it does, then it seems a bit redundant and prone to falling out of sync. If the latter, maybe a templated typedValue() method on Inspector::Result itself might make life easier, and mean we could drop Inspector::Result completely?

Yes, I think you're right that value could be removed. value is only ever set in combination with its corresponding Inspector::Result. I think this is an artifact from when I was under the wrong impression that Inspector::Result::value() was "live" and would return the current state of the inspected source plug.

A signature for Inspector::Result::typedValue() would be something like

template<typename T> std::optional<T> typedValue() const

?

The std::optional to account for the possibility the data isn't the type that was requested.

I think maybe for our current use cases we always want to get a result, and we also have a default value in mind? So perhaps more like this?

template<typename T> T typedValue( const T&defaultValue ) const;

Cool, I'll add that and make sure that the value can come from the new typedValue() and not cause unexpected side effects.

I added the typedValue() method in 3b471e4. And in 0b95883 I made use of it and removed the InspectionInfo struct.

I'm quite happy with this change, and it all checks out in the interactive testing. Removing InspectionInfo was nice, I removed the std::optional returns from inspectionInfo() related methods and test against a nullptr instead. I also got rid of the general xxxInfo naming conventions which I had never been too happy with. So overall it's a very nice change!

I didn't make any tests for typedValue() yet - I wanted to make sure the technique worked for LightToolHandle to begin with. If we're happy with this direction, it's probably worth adding some tests to 3b471e4.

johnhaddon · 2023-10-02T10:36:40Z

src/GafferSceneUI/LightTool.cpp

+		// Returns true and sets `info` if an inspection for the given `metaParameter` in the
+		// current contextexists. Returns false if no inspector or inspection result is available
+		// for `metaParameter`.
+		bool inspectionInfo( const InternedString &metaParameter, InspectionInfo &info ) const


Perhaps returning std::optional here would be a better interface than the bool return and info out parameter? It would make it impossible to use a stale info when false was returned.

I made the std::optional change in 02923ef, but went what I think is one better in 0b95883 and return nullptr to indicate no inspection is available.

Oh yep, loads better! Nice one!

ericmehl · 2023-10-04T15:09:17Z

Along with the new commits commented on in the inline discussions, I pushed 9dfef01 which tidies up the tooltips. Part of that is a new virtual method LightToolHandle::updateTooltipPosition() which lets the derived classes set the position.

When looking through a spot light, the top position is more stable
The tip for all lights move along with the mouse during drag (previously the spot light tool tip didn't move)
I kept the arcs drawn on a spotlight during drag fixed at the start. Having them moved seemed a bit busy to me but I'm happy reconsidering.

ericmehl · 2023-10-04T15:10:05Z

All of my interactive tests are passing now, so I'm going to move on to the handles for disk, sphere / point and cylinder now. I can revisit any of this as needed of course.

johnhaddon · 2023-10-04T16:01:01Z

All of my interactive tests are passing now, so I'm going to move on to the handles for disk, sphere / point and cylinder now.

Sounds good to me!

ericmehl · 2023-10-09T22:08:33Z

The latest set of commits adds handles for disk lights and cylinder lights. Now that there is some new function, it might be a good time to get your thoughts @murraystevenson.

Up next is the sphere / point light, then total light manipulator nirvana.

murraystevenson · 2023-10-11T05:59:15Z

Thanks Eric! Here's a rough list of thoughts after an initial round of testing:

Handles are no longer available for USD SpotLights.

Applying a scale transform to a DiskLight or CylinderLight can make for an overly scaled radius handle.

With the change in tool-tip behaviour tool-tip placement can now feel a bit disconnected while holding shift to make a precise drag. I also hit a number of situations where the tool-tip position wouldn't update and would stay at the starting position of the drag, but I'm not sure on how to replicate that at the moment.

toolTipShift.mp4

CylinderLight:

All the interaction occurs at the ends of the light, which can make it tricky to select the length handle and not the radius handle when the light is viewed side-on. The radius handle interaction also starts feeling a bit imprecise when viewed side-on, sometimes I'd have to stop to figure out how to drag to reduce the radius as what seemed the intuitive direction wasn't actually working as expected and I'd hit a limit where the radius wouldn't decrease. Compared to the Radius handle, the RectLight width/height handle interactions feel a lot more solid at odd angles.

DiskLight:

Radius handle drag feels better here as you're more likely to be viewing more of the face of the light while interacting with it, though the unclamped drag past zero does feel inconsistent compared to what we're doing with the other light handles.

ericmehl · 2023-10-17T21:46:47Z

Thanks for the user feedback Murray! I have those changes made and it's much improved. I have those and my own usability tests passing nicely, and I've squashed a lot of commits down to make for a large but hopefully fairly logical history.

The big refactor in the second commit makes way for much more reasonable handle additions for the new light handle types.

With those in place, and provided CI is happy, it's ready for a new usability and code review.

murraystevenson · 2023-10-20T01:13:25Z

These feel a lot more solid to me, nice work Eric!

We'll also need to re-enable the spotlight handle for USD DiskLights with "shaping:cone:angle" applied. Gaffer's USD "SpotLight" is based on a SphereLight with a cone, but I've seen other DCCs produce SpotLights based on DiskLights. Technically, you can put a cone on anything and we should be able to manipulate any cone we can correctly visualise, but Sphere and DiskLights are the two most common cases...

This falls outside of the scope of this PR but it might be nice to offset the rays drawn for the CylinderLight visualiser so they don't overlap with the new radius handles? I've mocked it up with via the visualiser scale here.

cylinderLightVisualiser.mp4

danieldresser-ie

Had a look through to see if I could spot any math nitpicks in the low level code, but didn't find much - I'd probably have to actually play with it interactively if I wanted to understand what some of this means, but I think other folks probably have that part covered.

include/GafferSceneUI/Private/Inspector.inl

src/GafferSceneUI/LightTool.cpp

johnhaddon

Thanks Eric! Bit of a superficial-friday-afternoon-review from me I'm afraid, but this all seems to be working nicely, and the refactor seems to have done a good job of simplifying the dropping in of new handle types, so let's get it merged. Couple of minor comments inline.

Minor nitpick on the presentation : is it possible to depth cull the cylinder handles against the cylinder visualisation, or backface cull them some other way? I was finding the "backfacing" handles a bit distracting. Not a big deal though, and can totally be done separate to this PR.

include/GafferSceneUI/Private/Inspector.h

src/GafferSceneUI/LightTool.cpp

ericmehl · 2023-10-20T21:20:28Z

Thanks for having a look everyone!

Technically, you can put a cone on anything

I went ahead and added the cone handles to disk lights and also quad and distant lights. Those all draw correctly, but the cylinder light does not - it doesn't take account of the orientation metadata to transform the cone. If it makes more sense to remove the quad and distant, I'm happy to do that too.

That is squashed into 3ba375d.

and can totally be done separate to this PR.

I think I might go that route. I have got a start on it this afternoon, but I think there are enough details to keep straight that it's maybe better to not accumulate even more onto this PR.

Otherwise I think I've taken care of the comments so it's ready for hopefully a final look and squash!

johnhaddon · 2023-10-23T13:53:39Z

I think I've taken care of the comments so it's ready for hopefully a final look and squash!

Thanks Eric! Could you give it the old squash and merge please?

Though a cylinder light can have a cone attached to it, we don't currently draw the cone correctly in that case. I'm leaving the handles off for cylinder lights for now.

ericmehl · 2023-10-23T21:12:13Z

I double checked to make sure the Gaffer-created USD quad, disk, sphere and distant lights all work as expected, and they're good to go. Squashed... and merged!

johnhaddon reviewed Oct 2, 2023

View reviewed changes

ericmehl force-pushed the moreLightHandles branch 3 times, most recently from edc7f3d to 9dfef01 Compare October 4, 2023 14:57

ericmehl requested a review from murraystevenson October 9, 2023 22:08

ericmehl force-pushed the moreLightHandles branch 3 times, most recently from 7270fe2 to 1b7c5c3 Compare October 17, 2023 21:07

ericmehl marked this pull request as ready for review October 17, 2023 21:46

danieldresser-ie reviewed Oct 20, 2023

View reviewed changes

include/GafferSceneUI/Private/Inspector.inl Outdated Show resolved Hide resolved

src/GafferSceneUI/LightTool.cpp Outdated Show resolved Hide resolved

johnhaddon reviewed Oct 20, 2023

View reviewed changes

include/GafferSceneUI/Private/Inspector.h Outdated Show resolved Hide resolved

src/GafferSceneUI/LightTool.cpp Outdated Show resolved Hide resolved

src/GafferSceneUI/LightTool.cpp Outdated Show resolved Hide resolved

ericmehl force-pushed the moreLightHandles branch from dac26d5 to e5bd74c Compare October 20, 2023 19:29

ericmehl force-pushed the moreLightHandles branch from e5bd74c to a4aa6ab Compare October 23, 2023 14:04

ericmehl added 8 commits October 23, 2023 10:49

Inspector : Add typedValue() to Result

5533179

LightToolHandle : Separate responsibilities better

25afd7c

LightTool : Move tooltip constants for clarity

fe5bbd8

LightTool : Add RadiusHandle

a778655

LightTool : Add LengthHandle

e62f4ed

EdgeHandle : Generalize for cylinder radius

5c09fc7

SpotLightHandle : Tooltip track cone, not cursor

f7c55e9

EdgeHandle : Tooltip track edge, not cursor

d212df2

SpotLightHandle : Enable for Gaffer USD lights

b43b0e5

Though a cylinder light can have a cone attached to it, we don't currently draw the cone correctly in that case. I'm leaving the handles off for cylinder lights for now.

ericmehl force-pushed the moreLightHandles branch from a4aa6ab to b43b0e5 Compare October 23, 2023 14:51

ericmehl merged commit 5de3d6e into GafferHQ:1.3_maintenance Oct 23, 2023
4 checks passed

ericmehl mentioned this pull request Oct 27, 2023

LightTool : Remove unused variables #5517

Closed

4 tasks

ericmehl deleted the moreLightHandles branch October 27, 2023 15:17

		// May be overriden to update internal state based on the `scenePath`
		virtual void setScenePathInternal( ScenePathPtr scenePath )

		/// \todo Template the value so it can be other things than `float`?
		float value;

More light handles #5483

More light handles #5483

Conversation

ericmehl commented Sep 29, 2023

Checklist

johnhaddon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ericmehl commented Oct 4, 2023

ericmehl commented Oct 4, 2023

johnhaddon commented Oct 4, 2023

ericmehl commented Oct 9, 2023

murraystevenson commented Oct 11, 2023

ericmehl commented Oct 17, 2023

murraystevenson commented Oct 20, 2023

danieldresser-ie left a comment

Choose a reason for hiding this comment

johnhaddon left a comment

Choose a reason for hiding this comment

ericmehl commented Oct 20, 2023

johnhaddon commented Oct 23, 2023

ericmehl commented Oct 23, 2023