Fix T96520 Node editor: Tweak fails with unselected nodes

Use the same method for node selection and dragging that is used
in the 3D viewport and UV editor. Instead of relying on a modal
operator - use the keymap to handle click/drag events.

Details:

Failure to transform unselected nodes was caused by [0] & [1] however
detecting drag relied on specific behavior which I don't think we should
be depending on.

This error happened when selection was defined both in the key-map for
the tool and for the node-editor.

- The left mouse button would activate selection in both the tool
  and "Node Editor" keymap.

- The first selection would return `FINISHED | PASS_THROUGH` when
  selecting a previously unselected node.

- The same PRESS would trigger a second selection would return
  `RUNNING_MODAL | PASS_THROUGH`,
  (starting a NODE_OT_select as a modal operator).

  - In 3.1 (with tweak events) the modal operator would then exit and
    fall-back to the tweak event which would transform the selected
    nodes.

  - In 3.2 (as of [0]) the PRESS that starts the modal operator is
    considered "handled" and prevents drag event from being detected.

The correct behavior in this case isn't obvious:
If a modal operator starts on pressing a button, using that same the
release to generate drag/click events is disputable.

Even in the case or 3.1 it was inconsistent as tweak events were
generated but click events weren't.

Note: after investigating this bug it turns out a similar issue already
existed in 2.91 and all releases afterwards. While the bug is more
obscure, it's also caused by the tweak event being interrupted as
described here, this commit resolves T81824 as well.

[0]: 4d0f846b93
[1]: 4986f71848

Reviewed By: Severin

Ref D14499

release/scripts/presets/keyconfig/keymap_data/blender_default.py

@@ -2019,37 +2019,20 @@ def km_node_editor(params):
         {"items": items},
     )
 
-    def node_select_ops(select_mouse):
-        return [
-            ("node.select", {"type": select_mouse, "value": 'PRESS'},
-             {"properties": [("deselect_all", True)]}),
-            ("node.select", {"type": select_mouse, "value": 'PRESS', "ctrl": True}, None),
-            ("node.select", {"type": select_mouse, "value": 'PRESS', "alt": True}, None),
-            ("node.select", {"type": select_mouse, "value": 'PRESS', "ctrl": True, "alt": True}, None),
-            ("node.select", {"type": select_mouse, "value": 'PRESS', "shift": True},
-             {"properties": [("extend", True)]}),
-            ("node.select", {"type": select_mouse, "value": 'PRESS', "shift": True, "ctrl": True},
-             {"properties": [("extend", True)]}),
-            ("node.select", {"type": select_mouse, "value": 'PRESS', "shift": True, "alt": True},
-             {"properties": [("extend", True)]}),
-            ("node.select", {"type": select_mouse, "value": 'PRESS', "shift": True, "ctrl": True, "alt": True},
-             {"properties": [("extend", True)]}),
-        ]
-
-    # Allow node selection with both for RMB select
     if not params.legacy:
+        items.extend(_template_node_select(type=params.select_mouse,
+                     value=params.select_mouse_value, select_passthrough=True))
+        # Allow node selection with both for RMB select.
         if params.select_mouse == 'RIGHTMOUSE':
-            items.extend(node_select_ops('LEFTMOUSE'))
-            items.extend(node_select_ops('RIGHTMOUSE'))
-        else:
-            items.extend(node_select_ops('LEFTMOUSE'))
+            items.extend(_template_node_select(type='LEFTMOUSE', value='PRESS', select_passthrough=True))
     else:
-        items.extend(node_select_ops('RIGHTMOUSE'))
+        items.extend(_template_node_select(
+            type='RIGHTMOUSE', value=params.select_mouse_value, select_passthrough=False))
         items.extend([
             ("node.select", {"type": 'LEFTMOUSE', "value": 'PRESS'},
              {"properties": [("deselect_all", False)]}),
             ("node.select", {"type": 'LEFTMOUSE', "value": 'PRESS', "shift": True},
-             {"properties": [("extend", True)]}),
+             {"properties": [("toggle", True)]}),
         ])
 
     items.extend([
@@ -4782,6 +4765,35 @@ def _template_view3d_gpencil_select(*, type, value, legacy, use_select_mouse=Tru
     ]
 
 
+def _template_node_select(*, type, value, select_passthrough):
+    items = [
+        ("node.select", {"type": type, "value": value},
+         {"properties": [("deselect_all", True), ("select_passthrough", True)]}),
+        ("node.select", {"type": type, "value": value, "ctrl": True}, None),
+        ("node.select", {"type": type, "value": value, "alt": True}, None),
+        ("node.select", {"type": type, "value": value, "ctrl": True, "alt": True}, None),
+        ("node.select", {"type": type, "value": value, "shift": True},
+         {"properties": [("toggle", True)]}),
+        ("node.select", {"type": type, "value": value, "shift": True, "ctrl": True},
+         {"properties": [("toggle", True)]}),
+        ("node.select", {"type": type, "value": value, "shift": True, "alt": True},
+         {"properties": [("toggle", True)]}),
+        ("node.select", {"type": type, "value": value, "shift": True, "ctrl": True, "alt": True},
+         {"properties": [("toggle", True)]}),
+    ]
+
+    if select_passthrough and (value == 'PRESS'):
+        # Add an additional click item to de-select all other items,
+        # needed so pass-through is able to de-select other items.
+        items.append((
+            "node.select",
+            {"type": type, "value": 'CLICK'},
+            {"properties": [("deselect_all", True)]},
+        ))
+
+    return items
+
+
 def _template_uv_select(*, type, value, select_passthrough, legacy):
 
     # See: `use_tweak_select_passthrough` doc-string.
@@ -6543,10 +6555,8 @@ def km_node_editor_tool_select(params, *, fallback):
         _fallback_id("Node Tool: Tweak", fallback),
         {"space_type": 'NODE_EDITOR', "region_type": 'WINDOW'},
         {"items": [
-            *([] if (fallback and (params.select_mouse == 'RIGHTMOUSE')) else [
-                ("node.select", {"type": params.select_mouse, "value": 'PRESS'},
-                 {"properties": [("deselect_all", not params.legacy)]}),
-            ]),
+            *([] if (fallback and (params.select_mouse == 'RIGHTMOUSE')) else
+              _template_node_select(type=params.select_mouse, value='PRESS', select_passthrough=True)),
         ]},
     )
 
@@ -6563,6 +6573,8 @@ def km_node_editor_tool_select_box(params, *, fallback):
                    params.tool_tweak_event),
                 properties=[("tweak", True)],
             )),
+            *([] if (params.select_mouse == 'RIGHTMOUSE') else
+              _template_node_select(type='LEFTMOUSE', value='PRESS', select_passthrough=True)),
         ]},
     )
 

source/blender/editors/space_node/node_select.cc

@@ -193,11 +193,6 @@ static bool is_event_over_node_or_socket(bContext *C, const wmEvent *event)
   return is_position_over_node_or_socket(*snode, mouse);
 }
 
-static void node_toggle(bNode *node)
-{
-  nodeSetSelected(node, !(node->flag & SELECT));
-}
-
 void node_socket_select(bNode *node, bNodeSocket &sock)
 {
   sock.flag |= SELECT;
@@ -510,10 +505,10 @@ void node_select_single(bContext &C, bNode &node)
   WM_event_add_notifier(&C, NC_NODE | NA_SELECTED, nullptr);
 }
 
-static int node_mouse_select(bContext *C,
-                             wmOperator *op,
-                             const int mval[2],
-                             bool wait_to_deselect_others)
+static bool node_mouse_select(bContext *C,
+                              wmOperator *op,
+                              const int mval[2],
+                              struct SelectPick_Params *params)
 {
   Main &bmain = *CTX_data_main(C);
   SpaceNode &snode = *CTX_wm_space_node(C);
@@ -525,36 +520,38 @@ static int node_mouse_select(bContext *C,
   bNodeSocket *sock = nullptr;
   bNodeSocket *tsock;
   float cursor[2];
-  int ret_value = OPERATOR_CANCELLED;
 
-  const bool extend = RNA_boolean_get(op->ptr, "extend");
   /* always do socket_select when extending selection. */
-  const bool socket_select = extend || RNA_boolean_get(op->ptr, "socket_select");
-  const bool deselect_all = RNA_boolean_get(op->ptr, "deselect_all");
-
-  /* These cases are never modal. */
-  if (extend || socket_select) {
-    wait_to_deselect_others = false;
-  }
+  const bool socket_select = (params->sel_op == SEL_OP_XOR) ||
+                             RNA_boolean_get(op->ptr, "socket_select");
+  bool changed = false;
+  bool found = false;
+  bool node_was_selected = false;
 
   /* get mouse coordinates in view2d space */
   UI_view2d_region_to_view(&region.v2d, mval[0], mval[1], &cursor[0], &cursor[1]);
 
   /* first do socket selection, these generally overlap with nodes. */
   if (socket_select) {
+    /* NOTE: unlike nodes #SelectPick_Params isn't fully supported. */
+    const bool extend = (params->sel_op == SEL_OP_XOR);
     if (node_find_indicated_socket(snode, &node, &sock, cursor, SOCK_IN)) {
+      found = true;
+      node_was_selected = node->flag & SELECT;
+
       /* NOTE: SOCK_IN does not take into account the extend case...
        * This feature is not really used anyway currently? */
       node_socket_toggle(node, *sock, true);
-      ret_value = OPERATOR_FINISHED;
+      changed = true;
     }
     else if (node_find_indicated_socket(snode, &node, &sock, cursor, SOCK_OUT)) {
+      found = true;
+      node_was_selected = node->flag & SELECT;
+
       if (sock->flag & SELECT) {
         if (extend) {
           node_socket_deselect(node, *sock, true);
-        }
-        else {
-          ret_value = OPERATOR_FINISHED;
+          changed = true;
         }
       }
       else {
@@ -566,6 +563,7 @@ static int node_mouse_select(bContext *C,
               continue;
             }
             node_socket_deselect(node, *tsock, true);
+            changed = true;
           }
         }
         if (!extend) {
@@ -575,69 +573,71 @@ static int node_mouse_select(bContext *C,
             }
             for (tsock = (bNodeSocket *)tnode->outputs.first; tsock; tsock = tsock->next) {
               node_socket_deselect(tnode, *tsock, true);
+              changed = true;
             }
           }
         }
         node_socket_select(node, *sock);
-        ret_value = OPERATOR_FINISHED;
+        changed = true;
       }
     }
   }
 
   if (!sock) {
+
     /* find the closest visible node */
     node = node_under_mouse_select(*snode.edittree, (int)cursor[0], (int)cursor[1]);
+    found = (node != nullptr);
+    node_was_selected = node && (node->flag & SELECT);
 
-    if (extend) {
-      if (node != nullptr) {
-        /* If node is selected but not active, we want to make it active,
-         * but not toggle (deselect) it. */
-        if (!((node->flag & SELECT) && (node->flag & NODE_ACTIVE) == 0)) {
-          node_toggle(node);
-        }
-        ret_value = OPERATOR_FINISHED;
+    if (params->sel_op == SEL_OP_SET) {
+      if ((found && params->select_passthrough) && (node->flag & SELECT)) {
+        found = false;
       }
-    }
-    else if (deselect_all && node == nullptr) {
-      /* Rather than deselecting others, users may want to drag to box-select (drag from empty
-       * space) or tweak-translate an already selected item. If these cases may apply, delay
-       * deselection. */
-      if (wait_to_deselect_others) {
-        ret_value = OPERATOR_RUNNING_MODAL;
-      }
-      else {
-        /* Deselect in empty space. */
+      else if (found || params->deselect_all) {
+        /* Deselect everything. */
         for (tnode = (bNode *)snode.edittree->nodes.first; tnode; tnode = tnode->next) {
           nodeSetSelected(tnode, false);
         }
-        ret_value = OPERATOR_FINISHED;
+        changed = true;
       }
     }
-    else if (node != nullptr) {
-      /* When clicking on an already selected node, we want to wait to deselect
-       * others and allow the user to start moving the node without that. */
-      if (wait_to_deselect_others && (node->flag & SELECT)) {
-        ret_value = OPERATOR_RUNNING_MODAL;
-      }
-      else {
-        nodeSetSelected(node, true);
 
-        for (tnode = (bNode *)snode.edittree->nodes.first; tnode; tnode = tnode->next) {
-          if (tnode != node) {
-            nodeSetSelected(tnode, false);
-          }
+    if (found) {
+      switch (params->sel_op) {
+        case SEL_OP_ADD: {
+          nodeSetSelected(node, true);
+          break;
+        }
+        case SEL_OP_SUB: {
+          nodeSetSelected(node, false);
+          break;
+        }
+        case SEL_OP_XOR: {
+          /* Check active so clicking on an inactive node activates it. */
+          bool is_selected = (node->flag & NODE_SELECT) && (node->flag & NODE_ACTIVE);
+          nodeSetSelected(node, !is_selected);
+          break;
+        }
+        case SEL_OP_SET: {
+          nodeSetSelected(node, true);
+          break;
+        }
+        case SEL_OP_AND: {
+          BLI_assert_unreachable(); /* Doesn't make sense for picking. */
+          break;
         }
-
-        ret_value = OPERATOR_FINISHED;
       }
+
+      changed = true;
     }
   }
 
   /* update node order */
-  if (ret_value != OPERATOR_CANCELLED) {
+  if (changed || found) {
     bool active_texture_changed = false;
     bool viewer_node_changed = false;
-    if (node != nullptr && ret_value != OPERATOR_RUNNING_MODAL) {
+    if ((node != nullptr) && (node_was_selected == false || params->select_passthrough == false)) {
       viewer_node_changed = (node->flag & NODE_DO_OUTPUT) == 0 && node->type == GEO_NODE_VIEWER;
       ED_node_set_active(&bmain, &snode, snode.edittree, node, &active_texture_changed);
     }
@@ -654,23 +654,35 @@ static int node_mouse_select(bContext *C,
     WM_event_add_notifier(C, NC_NODE | NA_SELECTED, nullptr);
   }
 
-  return ret_value;
+  return changed || found;
 }
 
 static int node_select_exec(bContext *C, wmOperator *op)
 {
-  const bool wait_to_deselect_others = RNA_boolean_get(op->ptr, "wait_to_deselect_others");
-
   /* get settings from RNA properties for operator */
   int mval[2];
-  mval[0] = RNA_int_get(op->ptr, "mouse_x");
-  mval[1] = RNA_int_get(op->ptr, "mouse_y");
+  RNA_int_get_array(op->ptr, "location", mval);
+
+  struct SelectPick_Params params = {};
+  ED_select_pick_params_from_operator(op, &params);
 
   /* perform the select */
-  const int ret_value = node_mouse_select(C, op, mval, wait_to_deselect_others);
+  const bool changed = node_mouse_select(C, op, mval, &params);
 
-  /* allow tweak event to work too */
-  return ret_value | OPERATOR_PASS_THROUGH;
+  if (changed) {
+    return OPERATOR_PASS_THROUGH | OPERATOR_FINISHED;
+  }
+  /* Nothing selected, just passthrough. */
+  return OPERATOR_PASS_THROUGH | OPERATOR_CANCELLED;
+}
+
+static int node_select_invoke(bContext *C, wmOperator *op, const wmEvent *event)
+{
+  RNA_int_set_array(op->ptr, "location", event->mval);
+
+  const int retval = node_select_exec(C, op);
+
+  return WM_operator_flag_only_pass_through_on_press(retval, event);
 }
 
 void NODE_OT_select(wmOperatorType *ot)
@@ -684,24 +696,28 @@ void NODE_OT_select(wmOperatorType *ot)
 
   /* api callbacks */
   ot->exec = node_select_exec;
-  ot->invoke = WM_generic_select_invoke;
-  ot->modal = WM_generic_select_modal;
+  ot->invoke = node_select_invoke;
   ot->poll = ED_operator_node_active;
 
   /* flags */
   ot->flag = OPTYPE_REGISTER | OPTYPE_UNDO;
 
   /* properties */
-  WM_operator_properties_generic_select(ot);
-  prop = RNA_def_boolean(ot->srna, "extend", false, "Extend", "");
-  RNA_def_property_flag(prop, PROP_SKIP_SAVE);
+  WM_operator_properties_mouse_select(ot);
+
+  prop = RNA_def_int_vector(ot->srna,
+                            "location",
+                            2,
+                            NULL,
+                            INT_MIN,
+                            INT_MAX,
+                            "Location",
+                            "Mouse location",
+                            INT_MIN,
+                            INT_MAX);
+  RNA_def_property_flag(prop, PROP_HIDDEN);
+
   RNA_def_boolean(ot->srna, "socket_select", false, "Socket Select", "");
-  prop = RNA_def_boolean(ot->srna,
-                         "deselect_all",
-                         false,
-                         "Deselect On Nothing",
-                         "Deselect all when nothing under the cursor");
-  RNA_def_property_flag(prop, PROP_SKIP_SAVE);
 }
 
 /** \} */