Fix #127357: Expand operator crashes when cursor is not on mesh

Prior to 0df133559c, the `active_vert_` property was only ever
invalidated if the entire `SculptSession` was refreshed, even if the
cursor moved off of the mesh or if the underlying mesh type changed.

The Sculpt Expand operator was dependent on this value not being cleared
to still behave in a reasonable way from a user perspective

This commit introduces a new variable, `last_active_vert_` and
corresponding accessor methods to `SculptSession` to specifically fix
this usecase. When the active vert is cleared, if it had a valid value,
we store it in the new `last_active_vert_` variable.

Pull Request: https://projects.blender.org/blender/blender/pulls/127433

source/blender/blenkernel/BKE_paint.hh

@@ -589,6 +589,11 @@ struct SculptSession : blender::NonCopyable, blender::NonMovable {
    */
   ActiveVert active_vert_ = {};
 
+  /* This value should always exist except when the cursor has never been over the mesh, or when
+   * the underlying mesh type has changed and the last `active_vert_` value no longer corresponds
+   * to a value that can be correctly interpreted */
+  ActiveVert last_active_vert_ = {};
+
  public:
   SculptSession();
   ~SculptSession();
@@ -596,6 +601,9 @@ struct SculptSession : blender::NonCopyable, blender::NonMovable {
   PBVHVertRef active_vert_ref() const;
   ActiveVert active_vert() const;
 
+  PBVHVertRef last_active_vert_ref() const;
+  ActiveVert last_active_vert() const;
+
   /**
    * Retrieves the corresponding index of the ActiveVert inside a mesh-sized array.
    *
@@ -619,7 +627,7 @@ struct SculptSession : blender::NonCopyable, blender::NonMovable {
   blender::float3 active_vert_position(const Depsgraph &depsgraph, const Object &object) const;
 
   void set_active_vert(ActiveVert vert);
-  void clear_active_vert();
+  void clear_active_vert(bool persist_last_active);
 };
 
 void BKE_sculptsession_free(Object *ob);

source/blender/blenkernel/intern/paint.cc

@@ -1697,7 +1697,7 @@ void BKE_sculptsession_free_pbvh(Object &object)
   ss->vertex_info.boundary.clear_and_shrink();
   ss->fake_neighbors.fake_neighbor_index = {};
 
-  ss->clear_active_vert();
+  ss->clear_active_vert(false);
 }
 
 void BKE_sculptsession_bm_to_me_for_render(Object *object)
@@ -1787,6 +1787,27 @@ ActiveVert SculptSession::active_vert() const
   return active_vert_;
 }
 
+PBVHVertRef SculptSession::last_active_vert_ref() const
+{
+  if (std::holds_alternative<int>(last_active_vert_)) {
+    return {std::get<int>(last_active_vert_)};
+  }
+  if (std::holds_alternative<SubdivCCGCoord>(last_active_vert_)) {
+    const CCGKey key = BKE_subdiv_ccg_key_top_level(*this->subdiv_ccg);
+    const int index = std::get<SubdivCCGCoord>(last_active_vert_).to_index(key);
+    return {index};
+  }
+  if (std::holds_alternative<BMVert *>(last_active_vert_)) {
+    return {reinterpret_cast<intptr_t>(std::get<BMVert *>(last_active_vert_))};
+  }
+  return {PBVH_REF_NONE};
+}
+
+ActiveVert SculptSession::last_active_vert() const
+{
+  return active_vert_;
+}
+
 int SculptSession::active_vert_index() const
 {
   if (std::holds_alternative<int>(active_vert_)) {
@@ -1825,8 +1846,16 @@ blender::float3 SculptSession::active_vert_position(const Depsgraph &depsgraph,
   return float3(std::numeric_limits<float>::infinity());
 }
 
-void SculptSession::clear_active_vert()
+void SculptSession::clear_active_vert(bool persist_last_active)
 {
+  if (persist_last_active) {
+    if (!std::holds_alternative<std::monostate>(active_vert_)) {
+      last_active_vert_ = active_vert_;
+    }
+  }
+  else {
+    last_active_vert_ = {};
+  }
   active_vert_ = {};
 }
 

source/blender/editors/sculpt_paint/sculpt.cc

@@ -4971,7 +4971,7 @@ bool SCULPT_cursor_geometry_info_update(bContext *C,
     zero_v3(out->location);
     zero_v3(out->normal);
     zero_v3(out->active_vertex_co);
-    ss.clear_active_vert();
+    ss.clear_active_vert(false);
     return false;
   }
 
@@ -5015,7 +5015,7 @@ bool SCULPT_cursor_geometry_info_update(bContext *C,
     zero_v3(out->location);
     zero_v3(out->normal);
     zero_v3(out->active_vertex_co);
-    ss.clear_active_vert();
+    ss.clear_active_vert(true);
     return false;
   }
 

source/blender/editors/sculpt_paint/sculpt_expand.cc

@@ -2103,7 +2103,7 @@ static void find_active_connected_components_from_vert(const Depsgraph &depsgrap
  * Stores the active vertex, Face Set and mouse coordinates in the #Cache based on the
  * current cursor position.
  */
-static void set_initial_components_for_mouse(bContext *C,
+static bool set_initial_components_for_mouse(bContext *C,
                                              Object &ob,
                                              Cache &expand_cache,
                                              const float mval[2])
@@ -2116,7 +2116,10 @@ static void set_initial_components_for_mouse(bContext *C,
   if (initial_vertex.i == SCULPT_EXPAND_VERTEX_NONE) {
     /* Cursor not over the mesh, for creating valid initial falloffs, fallback to the last active
      * vertex in the sculpt session. */
-    initial_vertex = ss.active_vert_ref();
+    initial_vertex = ss.last_active_vert_ref();
+    if (initial_vertex.i == SCULPT_EXPAND_VERTEX_NONE) {
+      return false;
+    }
   }
 
   int initial_vertex_i = BKE_pbvh_vertex_to_index(*bke::object::pbvh_get(ob), initial_vertex);
@@ -2141,6 +2144,7 @@ static void set_initial_components_for_mouse(bContext *C,
   /* The new mouse position can be over a different connected component, so this needs to be
    * updated. */
   find_active_connected_components_from_vert(depsgraph, ob, expand_cache, initial_vertex);
+  return true;
 }
 
 /**
@@ -2633,14 +2637,21 @@ static int sculpt_expand_invoke(bContext *C, wmOperator *op, const wmEvent *even
 
   ensure_sculptsession_data(ob);
 
+  /* Set the initial element for expand from the event position. */
+  const float mouse[2] = {float(event->mval[0]), float(event->mval[1])};
+
+  /* When getting the initial active vert, in cases where the cursor is not over the mesh and
+   * the mesh type has changed, we cannot proceed with the expand operator, as there is no
+   * sensible last active vertex when switching between backing implementations. */
+  if (!set_initial_components_for_mouse(C, ob, *ss.expand_cache, mouse)) {
+    expand_cache_free(ss);
+    return OPERATOR_CANCELLED;
+  }
+
   /* Initialize undo. */
   undo::push_begin(ob, op);
   undo_push(*depsgraph, ob, *ss.expand_cache);
 
-  /* Set the initial element for expand from the event position. */
-  const float mouse[2] = {float(event->mval[0]), float(event->mval[1])};
-  set_initial_components_for_mouse(C, ob, *ss.expand_cache, mouse);
-
   /* Cache bke::pbvh::Tree nodes. */
   ss.expand_cache->node_mask = bke::pbvh::all_leaf_nodes(pbvh, ss.expand_cache->node_mask_memory);