From b339e3937d8dc580b655efe599c3b0f97f412fb1 Mon Sep 17 00:00:00 2001 From: Hans Goudey Date: Tue, 29 Aug 2023 17:07:42 +0200 Subject: [PATCH] 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 | 6 ++++++ .../blender/blenkernel/intern/mesh_runtime.cc | 5 +++++ source/blender/blenkernel/intern/pbvh.cc | 4 ++-- source/blender/blenlib/BLI_shared_cache.hh | 19 +++++++++++++++++++ source/blender/editors/sculpt_paint/sculpt.cc | 11 ++++++++--- 5 files changed, 40 insertions(+), 5 deletions(-) diff --git a/source/blender/blenkernel/BKE_mesh.h b/source/blender/blenkernel/BKE_mesh.h index 97e37490cab..f2c3c016335 100644 --- a/source/blender/blenkernel/BKE_mesh.h +++ b/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. */ diff --git a/source/blender/blenkernel/intern/mesh_runtime.cc b/source/blender/blenkernel/intern/mesh_runtime.cc index b824472e22a..2f4ef7bb81d 100644 --- a/source/blender/blenkernel/intern/mesh_runtime.cc +++ b/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(); diff --git a/source/blender/blenkernel/intern/pbvh.cc b/source/blender/blenkernel/intern/pbvh.cc index 6d5ebcb6ec3..34f5e665423 100644 --- a/source/blender/blenkernel/intern/pbvh.cc +++ b/source/blender/blenkernel/intern/pbvh.cc @@ -1316,7 +1316,7 @@ static void pbvh_faces_update_normals(PBVH *pbvh, Span nodes, Mesh & VectorSet verts_to_update; threading::parallel_invoke( [&]() { - mesh.runtime->face_normals_cache.ensure([&](Vector &r_data) { + mesh.runtime->face_normals_cache.update([&](Vector &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 nodes, Mesh & }); const Span face_normals = mesh.face_normals(); - mesh.runtime->vert_normals_cache.ensure([&](Vector &r_data) { + mesh.runtime->vert_normals_cache.update([&](Vector &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); diff --git a/source/blender/blenlib/BLI_shared_cache.hh b/source/blender/blenlib/BLI_shared_cache.hh index d0e83bc8525..5f715ef4191 100644 --- a/source/blender/blenlib/BLI_shared_cache.hh +++ b/source/blender/blenlib/BLI_shared_cache.hh @@ -30,6 +30,8 @@ template class SharedCache { struct CacheData { CacheMutex mutex; T data; + CacheData() = default; + CacheData(const T &data) : data(data) {} }; std::shared_ptr cache_; @@ -60,6 +62,23 @@ template 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 compute_cache) + { + if (cache_.unique()) { + cache_->mutex.tag_dirty(); + } + else { + cache_ = std::make_shared(cache_->data); + } + cache_->mutex.ensure([&]() { compute_cache(this->cache_->data); }); + } + /** Retrieve the cached data. */ const T &data() const { diff --git a/source/blender/editors/sculpt_paint/sculpt.cc b/source/blender/editors/sculpt_paint/sculpt.cc index ff0f1e39db6..b2eb8c4eef7 100644 --- a/source/blender/editors/sculpt_paint/sculpt.cc +++ b/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 bounds; BKE_pbvh_bounding_box(ob->sculpt->pbvh, bounds.min, bounds.max); mesh->bounds_set_eager(bounds);