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

@@ -499,7 +499,7 @@ struct PBVHVertexIter {
 
   /* mesh */
   blender::MutableSpan<blender::float3> vert_positions;
-  blender::MutableSpan<blender::float3> vert_normals;
+  blender::Span<blender::float3> 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;
 };

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<PBVHNode *> nodes)
+static void pbvh_faces_update_normals(PBVH *pbvh, Span<PBVHNode *> nodes, Mesh &mesh)
 {
   using namespace blender;
   using namespace blender::bke;
@@ -1319,17 +1319,16 @@ static void pbvh_faces_update_normals(PBVH *pbvh, Span<PBVHNode *> nodes)
     return;
   }
 
-  MutableSpan<float3> vert_normals = pbvh->vert_normals;
-  MutableSpan<float3> face_normals = pbvh->face_normals;
-
   VectorSet<int> verts_to_update;
   threading::parallel_invoke(
       [&]() {
+        MutableSpan<float3> 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<PBVHNode *> nodes)
         }
       });
 
+  const Span<float3> face_normals = mesh.runtime->face_normals;
+  MutableSpan<float3> 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<PBVHNode *> 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;

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<blender::float3> vert_normals;
-  blender::MutableSpan<blender::float3> face_normals;
+  blender::Span<blender::float3> vert_normals;
+  blender::Span<blender::float3> face_normals;
   bool *hide_vert;
   blender::MutableSpan<blender::float3> vert_positions;
   /** Local vertex positions owned by the PVBH when not sculpting base mesh positions directly. */

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);
         }