From 0dfccb18f5649f932b7d9acf8d6311d8067088e4 Mon Sep 17 00:00:00 2001 From: Hans Goudey Date: Sat, 29 Apr 2023 00:16:22 -0400 Subject: [PATCH 1/3] Fix #107123: Refactor sculpt normal calculation to require vert poly map TODO: Proper description of change --- source/blender/blenkernel/intern/paint.cc | 24 ++- source/blender/blenkernel/intern/pbvh.cc | 177 ++++++------------ .../blender/blenkernel/intern/pbvh_intern.hh | 3 + 3 files changed, 74 insertions(+), 130 deletions(-) diff --git a/source/blender/blenkernel/intern/paint.cc b/source/blender/blenkernel/intern/paint.cc index 14be0979940..75bf23ccda2 100644 --- a/source/blender/blenkernel/intern/paint.cc +++ b/source/blender/blenkernel/intern/paint.cc @@ -1659,8 +1659,10 @@ static void sculpt_update_persistent_base(Object *ob) ob, ATTR_DOMAIN_POINT, CD_PROP_FLOAT, SCULPT_ATTRIBUTE_NAME(persistent_disp)); } -static void sculpt_update_object( - Depsgraph *depsgraph, Object *ob, Object *ob_eval, bool need_pmap, bool is_paint_tool) +static void sculpt_update_object(Depsgraph *depsgraph, + Object *ob, + Object *ob_eval, + bool is_paint_tool) { Scene *scene = DEG_get_input_scene(depsgraph); Sculpt *sd = scene->toolsettings->sculpt; @@ -1766,13 +1768,13 @@ static void sculpt_update_object( sculpt_attribute_update_refs(ob); sculpt_update_persistent_base(ob); - if (need_pmap && ob->type == OB_MESH && !ss->pmap) { + if (ob->type == OB_MESH && !ss->pmap) { BKE_mesh_vert_poly_map_create( &ss->pmap, &ss->pmap_mem, me->polys(), me->corner_verts().data(), me->totvert); + } - if (ss->pbvh) { - BKE_pbvh_pmap_set(ss->pbvh, ss->pmap); - } + if (ss->pbvh) { + BKE_pbvh_pmap_set(ss->pbvh, ss->pmap); } if (ss->deform_modifiers_active) { @@ -1913,7 +1915,7 @@ void BKE_sculpt_update_object_after_eval(Depsgraph *depsgraph, Object *ob_eval) * other data when modifiers change the mesh. */ Object *ob_orig = DEG_get_original_object(ob_eval); - sculpt_update_object(depsgraph, ob_orig, ob_eval, false, false); + sculpt_update_object(depsgraph, ob_orig, ob_eval, false); } void BKE_sculpt_color_layer_create_if_needed(Object *object) @@ -1942,14 +1944,16 @@ void BKE_sculpt_color_layer_create_if_needed(Object *object) } } -void BKE_sculpt_update_object_for_edit( - Depsgraph *depsgraph, Object *ob_orig, bool need_pmap, bool /*need_mask*/, bool is_paint_tool) +void BKE_sculpt_update_object_for_edit(Depsgraph *depsgraph, + Object *ob_orig, + bool /*need_mask*/, + bool is_paint_tool) { BLI_assert(ob_orig == DEG_get_original_object(ob_orig)); Object *ob_eval = DEG_get_evaluated_object(depsgraph, ob_orig); - sculpt_update_object(depsgraph, ob_orig, ob_eval, need_pmap, is_paint_tool); + sculpt_update_object(depsgraph, ob_orig, ob_eval, is_paint_tool); } int *BKE_sculpt_face_sets_ensure(Mesh *mesh) diff --git a/source/blender/blenkernel/intern/pbvh.cc b/source/blender/blenkernel/intern/pbvh.cc index f4c26dcf7a8..afeb9715feb 100644 --- a/source/blender/blenkernel/intern/pbvh.cc +++ b/source/blender/blenkernel/intern/pbvh.cc @@ -11,8 +11,10 @@ #include "BLI_bitmap.h" #include "BLI_ghash.h" #include "BLI_math.h" +#include "BLI_math_vector.hh" #include "BLI_rand.h" #include "BLI_task.h" +#include "BLI_task.hh" #include "BLI_utildefines.h" #include "BLI_vector.hh" @@ -38,6 +40,7 @@ #include "pbvh_intern.hh" using blender::float3; +using blender::IndexRange; using blender::MutableSpan; using blender::Span; using blender::Vector; @@ -858,7 +861,10 @@ void BKE_pbvh_update_mesh_pointers(PBVH *pbvh, Mesh *mesh) /* Make sure cached normals start out calculated. */ mesh->vert_normals(); + mesh->poly_normals(); + pbvh->vert_normals = BKE_mesh_vert_normals_for_write(mesh); + pbvh->poly_normals = mesh->runtime->poly_normals; pbvh->vdata = &mesh->vdata; pbvh->ldata = &mesh->ldata; @@ -1373,131 +1379,62 @@ struct PBVHUpdateData { PBVHUpdateData(PBVH *pbvh_, Span nodes_) : pbvh(pbvh_), nodes(nodes_) {} }; -static void pbvh_update_normals_clear_task_cb(void *__restrict userdata, - const int n, - const TaskParallelTLS *__restrict /*tls*/) -{ - PBVHUpdateData *data = static_cast(userdata); - PBVH *pbvh = data->pbvh; - PBVHNode *node = data->nodes[n]; - float(*vert_normals)[3] = data->vert_normals; - - if (node->flag & PBVH_UpdateNormals) { - const int *verts = node->vert_indices; - const int totvert = node->uniq_verts; - for (int i = 0; i < totvert; i++) { - const int v = verts[i]; - if (pbvh->vert_bitmap[v]) { - zero_v3(vert_normals[v]); - } - } - } -} - -static void pbvh_update_normals_accum_task_cb(void *__restrict userdata, - const int n, - const TaskParallelTLS *__restrict /*tls*/) -{ - PBVHUpdateData *data = static_cast(userdata); - - PBVH *pbvh = data->pbvh; - PBVHNode *node = data->nodes[n]; - float(*vert_normals)[3] = data->vert_normals; - - if (node->flag & PBVH_UpdateNormals) { - uint mpoly_prev = UINT_MAX; - blender::float3 fn; - - const int *faces = node->prim_indices; - const int totface = node->totprim; - - for (int i = 0; i < totface; i++) { - const MLoopTri *lt = &pbvh->looptri[faces[i]]; - const int vtri[3] = { - pbvh->corner_verts[lt->tri[0]], - pbvh->corner_verts[lt->tri[1]], - pbvh->corner_verts[lt->tri[2]], - }; - const int sides = 3; - - /* Face normal and mask */ - if (lt->poly != mpoly_prev) { - const blender::IndexRange poly = pbvh->polys[lt->poly]; - fn = blender::bke::mesh::poly_normal_calc( - {reinterpret_cast(pbvh->vert_positions), pbvh->totvert}, - {&pbvh->corner_verts[poly.start()], poly.size()}); - mpoly_prev = lt->poly; - } - - for (int j = sides; j--;) { - const int v = vtri[j]; - - if (pbvh->vert_bitmap[v]) { - /* NOTE: This avoids `lock, add_v3_v3, unlock` - * and is five to ten times quicker than a spin-lock. - * Not exact equivalent though, since atomicity is only ensured for one component - * of the vector at a time, but here it shall not make any sensible difference. */ - for (int k = 3; k--;) { - atomic_add_and_fetch_fl(&vert_normals[v][k], fn[k]); - } - } - } - } - } -} - -static void pbvh_update_normals_store_task_cb(void *__restrict userdata, - const int n, - const TaskParallelTLS *__restrict /*tls*/) -{ - PBVHUpdateData *data = static_cast(userdata); - PBVH *pbvh = data->pbvh; - PBVHNode *node = data->nodes[n]; - float(*vert_normals)[3] = data->vert_normals; - - if (node->flag & PBVH_UpdateNormals) { - const int *verts = node->vert_indices; - const int totvert = node->uniq_verts; - - for (int i = 0; i < totvert; i++) { - const int v = verts[i]; - - /* No atomics necessary because we are iterating over uniq_verts only, - * so we know only this thread will handle this vertex. */ - if (pbvh->vert_bitmap[v]) { - normalize_v3(vert_normals[v]); - pbvh->vert_bitmap[v] = false; - } - } - - node->flag &= ~PBVH_UpdateNormals; - } -} - static void pbvh_faces_update_normals(PBVH *pbvh, Span nodes) { - /* subtle assumptions: - * - We know that for all edited vertices, the nodes with faces - * adjacent to these vertices have been marked with PBVH_UpdateNormals. - * This is true because if the vertex is inside the brush radius, the - * bounding box of its adjacent faces will be as well. - * - However this is only true for the vertices that have actually been - * edited, not for all vertices in the nodes marked for update, so we - * can only update vertices marked in the `vert_bitmap`. - */ + using namespace blender; + using namespace blender::bke; - PBVHUpdateData data(pbvh, nodes); - data.pbvh = pbvh; - data.nodes = nodes; - data.vert_normals = pbvh->vert_normals; + const Span positions(reinterpret_cast(pbvh->vert_positions), + pbvh->totvert); + const OffsetIndices polys = pbvh->polys; + const Span corner_verts(pbvh->corner_verts, pbvh->mesh->totloop); - TaskParallelSettings settings; - BKE_pbvh_parallel_range_settings(&settings, true, nodes.size()); + MutableSpan update_tags(pbvh->vert_bitmap, pbvh->totvert); + Vector polys_to_update; + for (const int vert : IndexRange(pbvh->totvert)) { + if (update_tags[vert]) { + polys_to_update.extend({pbvh->pmap[vert].indices, pbvh->pmap[vert].count}); + } + } - /* Zero normals before accumulation. */ - BLI_task_parallel_range(0, nodes.size(), &data, pbvh_update_normals_clear_task_cb, &settings); - BLI_task_parallel_range(0, nodes.size(), &data, pbvh_update_normals_accum_task_cb, &settings); - BLI_task_parallel_range(0, nodes.size(), &data, pbvh_update_normals_store_task_cb, &settings); + if (polys_to_update.is_empty()) { + return; + } + + /* Also update vertices connected to updated faces, even if they weren't explicitly tagged. */ + for (const int poly : polys_to_update) { + for (const int vert : corner_verts.slice(polys[poly])) { + update_tags[vert] = true; + } + } + + MutableSpan vert_normals(reinterpret_cast(pbvh->vert_normals), pbvh->totvert); + MutableSpan poly_normals = pbvh->poly_normals; + + threading::parallel_for(polys_to_update.index_range(), 1024, [&](const IndexRange range) { + for (const int i : polys_to_update.as_span().slice(range)) { + pbvh->poly_normals[i] = mesh::poly_normal_calc(positions, corner_verts.slice(polys[i])); + } + }); + + threading::parallel_for(positions.index_range(), 1024, [&](const IndexRange range) { + for (const int vert : range) { + if (!update_tags[vert]) { + continue; + } + + float3 vert_normal(0.0f); + for (const int poly : Span(pbvh->pmap[vert].indices, pbvh->pmap[vert].count)) { + vert_normal += poly_normals[poly]; + } + vert_normals[vert] = math::normalize(vert_normal); + update_tags[vert] = false; + } + }); + + for (PBVHNode *node : nodes) { + node->flag &= ~PBVH_UpdateNormals; + } } static void pbvh_update_mask_redraw_task_cb(void *__restrict userdata, diff --git a/source/blender/blenkernel/intern/pbvh_intern.hh b/source/blender/blenkernel/intern/pbvh_intern.hh index f714f03a580..c6ea1efb19a 100644 --- a/source/blender/blenkernel/intern/pbvh_intern.hh +++ b/source/blender/blenkernel/intern/pbvh_intern.hh @@ -3,6 +3,8 @@ #pragma once #include "BLI_vector.hh" +#include "BLI_math_vector_types.hh" +#include "BLI_span.hh" /** \file * \ingroup bke @@ -155,6 +157,7 @@ struct PBVH { /* NOTE: Normals are not `const` because they can be updated for drawing by sculpt code. */ float (*vert_normals)[3]; + blender::MutableSpan poly_normals; bool *hide_vert; float (*vert_positions)[3]; blender::OffsetIndices polys; -- 2.30.2 From dfe91ba5fb2cc8590a444336aea36e494a7fdc44 Mon Sep 17 00:00:00 2001 From: Hans Goudey Date: Sun, 30 Apr 2023 16:00:28 -0400 Subject: [PATCH 2/3] Cleanups/fixes --- source/blender/blenkernel/intern/paint.cc | 16 ++---- source/blender/blenkernel/intern/pbvh.cc | 68 ++++++++++++----------- 2 files changed, 41 insertions(+), 43 deletions(-) diff --git a/source/blender/blenkernel/intern/paint.cc b/source/blender/blenkernel/intern/paint.cc index 75bf23ccda2..ccf14e79330 100644 --- a/source/blender/blenkernel/intern/paint.cc +++ b/source/blender/blenkernel/intern/paint.cc @@ -1659,10 +1659,8 @@ static void sculpt_update_persistent_base(Object *ob) ob, ATTR_DOMAIN_POINT, CD_PROP_FLOAT, SCULPT_ATTRIBUTE_NAME(persistent_disp)); } -static void sculpt_update_object(Depsgraph *depsgraph, - Object *ob, - Object *ob_eval, - bool is_paint_tool) +static void sculpt_update_object( + Depsgraph *depsgraph, Object *ob, Object *ob_eval, bool /*need_pmap*/, bool is_paint_tool) { Scene *scene = DEG_get_input_scene(depsgraph); Sculpt *sd = scene->toolsettings->sculpt; @@ -1915,7 +1913,7 @@ void BKE_sculpt_update_object_after_eval(Depsgraph *depsgraph, Object *ob_eval) * other data when modifiers change the mesh. */ Object *ob_orig = DEG_get_original_object(ob_eval); - sculpt_update_object(depsgraph, ob_orig, ob_eval, false); + sculpt_update_object(depsgraph, ob_orig, ob_eval, false, false); } void BKE_sculpt_color_layer_create_if_needed(Object *object) @@ -1944,16 +1942,14 @@ void BKE_sculpt_color_layer_create_if_needed(Object *object) } } -void BKE_sculpt_update_object_for_edit(Depsgraph *depsgraph, - Object *ob_orig, - bool /*need_mask*/, - bool is_paint_tool) +void BKE_sculpt_update_object_for_edit( + Depsgraph *depsgraph, Object *ob_orig, bool need_pmap, bool /*need_mask*/, bool is_paint_tool) { BLI_assert(ob_orig == DEG_get_original_object(ob_orig)); Object *ob_eval = DEG_get_evaluated_object(depsgraph, ob_orig); - sculpt_update_object(depsgraph, ob_orig, ob_eval, is_paint_tool); + sculpt_update_object(depsgraph, ob_orig, ob_eval, need_pmap, is_paint_tool); } int *BKE_sculpt_face_sets_ensure(Mesh *mesh) diff --git a/source/blender/blenkernel/intern/pbvh.cc b/source/blender/blenkernel/intern/pbvh.cc index afeb9715feb..add51b585f1 100644 --- a/source/blender/blenkernel/intern/pbvh.cc +++ b/source/blender/blenkernel/intern/pbvh.cc @@ -17,6 +17,7 @@ #include "BLI_task.hh" #include "BLI_utildefines.h" #include "BLI_vector.hh" +#include "BLI_vector_set.hh" #include "DNA_mesh_types.h" #include "DNA_meshdata_types.h" @@ -40,7 +41,6 @@ #include "pbvh_intern.hh" using blender::float3; -using blender::IndexRange; using blender::MutableSpan; using blender::Span; using blender::Vector; @@ -1370,7 +1370,6 @@ struct PBVHUpdateData { PBVH *pbvh; Span nodes; - float (*vert_normals)[3] = nullptr; int flag = 0; bool show_sculpt_face_sets = false; PBVHAttrReq *attrs = nullptr; @@ -1383,17 +1382,19 @@ static void pbvh_faces_update_normals(PBVH *pbvh, Span nodes) { using namespace blender; using namespace blender::bke; - const Span positions(reinterpret_cast(pbvh->vert_positions), pbvh->totvert); const OffsetIndices polys = pbvh->polys; const Span corner_verts(pbvh->corner_verts, pbvh->mesh->totloop); MutableSpan update_tags(pbvh->vert_bitmap, pbvh->totvert); - Vector polys_to_update; - for (const int vert : IndexRange(pbvh->totvert)) { - if (update_tags[vert]) { - polys_to_update.extend({pbvh->pmap[vert].indices, pbvh->pmap[vert].count}); + + VectorSet polys_to_update; + for (const PBVHNode *node : nodes) { + for (const int vert : Span(node->vert_indices, node->uniq_verts)) { + if (update_tags[vert]) { + polys_to_update.add_multiple({pbvh->pmap[vert].indices, pbvh->pmap[vert].count}); + } } } @@ -1401,40 +1402,41 @@ static void pbvh_faces_update_normals(PBVH *pbvh, Span nodes) return; } - /* Also update vertices connected to updated faces, even if they weren't explicitly tagged. */ - for (const int poly : polys_to_update) { - for (const int vert : corner_verts.slice(polys[poly])) { - update_tags[vert] = true; - } - } - MutableSpan vert_normals(reinterpret_cast(pbvh->vert_normals), pbvh->totvert); MutableSpan poly_normals = pbvh->poly_normals; - threading::parallel_for(polys_to_update.index_range(), 1024, [&](const IndexRange range) { - for (const int i : polys_to_update.as_span().slice(range)) { - pbvh->poly_normals[i] = mesh::poly_normal_calc(positions, corner_verts.slice(polys[i])); - } - }); + VectorSet verts_to_update; + threading::parallel_invoke( + [&]() { + threading::parallel_for(polys_to_update.index_range(), 512, [&](const IndexRange range) { + for (const int i : polys_to_update.as_span().slice(range)) { + poly_normals[i] = mesh::poly_normal_calc(positions, corner_verts.slice(polys[i])); + } + }); + }, + [&]() { + /* Update all normals connected to affected faces faces, even if not explicitly tagged. */ + verts_to_update.reserve(polys_to_update.size()); + for (const int poly : polys_to_update) { + verts_to_update.add_multiple(corner_verts.slice(polys[poly])); + } + }, + [&]() { + update_tags.fill(false); + for (PBVHNode *node : nodes) { + node->flag &= ~PBVH_UpdateNormals; + } + }); - threading::parallel_for(positions.index_range(), 1024, [&](const IndexRange range) { - for (const int vert : range) { - if (!update_tags[vert]) { - continue; - } - - float3 vert_normal(0.0f); + 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); for (const int poly : Span(pbvh->pmap[vert].indices, pbvh->pmap[vert].count)) { - vert_normal += poly_normals[poly]; + normal += poly_normals[poly]; } - vert_normals[vert] = math::normalize(vert_normal); - update_tags[vert] = false; + vert_normals[vert] = math::normalize(normal); } }); - - for (PBVHNode *node : nodes) { - node->flag &= ~PBVH_UpdateNormals; - } } static void pbvh_update_mask_redraw_task_cb(void *__restrict userdata, -- 2.30.2 From 08acc45f93e58147c5fbde6e2df4136a43f5e62f Mon Sep 17 00:00:00 2001 From: Hans Goudey Date: Mon, 1 May 2023 16:31:14 -0400 Subject: [PATCH 3/3] Only reset tagged booleans --- source/blender/blenkernel/intern/pbvh.cc | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/source/blender/blenkernel/intern/pbvh.cc b/source/blender/blenkernel/intern/pbvh.cc index add51b585f1..cdd1be3fea5 100644 --- a/source/blender/blenkernel/intern/pbvh.cc +++ b/source/blender/blenkernel/intern/pbvh.cc @@ -1420,9 +1420,10 @@ static void pbvh_faces_update_normals(PBVH *pbvh, Span nodes) for (const int poly : polys_to_update) { verts_to_update.add_multiple(corner_verts.slice(polys[poly])); } - }, - [&]() { - update_tags.fill(false); + + for (const int vert : verts_to_update) { + update_tags[vert] = false; + } for (PBVHNode *node : nodes) { node->flag &= ~PBVH_UpdateNormals; } -- 2.30.2