From 8a11f0f3a23ef9b3c11caa80f1f8646a1c19f479 Mon Sep 17 00:00:00 2001 From: Hans Goudey Date: Tue, 13 Jun 2023 14:10:13 +0200 Subject: [PATCH] Fix #108517: Mesh split edges can give invalid indices The split edges code had a complex method of merging duplicate edges, going backwards to avoid shifting elements in a vector. Sometimes it could result in incorrect corner edge indices though, if it moved an index that matched one of the local variables (I think! I've bee trying to understand this all day and still struggling). Instead, replace it with a `VectorSet` that handles the deduplication by itself, and avoid creating the new edges until the end. I think this code could still be simpler if we tried to reduce the amount of things happening at the same time, making more code deal with the input or final state rather than an in-between one. But to avoid making the change too complicated I stopped here. Pull Request: https://projects.blender.org/blender/blender/pulls/108826 --- .../blenkernel/intern/mesh_calc_edges.cc | 52 ++--- source/blender/blenlib/BLI_ordered_edge.hh | 47 +++++ source/blender/blenlib/CMakeLists.txt | 1 + .../geometry/intern/mesh_split_edges.cc | 177 ++++++------------ 4 files changed, 120 insertions(+), 157 deletions(-) create mode 100644 source/blender/blenlib/BLI_ordered_edge.hh diff --git a/source/blender/blenkernel/intern/mesh_calc_edges.cc b/source/blender/blenkernel/intern/mesh_calc_edges.cc index 203f41863bd..6b01926da06 100644 --- a/source/blender/blenkernel/intern/mesh_calc_edges.cc +++ b/source/blender/blenkernel/intern/mesh_calc_edges.cc @@ -9,6 +9,7 @@ #include "DNA_object_types.h" #include "BLI_map.hh" +#include "BLI_ordered_edge.hh" #include "BLI_task.hh" #include "BLI_threads.h" #include "BLI_timeit.hh" @@ -19,43 +20,14 @@ namespace blender::bke::calc_edges { -/** This is used to uniquely identify edges in a hash map. */ -struct OrderedEdge { - int v_low, v_high; - - OrderedEdge(const int v1, const int v2) - { - if (v1 < v2) { - v_low = v1; - v_high = v2; - } - else { - v_low = v2; - v_high = v1; - } - } - - OrderedEdge(const uint v1, const uint v2) : OrderedEdge(int(v1), int(v2)) {} - - uint64_t hash() const - { - return (this->v_low << 8) ^ this->v_high; - } - - /** Return a hash value that is likely to be different in the low bits from the normal `hash()` - * function. This is necessary to avoid collisions in #BKE_mesh_calc_edges. */ - uint64_t hash2() const - { - return this->v_low; - } - - friend bool operator==(const OrderedEdge &e1, const OrderedEdge &e2) - { - BLI_assert(e1.v_low < e1.v_high); - BLI_assert(e2.v_low < e2.v_high); - return e1.v_low == e2.v_low && e1.v_high == e2.v_high; - } -}; +/** + * Return a hash value that is likely to be different in the low bits from the normal `hash()` + * function. This is necessary to avoid collisions in #BKE_mesh_calc_edges. + */ +static uint64_t edge_hash_2(const OrderedEdge &edge) +{ + return edge.v_low; +} /* The map first contains an edge pointer and later an index. */ union OrigEdgeOrIndex { @@ -84,7 +56,7 @@ static void add_existing_edges_to_hash_maps(Mesh *mesh, for (const int2 &edge : edges) { OrderedEdge ordered_edge{edge[0], edge[1]}; /* Only add the edge when it belongs into this map. */ - if (task_index == (parallel_mask & ordered_edge.hash2())) { + if (task_index == (parallel_mask & edge_hash_2(ordered_edge))) { edge_map.add_new(ordered_edge, {&edge}); } } @@ -107,7 +79,7 @@ static void add_polygon_edges_to_hash_maps(Mesh *mesh, if (vert_prev != vert) { OrderedEdge ordered_edge{vert_prev, vert}; /* Only add the edge when it belongs into this map. */ - if (task_index == (parallel_mask & ordered_edge.hash2())) { + if (task_index == (parallel_mask & edge_hash_2(ordered_edge))) { edge_map.lookup_or_add(ordered_edge, {nullptr}); } } @@ -169,7 +141,7 @@ static void update_edge_indices_in_poly_loops(const OffsetIndices polys, if (vert_prev != vert) { OrderedEdge ordered_edge{vert_prev, vert}; /* Double lookup: First find the map that contains the edge, then lookup the edge. */ - const EdgeMap &edge_map = edge_maps[parallel_mask & ordered_edge.hash2()]; + const EdgeMap &edge_map = edge_maps[parallel_mask & edge_hash_2(ordered_edge)]; edge_index = edge_map.lookup(ordered_edge).index; } else { diff --git a/source/blender/blenlib/BLI_ordered_edge.hh b/source/blender/blenlib/BLI_ordered_edge.hh new file mode 100644 index 00000000000..d07a6f6fc68 --- /dev/null +++ b/source/blender/blenlib/BLI_ordered_edge.hh @@ -0,0 +1,47 @@ +/* SPDX-FileCopyrightText: 2023 Blender Foundation + * + * SPDX-License-Identifier: GPL-2.0-or-later */ + +#pragma once + +#include "BLI_assert.h" +#include "BLI_math_vector_types.hh" + +namespace blender { + +/** + * A version of `int2` used as a key for hash-maps, agnostic of the arbitrary order of the two + * vertices in a mesh edge. + */ +struct OrderedEdge { + int v_low; + int v_high; + + OrderedEdge(const int v1, const int v2) + { + if (v1 < v2) { + v_low = v1; + v_high = v2; + } + else { + v_low = v2; + v_high = v1; + } + } + OrderedEdge(const int2 edge) : OrderedEdge(edge[0], edge[1]) {} + OrderedEdge(const uint v1, const uint v2) : OrderedEdge(int(v1), int(v2)) {} + + uint64_t hash() const + { + return (this->v_low << 8) ^ this->v_high; + } + + friend bool operator==(const OrderedEdge &e1, const OrderedEdge &e2) + { + BLI_assert(e1.v_low < e1.v_high); + BLI_assert(e2.v_low < e2.v_high); + return e1.v_low == e2.v_low && e1.v_high == e2.v_high; + } +}; + +} // namespace blender diff --git a/source/blender/blenlib/CMakeLists.txt b/source/blender/blenlib/CMakeLists.txt index 1ddadeec864..6455e43f72b 100644 --- a/source/blender/blenlib/CMakeLists.txt +++ b/source/blender/blenlib/CMakeLists.txt @@ -318,6 +318,7 @@ set(SRC BLI_noise.h BLI_noise.hh BLI_offset_indices.hh + BLI_ordered_edge.hh BLI_parameter_pack_utils.hh BLI_path_util.h BLI_polyfill_2d.h diff --git a/source/blender/geometry/intern/mesh_split_edges.cc b/source/blender/geometry/intern/mesh_split_edges.cc index 9594ce346f2..48ff8d828ca 100644 --- a/source/blender/geometry/intern/mesh_split_edges.cc +++ b/source/blender/geometry/intern/mesh_split_edges.cc @@ -2,6 +2,9 @@ #include "BLI_array_utils.hh" #include "BLI_index_mask.hh" +#include "BLI_index_mask_ops.hh" +#include "BLI_ordered_edge.hh" +#include "BLI_vector_set.hh" #include "BKE_attribute.hh" #include "BKE_attribute_math.hh" @@ -12,12 +15,6 @@ namespace blender::geometry { -/* Naively checks if the first vertices and the second vertices are the same. */ -static inline bool naive_edges_equal(const int2 &edge1, const int2 &edge2) -{ - return edge1 == edge2; -} - static void add_new_vertices(Mesh &mesh, const Span new_to_old_verts_map) { /* These types aren't supported for interpolation below. */ @@ -153,70 +150,6 @@ static void add_new_edges(Mesh &mesh, } } -/** - * Merge the new_edge into the original edge. - * - * NOTE: This function is very specific to the situation and makes a lot of assumptions. - */ -static void merge_edges(const int orig_edge_i, - const int new_edge_i, - MutableSpan new_corner_edges, - Vector> &edge_to_loop_map, - Vector &new_edges, - Vector &new_to_old_edges_map) -{ - /* Merge back into the original edge by undoing the topology changes. */ - BLI_assert(edge_to_loop_map[new_edge_i].size() == 1); - const int loop_i = edge_to_loop_map[new_edge_i][0]; - new_corner_edges[loop_i] = orig_edge_i; - - /* We are putting the last edge in the location of new_edge in all the maps, to remove - * new_edge efficiently. We have to update the topology information for this last edge - * though. Essentially we are replacing every instance of last_edge_i with new_edge_i. */ - const int last_edge_i = new_edges.size() - 1; - if (last_edge_i != new_edge_i) { - BLI_assert(edge_to_loop_map[last_edge_i].size() == 1); - const int last_edge_loop_i = edge_to_loop_map[last_edge_i][0]; - new_corner_edges[last_edge_loop_i] = new_edge_i; - } - - /* We can now safely swap-remove. */ - new_edges.remove_and_reorder(new_edge_i); - edge_to_loop_map.remove_and_reorder(new_edge_i); - new_to_old_edges_map.remove_and_reorder(new_edge_i); -} - -/** - * Replace the vertex of an edge with a new one, and update the connected loops. - * - * NOTE: This only updates the loops containing the edge and the old vertex. It should therefore - * also be called on the adjacent edge. - */ -static void swap_vertex_of_edge(int2 &edge, - const int old_vert, - const int new_vert, - MutableSpan corner_verts, - const Span connected_loops) -{ - if (edge[0] == old_vert) { - edge[0] = new_vert; - } - else if (edge[1] == old_vert) { - edge[1] = new_vert; - } - else { - BLI_assert_unreachable(); - } - - for (const int loop_i : connected_loops) { - if (corner_verts[loop_i] == old_vert) { - corner_verts[loop_i] = new_vert; - } - /* The old vertex is on the loop containing the adjacent edge. Since this function is also - * called on the adjacent edge, we don't replace it here. */ - } -} - /** Split the vertex into duplicates so that each fan has a different vertex. */ static void split_vertex_per_fan(const int vertex, const int start_offset, @@ -224,7 +157,6 @@ static void split_vertex_per_fan(const int vertex, const Span fans, const Span fan_sizes, const Span> edge_to_loop_map, - MutableSpan new_edges, MutableSpan corner_verts, MutableSpan new_to_old_verts_map) { @@ -236,8 +168,13 @@ static void split_vertex_per_fan(const int vertex, new_to_old_verts_map[new_vert_i - orig_verts_num] = vertex; for (const int edge_i : fans.slice(fan_start, fan_sizes[i])) { - swap_vertex_of_edge( - new_edges[edge_i], vertex, new_vert_i, corner_verts, edge_to_loop_map[edge_i]); + for (const int loop_i : edge_to_loop_map[edge_i]) { + if (corner_verts[loop_i] == vertex) { + corner_verts[loop_i] = new_vert_i; + } + /* The old vertex is on the loop containing the adjacent edge. Since this function is also + * called on the adjacent edge, we don't replace it here. */ + } } fan_start += fan_sizes[i]; } @@ -266,7 +203,7 @@ static int adjacent_edge(const Span corner_verts, * be used to retrieve the fans from connected_edges. */ static void calc_vertex_fans(const int vertex, - const Span new_corner_verts, + const Span corner_verts, const Span new_corner_edges, const OffsetIndices polys, const Span> edge_to_loop_map, @@ -297,7 +234,7 @@ static void calc_vertex_fans(const int vertex, /* Add adjacent edges to search stack. */ for (const int loop_i : edge_to_loop_map[curr_edge_i]) { const int adjacent_edge_i = adjacent_edge( - new_corner_verts, new_corner_edges, loop_i, polys[loop_to_poly_map[loop_i]], vertex); + corner_verts, new_corner_edges, loop_i, polys[loop_to_poly_map[loop_i]], vertex); /* Find out if this edge was visited already. */ int i = curr_i + 1; @@ -339,18 +276,13 @@ static void calc_vertex_fans(const int vertex, static void split_edge_per_poly(const int edge_i, const int new_edge_start, MutableSpan> edge_to_loop_map, - MutableSpan corner_edges, - MutableSpan new_edges, - MutableSpan new_to_old_edges_map) + MutableSpan corner_edges) { if (edge_to_loop_map[edge_i].size() <= 1) { return; } int new_edge_index = new_edge_start; for (const int loop_i : edge_to_loop_map[edge_i].as_span().drop_front(1)) { - const int2 &new_edge(new_edges[edge_i]); - new_edges[new_edge_index] = new_edge; - new_to_old_edges_map[new_edge_index] = edge_i; edge_to_loop_map[new_edge_index].append({loop_i}); corner_edges[loop_i] = new_edge_index; new_edge_index++; @@ -393,30 +325,35 @@ void split_edges(Mesh &mesh, } const OffsetIndices polys = mesh.polys(); - - MutableSpan corner_verts = mesh.corner_verts_for_write(); - MutableSpan corner_edges = mesh.corner_edges_for_write(); - Vector new_edges(new_edges_size); - new_edges.as_mutable_span().take_front(edges.size()).copy_from(edges); + const Array orig_corner_edges = mesh.corner_edges(); + Vector memory; + const bke::LooseEdgeCache loose_edges_cache = mesh.loose_edges(); + IndexMask loose_edges; + if (loose_edges_cache.count > 0) { + loose_edges = index_mask_ops::find_indices_based_on_predicate( + edges.index_range(), 4096, memory, [&](const int64_t i) { + return loose_edges_cache.is_loose_bits[i]; + }); + } edge_to_loop_map.resize(new_edges_size); - /* Used for transferring attributes. */ - Vector new_to_old_edges_map(IndexRange(new_edges.size()).as_span()); + MutableSpan corner_edges = mesh.corner_edges_for_write(); /* Step 1: Split the edges. */ threading::parallel_for(mask.index_range(), 512, [&](IndexRange range) { for (const int mask_i : range) { const int edge_i = mask[mask_i]; - split_edge_per_poly(edge_i, - edge_offsets[edge_i], - edge_to_loop_map, - corner_edges, - new_edges, - new_to_old_edges_map); + split_edge_per_poly(edge_i, edge_offsets[edge_i], edge_to_loop_map, corner_edges); } }); + /* Step 1: Split the edges. */ + + mask.foreach_index([&](const int edge_i) { + split_edge_per_poly(edge_i, edge_offsets[edge_i], edge_to_loop_map, corner_edges); + }); + /* Step 1.5: Update topology information (can't parallelize). */ for (const int edge_i : mask) { const int2 &edge = edges[edge_i]; @@ -426,6 +363,8 @@ void split_edges(Mesh &mesh, } } + MutableSpan corner_verts = mesh.corner_verts_for_write(); + /* Step 2: Calculate vertex fans. */ Array> vertex_fan_sizes(mesh.totvert); threading::parallel_for(IndexRange(mesh.totvert), 512, [&](IndexRange range) { @@ -471,39 +410,43 @@ void split_edges(Mesh &mesh, vert_to_edge_map[vert], vertex_fan_sizes[vert], edge_to_loop_map, - new_edges, corner_verts, new_to_old_verts_map); } }); - /* Step 4: Deduplicate edges. We loop backwards so we can use remove_and_reorder. Although this - * does look bad (3 nested loops), in practice the inner loops are very small. For most meshes, - * there are at most 2 polygons connected to each edge, and hence you'll only get at most 1 - * duplicate per edge. */ - for (int mask_i = mask.size() - 1; mask_i >= 0; mask_i--) { - const int edge = mask[mask_i]; - int start_of_duplicates = edge_offsets[edge]; - int end_of_duplicates = start_of_duplicates + num_edge_duplicates[edge] - 1; - for (int duplicate = end_of_duplicates; duplicate >= start_of_duplicates; duplicate--) { - if (naive_edges_equal(new_edges[edge], new_edges[duplicate])) { - merge_edges( - edge, duplicate, corner_edges, edge_to_loop_map, new_edges, new_to_old_edges_map); - break; - } - for (int other = start_of_duplicates; other < duplicate; other++) { - if (naive_edges_equal(new_edges[other], new_edges[duplicate])) { - merge_edges( - other, duplicate, corner_edges, edge_to_loop_map, new_edges, new_to_old_edges_map); - break; - } - } + VectorSet new_edges; + new_edges.reserve(new_edges_size + loose_edges.size()); + for (const int i : polys.index_range()) { + const IndexRange poly = polys[i]; + for (const int corner : poly) { + const int vert_1 = corner_verts[corner]; + const int vert_2 = corner_verts[bke::mesh::poly_corner_next(poly, corner)]; + corner_edges[corner] = new_edges.index_of_or_add_as(OrderedEdge(vert_1, vert_2)); + } + } + loose_edges.foreach_index([&](const int64_t i) { new_edges.add(OrderedEdge(edges[i])); }); + + Array new_to_old_edges_map(new_edges.size()); + auto index_mask_to_indices = [&](const IndexMask &mask, MutableSpan indices) { + for (const int i : mask.index_range()) { + indices[i] = mask[i]; + } + }; + index_mask_to_indices(loose_edges, + new_to_old_edges_map.as_mutable_span().take_back(loose_edges.size())); + for (const int i : polys.index_range()) { + const IndexRange poly = polys[i]; + for (const int corner : poly) { + const int new_edge_i = corner_edges[corner]; + const int old_edge_i = orig_corner_edges[corner]; + new_to_old_edges_map[new_edge_i] = old_edge_i; } } /* Step 5: Resize the mesh to add the new vertices and rebuild the edges. */ add_new_vertices(mesh, new_to_old_verts_map); - add_new_edges(mesh, new_edges, new_to_old_edges_map, propagation_info); + add_new_edges(mesh, new_edges.as_span().cast(), new_to_old_edges_map, propagation_info); BKE_mesh_tag_edges_split(&mesh); }