From 3db246a3ce6fd02cae47f1e1c551dab0e65fc8b3 Mon Sep 17 00:00:00 2001 From: Hans Goudey Date: Sun, 26 Feb 2023 23:59:02 +0100 Subject: [PATCH 1/2] Fix #104915: Race condition writing subsurf optimal display edges Writing to a bitmap from multiple threads causes races when writing to bits within the same integer. Instead, write to a separate boolean array while subdividing, then move that to the final mesh bit vector. Notes: - The final copy to the bit vector could be replaced by a generic `copy_from(Span)` call in the future. - Theoretically we could entirely replace the `BitVector` with an `Array`, but 1/8 the memory use for edges is likely worth it. Pull Request #105156 --- .../blender/blenkernel/intern/subdiv_mesh.cc | 25 ++++++++++++++++--- 1 file changed, 22 insertions(+), 3 deletions(-) diff --git a/source/blender/blenkernel/intern/subdiv_mesh.cc b/source/blender/blenkernel/intern/subdiv_mesh.cc index ee9e02883c6..c68c3434418 100644 --- a/source/blender/blenkernel/intern/subdiv_mesh.cc +++ b/source/blender/blenkernel/intern/subdiv_mesh.cc @@ -17,6 +17,7 @@ #include "BLI_bitmap.h" #include "BLI_math_vector.h" #include "BLI_math_vector_types.hh" +#include "BLI_task.hh" #include "BKE_customdata.h" #include "BKE_key.h" @@ -68,6 +69,10 @@ struct SubdivMeshContext { int *accumulated_counters; bool have_displacement; + /* Write optimal display edge tags into a boolean array rather than the final bit vector + * to avoid race conditions when setting bits. */ + blender::Array subdiv_display_edges; + /* Lazily initialize a map from vertices to connected edges. */ std::mutex vert_to_edge_map_mutex; int *vert_to_edge_buffer; @@ -537,8 +542,7 @@ static bool subdiv_mesh_topology_info(const SubdivForeachContext *foreach_contex subdiv_context->subdiv_mesh->runtime->subsurf_face_dot_tags.clear(); subdiv_context->subdiv_mesh->runtime->subsurf_face_dot_tags.resize(num_vertices); if (subdiv_context->settings->use_optimal_display) { - subdiv_context->subdiv_mesh->runtime->subsurf_optimal_display_edges.clear(); - subdiv_context->subdiv_mesh->runtime->subsurf_optimal_display_edges.resize(num_edges); + subdiv_context->subdiv_display_edges = blender::Array(num_edges, false); } return true; } @@ -800,7 +804,7 @@ static void subdiv_copy_edge_data(SubdivMeshContext *ctx, CustomData_copy_data( &ctx->coarse_mesh->edata, &ctx->subdiv_mesh->edata, coarse_edge_index, subdiv_edge_index, 1); if (ctx->settings->use_optimal_display) { - ctx->subdiv_mesh->runtime->subsurf_optimal_display_edges[subdiv_edge_index].set(); + ctx->subdiv_display_edges[subdiv_edge_index] = true; } } @@ -1162,6 +1166,7 @@ Mesh *BKE_subdiv_to_mesh(Subdiv *subdiv, const SubdivToMeshSettings *settings, const Mesh *coarse_mesh) { + using namespace blender; BKE_subdiv_stats_begin(&subdiv->stats, SUBDIV_STATS_SUBDIV_TO_MESH); /* Make sure evaluator is up to date with possible new topology, and that * it is refined for the new positions of coarse vertices. */ @@ -1200,6 +1205,20 @@ Mesh *BKE_subdiv_to_mesh(Subdiv *subdiv, BKE_subdiv_foreach_subdiv_geometry(subdiv, &foreach_context, settings, coarse_mesh); BKE_subdiv_stats_end(&subdiv->stats, SUBDIV_STATS_SUBDIV_TO_MESH_GEOMETRY); Mesh *result = subdiv_context.subdiv_mesh; + + /* Move the optimal display edge array to the final bit vector. */ + if (!subdiv_context.subdiv_display_edges.is_empty()) { + const Span span = subdiv_context.subdiv_display_edges; + BitVector<> &bit_vector = result->runtime->subsurf_optimal_display_edges; + bit_vector.clear(); + bit_vector.resize(subdiv_context.subdiv_display_edges.size()); + threading::parallel_for_aligned(span.index_range(), 4096, 64, [&](const IndexRange range) { + for (const int i : range) { + bit_vector[i].set(span[i]); + } + }); + } + // BKE_mesh_validate(result, true, true); BKE_subdiv_stats_end(&subdiv->stats, SUBDIV_STATS_SUBDIV_TO_MESH); /* Using normals from the limit surface gives different results than Blender's vertex normal From 97a8bb450cdb9b6457f907cf0a128ecdd0a652f4 Mon Sep 17 00:00:00 2001 From: Hans Goudey Date: Mon, 27 Feb 2023 00:01:01 +0100 Subject: [PATCH 2/2] Fix #103387: Radius affects curves bounding box e8f4010611e76e0b88cd unified the bounds computation for the new curves object type and the rest of the curves system used by geometry nodes. In the process, it made bounds affected by the control point radius. In theory that makes sense; the bounds are supposed to be the extents of the visible geometry. But in practice the change wasn't expected, for a few reasons: - The radius has never affected the bounds for the legacy curve type - The default radius of legacy curve objects is absurdly large at 1.0m - Only the new curve object has visible radius, and only in "strip" mode or when rendering with Cycles Currently the bounds are only used for the "Bounding Box" geometry node and the panel in the 3D viewport sidebar, so there isn't any incentive to choose less intuitive behavior yet. Long term, the correct behavior is probably to include the radius in the bounds, but this commit postpones that change to when it works better with the rest of the curves system. Pull Request #105154 --- .../blender/blenkernel/intern/curves_geometry.cc | 16 ++-------------- 1 file changed, 2 insertions(+), 14 deletions(-) diff --git a/source/blender/blenkernel/intern/curves_geometry.cc b/source/blender/blenkernel/intern/curves_geometry.cc index 704bbb34f49..b62cb7cafcc 100644 --- a/source/blender/blenkernel/intern/curves_geometry.cc +++ b/source/blender/blenkernel/intern/curves_geometry.cc @@ -981,7 +981,6 @@ void CurvesGeometry::tag_normals_changed() } void CurvesGeometry::tag_radii_changed() { - this->runtime->bounds_cache.tag_dirty(); } static void translate_positions(MutableSpan positions, const float3 &translation) @@ -1064,19 +1063,8 @@ bool CurvesGeometry::bounds_min_max(float3 &min, float3 &max) const return false; } - this->runtime->bounds_cache.ensure([&](Bounds &r_bounds) { - const Span positions = this->evaluated_positions(); - if (this->attributes().contains("radius")) { - const VArraySpan radii = this->attributes().lookup("radius"); - Array evaluated_radii(this->evaluated_points_num()); - this->ensure_can_interpolate_to_evaluated(); - this->interpolate_to_evaluated(radii, evaluated_radii.as_mutable_span()); - r_bounds = *bounds::min_max_with_radii(positions, evaluated_radii.as_span()); - } - else { - r_bounds = *bounds::min_max(positions); - } - }); + this->runtime->bounds_cache.ensure( + [&](Bounds &r_bounds) { r_bounds = *bounds::min_max(this->evaluated_positions()); }); const Bounds &bounds = this->runtime->bounds_cache.data(); min = math::min(bounds.min, min);