Fix: Crash in sculpt mode with shared normals caches #4

Closed
Hans Goudey wants to merge 3 commits from fix-pbvh-normals-crash into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
5 changed files with 32 additions and 7 deletions

View File

@ -1492,7 +1492,7 @@ std::optional<blender::Bounds<blender::float3>> Mesh::bounds_min_max() const
void Mesh::bounds_set_eager(const blender::Bounds<float3> &bounds) void Mesh::bounds_set_eager(const blender::Bounds<float3> &bounds)
{ {
this->runtime->bounds_cache.ensure([&](blender::Bounds<float3> &r_data) { r_data = bounds; }); this->runtime->bounds_cache.update([&](blender::Bounds<float3> &r_data) { r_data = bounds; });
} }
void BKE_mesh_transform(Mesh *me, const float mat[4][4], bool do_keys) void BKE_mesh_transform(Mesh *me, const float mat[4][4], bool do_keys)

View File

@ -1316,7 +1316,7 @@ static void pbvh_faces_update_normals(PBVH *pbvh, Span<PBVHNode *> nodes, Mesh &
VectorSet<int> verts_to_update; VectorSet<int> verts_to_update;
threading::parallel_invoke( 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) { threading::parallel_for(faces_to_update.index_range(), 512, [&](const IndexRange range) {
for (const int i : faces_to_update.as_span().slice(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])); 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(); 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) { threading::parallel_for(verts_to_update.index_range(), 1024, [&](const IndexRange range) {
for (const int vert : verts_to_update.as_span().slice(range)) { for (const int vert : verts_to_update.as_span().slice(range)) {
float3 normal(0.0f); float3 normal(0.0f);

View File

@ -30,6 +30,8 @@ template<typename T> class SharedCache {
struct CacheData { struct CacheData {
CacheMutex mutex; CacheMutex mutex;
T data; T data;
CacheData() = default;
CacheData(const T &data) : data(data) {}
}; };
std::shared_ptr<CacheData> cache_; std::shared_ptr<CacheData> cache_;
@ -60,6 +62,23 @@ template<typename T> class SharedCache {
cache_->mutex.ensure([&]() { compute_cache(this->cache_->data); }); 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. */ /** Retrieve the cached data. */
const T &data() const const T &data() const
{ {

View File

@ -5421,9 +5421,12 @@ void SCULPT_flush_update_step(bContext *C, SculptUpdateType update_flags)
if (update_flags & SCULPT_UPDATE_COORDS && !ss->shapekey_active) { if (update_flags & SCULPT_UPDATE_COORDS && !ss->shapekey_active) {
if (BKE_pbvh_type(ss->pbvh) == PBVH_FACES) { if (BKE_pbvh_type(ss->pbvh) == PBVH_FACES) {
/* When sculpting and changing the positions of a mesh, tag them as changed and update. */ /* Updating mesh positions without marking caches dirty is generally not good, but since
BKE_mesh_tag_positions_changed(mesh); * sculpt mode has special requirements and is expected to have sole ownership of the mesh it
/* Update the mesh's bounds eagerly since the PBVH already has that information. */ * 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<float3> bounds; Bounds<float3> bounds;
BKE_pbvh_bounding_box(ob->sculpt->pbvh, bounds.min, bounds.max); BKE_pbvh_bounding_box(ob->sculpt->pbvh, bounds.min, bounds.max);
mesh->bounds_set_eager(bounds); mesh->bounds_set_eager(bounds);

View File

@ -313,7 +313,10 @@ typedef struct Mesh {
*/ */
std::optional<blender::Bounds<blender::float3>> bounds_min_max() const; std::optional<blender::Bounds<blender::float3>> 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<blender::float3> &bounds); void bounds_set_eager(const blender::Bounds<blender::float3> &bounds);
/** /**