StandardConnectionGadget : allow Ctrl-Click to add Dots

[mattigruener]

Hey John,

here's a first version of what that DotAdderThingy could look like.

Some things are not ideal yet:

• the leave signal is not emitted properly when Dot preview is active (we only leave the dot when moving the mouse pretty quickly)
• it would be nice to click-drag the new Dot to the desired location during creation

[mattigruener]

I wasn't sure whether to consider these private to the DotAdderThingy implementation or proper class methods without the double underscores.

[johnhaddon]

There's no need for the "__" prefix in C++ - in fact I don't think it's even allowed. In C++ we have proper access control via private : and protected :, and use that instead. It's only in Python that we're in the wild west and have to use a naming convention to denote protected/private.

[mattigruener]

Urg, let's never talk of this again... -.- Updated.

[johnhaddon]

the leave signal is not emitted properly when Dot preview is active (we only leave the dot when moving the mouse pretty quickly)

I think my intuitive expectation is that the "dot preview thingy" would be snapped to the connection anyway, so moving away from the connection would naturally lead to a leave signal. Does that make sense?

This leads me to a related topic - I think curvy noodles are bad. It makes it really hard to distinguish between long criss-crossed connections because the tangents all tend to be flattened together in the middle. I wish I'd made the connections straight, possible with little stubs so they jutted out from the nodes before going on their way. That would simplify the "snap to curve" part of the above. Too much for now though?

it would be nice to click-drag the new Dot to the desired location during creation

Definitely. Could you experiment with that? We manage it for "click to select and then drag to move" in the NodeGraph, so maybe that could give you some clues? If it turns out to be tricky then drop me an email and we can go from there...

[mattigruener]

I don't like the curvy noodles either. If you feel like it's worth changing, I'm happy to look at making them straight as part of this ticket. We could make this a "make noodle interaction nicer" ticket that runs a bit longer - I don't think I've heard anyone screaming for that dotAdder feature.

On that note - I talked to Ivan today and he made a good suggestion. Currently when selection a node, the noodles are highlighted and it's easy to determine the inputs to the node. Putting a Dot between two nodes breaks that a little because only the second part of that connection is highlighted. He suggested promoting the state of being highlighted across Dots - if that explanation makes sense. I could look at that as part of this ticket as well.

As for the click-dragging - I'll have a look at the implementation in the NodeGraph, thanks! :)

[andrewkaufman]

I don't like the curvy noodles either. If you feel like it's worth changing, I'm happy to look at making them straight as part of this ticket.

I don't disagree necessarily, but that sounds like a big UI change visually, so I think we'd do best to survey some users first. I imagine they'll come back saying "sometimes straight, sometimes curvy, give me a user pref for it" which won't make anything easier...

[andrewkaufman]

He suggested promoting the state of being highlighted across Dots - if that explanation makes sense.

I was wondering if this should be across Switches too (e.g. highlight all the way to the source())?

[mattigruener]

I think we already had users request to highlight which input to a switch node is actually used.

IF we decide to change the noodles drawing, another option would be doing this. I think I've seen that somewhere and liked it at the time. Was that nuke and they've removed it by now? Maybe there was a good reason to not keep it.

           +------------+
           |            |
           +------------+
                  .
                  .
                  ..........
                           .
                           .
                     +------------+
                     |            |
                     +------------+

[johnhaddon]

Hey Matti,
How does that latest commit perform when panning around a large graph? The reason I ask is that nodeSelected() is called from doRender(), so it'll be called for every redraw, and therefore needs to be fast. It looks like a lot is going on there (including computes if the index on a switch has an expression!), so I'm a bit worried this may not be the best approach...
Cheers...
John

[mattigruener]

Hey John,

if this commit makes this PR too busy, I'm happy to break it off into a new PR. Maybe that would make sense if you anticipate major changes.

Will run this on a bigger graph now and report back. We could get rid of some std::vectors being constructed even though it's not necessary - I just kept it that way because it made the code more compact.

Generally, this should not really slow down big graphs if they don't contain dots and switches, right? Those two ifs and the cast shouldn't be a huge problem? Or would you consider the casting a big bottleneck already, even if the body of the if is never executed?

Do you see a better way for this feature or would you rather not put in that feature at all?

Thanks :)

[johnhaddon]

Thanks Matti - I think it would be good to keep this PR a bit more focussed if we can. I suspect any large graph will contain a fair number of dots and switches, so it'll be interesting to see what you find. Might be no problem at all, but worth checking I think. The other problem is that the call to switchNode->activeInPlug() is potentially triggering a compute, but without a context being scoped - we've had that break the UI in the past when an expression has expected a context variable to be available from the script level. The alternative I'm considering is that perhaps the NodeGraph should do some explicit highlighting of connections when the selection changes, so we don't do any work at all in doRender().

[mattigruener]

Dropped that commit here and moved it to #2075

[mattigruener]

Hey John,

I finally got around to looking at this one again. Could you take a butchers?

The code that I've pushed now takes care of the two issues I had flagged before. We don't have the problem with the leave signal anymore (thanks to snapping powered by @danieldresser-ie's new noodles) and I've got the click'n'drag to work.
Regarding the latter, I'm not sure if you like the way I've done it - might not be the bee's knees yet.
The GraphGadget will now pass control to the StandardConnectionGadget in order for it to create a Dot, Bob's your uncle, click-dragging works. My reasoning entails that the GraphGadget is responsible for the click-dragging, but the StandardConnectionGadget is the one that will have to render the dot preview. Because of that it seems sensible to also have the StandardConnectionGadget insert the dot (it knows where, it knows the modifier used...). Does that make sense? Would you rather have the GraphGadget insert the Dot and query the StandardConnectionGadget for the information it needs to do so?

Thanks! Toodle pip,

Matti.

[mattigruener]

When looking at this it might be worth keeping in mind that we might also want to add a shift-click option to the connections that allows dragging a new connection with the same source to another nodule as a quick way to connect distant inputs to multiple outputs.

I had a quick look and thought about adding a bool m_addConnection to the StandardConnectionGadget that gets set on shift-clicking and that causes the rendering of two noodles - the original one and one that's being dragged. The nodule that receives the drop event would have to query the connection through a public isAddingConnection() method and would then just not remove the input of the previously connected dstPlug. Does that make sense? The problem with that would be that we're doing a IECore::runTimeCast<ConnectionGadget>( event.sourceGadget ) in StandardNodule instead of to StandardConnectionGadget. I assume that's on purpose and you might not like my cast to StandardConnectionGadget in this PR (in GraphGadget.cpp) either.

I'll hold off on this for now as other tickets are given higher priority and I'd rather talk to you about the approach here first. Thanks :)

[johnhaddon]

Could you take a butchers?

Thanks Matti! This made me chuckle!

My reasoning entails that the GraphGadget is responsible for the click-dragging, but the StandardConnectionGadget is the one that will have to render the dot preview. Because of that it seems sensible to also have the StandardConnectionGadget insert the dot (it knows where, it knows the modifier used...). Does that make sense?

Yep, makes total sense. The dropDotIfRequired( const ButtonEvent &event ) coupling between GraphGadget and StandardConnectionGadget does feel awkward though, since the ideal would to leave GraphGadget untouched, and implement the ConnectionGadget features entirely within the ConnectionGadget. There's already a bit more coupling between the components of the NodeGraph than I'd like, as the more dependencies and public methods there are the harder it all gets to maintain. I wonder if it would be possible to add the dot in StandardConnectionGadget::buttonPress(), but then return false so that the GraphGadget then gets the chance to handle the event, leading to it naturally selecting and dragging the dot?

When looking at this it might be worth keeping in mind that we might also want to add a shift-click option to the connections that allows dragging a new connection with the same source to another nodule as a quick way to connect distant inputs to multiple outputs.

Good point. Since you mentioned dragging, I guess another possibility is that we handle the dragging of the Dot entirely within the StandardConnectionGadget. This might not be as good as letting the GraphGadget do it, but I think it might be better than the dropDotIfRequired() coupling.

The nodule that receives the drop event would have to query the connection through a public isAddingConnection() method and would then just not remove the input of the previously connected dstPlug. Does that make sense?

That does make sense, but again it feels like we'd be leaking the implementation of StandardConnectionGadget features out into the other components. I think arguably we're already doing this with the existing ConnectionGadget handling that's happening in StandardNodule::drop(). I wonder if we could move that handling into StandardConnectionGadget::dragEnd()? If so, that would seem a lot cleaner to me...

I'll hold off on this for now as other tickets are given higher priority and I'd rather talk to you about the approach here first.

I think there's a bit of experimenting to do to see if we can get this a little bit cleaner, and it definitely makes sense to add the "second connection dragging" feature at the same time. Could you maybe give a few of the ideas above a try, and see if they're feasible?

Cheers...
John

[mattigruener]

Could you take a butchers?

Thanks Matti! This made me chuckle!

I'm trying some of the slang Don has taught me ;)

I wonder if it would be possible to add the dot in StandardConnectionGadget::buttonPress(), but then return false so that the GraphGadget then gets the chance to handle the event, leading to it naturally selecting and dragging the dot?

That's what I tried initially as it seemed a whole lot cleaner, but then I ran into the problem that the Ctrl behaviour is somewhat overloaded. If we let the GraphGadget handle the sneakily inserted Dot, it will go ahead and select all downstream nodes as that's what the Ctrl modifier does at that level... My solution to that problem was to have the GraphGadget let the StandardConnectionGadget handle it and then return after doing that special selection update. Maybe I'm missing something, but it felt to me like we'd definitely have to introduce a little bit of coupling so that the two of them can figure out what Ctrl means.

Since you mentioned dragging, I guess another possibility is that we handle the dragging of the Dot entirely within the StandardConnectionGadget. This might not be as good as letting the GraphGadget do it, but I think it might be better than the dropDotIfRequired() coupling.

When I tried this one yesterday, I think I ran into the problem of not getting calls to dragMove() in StandardConnectionGadget because I was returning the Dot from dragBegin() instead of a nodule. It seemed odd to me to me to return a Dot there anyway and decided to go with the current implementation instead.

I wonder if we could move that handling into StandardConnectionGadget::dragEnd()? If so, that would seem a lot cleaner to me...

Replacing this bit, I guess? I can look into that the next time I'm touching this :)

Thanks for your response :)

[johnhaddon]

I wonder if it would be possible to add the dot in StandardConnectionGadget::buttonPress(), but then return false so that the GraphGadget then gets the chance to handle the event, leading to it naturally selecting and dragging the dot?

If we let the GraphGadget handle the sneakily inserted Dot, it will go ahead and select all downstream nodes as that's what the Ctrl modifier does at that level...

I just gave this a bash and it seemed to actually work - I think maybe the difference was that my implementation selects the Dot in the connection gadget?

Since you mentioned dragging, I guess another possibility is that we handle the dragging of the Dot entirely within the StandardConnectionGadget. This might not be as good as letting the GraphGadget do it, but I think it might be better than the dropDotIfRequired() coupling.

When I tried this one yesterday, I think I ran into the problem of not getting calls to dragMove() in StandardConnectionGadget because I was returning the Dot from dragBegin() instead of a nodule. It seemed odd to me to me to return a Dot there anyway and decided to go with the current implementation instead.

To get dragMove() calls you need to return true from dragEnter() - maybe you just needed to update that? I think returning the Dot from dragBegin() would make sense, because that is the thing that's being dragged - so dragging it out into the ScriptEditor should do something sensible for instance.

I wonder if we could move that handling into StandardConnectionGadget::dragEnd()? If so, that would seem a lot cleaner to me...

Replacing this bit, I guess? I can look into that the next time I'm touching this :)

Yes, I think so. I do wonder if there's some long-forgotten reason I'm not doing that though....

[mattigruener]

I just gave this a bash and it seemed to actually work - I think maybe the difference was that my implementation selects the Dot in the connection gadget?

Yeah, I guess that's why. Just tested your version and it seems to work fine, indeed :) Are you ok with making the StandardConnectionGadget responsible for the selection?

To get dragMove() calls you need to return true from dragEnter() - maybe you just needed to update that? I think returning the Dot from dragBegin() would make sense, because that is the thing that's being dragged - so dragging it out into the ScriptEditor should do something sensible for instance.

Mh, I see. Dragging it to the ScriptEditor during the creation of the Dot doesn't really make sense though, does it? Users would add the Dot, leave "dot adding mode" and then they can drag the dot wherever they want through GraphGadget functionality? Now that the selection problem is fixed, the ctrl-clicking is pretty much done? Or would you like us to explore the option of handling all of it directly in StandardConnectionGadget?

[johnhaddon]

Now that the selection problem is fixed, the ctrl-clicking is pretty much done?

Yep.

Or would you like us to explore the option of handling all of it directly in StandardConnectionGadget?

Not necessarily - was just mentioning it as an option. We can just get this tidied up with the graph gadget doing the dragging and then make a separate PR for the extra-connection dragging if that works for you...

[mattigruener]

Tests failed (2 timeouts, 1 error). Will restart, error below.

testCatalogueName (GafferImageTest.CatalogueTest.CatalogueTest) ... terminate called after throwing an instance of 'std::runtime_error'
  what():  tbb_thread::join: Resource deadlock avoided
./config/travis/runGafferTest: line 3: 15039 Aborted                 (core dumped) ./install/gaffer-*/bin/gaffer test $test_module
Gaffer Test Failed : GafferImageTest

[johnhaddon]

Tests failed (2 timeouts, 1 error). Will restart, error below.

Yeah, everything is failing or timing out recently. Nothing to do with the PRs themselves unfortunately. I've tried to reproduce the Catalogue problem locally but without success so far. Will try again...

[mattigruener]

I got the same failure in #2135. I'm not restarting for now in case there's anything you can get out of that log that helps.

[johnhaddon]

Thanks Matti! This is a really nice feature.

When I was just testing this I did very occasionally manage to end up dragging the nodule from the new Dot, rather than the Dot itself. I think it happens most often when zoomed out, and when dragging in the direction of the connection (making it more likely the mouse has moved onto the nodule before the dragBegin event happens).

I've merged anyway as it's a great feature and I don't think the problem is in the new code. I'm investigating now...

[johnhaddon]

Ah, got it. Here's some steps to reproduce :

1. Make a horizontalish connection between some otherwise vertically flowing nodes
   
2. Hover and hold control, but click and drag at the edge of the preview, where you imagine the nodule will be
   
3. The press/drag gets picked up by the new nodule, not the dot itself
   .

There appears to be two problems here :

1. Because the Dot is created at the snapped location, GraphGadget::buttonPress() might not find the Dot underneath the mouse (because the nodule is there instead). The GraphGadget then doesn't handle the event at all.
2. In this case, the low level event handling emits a second button press, which is picked up by the nodule, and the nodule then starts the drag.

If we fix the second problem, then the first problem will still cause the Dot not to be dragged, we just won't get the nodule drag either. One easy solution is to create the Dot at the current mouse position, but that doesn't feel as nice as creating it at the snapped position because then the new dot isn't quite where the preview was. Another solution would be to have the ConnectionGadget handle the drag for the newly created dot like we discussed before.

Any thoughts?
Cheers...
John

[johnhaddon]

For reference, this is the code that causes the unwanted second button press - we have two event filters connected when there's a QAbstractScrollArea in play. I don't know why we don't just connect to one or the other like we do in the mouse tracking code in the function below, and I couldn't find anything in the git log to provide context, except that it was added to support the MultiLineTextWidget in 8ca86ff.

[mattigruener]

Ha, thanks for catching that and tracking down what's happening!

I'm happy to look into handling the dragging on the StandardConnectionGadget instead. That definitely seems nicer than disabling the snapping. Probably the best solution?

[johnhaddon]

Yeah, I was leaning towards that being the best solution. But I forgot that the GraphGadget has niceties like snapping to the vertical/horizontal that we wouldn't want to miss out on, and we certainly wouldn't want to have to reimplement again. Do you want to have a quick play with the "make the dot under the mouse" approach and see how it feels, since it's now our only easy option?