Fix: Crash in sculpt mode with shared normals caches

Since the normals are stored in a shared cache, tagging them dirty
recreated the cache from scratch when it was shared. Instead,
add a function that updates the cache in the same call as tagging
it dirty. This keeps the old state of the cache around even if it was
shared, and reflects the way that it's really the PBVH and sculpt
mode managing the dirty status of normals while sculpt mode
is active.

One consequence is that the BVH cache and the triangulation
cache need to be tagged dirty manually. I'd like to avoid abstracting
this more than necessary, because I'm hoping in the long term
different caching abstractions like a more global cache that takes
implicit sharing versions into account will make this complexity
unnecessary.

Fixes #111628, #111563

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

source/blender/blenkernel/BKE_mesh.h

@@ -58,6 +58,12 @@ typedef enum eMeshBatchDirtyMode {
  */
 void BKE_mesh_tag_positions_changed(struct Mesh *mesh);
 
+/**
+ * The same as #BKE_mesh_tag_positions_changed but doesn't tag normals dirty, instead expecting
+ * them to be updated separately.
+ */
+void BKE_mesh_tag_positions_changed_no_normals(struct Mesh *mesh);
+
 /**
  * Call after moving every mesh vertex by the same translation.
  */

source/blender/blenkernel/intern/mesh_runtime.cc

@@ -309,6 +309,11 @@ void BKE_mesh_tag_positions_changed(Mesh *mesh)
 {
   mesh->runtime->vert_normals_cache.tag_dirty();
   mesh->runtime->face_normals_cache.tag_dirty();
+  BKE_mesh_tag_positions_changed_no_normals(mesh);
+}
+
+void BKE_mesh_tag_positions_changed_no_normals(Mesh *mesh)
+{
   free_bvh_cache(*mesh->runtime);
   mesh->runtime->looptris_cache.tag_dirty();
   mesh->runtime->bounds_cache.tag_dirty();

source/blender/blenkernel/intern/pbvh.cc

@@ -1316,7 +1316,7 @@ static void pbvh_faces_update_normals(PBVH *pbvh, Span<PBVHNode *> nodes, Mesh &
   VectorSet<int> verts_to_update;
   threading::parallel_invoke(
       [&]() {
-        mesh.runtime->face_normals_cache.ensure([&](Vector<float3> &r_data) {
+        mesh.runtime->face_normals_cache.update([&](Vector<float3> &r_data) {
           threading::parallel_for(faces_to_update.index_range(), 512, [&](const IndexRange range) {
             for (const int i : faces_to_update.as_span().slice(range)) {
               r_data[i] = mesh::face_normal_calc(positions, corner_verts.slice(faces[i]));
@@ -1340,7 +1340,7 @@ static void pbvh_faces_update_normals(PBVH *pbvh, Span<PBVHNode *> nodes, Mesh &
       });
 
   const Span<float3> face_normals = mesh.face_normals();
-  mesh.runtime->vert_normals_cache.ensure([&](Vector<float3> &r_data) {
+  mesh.runtime->vert_normals_cache.update([&](Vector<float3> &r_data) {
     threading::parallel_for(verts_to_update.index_range(), 1024, [&](const IndexRange range) {
       for (const int vert : verts_to_update.as_span().slice(range)) {
         float3 normal(0.0f);

source/blender/blenlib/BLI_shared_cache.hh

@@ -30,6 +30,8 @@ template<typename T> class SharedCache {
   struct CacheData {
     CacheMutex mutex;
     T data;
+    CacheData() = default;
+    CacheData(const T &data) : data(data) {}
   };
   std::shared_ptr<CacheData> cache_;
 
@@ -60,6 +62,23 @@ template<typename T> class SharedCache {
     cache_->mutex.ensure([&]() { compute_cache(this->cache_->data); });
   }
 
+  /**
+   * Represents a combination of "tag dirty" and "update cache for new data." Existing cached
+   * values are kept available (copied from shared data if necessary). This can be helpful when
+   * the recalculation is only expected to make a small change to the cached data, since using
+   * #tag_dirty() and #ensure() separately may require rebuilding the cache from scratch.
+   */
+  void update(FunctionRef<void(T &data)> compute_cache)
+  {
+    if (cache_.unique()) {
+      cache_->mutex.tag_dirty();
+    }
+    else {
+      cache_ = std::make_shared<CacheData>(cache_->data);
+    }
+    cache_->mutex.ensure([&]() { compute_cache(this->cache_->data); });
+  }
+
   /** Retrieve the cached data. */
   const T &data() const
   {

source/blender/editors/sculpt_paint/sculpt.cc

@@ -5430,9 +5430,14 @@ void SCULPT_flush_update_step(bContext *C, SculptUpdateType update_flags)
 
   if (update_flags & SCULPT_UPDATE_COORDS && !ss->shapekey_active) {
     if (BKE_pbvh_type(ss->pbvh) == PBVH_FACES) {
-      /* When sculpting and changing the positions of a mesh, tag them as changed and update. */
-      BKE_mesh_tag_positions_changed(mesh);
-      /* Update the mesh's bounds eagerly since the PBVH already has that information. */
+      /* Updating mesh positions without marking caches dirty is generally not good, but since
+       * sculpt mode has special requirements and is expected to have sole ownership of the mesh it
+       * modifies, it's generally okay.
+       *
+       * Vertex and face normals are updated later in #BKE_pbvh_update_normals. However, we update
+       * the mesh's bounds eagerly here since they are trivial to access from the PBVH. */
+      BKE_mesh_tag_positions_changed_no_normals(mesh);
+
       Bounds<float3> bounds;
       BKE_pbvh_bounding_box(ob->sculpt->pbvh, bounds.min, bounds.max);
       mesh->bounds_set_eager(bounds);