From 6b0fa709fa87366ba4bc22eb1265d19200bbcb82 Mon Sep 17 00:00:00 2001 From: Sean Kim Date: Tue, 21 Jan 2025 23:19:51 +0100 Subject: [PATCH] Fix #133128: Multires cavity automasking creates artifacts Prior to the brush refactor in 4.3, the cavity automasking feature had an implicit dependency on stable position values. In many cases, such as during brush evaulation, this was upheld by the usage of the proxy system, which meant that even if a vertex's neighbor was changed during a stroke in paralell, these changes would not be accessed until after a stroke finished. With the new method of brush evaulation removing the proxy system, this introduced subtle artifacts on BVH node boundaries, as vertices were guaranteed to change and cause differences as there was no temporary storage to use the "old" position data. This effect was most noticable in 4.3 and beyond in multires sculpting, though similar artifacts can be seen when using the mesh filter in all prior versions. To fix these issues, instead of calculating the internal cavity factor during a stroke or during deformations, we calculate it ahead of time based on the affected nodes of a stroke. This has the benefit of now behaving consistently for a given mesh and given brush or filter application. From a performance perspective, this change should have no noticeable impact, as each BVH node, and by extension each vertex, only has its corresponding cavity factor calculated once, when the stroke would affect the node. Related: #102745 Pull Request: https://projects.blender.org/blender/blender/pulls/133224 --- source/blender/editors/sculpt_paint/sculpt.cc | 6 ++ .../editors/sculpt_paint/sculpt_automask.hh | 15 +++ .../sculpt_paint/sculpt_automasking.cc | 94 +++++++++++++++---- .../editors/sculpt_paint/sculpt_cloth.cc | 6 ++ .../sculpt_paint/sculpt_filter_color.cc | 6 ++ .../sculpt_paint/sculpt_filter_mesh.cc | 5 + 6 files changed, 112 insertions(+), 20 deletions(-) diff --git a/source/blender/editors/sculpt_paint/sculpt.cc b/source/blender/editors/sculpt_paint/sculpt.cc index 118128907c9..e759e4da1d4 100644 --- a/source/blender/editors/sculpt_paint/sculpt.cc +++ b/source/blender/editors/sculpt_paint/sculpt.cc @@ -3151,6 +3151,12 @@ static void do_brush_action(const Depsgraph &depsgraph, return; } + if (auto_mask::is_enabled(sd, ob, &brush) && ss.cache->automasking && + ss.cache->automasking->settings.flags & BRUSH_AUTOMASKING_CAVITY_ALL) + { + ss.cache->automasking->calc_cavity_factor(depsgraph, ob, node_mask); + } + if (!use_pixels) { push_undo_nodes(depsgraph, ob, brush, node_mask); } diff --git a/source/blender/editors/sculpt_paint/sculpt_automask.hh b/source/blender/editors/sculpt_paint/sculpt_automask.hh index cefe25f305d..c20d9952e1b 100644 --- a/source/blender/editors/sculpt_paint/sculpt_automask.hh +++ b/source/blender/editors/sculpt_paint/sculpt_automask.hh @@ -76,6 +76,21 @@ struct Cache { * \note -1 means the vertex value still needs to be calculated. */ Array cavity_factor; + + /** + * Calculates the cavity factor for a set of nodes. + * + * Has no effect on an individual node level if the factor for a vertex has already been + * calculated. This data is best calculated outside of any loops that affect the stroke's + * position, as the curvature calculation is sensitive to small changes, meaning processing + * inside the normal brush update step may result in odd artifacts from ordering of position + * updates. + * + * \note Should be called prior to any call that may use the cavity mode. + */ + void calc_cavity_factor(const Depsgraph &depsgraph, + const Object &object, + const IndexMask &node_mask); }; /** diff --git a/source/blender/editors/sculpt_paint/sculpt_automasking.cc b/source/blender/editors/sculpt_paint/sculpt_automasking.cc index 4a5e809dd47..cc793561e6c 100644 --- a/source/blender/editors/sculpt_paint/sculpt_automasking.cc +++ b/source/blender/editors/sculpt_paint/sculpt_automasking.cc @@ -590,10 +590,10 @@ static float process_cavity_factor(const Cache &automasking, float factor) return factor; } -static float calc_cavity_factor_mesh(const Depsgraph &depsgraph, - const Cache &automasking, - const Object &object, - const int vert) +static void calc_cavity_factor_mesh(const Depsgraph &depsgraph, + const Cache &automasking, + const Object &object, + const int vert) { if (automasking.cavity_factor[vert] == -1.0f) { calc_blurred_cavity_mesh(depsgraph, @@ -603,13 +603,12 @@ static float calc_cavity_factor_mesh(const Depsgraph &depsgraph, vert, const_cast(automasking).cavity_factor); } - return process_cavity_factor(automasking, automasking.cavity_factor[vert]); } -static float calc_cavity_factor_grids(const CCGKey &key, - const Cache &automasking, - const Object &object, - const int vert) +static void calc_cavity_factor_grids(const CCGKey &key, + const Cache &automasking, + const Object &object, + const int vert) { if (automasking.cavity_factor[vert] == -1.0f) { calc_blurred_cavity_grids(object, @@ -618,10 +617,9 @@ static float calc_cavity_factor_grids(const CCGKey &key, SubdivCCGCoord::from_index(key, vert), const_cast(automasking).cavity_factor); } - return process_cavity_factor(automasking, automasking.cavity_factor[vert]); } -static float calc_cavity_factor_bmesh(const Cache &automasking, BMVert *vert, const int vert_i) +static void calc_cavity_factor_bmesh(const Cache &automasking, BMVert *vert, const int vert_i) { if (automasking.cavity_factor[vert_i] == -1.0f) { calc_blurred_cavity_bmesh(automasking, @@ -629,7 +627,6 @@ static float calc_cavity_factor_bmesh(const Cache &automasking, BMVert *vert, co vert, const_cast(automasking).cavity_factor); } - return process_cavity_factor(automasking, automasking.cavity_factor[vert_i]); } void calc_vert_factors(const Depsgraph &depsgraph, @@ -676,7 +673,8 @@ void calc_vert_factors(const Depsgraph &depsgraph, float cached_factor = automasking.factor[vert]; if (automasking.settings.flags & BRUSH_AUTOMASKING_CAVITY_ALL) { - cached_factor *= calc_cavity_factor_mesh(depsgraph, automasking, object, vert); + BLI_assert(automasking.cavity_factor[vert] != -1.0f); + cached_factor *= process_cavity_factor(automasking, automasking.cavity_factor[vert]); } factors[i] *= cached_factor; @@ -738,7 +736,8 @@ void calc_vert_factors(const Depsgraph &depsgraph, } if (automasking.settings.flags & BRUSH_AUTOMASKING_CAVITY_ALL) { - factors[i] *= calc_cavity_factor_mesh(depsgraph, automasking, object, vert); + BLI_assert(automasking.cavity_factor[vert] != -1.0f); + factors[i] *= process_cavity_factor(automasking, automasking.cavity_factor[vert]); } } } @@ -783,7 +782,8 @@ void calc_face_factors(const Depsgraph &depsgraph, float cached_factor = automasking.factor[vert]; if (automasking.settings.flags & BRUSH_AUTOMASKING_CAVITY_ALL) { - cached_factor *= calc_cavity_factor_mesh(depsgraph, automasking, object, vert); + BLI_assert(automasking.cavity_factor[vert] != -1.0f); + cached_factor *= process_cavity_factor(automasking, automasking.cavity_factor[vert]); } factor *= cached_factor; @@ -845,7 +845,8 @@ void calc_face_factors(const Depsgraph &depsgraph, } if (automasking.settings.flags & BRUSH_AUTOMASKING_CAVITY_ALL) { - factor *= calc_cavity_factor_mesh(depsgraph, automasking, object, vert); + BLI_assert(automasking.cavity_factor[vert] != -1.0f); + factor *= process_cavity_factor(automasking, automasking.cavity_factor[vert]); } } factors[i] *= sum * math::rcp(float(face_verts.size())); @@ -907,7 +908,8 @@ void calc_grids_factors(const Depsgraph &depsgraph, float cached_factor = automasking.factor[vert]; if (automasking.settings.flags & BRUSH_AUTOMASKING_CAVITY_ALL) { - cached_factor *= calc_cavity_factor_grids(key, automasking, object, vert); + BLI_assert(automasking.cavity_factor[vert] != -1.0f); + cached_factor *= process_cavity_factor(automasking, automasking.cavity_factor[vert]); } factors[node_vert] *= cached_factor; @@ -974,7 +976,8 @@ void calc_grids_factors(const Depsgraph &depsgraph, } if (automasking.settings.flags & BRUSH_AUTOMASKING_CAVITY_ALL) { - factors[node_vert] *= calc_cavity_factor_grids(key, automasking, object, vert); + BLI_assert(automasking.cavity_factor[vert] != -1.0f); + factors[node_vert] *= process_cavity_factor(automasking, automasking.cavity_factor[vert]); } } } @@ -1020,7 +1023,8 @@ void calc_vert_factors(const Depsgraph &depsgraph, float cached_factor = automasking.factor[vert_i]; if (automasking.settings.flags & BRUSH_AUTOMASKING_CAVITY_ALL) { - cached_factor *= calc_cavity_factor_bmesh(automasking, vert, vert_i); + BLI_assert(automasking.cavity_factor[vert_i] != -1.0f); + cached_factor *= process_cavity_factor(automasking, automasking.cavity_factor[vert_i]); } factors[i] *= cached_factor; @@ -1082,7 +1086,8 @@ void calc_vert_factors(const Depsgraph &depsgraph, } if (automasking.settings.flags & BRUSH_AUTOMASKING_CAVITY_ALL) { - factors[i] *= calc_cavity_factor_bmesh(automasking, vert, vert_i); + BLI_assert(automasking.cavity_factor[vert_i] != -1.0f); + factors[i] *= process_cavity_factor(automasking, automasking.cavity_factor[vert_i]); } } } @@ -1723,4 +1728,53 @@ std::unique_ptr cache_init(const Depsgraph &depsgraph, return automasking; } +void Cache::calc_cavity_factor(const Depsgraph &depsgraph, + const Object &object, + const IndexMask &node_mask) +{ + if ((this->settings.flags & BRUSH_AUTOMASKING_CAVITY_ALL) == 0) { + return; + } + + BLI_assert(!this->cavity_factor.is_empty()); + + const SculptSession &ss = *object.sculpt; + const bke::pbvh::Tree &pbvh = *bke::object::pbvh_get(object); + switch (pbvh.type()) { + case bke::pbvh::Type::Mesh: { + const Span nodes = pbvh.nodes(); + node_mask.foreach_index(GrainSize(1), [&](const int i) { + const Span verts = nodes[i].verts(); + for (const int vert : verts) { + calc_cavity_factor_mesh(depsgraph, *this, object, vert); + } + }); + break; + } + case bke::pbvh::Type::Grids: { + const SubdivCCG &subdiv_ccg = *ss.subdiv_ccg; + const CCGKey key = BKE_subdiv_ccg_key_top_level(subdiv_ccg); + const Span nodes = pbvh.nodes(); + node_mask.foreach_index(GrainSize(1), [&](const int i) { + const Span grids = nodes[i].grids(); + for (const int grid : grids) { + for (const int vert : bke::ccg::grid_range(subdiv_ccg.grid_area, grid)) { + calc_cavity_factor_grids(key, *this, object, vert); + } + } + }); + break; + } + case bke::pbvh::Type::BMesh: { + const Span nodes = pbvh.nodes(); + node_mask.foreach_index(GrainSize(1), [&](const int i) { + const Set verts = nodes[i].bm_unique_verts_; + for (BMVert *vert : verts) { + calc_cavity_factor_bmesh(*this, vert, BM_elem_index_get(vert)); + } + }); + } + } +} + } // namespace blender::ed::sculpt_paint::auto_mask diff --git a/source/blender/editors/sculpt_paint/sculpt_cloth.cc b/source/blender/editors/sculpt_paint/sculpt_cloth.cc index 9769df7afc9..a0f06da95d7 100644 --- a/source/blender/editors/sculpt_paint/sculpt_cloth.cc +++ b/source/blender/editors/sculpt_paint/sculpt_cloth.cc @@ -2296,6 +2296,12 @@ static int sculpt_cloth_filter_modal(bContext *C, wmOperator *op, const wmEvent const IndexMask &node_mask = ss.filter_cache->node_mask; + if (auto_mask::is_enabled(sd, object, nullptr) && ss.filter_cache->automasking && + ss.filter_cache->automasking->settings.flags & BRUSH_AUTOMASKING_CAVITY_ALL) + { + ss.filter_cache->automasking->calc_cavity_factor(*depsgraph, object, node_mask); + } + float3 gravity(0.0f); if (sd.gravity_object) { gravity = sd.gravity_object->object_to_world().ptr()[2]; diff --git a/source/blender/editors/sculpt_paint/sculpt_filter_color.cc b/source/blender/editors/sculpt_paint/sculpt_filter_color.cc index 676aa23379b..1ef35b344f5 100644 --- a/source/blender/editors/sculpt_paint/sculpt_filter_color.cc +++ b/source/blender/editors/sculpt_paint/sculpt_filter_color.cc @@ -371,6 +371,7 @@ static void sculpt_color_presmooth_init(const Mesh &mesh, Object &object) static void sculpt_color_filter_apply(bContext *C, wmOperator *op, Object &ob) { const Depsgraph &depsgraph = *CTX_data_depsgraph_pointer(C); + const Sculpt &sd = *CTX_data_tool_settings(C)->sculpt; SculptSession &ss = *ob.sculpt; bke::pbvh::Tree &pbvh = *bke::object::pbvh_get(ob); MutableSpan nodes = pbvh.nodes(); @@ -388,6 +389,11 @@ static void sculpt_color_filter_apply(bContext *C, wmOperator *op, Object &ob) } const IndexMask &node_mask = ss.filter_cache->node_mask; + if (auto_mask::is_enabled(sd, ob, nullptr) && ss.filter_cache->automasking && + ss.filter_cache->automasking->settings.flags & BRUSH_AUTOMASKING_CAVITY_ALL) + { + ss.filter_cache->automasking->calc_cavity_factor(depsgraph, ob, node_mask); + } const OffsetIndices faces = mesh.faces(); const Span corner_verts = mesh.corner_verts(); diff --git a/source/blender/editors/sculpt_paint/sculpt_filter_mesh.cc b/source/blender/editors/sculpt_paint/sculpt_filter_mesh.cc index f1f71bb374c..e863bc0645a 100644 --- a/source/blender/editors/sculpt_paint/sculpt_filter_mesh.cc +++ b/source/blender/editors/sculpt_paint/sculpt_filter_mesh.cc @@ -2132,6 +2132,11 @@ static void sculpt_mesh_filter_apply(bContext *C, wmOperator *op, bool is_replay SCULPT_vertex_random_access_ensure(ob); const IndexMask &node_mask = ss.filter_cache->node_mask; + if (auto_mask::is_enabled(sd, ob, nullptr) && ss.filter_cache->automasking && + ss.filter_cache->automasking->settings.flags & BRUSH_AUTOMASKING_CAVITY_ALL) + { + ss.filter_cache->automasking->calc_cavity_factor(depsgraph, ob, node_mask); + } switch (filter_type) { case MeshFilterType::Smooth: calc_smooth_filter(depsgraph,