Cleanup: various improvements to ed_object_select_pick

- Document parameters.
- Add code-comments.
- Remove some historic/unhelpful code-comments.
- Rename argument names that were ambiguous
  (object was a boolean for e.g.).
- Move `gpu` picking into an allocated struct which is only
  allocated & used when using GPU picking.
- Move variable declarations after menu picking has been handled.

1 files changed, 137 insertions, 88 deletions

diff --git a/source/blender/editors/space_view3d/view3d_select.c b/source/blender/editors/space_view3d/view3d_select.c
index 6b209fb0469..7bc24b75877 100644
--- a/source/blender/editors/space_view3d/view3d_select.c
+++ b/source/blender/editors/space_view3d/view3d_select.c
@@ -2419,76 +2419,95 @@ static bool ed_object_select_pick_camera_track(bContext *C,
   return changed || found;
 }
 
+/**
+ * Cursor selection picking for object & pose-mode.
+ *
+ * \param mval: Region relative cursor coordinates.
+ * \param params: Selection parameters.
+ * \param center: Select by the cursors on-screen distances to the center/origin
+ * instead of the geometry any other contents of the item being selected.
+ * This could be used to select by bones by their origin too, currently it's only used for objects.
+ * \param enumerate: Show a menu for objects at the cursor location.
+ * Otherwise fall-through to non-menu selection.
+ * \param object_only: Only select objects (not bones / track markers).
+ */
 static bool ed_object_select_pick(bContext *C,
                                   const int mval[2],
                                   const struct SelectPick_Params *params,
-                                  bool obcenter,
-                                  bool enumerate,
-                                  bool object)
+                                  const bool center,
+                                  const bool enumerate,
+                                  const bool object_only)
 {
   Depsgraph *depsgraph = CTX_data_ensure_evaluated_depsgraph(C);
   ViewContext vc;
   /* Setup view context for argument to callbacks. */
   ED_view3d_viewcontext_init(C, &vc, depsgraph);
 
-  Scene *scene = CTX_data_scene(C);
-  ViewLayer *view_layer = CTX_data_view_layer(C);
-  View3D *v3d = CTX_wm_view3d(C);
-  /* Don't set when the context has no active object (hidden), see: T60807. */
-  const Base *oldbasact = vc.obact ? BASACT(view_layer) : NULL;
-  Base *startbase = NULL, *basact = NULL, *basact_override = NULL;
-  const eObjectMode object_mode = oldbasact ? oldbasact->object->mode : OB_MODE_OBJECT;
-  const bool is_obedit = (vc.obedit != NULL);
-
-  /* Handle setting the new base active */
-  bool use_activate_selected_base = false;
+  Scene *scene = vc.scene;
+  View3D *v3d = vc.v3d;
 
-  /* When enabled, don't attempt any further selection. */
-  bool handled = false;
-  bool changed = false;
+  /* Menu activation may find a base to make active (if it only finds a single item to select). */
+  Base *basact_override = NULL;
 
-  if (object) {
+  const bool is_obedit = (vc.obedit != NULL);
+  if (object_only) {
     /* Signal for #view3d_opengl_select to skip edit-mode objects. */
     vc.obedit = NULL;
   }
 
-  /* Always start list from `basact` when cycling the selection. */
-  startbase = FIRSTBASE(view_layer);
-  if (oldbasact && oldbasact->next) {
-    startbase = oldbasact->next;
-  }
+  /* Set for GPU depth buffer picking, leave NULL when selecting by center. */
+  struct {
+    GPUSelectResult buffer[MAXPICKELEMS];
+    int hits;
+    bool do_nearest;
+    bool has_bones;
+  } *gpu = NULL;
 
-  GPUSelectResult buffer[MAXPICKELEMS];
-  int hits = 0;
-  bool do_nearest = false;
-  bool has_bones = false;
+  /* First handle menu selection, early exit if a menu opens
+   * since this takes ownership of the selection action.
+   *
+   * Even when there is no menu `basact_override` may be set to avoid having to re-find
+   * the item under the cursor. */
+
+  if (center == false) {
+    gpu = MEM_mallocN(sizeof(*gpu), __func__);
+    gpu->do_nearest = false;
+    gpu->has_bones = false;
 
-  if (obcenter == false) {
     /* If objects have pose-mode set, the bones are in the same selection buffer. */
-    const eV3DSelectObjectFilter select_filter = ((object == false) ?
+    const eV3DSelectObjectFilter select_filter = ((object_only == false) ?
                                                       ED_view3d_select_filter_from_mode(scene,
                                                                                         vc.obact) :
                                                       VIEW3D_SELECT_FILTER_NOP);
-    hits = mixed_bones_object_selectbuffer_extended(
-        &vc, buffer, ARRAY_SIZE(buffer), mval, select_filter, true, enumerate, &do_nearest);
-    has_bones = (object && hits > 0) ? false : selectbuffer_has_bones(buffer, hits);
+    gpu->hits = mixed_bones_object_selectbuffer_extended(&vc,
+                                                         gpu->buffer,
+                                                         ARRAY_SIZE(gpu->buffer),
+                                                         mval,
+                                                         select_filter,
+                                                         true,
+                                                         enumerate,
+                                                         &gpu->do_nearest);
+    gpu->has_bones = (object_only && gpu->hits > 0) ?
+                         false :
+                         selectbuffer_has_bones(gpu->buffer, gpu->hits);
   }
 
   /* First handle menu selection, early exit when a menu was opened.
    * Otherwise fall through to regular selection. */
   if (enumerate) {
     bool has_menu = false;
-    if (obcenter) {
+    if (center) {
       if (object_mouse_select_menu(C, &vc, NULL, 0, mval, params, &basact_override)) {
         has_menu = true;
       }
     }
     else {
-      if (hits != 0) {
-        if (has_bones && bone_mouse_select_menu(C, buffer, hits, false, params)) {
+      if (gpu->hits != 0) {
+        if (gpu->has_bones && bone_mouse_select_menu(C, gpu->buffer, gpu->hits, false, params)) {
           has_menu = true;
         }
-        else if (object_mouse_select_menu(C, &vc, buffer, hits, mval, params, &basact_override)) {
+        else if (object_mouse_select_menu(
+                     C, &vc, gpu->buffer, gpu->hits, mval, params, &basact_override)) {
           has_menu = true;
         }
       }
@@ -2496,15 +2515,33 @@ static bool ed_object_select_pick(bContext *C,
 
     /* Let the menu handle any further actions. */
     if (has_menu) {
+      if (gpu != NULL) {
+        MEM_freeN(gpu);
+      }
       return false;
     }
   }
 
-  /* This block uses the control key to make the object selected
-   * by its center point rather than its contents */
+  /* No menu, continue with selection. */
 
-  /* In edit-mode do not activate. */
-  if (obcenter) {
+  ViewLayer *view_layer = vc.view_layer;
+  /* Don't set when the context has no active object (hidden), see: T60807. */
+  const Base *oldbasact = vc.obact ? BASACT(view_layer) : NULL;
+  /* Always start list from `basact` when cycling the selection. */
+  Base *startbase = (oldbasact && oldbasact->next) ? oldbasact->next : FIRSTBASE(view_layer);
+
+  /* The next object's base to make active. */
+  Base *basact = NULL;
+  const eObjectMode object_mode = oldbasact ? oldbasact->object->mode : OB_MODE_OBJECT;
+
+  /* When enabled, don't attempt any further selection. */
+  bool handled = false;
+  bool changed = false;
+
+  /* Handle setting the new base active (even when `handled == true`). */
+  bool use_activate_selected_base = false;
+
+  if (center) {
     if (basact_override) {
       basact = basact_override;
     }
@@ -2517,19 +2554,23 @@ static bool ed_object_select_pick(bContext *C,
       basact = basact_override;
     }
     else {
-      basact = (hits > 0) ? mouse_select_eval_buffer(
-                                &vc, buffer, hits, startbase, has_bones, do_nearest, NULL) :
-                            NULL;
+      basact =
+          (gpu->hits > 0) ?
+              mouse_select_eval_buffer(
+                  &vc, gpu->buffer, gpu->hits, startbase, gpu->has_bones, gpu->do_nearest, NULL) :
+              NULL;
     }
 
-    if (((hits > 0) && has_bones) ||
+    /* Select pose-bones or camera-tracks. */
+    if (((gpu->hits > 0) && gpu->has_bones) ||
         /* Special case, even when there are no hits, pose logic may de-select all bones. */
-        ((hits == 0) && (object_mode & OB_MODE_POSE))) {
+        ((gpu->hits == 0) && (object_mode & OB_MODE_POSE))) {
 
-      if (basact && (has_bones && (basact->object->type == OB_CAMERA))) {
+      if (basact && (gpu->has_bones && (basact->object->type == OB_CAMERA))) {
         MovieClip *clip = BKE_object_movieclip_get(scene, basact->object, false);
         if (clip != NULL) {
-          if (ed_object_select_pick_camera_track(C, scene, basact, clip, buffer, hits, params)) {
+          if (ed_object_select_pick_camera_track(
+                  C, scene, basact, clip, gpu->buffer, gpu->hits, params)) {
             ED_object_base_select(basact, BA_SELECT);
 
             /* Don't set `handled` here as the object activation may be necessary. */
@@ -2539,31 +2580,32 @@ static bool ed_object_select_pick(bContext *C,
             /* Fallback to regular object selection if no new bundles were selected,
              * allows to select object parented to reconstruction object. */
             basact = mouse_select_eval_buffer(
-                &vc, buffer, hits, startbase, false, do_nearest, NULL);
+                &vc, gpu->buffer, gpu->hits, startbase, false, gpu->do_nearest, NULL);
           }
         }
       }
       else if (ED_armature_pose_select_pick_with_buffer(view_layer,
                                                         v3d,
                                                         basact ? basact : (Base *)oldbasact,
-                                                        buffer,
-                                                        hits,
+                                                        gpu->buffer,
+                                                        gpu->hits,
                                                         params,
-                                                        do_nearest)) {
+                                                        gpu->do_nearest)) {
         /* When there is no `baseact` this will have operated on `oldbasact`,
-         * no object operations are needed. */
+         * allowing #SelectPick_Params.deselect_all work in pose-mode.
+         * In this case no object operations are needed. */
         if (basact != NULL) {
-          /* then bone is found */
-          /* we make the armature selected:
-           * not-selected active object in pose-mode won't work well for tools */
+          /* By convention the armature-object is selected when in pose-mode.
+           * While leaving it unselected will work, leaving pose-mode would leave the object
+           * active + unselected which isn't ideal when performing other actions on the object. */
           ED_object_base_select(basact, BA_SELECT);
 
           WM_event_add_notifier(C, NC_OBJECT | ND_BONE_SELECT, basact->object);
           WM_event_add_notifier(C, NC_OBJECT | ND_BONE_ACTIVE, basact->object);
           DEG_id_tag_update(&scene->id, ID_RECALC_BASE_FLAGS);
 
-          /* In weight-paint, we use selected bone to select vertex-group,
-           * so don't switch to new active object. */
+          /* In weight-paint, we use selected bone to select vertex-group.
+           * In this case the active object mustn't change as it would leave weight-paint mode. */
           if (oldbasact) {
             if (oldbasact->object->mode & OB_MODE_ALL_WEIGHT_PAINT) {
               /* Prevent activating.
@@ -2601,33 +2643,34 @@ static bool ed_object_select_pick(bContext *C,
     }
   }
 
-  if ((scene->toolsettings->object_flag & SCE_OBJECT_MODE_LOCK) &&
-      /* No further selection should take place. */
-      (handled == false) &&
+  if (handled == false) {
+    if (scene->toolsettings->object_flag & SCE_OBJECT_MODE_LOCK) {
       /* No special logic in edit-mode. */
-      (is_obedit == false)) {
-
-    if (basact && !BKE_object_is_mode_compat(basact->object, object_mode)) {
-      if (object_mode == OB_MODE_OBJECT) {
-        struct Main *bmain = CTX_data_main(C);
-        ED_object_mode_generic_exit(bmain, vc.depsgraph, scene, basact->object);
-      }
-      if (!BKE_object_is_mode_compat(basact->object, object_mode)) {
-        basact = NULL;
-      }
-    }
+      if (is_obedit == false) {
+        if (basact && !BKE_object_is_mode_compat(basact->object, object_mode)) {
+          if (object_mode == OB_MODE_OBJECT) {
+            struct Main *bmain = vc.bmain;
+            ED_object_mode_generic_exit(bmain, vc.depsgraph, scene, basact->object);
+          }
+          if (!BKE_object_is_mode_compat(basact->object, object_mode)) {
+            basact = NULL;
+          }
+        }
 
-    /* Disallow switching modes,
-     * special exception for edit-mode - vertex-parent operator. */
-    if (basact && oldbasact) {
-      if ((oldbasact->object->mode != basact->object->mode) &&
-          (oldbasact->object->mode & basact->object->mode) == 0) {
-        basact = NULL;
+        /* Disallow switching modes,
+         * special exception for edit-mode - vertex-parent operator. */
+        if (basact && oldbasact) {
+          if ((oldbasact->object->mode != basact->object->mode) &&
+              (oldbasact->object->mode & basact->object->mode) == 0) {
+            basact = NULL;
+          }
+        }
       }
     }
   }
 
-  /* Ensure code above doesn't change the active base. */
+  /* Ensure code above doesn't change the active base. This code is already fairly involved,
+   * it's best if changing the active object is localized to a single place. */
   BLI_assert(oldbasact == (vc.obact ? BASACT(view_layer) : NULL));
 
   bool found = (basact != NULL);
@@ -2646,12 +2689,12 @@ static bool ed_object_select_pick(bContext *C,
     }
   }
 
-  /* so, do we have something selected? */
   if ((handled == false) && found) {
     changed = true;
 
     if (vc.obedit) {
-      /* Only do the select (use for setting vertex parents & hooks). */
+      /* Only do the select (use for setting vertex parents & hooks).
+       * In edit-mode do not activate. */
       object_deselect_all_except(view_layer, basact);
       ED_object_base_select(basact, BA_SELECT);
     }
@@ -2696,6 +2739,8 @@ static bool ed_object_select_pick(bContext *C,
     WM_event_add_notifier(C, NC_SCENE | ND_OB_SELECT, scene);
   }
 
+  /* Perform the activation even when 'handled', since this is used to ensure
+   * the object from the pose-bone selected is also activated. */
   if (use_activate_selected_base && (basact != NULL)) {
     changed = true;
     ED_object_base_activate(C, basact); /* adds notifier */
@@ -2713,6 +2758,10 @@ static bool ed_object_select_pick(bContext *C,
     }
   }
 
+  if (gpu != NULL) {
+    MEM_freeN(gpu);
+  }
+
   return changed;
 }
 
@@ -2810,11 +2859,11 @@ static int view3d_select_exec(bContext *C, wmOperator *op)
   bool enumerate = RNA_boolean_get(op->ptr, "enumerate");
   /* Only force object select for edit-mode to support vertex parenting,
    * or paint-select to allow pose bone select with vert/face select. */
-  bool object = (RNA_boolean_get(op->ptr, "object") &&
-                 (obedit || BKE_paint_select_elem_test(obact) ||
-                  /* so its possible to select bones in weight-paint mode (LMB select) */
-                  (obact && (obact->mode & OB_MODE_ALL_WEIGHT_PAINT) &&
-                   BKE_object_pose_armature_get(obact))));
+  bool object_only = (RNA_boolean_get(op->ptr, "object") &&
+                      (obedit || BKE_paint_select_elem_test(obact) ||
+                       /* so its possible to select bones in weight-paint mode (LMB select) */
+                       (obact && (obact->mode & OB_MODE_ALL_WEIGHT_PAINT) &&
+                        BKE_object_pose_armature_get(obact))));
 
   /* This could be called "changed_or_found" since this is true when there is an element
    * under the cursor to select, even if it happens that the selection & active state doesn't
@@ -2827,7 +2876,7 @@ static int view3d_select_exec(bContext *C, wmOperator *op)
   view3d_operator_needs_opengl(C);
   BKE_object_update_select_id(CTX_data_main(C));
 
-  if (object) {
+  if (object_only) {
     obedit = NULL;
     obact = NULL;
 
@@ -2837,7 +2886,7 @@ static int view3d_select_exec(bContext *C, wmOperator *op)
     center = false;
   }
 
-  if (obedit && object == false) {
+  if (obedit && object_only == false) {
     if (obedit->type == OB_MESH) {
       changed = EDBM_select_pick(C, mval, &params);
     }
@@ -2879,7 +2928,7 @@ static int view3d_select_exec(bContext *C, wmOperator *op)
     changed = ed_wpaint_vertex_select_pick(C, mval, &params, obact);
   }
   else {
-    changed = ed_object_select_pick(C, mval, &params, center, enumerate, object);
+    changed = ed_object_select_pick(C, mval, &params, center, enumerate, object_only);
   }
 
   /* Pass-through flag may be cleared, see #WM_operator_flag_only_pass_through_on_press. */