From 111e5864248043c477ba13b1c531e20cdf7d7f47 Mon Sep 17 00:00:00 2001 From: Hans Goudey Date: Thu, 24 Aug 2023 16:47:55 +0200 Subject: [PATCH] Cleanup: Sculpt: Avoid unnecessarily changing vertex normals Keeping a mutable reference to vertex normals for the entire lifetime of the PBVH structure makes caching the normals and sharing the cache harder than it should be. Generally code is safer when we reduce the number of mutable references to data. Currently the normals are modified in two places. First is the sculpt mesh normal recalculation. There we can just retrieve the normals from the mesh each time. Second is the restore from an undo step. That is unnecessary because the normals are marked for recalculation anyway. It doesn't even make much sense to store the normals in an undo step when we can easily recalculate them based on new positions. This change helps with #110479. These were also the last place that kept a mutable reference to normals. I tested undo and redo after sculpting, and it works well for each PBVH type. Pull Request: https://projects.blender.org/blender/blender/pulls/111470 --- source/blender/blenkernel/BKE_pbvh_api.hh | 6 +++--- source/blender/blenkernel/intern/pbvh.cc | 12 +++++++----- source/blender/blenkernel/intern/pbvh_intern.hh | 5 ++--- source/blender/editors/sculpt_paint/sculpt.cc | 6 ------ 4 files changed, 12 insertions(+), 17 deletions(-) diff --git a/source/blender/blenkernel/BKE_pbvh_api.hh b/source/blender/blenkernel/BKE_pbvh_api.hh index 90a3078d5e2..cde2dce967a 100644 --- a/source/blender/blenkernel/BKE_pbvh_api.hh +++ b/source/blender/blenkernel/BKE_pbvh_api.hh @@ -499,7 +499,7 @@ struct PBVHVertexIter { /* mesh */ blender::MutableSpan vert_positions; - blender::MutableSpan vert_normals; + blender::Span vert_normals; const bool *hide_vert; int totvert; const int *vert_indices; @@ -516,8 +516,8 @@ struct PBVHVertexIter { * that compiler optimization's will skip the ones we don't use */ BMVert *bm_vert; float *co; - float *no; - float *fno; + const float *no; + const float *fno; float *mask; bool visible; }; diff --git a/source/blender/blenkernel/intern/pbvh.cc b/source/blender/blenkernel/intern/pbvh.cc index 4b0af2bf691..cbfa03ff68c 100644 --- a/source/blender/blenkernel/intern/pbvh.cc +++ b/source/blender/blenkernel/intern/pbvh.cc @@ -1296,7 +1296,7 @@ static bool update_search(PBVHNode *node, const int flag) return true; } -static void pbvh_faces_update_normals(PBVH *pbvh, Span nodes) +static void pbvh_faces_update_normals(PBVH *pbvh, Span nodes, Mesh &mesh) { using namespace blender; using namespace blender::bke; @@ -1319,17 +1319,16 @@ static void pbvh_faces_update_normals(PBVH *pbvh, Span nodes) return; } - MutableSpan vert_normals = pbvh->vert_normals; - MutableSpan face_normals = pbvh->face_normals; - VectorSet verts_to_update; threading::parallel_invoke( [&]() { + MutableSpan face_normals = mesh.runtime->face_normals; threading::parallel_for(faces_to_update.index_range(), 512, [&](const IndexRange range) { for (const int i : faces_to_update.as_span().slice(range)) { face_normals[i] = mesh::face_normal_calc(positions, corner_verts.slice(faces[i])); } }); + mesh.runtime->face_normals_dirty = false; }, [&]() { /* Update all normals connected to affected faces, even if not explicitly tagged. */ @@ -1346,6 +1345,8 @@ static void pbvh_faces_update_normals(PBVH *pbvh, Span nodes) } }); + const Span face_normals = mesh.runtime->face_normals; + MutableSpan vert_normals = mesh.runtime->vert_normals; 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); @@ -1355,6 +1356,7 @@ static void pbvh_faces_update_normals(PBVH *pbvh, Span nodes) vert_normals[vert] = math::normalize(normal); } }); + mesh.runtime->vert_normals_dirty = false; } static void node_update_mask_redraw(PBVH &pbvh, PBVHNode &node) @@ -2849,7 +2851,7 @@ void BKE_pbvh_update_normals(PBVH *pbvh, SubdivCCG *subdiv_ccg) pbvh_bmesh_normals_update(nodes); } else if (pbvh->header.type == PBVH_FACES) { - pbvh_faces_update_normals(pbvh, nodes); + pbvh_faces_update_normals(pbvh, nodes, *pbvh->mesh); } else if (pbvh->header.type == PBVH_GRIDS) { CCGFace **faces; diff --git a/source/blender/blenkernel/intern/pbvh_intern.hh b/source/blender/blenkernel/intern/pbvh_intern.hh index 0dc1584ac3c..514fabc27e7 100644 --- a/source/blender/blenkernel/intern/pbvh_intern.hh +++ b/source/blender/blenkernel/intern/pbvh_intern.hh @@ -151,9 +151,8 @@ struct PBVH { /* Mesh data */ Mesh *mesh; - /* NOTE: Normals are not `const` because they can be updated for drawing by sculpt code. */ - blender::MutableSpan vert_normals; - blender::MutableSpan face_normals; + blender::Span vert_normals; + blender::Span face_normals; bool *hide_vert; blender::MutableSpan vert_positions; /** Local vertex positions owned by the PVBH when not sculpting base mesh positions directly. */ diff --git a/source/blender/editors/sculpt_paint/sculpt.cc b/source/blender/editors/sculpt_paint/sculpt.cc index 86a18d3b73a..9b64a04c644 100644 --- a/source/blender/editors/sculpt_paint/sculpt.cc +++ b/source/blender/editors/sculpt_paint/sculpt.cc @@ -1564,12 +1564,6 @@ static void paint_mesh_restore_co_task_cb(void *__restrict userdata, BKE_pbvh_vertex_iter_begin (ss->pbvh, data->nodes[n], vd, PBVH_ITER_UNIQUE) { SCULPT_orig_vert_data_update(&orig_vert_data, &vd); copy_v3_v3(vd.co, orig_vert_data.co); - if (vd.no) { - copy_v3_v3(vd.no, orig_vert_data.no); - } - else { - copy_v3_v3(vd.fno, orig_vert_data.no); - } if (vd.is_mesh) { BKE_pbvh_vert_tag_update_normal(ss->pbvh, vd.vertex); }