LightPositionTool : Add tool for placing shadows

[ericmehl]

This adds a new tool to the scene viewer for placing shadows. Two points are set using Control + Click to set the pivot point and Shift + Click to set the shadow point. The selected light (or any selection, like a group) is repositioned so it lies along the line from the shadow point to the pivot, the same distance back from the pivot as its original distance from the pivot. It's also reoriented to face the shadow point.

This currently uses the last selected object as the source of the original position. I'll add a follow up commit to use the centroid of the full selection.

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.

[ericmehl]

When I use the shadow placement tool and then try and Undo, it doesn't work if the undo is targeted to the viewport. But it does if the mouse is hovering over the node graph. I think I'm getting the ScriptNode in the standard way, but maybe not. Currently it doesn't behave as the other transform tools do.

[ericmehl]

I fixed the undo behavior and in the process simplified the modifier logic. I had been tracking the "setting pivot" / "setting shadow point" state via keyPressSignal(), but handling Control was preventing Control + Z from getting its signal.

I found that MouseEvent has it's own keyboard modifier state, so I'm using that now.

[ericmehl]

I don't quite have transformations right in the case of multiple lights.

I think I'd like to find :

1. The space centered at the centroid of the light positions, oriented in the direction of the last selected light (averaging orientations seems like a tricky problem, so this is a fallback for now).
2. The space centered on the line from the shadow target to the shadow pivot, at the distance of the original centroid to the pivot. And oriented along that line with an up direction of... world Y up crossed with that line, then crossed with that line again?

Then find the transformation from space 1 to 2. I think I'm close, but my end-of-day linear algebra brain is a bit mushy. I'll return to it Monday or take any suggestions if I'm off-base.

[johnhaddon]

Thanks Eric, this is going to be very nice!

I haven't reviewed the maths side of this much at all, because you indicated you want to rework the centroid stuff, and my comments on the up-vector stuff might change things too. @danieldresser-ie might do a better job of reviewing anyway, once we've settled on the behaviour we want.

I mentioned in one comment that it'd be good to have some visual indicators for the pivot and shadow point. Here's a very quick straw-man mockup to get the conversation startup on that :

I suggest we spend a bit of time in today's meeting getting feedback from the rest of the team, and settling on something a bit more concrete...

Cheers...
John

[johnhaddon]

I'm not sure the orientation stuff is working great in this context. I tested with a spotlight with a gobo, and although the initial click keeps the gobo upright, after a while it has drifted off so it's rotated on its side. You can see the same thing with a quadlight - use a second viewer to look through the quadlight while positioning it in the other viewer, and you'll see the look-through get a twist on it.

I'm not sure what the behaviour should be if the light isn't upright (local Y aligned-ish with world Y) in the first place, but if it is upright it feels to me that it should remain that way (i.e. local Y is as aligned to world Y as it can be while still achieving the required aim).

[johnhaddon]

I'm still finding this to be the case in the latest version.

[ericmehl]

I pushed a new update just now that is mostly for checking the behavior of the positioning. I set the light-pivot distance when the pivot is set for the first time, and use that as the distance ever after (or, currently, until the selection changes or the tool is deactiveated).

That fixes the wandering light problem when changing the pivot point. My theory is that since the shadow softness is determined by the distance from the light to the pivot, this keeps a consistent sharpness when moving the pivot and shadow target.

Soon I'll add drag control to the light-pivot distance that will allow control over that distance after it's set.

I'm interested to get your feedback @murraystevenson.

A number of things are in progress :

• The drag for light-pivot distance
• Holding per-light pivot and target so it's restored by selection
• Tooltips on the cursor and target to indicate whether your click will place the target or pivot
• Probably more things...

[murraystevenson]

Thanks Eric, maintaining the distance to the pivot does feel like the intuitive thing to do. It'll be great to see this in action once we have the control for adjusting the light-pivot distance combined with remembering the pivot and target for each light. It feels like we may need to reset all state for a light if it is manually translated away from its line of shadow placement, but maintain the pivot & target while updating the light-pivot distance if the light is manually translated along the line?

You probably already have a plan for this when it comes to adding the light-pivot distance handle, but the current behaviour of the visualiser ray reacting to mouse hover is a bit distracting - the colour change makes you think the ray itself is somehow able to be interacted with, and it absorbs clicks when trying to place a target or pivot close to the ray...

[ericmehl]

My latest push has some new functionality :

• The tip of the arrow (cone) is repositioned so the apex of the cone is at the shadow target. Before the base of the cone was placed there which, besides being not quite correct, I think contributed to the handle going into hover state when you placed the target.
• I moved the viewport tip explaining how to place the target and pivot to the top.
• The other transform tools have a similar tip about using the V key.
• You can now drag the line from light to shadow target to change the distance of the light along that line. Anywhere you drag on the line will move the light relative to the start of the drag point. Is this discoverable enough? Would it be better to add a translate handle onto the light origin?
• Target and pivot points are stored based on the selection. They are now only cleared if you select a light that is not along the shadow line and pointing at the shadow target.
• Undo now applies to placing the target and pivot points as well as the actual light transformation. This is handy because of the way I'm resetting the points. Without this, if you have a shadow line, undo the last transform, then you'll also reset the shadow line since the light isn't in the place the shadow handle expects it to be.

A couple more items to go :

• Hold V and drag to continually move the shadow target. Probably for the pivot too?
• Rotate handle on the end of the shadow handle to rotate the light around the Z axis.
• Perhaps better orientation when placing the light?

[ericmehl]

I haven't been able to sort out why this is not hiding the tooltip widget in the case of activating the scale tool, which doesn't do targeting. Is there a subwidget other than self that I should be hiding instead? I always seem to get a small box regardless of what I've tried hiding.

[murraystevenson]

Awesome! This is really coming along. Here's a bit of a laundry list of nitpicky usability thoughts after trying out the latest:

Top centre could be a bit distracting for the instruction placement. They're the sort of thing you'd want to look past once you "get it" and that's a little hard to do front and centre. Maybe off to the left is better so they're more associated with the tools themselves? I'm also seeing the rest of the Viewer's top toolbar jump a bit when tools are deactivated and the instructions are no longer visible.

The translate/rotate tool instruction language of "place translation/orientation target" feels a bit off, "place target" suggests a persistent target rather than a one-off action. Maybe language along the lines of "Hold V and click to position on target" "Hold V and click to aim at target" is more clear?

We can currently translate a light past the shadow pivot while dragging along the visualiser line - I'm not sure if that makes sense to do so? Adjusting the pivot or target point afterwards results in the light moving back to the equivalent distance on the "far" side of the pivot. The visualisation line also breaks once the light passes beyond the shadow target point.

It'd be nice to have distinct cursor icon for pivot placement as a sanity check before you go and place the wrong thing. Undo definitely helps here, but I still find myself going through some mental gymnastics to ensure I'm doing the right thing while switching between pivot and target placement. A distinct icon could reinforce the learning of the behaviour and allow me to correct "in flight" rather than make a mistake, undo it, and try again...

Maybe we should make the pivot visualisation a sphere rather than a ring for less visual clutter, the line running through the ring reads a bit like a 🚫.

The line also runs through the end of the target cone on hover. Maybe this contributes to the visualisation getting in the way of making fine adjustments to the target placement?

You can now drag the line from light to shadow target to change the distance of the light along that line. Anywhere you drag on the line will move the light relative to the start of the drag point. Is this discoverable enough? Would it be better to add a translate handle onto the light origin?

I kind of want to reserve judgment on this until I see the "drag to continually move the pivot/target" behaviour. Though I think whatever we do here should be considered in terms of consistency with any other light placement tool...

[ericmehl]

Those are all nice improvements, thanks for the testing!
✅ Moved the viewer tips to the top-left.
✅ Improved the wording of the translation and rotation target tips.
✅ Limited the movement along the line to not go past the pivot point.
✅ Added a new cursor for the pivot placement mode. I went with a ^-like graphic. My thinking is that it gives an impression of pivoting / rotating / bending and also has a nice precise concept of where the target is, at the top-center of the graphic.
✅ Changed the pivot visualisation to a circle / sphere. I also made the target position, when there is no pivot, a cone oriented the same as the light. I had been doing a circle until the pivot point was chosen, but that would overlap with the pivot as a circle now. I think it still works well.
✅ Fixed the line running through the cone.

The one item I haven't sorted is the flickering when disabling the tool and the viewer tip goes away. For me it's not consistent, I get it maybe 20% of the time so I'll investigate more.

On to the rotation and target dragging now.

[ericmehl]

My latest push wraps up the final features for the shadow part of the light position tool. I added the rotate handle and continuous placement during drag, as well as a few fixes.

I will go through the code once more time to to see if there are any loose ends before ticking the Ready for review button.

[ericmehl]

I tidied up the code a bit and pushed that up. Throwing the switch to open it for review!

[ericmehl]

Pushed a small fixup to address a bad optional access bug when using the rotate handle without first dragging the shadow / pivot distance handle.

[murraystevenson]

Dragging the shadow target around a raytraced viewport is a lot of fun! The other V placement tools are missing out...

One thing noticed in usability testing is that dragging a target generates two undo events, one for the initial click and then a second for the drag, so you end up having to undo twice to revert to the previous shadow placement. Maybe not a huge deal, but it caught me off-guard the first time I tried undo-ing a dragged shadow target placement. The (single) undo didn't put the light back where it had started, as my click to instigate the drag was some distance away from the original shadow target position.

[ericmehl]

One thing noticed in usability testing is that dragging a target generates two undo events, one for the initial click and then a second for the drag, so you end up having to undo twice to revert to the previous shadow placement.

Good catch - that is odd behavior. I fixed it in 75236a8. Perhaps using TransformTool::dragEnd() is a bit questionable, since I'm doing it in buttonRelease(), but it's an expeditious way to increment m_mergeGroupId which is private to TransformTool and button presses and drags aren't completely unrelated, I figure.

[johnhaddon]

Thanks Eric, as Murray says, this is getting to be fun! Comments inline as usual...

[johnhaddon]

I've also started monkeying around with our Qt stuff to see if I can track down the cause of the annoying jiggle-on-deactivate. Haven't got anywhere yet (but I think did find the cause of another Qt annoyance) but will keep digging...

[ericmehl]

Thanks John! I addressed the feedback for the easy stuff today. I think there are a number of variables and methods that can be renamed to remove the shadowy-ness from them and prep for the highlight placement tool. It will just have a target, no pivot, so I don't see any particular reason why I can't drop shadowTarget wording for target and similar throughout.

Also next up is looking at the transforms which I'll tuck into in the morning.

[ericmehl]

I pushed a few new changes that I believe take care of all the remaining comments and improvements. I also updated the commit references my response to the comments - rebasing caused the commit hashes to change.

The fix to "undo" when drag placing the target / pivot is now in 2540da4.

Ready for a new look!

[johnhaddon]

Thanks for the update Eric. Got a couple of final comments inline, including one crash that we definitely will need to address. Nearly there though!

[ericmehl]

One thing I didn't about regarding the preventing an ABI break when I add highlight placement mode - I'm planning on adding an enum to indicate the tool mode. That would be public so we can bind it in Python.

Would that consittute an ABI break? I'm a little fuzzy on what counts as an ABI break, is it just adding new data members?

[ericmehl]

I have all the comments from today addressed - I think

• reviewing those commits
• double checking we're happy with the behavior
• considering the enum ABI break question
• getting Arnold downloads working again for CI to check out

are what's left.

[johnhaddon]

Thanks Eric, LGTM!

I'm a little fuzzy on what counts as an ABI break, is it just adding new data members?

There's also changing existing function signatures, removing data members, changing virtual functions, and a bunch of other subtlety I don't have the best handle on. Basically, stuff that changes the layout of a class in memory, or the calling convention of a function. Adding a new enum type doesn't affect any of that, so should be fine.

reviewing those commits

Done!

double checking we're happy with the behavior

I haven't done as extensive testing as @murraystevenson here - did you want one last check Murray or are you happy?

getting Arnold downloads working again for CI to check out

I hope I should have this taken care of by the time you get in 🤞.

One other small thing - I needed this patch to build on Mac :

--- a/include/GafferSceneUI/LightPositionTool.h
+++ b/include/GafferSceneUI/LightPositionTool.h
@@ -45,6 +45,8 @@
 
 #include "Gaffer/ScriptNode.h"
 
+#include <unordered_map>
+
 namespace GafferSceneUI
 {
 
diff --git a/src/GafferSceneUI/LightPositionTool.cpp b/src/GafferSceneUI/LightPositionTool.cpp
index 27af71ea6..ede376de1 100644
--- a/src/GafferSceneUI/LightPositionTool.cpp
+++ b/src/GafferSceneUI/LightPositionTool.cpp
@@ -685,8 +685,6 @@ void LightPositionTool::updateHandles( float rasterScale )
        V3f direction;
        transform.multDirMatrix( V3f( 0, 0, -1.f ), direction );
 
-       const V3f handleDir = ( shadowTarget.value() - shadowPivot.value() ).normalized();
-
        if(
                !m_drag &&
                (

Perhaps you could incorporate that while squashing down ready for merging?

[ericmehl]

Squashed and double checked that nothing was changed in the process. And included the fix for mac, thanks for pointing that out.

Once we're happy with the user-facing details, should be ready for a merge!

[murraystevenson]

Thanks Eric! This looks good to me for a release. I've added a couple of very pedantic comments inline, but otherwise seems good to merge. 👍

[ericmehl]

Thanks for the double check @murraystevenson! I fixed those two issues and squashed it down to their relevant commits. Should be ready for a merge now.

[johnhaddon]

Thanks Eric!