griefith/test
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

Fix #108517: Mesh split edges can give invalid indices

Browse Source 
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
This commit is contained in:
Hans Goudey
2023-06-13 14:10:13 +02:00
committed by Hans Goudey
parent 8013bad084
commit 8a11f0f3a2
4 changed files with 120 additions and 157 deletions
Download Patch File Download Diff File Expand all files Collapse all files

52
source/blender/blenkernel/intern/mesh_calc_edges.cc
View File

@@ -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<int> 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 {

47
source/blender/blenlib/BLI_ordered_edge.hh Normal file
View File

@@ -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

1
source/blender/blenlib/CMakeLists.txt
View File

@@ -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

177
source/blender/geometry/intern/mesh_split_edges.cc
View File

@@ -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<int> 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<int> new_corner_edges,
Vector<Vector<int>> &edge_to_loop_map,
Vector<int2> &new_edges,
Vector<int> &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<int> corner_verts,
const Span<int> 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<int> fans,
const Span<int> fan_sizes,
const Span<Vector<int>> edge_to_loop_map,
MutableSpan<int2> new_edges,
MutableSpan<int> corner_verts,
MutableSpan<int> 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<int> corner_verts,
* be used to retrieve the fans from connected_edges.
*/
static void calc_vertex_fans(const int vertex,
const Span<int> new_corner_verts,
const Span<int> corner_verts,
const Span<int> new_corner_edges,
const OffsetIndices<int> polys,
const Span<Vector<int>> 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<Vector<int>> edge_to_loop_map,
MutableSpan<int> corner_edges,
MutableSpan<int2> new_edges,
MutableSpan<int> new_to_old_edges_map)
MutableSpan<int> 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<int> corner_verts = mesh.corner_verts_for_write();
MutableSpan<int> corner_edges = mesh.corner_edges_for_write();
Vector<int2> new_edges(new_edges_size);
new_edges.as_mutable_span().take_front(edges.size()).copy_from(edges);
const Array<int> orig_corner_edges = mesh.corner_edges();
Vector<int64_t> 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<int> new_to_old_edges_map(IndexRange(new_edges.size()).as_span());
MutableSpan<int> 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<int> corner_verts = mesh.corner_verts_for_write();
/* Step 2: Calculate vertex fans. */
Array<Vector<int>> 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<OrderedEdge> 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<int> new_to_old_edges_map(new_edges.size());
auto index_mask_to_indices = [&](const IndexMask &mask, MutableSpan<int> 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<int2>(), new_to_old_edges_map, propagation_info);
BKE_mesh_tag_edges_split(&mesh);
}