From e690282e650eb15e4a871e71927d4060af42f920 Mon Sep 17 00:00:00 2001 From: Hans Goudey Date: Mon, 28 Aug 2023 15:51:36 -0400 Subject: [PATCH 1/2] Fix: Incorrect PBVH normals update with shared cache --- source/blender/blenkernel/intern/mesh.cc | 2 +- source/blender/blenlib/BLI_shared_cache.hh | 18 ++++++++++++++++++ source/blender/editors/sculpt_paint/sculpt.cc | 9 ++++++--- source/blender/makesdna/DNA_mesh_types.h | 5 ++++- 4 files changed, 29 insertions(+), 5 deletions(-) diff --git a/source/blender/blenkernel/intern/mesh.cc b/source/blender/blenkernel/intern/mesh.cc index 414425eb034..e1b814e418f 100644 --- a/source/blender/blenkernel/intern/mesh.cc +++ b/source/blender/blenkernel/intern/mesh.cc @@ -1492,7 +1492,7 @@ std::optional> Mesh::bounds_min_max() const void Mesh::bounds_set_eager(const blender::Bounds &bounds) { - this->runtime->bounds_cache.ensure([&](blender::Bounds &r_data) { r_data = bounds; }); + this->runtime->bounds_cache.update([&](blender::Bounds &r_data) { r_data = bounds; }); } void BKE_mesh_transform(Mesh *me, const float mat[4][4], bool do_keys) diff --git a/source/blender/blenlib/BLI_shared_cache.hh b/source/blender/blenlib/BLI_shared_cache.hh index d0e83bc8525..bc6d2dcd419 100644 --- a/source/blender/blenlib/BLI_shared_cache.hh +++ b/source/blender/blenlib/BLI_shared_cache.hh @@ -30,6 +30,7 @@ template class SharedCache { struct CacheData { CacheMutex mutex; T data; + CacheData(T data) : data(std::move(data)) {} }; std::shared_ptr cache_; @@ -60,6 +61,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 ea22eeb9edf..59826a7f207 100644 --- a/source/blender/editors/sculpt_paint/sculpt.cc +++ b/source/blender/editors/sculpt_paint/sculpt.cc @@ -5421,9 +5421,12 @@ 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. */ Bounds bounds; BKE_pbvh_bounding_box(ob->sculpt->pbvh, bounds.min, bounds.max); mesh->bounds_set_eager(bounds); diff --git a/source/blender/makesdna/DNA_mesh_types.h b/source/blender/makesdna/DNA_mesh_types.h index 5134f8ac955..2d974a411e4 100644 --- a/source/blender/makesdna/DNA_mesh_types.h +++ b/source/blender/makesdna/DNA_mesh_types.h @@ -313,7 +313,10 @@ typedef struct Mesh { */ std::optional> bounds_min_max() const; - /** Set cached mesh bounds to a known-correct value to avoid their lazy calculation later on. */ + /** + * After positions are changed, set cached mesh bounds to a known-correct value to avoid their + * lazy calculation later on. + */ void bounds_set_eager(const blender::Bounds &bounds); /** -- 2.30.2 From 5e859a085e3d22d4f76ecf24ca1b86ff534d18d5 Mon Sep 17 00:00:00 2001 From: Hans Goudey Date: Mon, 28 Aug 2023 20:39:25 -0400 Subject: [PATCH 2/2] Progress --- source/blender/blenkernel/intern/pbvh.cc | 4 ++-- source/blender/blenlib/BLI_shared_cache.hh | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/source/blender/blenkernel/intern/pbvh.cc b/source/blender/blenkernel/intern/pbvh.cc index ecc32fe9352..6f355ba9c6c 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 bc6d2dcd419..5f715ef4191 100644 --- a/source/blender/blenlib/BLI_shared_cache.hh +++ b/source/blender/blenlib/BLI_shared_cache.hh @@ -30,7 +30,8 @@ template class SharedCache { struct CacheData { CacheMutex mutex; T data; - CacheData(T data) : data(std::move(data)) {} + CacheData() = default; + CacheData(const T &data) : data(data) {} }; std::shared_ptr cache_; -- 2.30.2