Page MenuHome

Node Editor: Move All Selected Nodes when dragging
Closed, ResolvedPublic

Tokens
"Yellow Medal" token, awarded by Alrob."Like" token, awarded by EitanSomething."Love" token, awarded by monio."Love" token, awarded by Draise."Love" token, awarded by duarteframos."Like" token, awarded by aliasguru.
Authored By

Description

In the Node Editor, it should be possible to move more than just one node by dragging, if more are selected.
Just like files on a desktop, all selected items should move along when dragging on one of them.

Event Timeline

William Reynish (billreynish) triaged this task as Confirmed, High priority.

Problem here is that the 'click' PRESS event will always be caught before the TWEAK one, so selection will always happen before we start translation.

Only simple solution I can see so far is to trigger selection on RELEASE (or CLICK), not on PRESS (tried, seems to work fine, but gives an annoying delay between click and actual selection, especially on laptops with touchpad fancy 'click' handling :( ).

Other possibility, much more radical, would be to 'eat' the click event altogether when we detect a tweak one. Am quiet unsure that is even possible though, at the very least not without a noticeable delay for the user.

Need some thoughts, also probably inputs from @Campbell Barton (campbellbarton) on that one.

Would be interested to first consider where else this logic would apply
(where LMB tweak is used too).

  • Mask points.
  • Paint 'Curve' stroke method.
  • (Possibly Movie-Clip editor in the future?)

If we want this to work as many file-selectors do, we'd need to do as @Bastien Montagne (mont29) suggests and run this on click instead of press.

We could make it feel more responsive we could.

  • Press sets the node active, then either.
    • Click de-selects all others.
    • Tweak moves everything.

... Even this could back-fire (with undo steps and having a click sometimes performing two operations, and the first press event having to pass-through so the click event is registered).

All things considered we're probably best off making select happen on release (exactly like how a file manager works), because supporting press events which pass-through complicates event handling.


Other possibility, much more radical, would be to 'eat' the click event altogether when we detect a tweak one.

Not sure what's meant here., when a tweak event is registered a click event wont be sent.

Testing how it works with something like document files in the Mac Finder:

As you can see, when you click on an item that is already selected, it doesn't deselect other selections. This way they are still able to register the click event immediately (mousedown) and don't have to wait for the mouseup event.

@William Reynish (billreynish) it does not really help to compare a filebrowser with a node editor (and Blender in general), the later is much more complex and allows for many different actions…

I’d agree with @Campbell Barton (campbellbarton) here then, if we want that behavior then we just select on CLICK (or RELEASE) event.

Would not go into handling two different kind of events, this would make code even more tricky to follow, and more fragile too. Unless maybe we had a specific 'set active' operator for the 'PRESS' event, that would not register any undo steps? Still sounds cumbersome for little gain… :/

Btw, just realized that in 3DView at least, we do select on click, and not on PRESS event…

It actually depends if you use left or right click select. On press it feels more responsive so we kept it for right click, but it's incompatible with tools on left click select.

I'm not sure how we can put select on click instead of press for the node editor though? You want to be able to press to select and drag on a node immediately to move it?

@Bastien Montagne (mont29): Well, the issue is identical: How can we handle click input so that you can both select and drag the selected items, without having to use slower mouse-up events? It's the exact same thing as selecting and moving files on your desktop.

They solve it that way, by making it not deselect others if you click on an item that was already selected. The same solution could work in Blender too.

@Brecht Van Lommel (brecht) You want to be able to press to select and drag on a node immediately to move it? that’s kind of incompatible with behavior described in that task? Either we select, and then move on drag (what happens currently), or we give priority to drag, and only select on CLICK (i.e. when there was no drag detected).

Or we try to implement what @Campbell Barton (campbellbarton) described (some kind of two-steps selection, with PRESS adding to current selection, and then RELEASE removing all selected items and selecting again the active one). With all the PASSTHROUGH and double-undo history issues… yuck. :/

Btw, T63996 looks like a very similar can of worms.

@William Reynish (billreynish) Sure thing, all problems can be solved. But if we have to rethink/rewrite half of our event/operator handling code to support that, this is not a task for me, and this is not a task for 2.80. I (again) have the feeling that a tremendous amount of time and energy needs to be put to fix that kind of issues (and am sorry, but those still look pretty low-priority to me, things that are 'nice to have', but by no mean critical issues). Nice if we can fix them simply and quickly, but otherwise I would not really bother too much.

And as a reminder, I was not even involved in that left-click-select project/decision, not at all. So I do not really feel like spending too much of my time trying to fix the cans of worms it unveils. I have other priorities and projects to tackle too. For me, either we go with the cheap 'CLICK' to select things, or we live with current behavior.

@William Reynish (billreynish): ...without having to use slower mouse-up events? It's the exact same thing as selecting and moving files on your desktop...

The trick with our desktops: The timing of the deselection of other items changes depending on whether the new item is selected or not.

If the new item is not already selected then the other items are deselected on mouse-down. But if the item is already selected then peers are only unselected on mouse-up. This is what allows selecting and dragging singles or groups without having to wait for mouse up.

Ok, sure, it may be a bigger project to make selection more fundamentally different. I'm fine with any solution that works, even if it means using mouse-up events, if it makes it possible to move multiple nodes easily by simply dragging on your selection.

I think the way to solve it is actually to make changes to the select operator, so that clicking on selected items doesn't deselect others. Moving all selected items already works, so the issue is only that the select operator is first deselecting everything each time you click and drag.

It still needs to deselect others when releasing the mouse button, if you haven't started dragging already. One way of doing that would be to make the select operator a modal operator that finishes as soon as you release the mouse button (deselecting others), or a tweak event is started (doing nothing).

To me this seems possible in a way that's isolated to the select operator and not interfering with other operators that need to run right after. But I'm not sure how much work it would be.

@Brecht Van Lommel (brecht) Yes, that would be idea' - that it deselects others on mouse-up if the user didn't drag.

But IMO it would also be acceptable to do it in a simpler way so it just doesn't deselect others if you click on a selected item. Then you would have to click away to deselect first, or use the deselect operator. Not as nice, but eg the macOS Finder works this way and I never noticed it as being an issue.

One way of doing that would be to make the select operator a modal operator that finishes as soon as you release the mouse button (deselecting others), or a tweak event is started (doing nothing).
To me this seems possible in a way that's isolated to the select operator and not interfering with other operators that need to run right after. But I'm not sure how much work it would be.

Whats the advantage of this compared to having click for select, tweak for border select?

Both use the same threshold so it shouldn't be possible to move the cursor so it's moved too much for a click but not enough for a drag.

Whats the advantage of this compared to having click for select, tweak for border select?
Both use the same threshold so it shouldn't be possible to move the cursor so it's moved too much for a click but not enough for a drag.

The advantage is that you select and transform the node immediately. Otherwise you would have to click once to select, and then click again to transform the node.

P966 Is an attempt to implement @Brecht Van Lommel (brecht)'s suggestion, seems to work OK-ish.

Note that it relies on press/release events being the two triggers, think we can accept that hard-coded behavior here?

I remain pretty unhappy about that hack though. To have to add such complexity to a simple operator, just to work around (imho very bad) design decision to merge action and select buttons together… :(

@Bastien Montagne (mont29) Maybe I should have asked in here and not in the diff snippet:

This seems to work well!

Question: How generic is it? Could we re-use same logic for sequencer strips or NLA clips etc?

Lol… copy-pasting my answer from P966 here then. :P

Copy-pasting similar logic should work, yes, at least for similar selection (like in most 2D editors), would expect some complications if we tried to do so with e.g. 3DView selection (also we might have to do similar things there, cf. can of worms II, T63996). This is not a small change though, as pretty much all select ops do things a bit differently. Kind of like what I did for the 'deselect on nothing' change, but several times bigger every time…

My concern with that change is that it adds a whole other level of complexity over selection logic, which already has to deal with extend, select_all, etc. It also makes behavior in different kind of scenarii more complicated to predict (like, what if user adds a keymap using a totally different shortcut to select nodes? What about macros [e.g. just realized that current patch breaks the 'link to viewer node' macro, the one using sockets selection]? etc. etc.). That code can probably be refactored further to make it more easy to follow, but still… am not so happy with it. This adds technical debt for a bad reason imho. :|

@Bastien Montagne (mont29)

Well, obviously I don't agree with "for a bad reason". So many users are already finding Blender 2.8 much easier to use, and the ability to select and manipulate things more easily is a core reason why.

As for code complexity, I guess ideally we could somehow share this logic so it's not duplicated several places. We could also just start by adding this to the Node Editor only for 2.80, and then later we could refactor and generalize the logic to make it easier to re-use in other places.

Hello, I'm new here, but I wonder if this might be a helpful way to approach this:

Since dragging multiple nodes works as expected when they are contained within a frame, would it be possible to create an invisible frame around all selected nodes on mouse press, tweak to move it, then mouse release destroys the frame? (which the user never saw to begin with)

I did an experiment and replaced the Join Nodes - Keymapping (Alt+J by default, I think, I'm using my own keymap) with a Left Click, and while it didn't work if you selected a bunch of nodes and then clicked on one of them to drag, it DID work when you selected a bunch of nodes and clicked anywhere OTHER than a node.

So any misclicks ended up creating empty frames, perhaps that could be fixed with a check on mouse press, but that might then be back in the problem of adding latency.

Or, if they're invisible, creating a million nodes in the background that you never see.

Then again, maybe you could map a function that deletes all empty frames in addition to the select box frame on mouse release?

Just a thought.

@Rob (SpectralVectors) That sounds even more complicated than the modal selection op solution?

@Brecht Van Lommel (brecht) @Campbell Barton (campbellbarton) We should decide now what we do here. Afaics, we have two options:

  1. Select on RELEASE, which solves present issue, is simple and safe, but makes selection experience a tad less nice for user, and prevents the 'click-drag' behavior to click on a node and move it in a single action.
  2. Modal selection performing in two steps, which does everything user might expect, but makes selection code much more hairy than it already is (and hence prone to bugs).

I am a bit torn here, for me solution 1 is far better from a safety/'code quality'/maintainability point of view, while solution 2 is ideal from user point of view.

If we decide to take the risk of modal op solution, I would then like to do it as a separate operator, explicitly only modal, and explicitly not handling all extra cases (like extend selection, selection of sockets, etc.). That should keep code complexity to a more reasonable/maintainable level.

I think we should do 2), it's a nice improvement.

But there are some issues with the implementation that I found:

  • When the node is not initially selected, then dragging on it should not drag all other nodes. It should only do that when the node is already selected. File browsers work similar, and it keeps tweaking individual node positions quick (which I do much more often than moving multiple nodes). In this case the operator could finish immediately and not start the running modal.
  • When clicking and dragging a small distance (by accident), it seems to not deselect other nodes. The threshold should be the same as starting the transform. It may help to listen for EVT_TWEAK_L/M/R events instead of doing our own distance calculation with mouse events.

I'm not entirely sure making it a separate operator will reduce code complexity? We would have to see how that works out, but I fear it would complicate the keymaps. I do think that the socket selection should be handled separately, that could be in its own function, and if a socket is found it can skip running modal and finish immediately.

For the implementation, I guess storing active_item is not actually needed? As far as I can tell it's only used for asserts, so removing it could simplify the implementation.

I think we should do 2), it's a nice improvement.

OK… Still not totally happy with it, but am happier with new version of patch (P966), so guess it’s OK. ;)

But there are some issues with the implementation that I found:

  • When the node is not initially selected, then dragging on it should not drag all other nodes. It should only do that when the node is already selected. File browsers work similar, and it keeps tweaking individual node positions quick (which I do much more often than moving multiple nodes). In this case the operator could finish immediately and not start the running modal.

Good point, fixed in new patch.

  • When clicking and dragging a small distance (by accident), it seems to not deselect other nodes. The threshold should be the same as starting the transform. It may help to listen for EVT_TWEAK_L/M/R events instead of doing our own distance calculation with mouse events.

Threshold is the same as starting tweak (see WM_EVENT_CLICK_TWEAK_THRESHOLD in wm_event_system.c). Also, EVT_TWEAK_L/M/R never reach our modal handler, not sure why (would assume WM event code does not always trigger them?). But previous code was buggy (using wrong coordinate values), new version of patch seems to work as expected here as well.

I'm not entirely sure making it a separate operator will reduce code complexity? We would have to see how that works out, but I fear it would complicate the keymaps. I do think that the socket selection should be handled separately, that could be in its own function, and if a socket is found it can skip running modal and finish immediately.
For the implementation, I guess storing active_item is not actually needed? As far as I can tell it's only used for asserts, so removing it could simplify the implementation.

Good points, new patch is much more satisfying on simplicity side imho (also made it more strict to only accept regular selection in modal 'two steps' mode, other options just get always executed in basic exec mode.

1diff --git a/source/blender/editors/space_node/node_select.c b/source/blender/editors/space_node/node_select.c
2index 07b01e576bc..681bf693246 100644
3--- a/source/blender/editors/space_node/node_select.c
4+++ b/source/blender/editors/space_node/node_select.c
5@@ -435,13 +435,21 @@ static int node_mouse_select(Main *bmain,
6 const int mval[2],
7 const bool extend,
8 const bool socket_select,
9- const bool deselect_all)
10+ const bool deselect_all,
11+ const bool do_first_step,
12+ const bool do_second_step)
13 {
14 bNode *node, *tnode;
15 bNodeSocket *sock = NULL;
16 bNodeSocket *tsock;
17 float cursor[2];
18- bool selected = false;
19+ int ret_value = OPERATOR_CANCELLED;
20+
21+ /* Handle two-steps selection (with deselection of already selected items as second step). */
22+ const bool use_two_steps = do_first_step || do_second_step;
23+
24+ /* Two-steps modal selection is only allowed for basic selection. */
25+ BLI_assert(!(extend || socket_select) || !use_two_steps);
26
27 /* get mouse coordinates in view2d space */
28 UI_view2d_region_to_view(&ar->v2d, mval[0], mval[1], &cursor[0], &cursor[1]);
29@@ -452,7 +460,7 @@ static int node_mouse_select(Main *bmain,
30 /* NOTE: SOCK_IN does not take into account the extend case...
31 * This feature is not really used anyway currently? */
32 node_socket_toggle(node, sock, true);
33- selected = true;
34+ ret_value = OPERATOR_FINISHED;
35 }
36 else if (node_find_indicated_socket(snode, &node, &sock, cursor, SOCK_OUT)) {
37 if (sock->flag & SELECT) {
38@@ -460,7 +468,7 @@ static int node_mouse_select(Main *bmain,
39 node_socket_deselect(node, sock, true);
40 }
41 else {
42- selected = true;
43+ ret_value = OPERATOR_FINISHED;
44 }
45 }
46 else {
47@@ -485,7 +493,7 @@ static int node_mouse_select(Main *bmain,
48 }
49 }
50 node_socket_select(node, sock);
51- selected = true;
52+ ret_value = OPERATOR_FINISHED;
53 }
54 }
55 }
56@@ -501,24 +509,43 @@ static int node_mouse_select(Main *bmain,
57 if (!((node->flag & SELECT) && (node->flag & NODE_ACTIVE) == 0)) {
58 node_toggle(node);
59 }
60- selected = true;
61+ ret_value = OPERATOR_FINISHED;
62 }
63 }
64 else {
65 if (node != NULL || deselect_all) {
66- for (tnode = snode->edittree->nodes.first; tnode; tnode = tnode->next) {
67- nodeSetSelected(tnode, false);
68+ /* We want to deselect all other nodes in three cases:
69+ * - If 'deselect on nothing' is enabled, and we clicked on nothing.
70+ * - If we are in single-step selection, or in second step of two-steps selection.
71+ * - If we are in first step of two-steps selection, and node on which we clicked
72+ * was not selected (that way, click-drag on non-selected node will only move that one,
73+ * not also all other previously selected nodes).
74+ */
75+ if ((deselect_all && node == NULL) || (!use_two_steps || do_second_step) ||
76+ (do_first_step && node != NULL && (node->flag & SELECT) == 0)) {
77+ for (tnode = snode->edittree->nodes.first; tnode; tnode = tnode->next) {
78+ if (tnode == node) {
79+ continue;
80+ }
81+ nodeSetSelected(tnode, false);
82+ }
83+ if (node != NULL) {
84+ nodeSetSelected(node, true);
85+ }
86+ /* In case we also do de-selection of previously selected nodes, we are done,
87+ * no need to keep modal running. */
88+ ret_value = OPERATOR_FINISHED;
89 }
90- selected = true;
91- if (node != NULL) {
92+ else if (node != NULL) {
93 nodeSetSelected(node, true);
94+ ret_value = do_first_step ? OPERATOR_RUNNING_MODAL : OPERATOR_FINISHED;
95 }
96 }
97 }
98 }
99
100 /* update node order */
101- if (selected) {
102+ if (ret_value != OPERATOR_CANCELLED) {
103 if (node != NULL) {
104 ED_node_set_active(bmain, snode->edittree, node);
105 }
106@@ -526,7 +553,7 @@ static int node_mouse_select(Main *bmain,
107 ED_node_sort(snode->edittree);
108 }
109
110- return selected;
111+ return ret_value;
112 }
113
114 static int node_select_exec(bContext *C, wmOperator *op)
115@@ -546,17 +573,90 @@ static int node_select_exec(bContext *C, wmOperator *op)
116 const bool deselect_all = RNA_boolean_get(op->ptr, "deselect_all");
117
118 /* perform the select */
119- if (node_mouse_select(bmain, snode, ar, mval, extend, socket_select, deselect_all)) {
120+ const int ret_value = node_mouse_select(
121+ bmain, snode, ar, mval, extend, socket_select, deselect_all, false, false);
122+ if (ret_value & OPERATOR_FINISHED) {
123 /* send notifiers */
124 WM_event_add_notifier(C, NC_NODE | NA_SELECTED, NULL);
125+ }
126+ /* allow tweak event to work too */
127+ return ret_value | OPERATOR_PASS_THROUGH;
128+}
129
130- /* allow tweak event to work too */
131- return OPERATOR_FINISHED | OPERATOR_PASS_THROUGH;
132+static int node_select_modal(bContext *C, wmOperator *op, const wmEvent *event)
133+{
134+ Main *bmain = CTX_data_main(C);
135+ SpaceNode *snode = CTX_wm_space_node(C);
136+ ARegion *ar = CTX_wm_region(C);
137+
138+ const short init_event_type = (short)POINTER_AS_INT(op->customdata);
139+
140+ /* get settings from RNA properties for operator */
141+ int mval[2];
142+ mval[0] = RNA_int_get(op->ptr, "mouse_x");
143+ mval[1] = RNA_int_get(op->ptr, "mouse_y");
144+
145+ const bool extend = RNA_boolean_get(op->ptr, "extend");
146+ /* always do socket_select when extending selection. */
147+ const bool socket_select = extend || RNA_boolean_get(op->ptr, "socket_select");
148+ const bool deselect_all = RNA_boolean_get(op->ptr, "deselect_all");
149+
150+ /* These cases are never modal. */
151+ if (extend || socket_select) {
152+ return node_select_exec(C, op);
153 }
154- else {
155- /* allow tweak event to work too */
156- return OPERATOR_CANCELLED | OPERATOR_PASS_THROUGH;
157+
158+ if (init_event_type == 0) {
159+ if (event->val == KM_PRESS) {
160+ const int ret_value = node_mouse_select(
161+ bmain, snode, ar, mval, extend, socket_select, deselect_all, true, false);
162+
163+ op->customdata = POINTER_FROM_INT((int)event->type);
164+ if (ret_value & OPERATOR_RUNNING_MODAL) {
165+ WM_event_add_modal_handler(C, op);
166+ }
167+ if (ret_value & (OPERATOR_RUNNING_MODAL | OPERATOR_FINISHED)) {
168+ /* send notifiers */
169+ WM_event_add_notifier(C, NC_NODE | NA_SELECTED, NULL);
170+ }
171+ return ret_value | OPERATOR_PASS_THROUGH;
172+ }
173+ else {
174+ /* If we are in init phase, and cannot validate init of modal operations,
175+ * just fall back to basic exec.
176+ */
177+ return node_select_exec(C, op);
178+ }
179+ }
180+ else if (event->type == init_event_type && event->val == KM_RELEASE) {
181+ const int ret_value = node_mouse_select(
182+ bmain, snode, ar, mval, extend, socket_select, deselect_all, false, true);
183+
184+ if (ret_value & (OPERATOR_RUNNING_MODAL | OPERATOR_FINISHED)) {
185+ /* send notifiers */
186+ WM_event_add_notifier(C, NC_NODE | NA_SELECTED, NULL);
187+ }
188+
189+ return ret_value | OPERATOR_PASS_THROUGH;
190 }
191+ else if (ELEM(event->type, MOUSEMOVE, INBETWEEN_MOUSEMOVE)) {
192+ const int dx = mval[0] - event->mval[0];
193+ const int dy = mval[1] - event->mval[1];
194+ const float tweak_threshold = U.tweak_threshold * U.dpi_fac;
195+ /* If user moves mouse more than defined threshold, we consider select operator as
196+ * finished. Otherwise, it is still running until we get an 'release' event. In any
197+ * case, we pass through event, but select op is not finished yet. */
198+ if (abs(dx) + abs(dy) > tweak_threshold) {
199+ return OPERATOR_FINISHED | OPERATOR_PASS_THROUGH;
200+ }
201+ else {
202+ /* Important not to return anything other than PASS_THROUGH here,
203+ * otherwise it prevents underlying tweak detection code to work properly. */
204+ return OPERATOR_PASS_THROUGH;
205+ }
206+ }
207+
208+ return OPERATOR_FINISHED | OPERATOR_PASS_THROUGH;
209 }
210
211 static int node_select_invoke(bContext *C, wmOperator *op, const wmEvent *event)
212@@ -564,7 +664,9 @@ static int node_select_invoke(bContext *C, wmOperator *op, const wmEvent *event)
213 RNA_int_set(op->ptr, "mouse_x", event->mval[0]);
214 RNA_int_set(op->ptr, "mouse_y", event->mval[1]);
215
216- return node_select_exec(C, op);
217+ op->customdata = POINTER_FROM_INT(0);
218+
219+ return node_select_modal(C, op, event);
220 }
221
222 void NODE_OT_select(wmOperatorType *ot)
223@@ -577,6 +679,7 @@ void NODE_OT_select(wmOperatorType *ot)
224 /* api callbacks */
225 ot->invoke = node_select_invoke;
226 ot->exec = node_select_exec;
227+ ot->modal = node_select_modal;
228 ot->poll = ED_operator_node_active;
229
230 /* flags */

I think the patch works very well.

The code can be simplified further:

Seems ok to commit.

Ah yes, nice improvements, thanks.