blender
/
blender
134
421
Fork 604
Code Issues 6.4k Pull Requests 770 Projects 19 Wiki Activity

Mesh: Rewrite split edges algorithm #110661

Merged
Hans Goudey merged 78 commits from HooglyBoogly/blender:split-edges-rewrite-2 into main 2023-08-30 14:23:49 +02:00
Conversation 8 Commits 78 Files Changed 1 +122 -92
1 changed files with 122 additions and 92 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit c73a2be382 - Show all commits

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

@ -214,25 +214,26 @@ static int first_index_of_non_zero(const Span<int> span)
return -1;
}
/**
* Get the index of the adjacent edge to a loop connected to a vertex. In other words, for the
* given polygon return the unique edge connected to the given vertex and not on the given loop.
*/
static int adjacent_edge(const Span<int> corner_verts,
const Span<int> corner_edges,
const int corner,
const IndexRange poly,
const int vert)
static int adjacent_corner(const Span<int> corner_verts,
const int corner,
const IndexRange poly,
const int vert)
{
const int adjacent_corner = (corner_verts[corner] == vert) ?
bke::mesh::poly_corner_prev(poly, corner) :
bke::mesh::poly_corner_next(poly, corner);
return corner_edges[adjacent_corner];
return corner_verts[corner] == vert ? bke::mesh::poly_corner_prev(poly, corner) :
HooglyBoogly marked this conversation as resolved Outdated
Wannes Malfait commented 2023-08-02 14:31:52 +02:00
Outdated
Review
Copy Link

Took me a while to realize that split is the same as selected. Maybe just change this to selected to be more consistent with the comment above.

Took me a while to realize that `split` is the same as `selected`. Maybe just change this to `selected` to be more consistent with the comment above.
bke::mesh::poly_corner_next(poly, corner);
}
using VertCornerFans = Vector<Vector<int>>;
using CornerFan = Vector<int>;
static Vector<int> gather_edges_in_fan(const OffsetIndices<int> polys,
struct VertFans {
Vector<CornerFan> corner_fans;
/** Each split loose edge is its own "fan" made up of just that edge. */
Vector<int> split_loose_edges;
/** All non split loose edges can be attached to the same vertex in the final mesh. */
Vector<int> non_split_loose_edges;
};
static CornerFan gather_corners_in_fan(const OffsetIndices<int> polys,
const Span<int> corner_verts,
const Span<int> corner_edges,
const GroupedSpan<int> edge_to_corner_map,
@ -240,50 +241,57 @@ static Vector<int> gather_edges_in_fan(const OffsetIndices<int> polys,
const Span<int> connected_edges,
const BitSpan split_edges,
const int vert,
const int start,
const int start_corner,
MutableSpan<int> users_left)
{
// TODO: Switch to storing corner indices in the fan
Vector<int> corner_fan;
Vector<int> edge_stack({connected_edges[start]});
while (!edge_stack.is_empty()) {
const int edge = edge_stack.pop_last();
const int current = connected_edges.first_index(edge);
CornerFan fan;
HooglyBoogly marked this conversation as resolved Outdated
Wannes Malfait commented 2023-08-02 14:31:48 +02:00
Outdated
Review
Copy Link

"affected vertex index to the of output vertex" -> "affected vertex index to the index of the output vertex"

"affected vertex index to the of output vertex" -> "affected vertex index to the index of the output vertex"
corner_fan.append(edge);
users_left[current]--;
if (split_edges[edge] && current != start) {
continue;
}
Vector<int> corners_to_follow({start_corner});
auto gather_corners_connected_to_edge = [&](const int edge) { // TODO: Use disjoint set?
BLI_assert(edge_to_corner_map[edge].size() >= 1);
for (const int corner : edge_to_corner_map[edge]) {
const int poly = corner_to_poly_map[corner];
const int next_edge = adjacent_edge(corner_verts, corner_edges, corner, polys[poly], vert);
const int next_corner = adjacent_corner(corner_verts, corner, polys[poly], vert);
const int next_edge = corner_edges[next_corner];
const int next = connected_edges.first_index(next_edge);
if (users_left[next] == 0) {
continue;
}
edge_stack.append(next_edge);
corners_to_follow.append(next_corner);
}
};
gather_corners_connected_to_edge(corner_edges[start_corner]);
wannes.malfait marked this conversation as resolved Outdated
Wannes Malfait commented 2023-08-02 14:31:46 +02:00
Outdated
Review
Copy Link

Is it better to do this here, than before the loop? Also, why does it need to be filled with -1? The comment above the function mentions nothing about it possibly containing invalid indices. Looking at the code, this also seems to be impossible. Maybe this was from a previous version of the code?

Is it better to do this here, than before the loop? Also, why does it need to be filled with `-1`? The comment above the function mentions nothing about it possibly containing invalid indices. Looking at the code, this also seems to be impossible. Maybe this was from a previous version of the code?
Hans Goudey commented 2023-08-02 15:45:28 +02:00
Outdated
Review
Copy Link

I did the initialization in the loop to use the cache better, as opposed to filling the entire array on one thread first. Not sure how much of a difference that makes, but it shouldn't hurt.

The -1 is to account for the reused vert, I added a comment.

I did the initialization in the loop to use the cache better, as opposed to filling the entire array on one thread first. Not sure how much of a difference that makes, but it shouldn't hurt. The -1 is to account for the reused vert, I added a comment.
Wannes Malfait commented 2023-08-02 15:56:29 +02:00
Outdated
Review
Copy Link

Ok, I see now. That makes a lot of sense

Ok, I see now. That makes a lot of sense
Jacques Lucke commented 2023-08-02 16:08:09 +02:00
Review
Copy Link

Maybe locally add MutableSpan<int> new_verts_nums = offset_data;.

Maybe locally add `MutableSpan<int> new_verts_nums = offset_data;`.
while (!corners_to_follow.is_empty()) {
JacquesLucke marked this conversation as resolved Outdated
Jacques Lucke commented 2023-08-02 16:05:26 +02:00
Outdated
Review
Copy Link

Add assert for the second sentence.

Add assert for the second sentence.
Hans Goudey commented 2023-08-02 16:55:18 +02:00
Outdated
Review
Copy Link

accumulate_counts_to_offsets already has an assert for that

`accumulate_counts_to_offsets` already has an assert for that
const int corner = corners_to_follow.pop_last();
Jacques Lucke commented 2023-08-02 16:05:56 +02:00
Outdated
Review
Copy Link

Seems like it could be easier and faster to just subtract 1 in the loop below and change += to =?

Seems like it could be easier and faster to just subtract 1 in the loop below and change `+=` to `=`?
Hans Goudey commented 2023-08-02 16:56:52 +02:00
Outdated
Review
Copy Link

Maybe, but then the loose edges and corner fan cases become dependent on each other, since they can only subtract 1 if the other isn't empty. So this is a bit simpler.

Maybe, but then the loose edges and corner fan cases become dependent on each other, since they can only subtract 1 if the other isn't empty. So this is a bit simpler.
Jacques Lucke commented 2023-08-02 17:56:13 +02:00
Outdated
Review
Copy Link

Hm, I don't get why first setting everything to -1 and then adding something to every element afterwards is any better than just doing this:

for (const int i : range) {
  /* -1 because the first vertex is reused. */
  new_verts_nums[i] = corner_fans[i].size() - 1;
}
Hm, I don't get why first setting everything to `-1` and then adding something to every element afterwards is any better than just doing this: ``` for (const int i : range) { /* -1 because the first vertex is reused. */ new_verts_nums[i] = corner_fans[i].size() - 1; } ```
Hans Goudey commented 2023-08-02 18:06:04 +02:00
Outdated
Review
Copy Link

Because we also need need to reuse vertices for loose edges:

new_verts_nums[mask] += info.selected.size();

new_verts_nums[mask] += info.unselected.size() > 0;

Both of those can't use - 1 if any of the previous cases did. Rather than keeping track of whether we've already reused the vertex across the types, I thought it was simpler to just always add and start from -1.

Because we also need need to reuse vertices for loose edges: ``` new_verts_nums[mask] += info.selected.size(); ``` ``` new_verts_nums[mask] += info.unselected.size() > 0; ``` Both of those can't use `- 1` if any of the previous cases did. Rather than keeping track of whether we've already reused the vertex across the types, I thought it was simpler to just always add and start from -1.
const int edge = corner_edges[corner];
const int current = connected_edges.first_index(edge);
if (corner_verts[corner] == vert) {
fan.append(corner);
}
users_left[current]--;
if (!split_edges[edge]) {
gather_corners_connected_to_edge(edge);
}
}
return corner_fan;
return fan;
}
/* TODO: Use thread local allocators for #VertCornerFans memory. */
static VertCornerFans calc_fans_for_vert(const OffsetIndices<int> polys,
const Span<int> corner_verts,
const Span<int> corner_edges,
const GroupedSpan<int> vert_to_edge_map,
const GroupedSpan<int> edge_to_corner_map,
const Span<int> corner_to_poly_map,
const BitSpan split_edges,
const int vert)
static VertFans calc_fans_for_vert(const OffsetIndices<int> polys,
const Span<int> corner_verts,
HooglyBoogly marked this conversation as resolved Outdated
Wannes Malfait commented 2023-08-02 14:31:43 +02:00
Outdated
Review
Copy Link

"fan of edges" -> "fan of corners"

"fan of edges" -> "fan of corners"
const Span<int> corner_edges,
const BitSpan loose_edges,
const GroupedSpan<int> edge_to_corner_map,
const Span<int> corner_to_poly_map,
const BitSpan split_edges,
const Span<int> connected_edges,
const int vert)
{
const Span<int> connected_edges = vert_to_edge_map[vert];
if (connected_edges.size() <= 1) {
return Vector<Vector<int>>({Vector<int>(connected_edges)});
}
BLI_assert(!connected_edges.is_empty());
Array<int, 8> users_left(connected_edges.size());
for (const int i : users_left.index_range()) {
@ -291,58 +299,82 @@ static VertCornerFans calc_fans_for_vert(const OffsetIndices<int> polys,
users_left[i] = edge_to_corner_map[edge].size();
}
Vector<Vector<int>> edge_fans;
VertFans fans;
if (!loose_edges.is_empty()) {
for (const int i : connected_edges.index_range()) {
const int edge = connected_edges[i];
if (loose_edges[edge]) {
users_left[i]--;
if (split_edges[edge]) {
fans.split_loose_edges.append(edge);
}
else {
fans.non_split_loose_edges.append(edge);
}
}
}
HooglyBoogly marked this conversation as resolved Outdated
Jacques Lucke commented 2023-08-02 16:12:48 +02:00
Outdated
Review
Copy Link

I find the term merged confusing. Are we actually merging something or are we just not splitting these edges?

I find the term `merged` confusing. Are we actually merging something or are we just not splitting these edges?
}
while (!array_contains_non_zero(users_left)) {
const int start = first_index_of_non_zero(users_left);
edge_fans.append(gather_edges_in_fan(polys,
corner_verts,
corner_edges,
edge_to_corner_map,
corner_to_poly_map,
connected_edges,
split_edges,
vert,
start,
users_left));
const int start_edge = connected_edges[start];
const int start_corner = edge_to_corner_map[start_edge].first();
CornerFan fan = gather_corners_in_fan(polys,
corner_verts,
corner_edges,
edge_to_corner_map,
corner_to_poly_map,
connected_edges,
split_edges,
vert,
start_corner,
users_left);
fans.corner_fans.append(std::move(fan));
}
return edge_fans;
return fans;
}
/* Calculate groups of edges that are contiguously connected to each input vertex. */
static Array<VertCornerFans> calc_all_vert_fans(const OffsetIndices<int> polys,
const Span<int> corner_verts,
const Span<int> corner_edges,
const GroupedSpan<int> vert_to_edge_map,
const GroupedSpan<int> edge_to_corner_map,
const Span<int> corner_to_poly_map,
const BitSpan split_edges,
const IndexMask &vert_mask)
static Array<VertFans> calc_all_vert_fans(const OffsetIndices<int> polys,
const Span<int> corner_verts,
const Span<int> corner_edges,
const BitSpan loose_edges,
HooglyBoogly marked this conversation as resolved Outdated
Wannes Malfait commented 2023-08-29 22:29:14 +02:00
Outdated
Review
Copy Link

Took me a while to realize what new_edge_offsets was storing. Something like num_new_edges_per_old_edge would be clearer to me (but also too long probably). I'm fine if you leave it as is, since the current name does make sense as well in hindsight. What confused me was that "offset" is also used in other arrays like offset_no_merge or merged_offsets in a different way (cumulative offsets).

Took me a while to realize what `new_edge_offsets` was storing. Something like `num_new_edges_per_old_edge` would be clearer to me (but also too long probably). I'm fine if you leave it as is, since the current name does make sense as well in hindsight. What confused me was that "offset" is also used in other arrays like `offset_no_merge` or `merged_offsets` in a different way (cumulative offsets).
const GroupedSpan<int> vert_to_edge_map,
const GroupedSpan<int> edge_to_corner_map,
const Span<int> corner_to_poly_map,
const BitSpan split_edges,
const IndexMask &vert_mask)
{
Array<VertCornerFans> vert_fans(vert_mask.size()); /* TODO: Use NoInitialization() */
Array<VertFans> vert_fans(vert_mask.size()); /* TODO: Use NoInitialization() */
vert_mask.foreach_index(GrainSize(512), [&](const int vert, const int mask) {
HooglyBoogly marked this conversation as resolved Outdated
Jacques Lucke commented 2023-08-02 16:13:51 +02:00
Outdated
Review
Copy Link

Should use the r_ prefix.

Should use the `r_` prefix.
vert_fans[mask] = calc_fans_for_vert(polys,
HooglyBoogly marked this conversation as resolved Outdated
Wannes Malfait commented 2023-08-29 22:34:30 +02:00
Outdated
Review
Copy Link

I was not able to figure out why this line is there. I see that this is different from what is set in the loop, but it seems wrong to me to have this here:

  • last() is not necessarily a selected edge
  • I thought the offsets should point to the index the new edge would be located at. Now the last one is pointing out of bounds?

Probably there's something I'm not getting

I was not able to figure out why this line is there. I see that this is different from what is set in the loop, but it seems wrong to me to have this here: - `last()` is not necessarily a selected edge - I thought the offsets should point to the index the new edge would be located at. Now the last one is pointing out of bounds? Probably there's something I'm not getting
Hans Goudey commented 2023-08-29 23:07:33 +02:00
Outdated
Review
Copy Link

Ah, sorry, the extra element is left over from when I was using OffsetIndices with this array (which requires the extra).

Ah, sorry, the extra element is left over from when I was using `OffsetIndices` with this array (which requires the extra).
corner_verts,
corner_edges,
vert_to_edge_map,
loose_edges,
edge_to_corner_map,
HooglyBoogly marked this conversation as resolved Outdated
Wannes Malfait commented 2023-08-29 22:07:46 +02:00
Outdated
Review
Copy Link

Something seems wrong with this comment, the "instead" doesn't make sense to me.

Something seems wrong with this comment, the "instead" doesn't make sense to me.
Hans Goudey commented 2023-08-29 23:09:39 +02:00
Outdated
Review
Copy Link

Right, good point, "instead" is weird.

Right, good point, "instead" is weird.
corner_to_poly_map,
split_edges,
vert_to_edge_map[vert],
vert);
});
return vert_fans;
}
static OffsetIndices<int> calc_vert_ranges_per_old_vert(const IndexMask &vert_mask,
const Span<VertCornerFans> &vert_fans,
const Span<VertFans> &vert_fans,
Array<int> &offset_data)
{
offset_data.reinitialize(vert_mask.size() + 1);
threading::parallel_for(vert_mask.index_range(), 2048, [&](const IndexRange range) {
for (const int i : range) {
const VertFans &fans = vert_fans[i];
const int non_loose_edges = fans.corner_fans.size() + 1;
const int split_loose_edges = fans.split_loose_edges.size();
const int non_split_loose_edges = fans.non_split_loose_edges.is_empty() ? 0 : 1;
/* Reuse the original vertex for the last fan. */
offset_data[i] = vert_fans[i].size() - 1;
offset_data[i] = non_loose_edges + split_loose_edges + non_split_loose_edges - 1;
}
HooglyBoogly marked this conversation as resolved Outdated
Jacques Lucke commented 2023-08-02 16:14:48 +02:00
Outdated
Review
Copy Link

This might result in an unnecessary copy currently.

This might result in an unnecessary copy currently.
Hans Goudey commented 2023-08-02 16:58:48 +02:00
Outdated
Review
Copy Link

Oops, thanks!

Oops, thanks!
});
return offset_indices::accumulate_counts_to_offsets(offset_data);
@ -350,9 +382,7 @@ static OffsetIndices<int> calc_vert_ranges_per_old_vert(const IndexMask &vert_ma
static Array<int> calc_updated_corner_verts(const int orig_verts_num,
const Span<int> orig_corner_verts,
const GroupedSpan<int> edge_to_corner_map,
const IndexMask &vert_mask,
const Span<VertCornerFans> &vert_fans,
const Span<VertFans> &vert_fans,
const OffsetIndices<int> new_verts_by_masked_vert)
{
Array<int> new_corner_verts(orig_corner_verts.size());
@ -360,12 +390,15 @@ static Array<int> calc_updated_corner_verts(const int orig_verts_num,
/* Update corner verts so that each fan of edges gets its own vertex. For the last "new vertex"
* we can reuse the original vertex, which would otherwise become unused by any faces. */
vert_mask.foreach_index(GrainSize(512), [&](const int vert, const int mask) {
const VertCornerFans &fans = vert_fans[mask];
const IndexRange new_verts = new_verts_by_masked_vert[mask];
for (const int i : fans.index_range().drop_back(1)) {
const int new_vert = orig_verts_num + new_verts[i];
new_corner_verts.as_mutable_span().fill_indices(fans[i].as_span(), new_vert);
threading::parallel_for(vert_fans.index_range(), 512, [&](const IndexRange range) {
for (const int i : range) {
const VertFans &fans = vert_fans[i];
const IndexRange new_verts = new_verts_by_masked_vert[i];
for (const int i : fans.corner_fans.index_range().drop_back(1)) {
const int new_vert = orig_verts_num + new_verts[i];
const Span<int> corners = fans.corner_fans[i];
new_corner_verts.as_mutable_span().fill_indices(corners, new_vert);
}
}
});
HooglyBoogly marked this conversation as resolved Outdated
Wannes Malfait commented 2023-08-29 22:41:44 +02:00
Outdated
Review
Copy Link

Is this really the earliest we can conclude this? I think it should be possible to do something like this:

  • Add bool deduplicated = false; before the deduplication loop
  • The moment add_edge_or_find_index detects a duplicate (I think that is what it does), set deduplicated = true. I think this should be fine in terms of thread safety, since it doesn't matter if multiple threads want to set it to true.
  • At the end of that loop we simply check for deduplicated == false to know if we have an early exit. If so, we skip the accumulation and loop just above this line.

What do you think?

Is this really the earliest we can conclude this? I think it should be possible to do something like this: - Add `bool deduplicated = false;` before the deduplication loop - The moment `add_edge_or_find_index` detects a duplicate (I think that is what it does), set `deduplicated = true`. I think this should be fine in terms of thread safety, since it doesn't matter if multiple threads want to set it to `true`. - At the end of that loop we simply check for `deduplicated == false` to know if we have an early exit. If so, we skip the accumulation and loop just above this line. What do you think?
Hans Goudey commented 2023-08-29 23:15:41 +02:00
Outdated
Review
Copy Link

Hmm, I'm not sure that lets us skip any meaningful work, since when there are duplicates, we still need both the un-merged offsets and the merged offsets in order to remove the empty slots in the array that are unused because of the deduplicate. We could redo some of the work in the first loop to find the second offsets array, but I'm not sure that's worth it.

Maybe I'm missing something from your idea though.

Hmm, I'm not sure that lets us skip any meaningful work, since when there are duplicates, we still need both the un-merged offsets and the merged offsets in order to remove the empty slots in the array that are unused because of the deduplicate. We could redo some of the work in the first loop to find the second offsets array, but I'm not sure that's worth it. Maybe I'm missing something from your idea though.
Wannes Malfait commented 2023-08-30 09:32:12 +02:00
Outdated
Review
Copy Link

I was thinking about optimizing the "no duplicates" case a bit more. If this is the case, we can just return the no_merge_new_edges array. Currently, this is detected by checking if merged_offsets.total_size() == no_merge_new_edges.size(). My idea is that we could detect this, before calculating merged_offsets, using the method in my comment. That way, when there are no duplicates, we skip this calculation of merged_offsets. If there are duplicates, then we do still need to calculate it of course. Does that make sense?

I was thinking about optimizing the "no duplicates" case a bit more. If this is the case, we can just return the `no_merge_new_edges` array. Currently, this is detected by checking if `merged_offsets.total_size() == no_merge_new_edges.size()`. My idea is that we could detect this, before calculating `merged_offsets`, using the method in my comment. That way, when there are no duplicates, we skip this calculation of `merged_offsets`. If there are duplicates, then we do still need to calculate it of course. Does that make sense?
Hans Goudey commented 2023-08-30 14:00:42 +02:00
Outdated
Review
Copy Link

Thanks, I tried this and it is indeed faster when there are no merges (only in the 100% test case, but probably most real world cases too). In that it changed the timing from 56 to 35 ms.

Thanks, I tried this and it is indeed faster when there are no merges (only in the 100% test case, but probably most real world cases too). In that it changed the timing from 56 to 35 ms.
@ -459,7 +492,7 @@ void split_edges(Mesh &mesh,
HooglyBoogly marked this conversation as resolved Outdated
Wannes Malfait commented 2023-08-29 22:25:34 +02:00
Outdated
Review
Copy Link

This is also used for edges, so using vert in the comment and variable name is maybe not the best idea. Maybe a generic element would be good enough?

This is also used for edges, so using `vert` in the comment and variable name is maybe not the best idea. Maybe a generic `element` would be good enough?
IndexMaskMemory memory;
// const bke::LooseEdgeCache &loose_edges_cache = mesh.loose_edges();
const bke::LooseEdgeCache &loose_edges_cache = mesh.loose_edges();
// const IndexMask loose_edges = IndexMask::from_bits(loose_edges_cache.is_loose_bits, memory);
const IndexMask edge_mask_inverse = edge_mask.complement(orig_edges.index_range(), memory);
@ -467,25 +500,22 @@ void split_edges(Mesh &mesh,
orig_edges, edge_mask, orig_verts_num, memory);
const BitVector<> selection_bits = selection_to_bit_vector(edge_mask, orig_edges.size());
const Array<VertCornerFans> vert_fans = calc_all_vert_fans(polys,
orig_corner_verts,
orig_corner_edges,
vert_to_edge_map,
edge_to_corner_map,
corner_to_poly_map,
selection_bits,
vert_mask);
const Array<VertFans> vert_fans = calc_all_vert_fans(polys,
orig_corner_verts,
orig_corner_edges,
loose_edges_cache.is_loose_bits,
vert_to_edge_map,
edge_to_corner_map,
corner_to_poly_map,
selection_bits,
vert_mask);
Array<int> vert_new_vert_offset_data;
const OffsetIndices new_verts_by_masked_vert = calc_vert_ranges_per_old_vert(
vert_mask, vert_fans, vert_new_vert_offset_data);
Array<int> new_corner_verts = calc_updated_corner_verts(orig_verts_num,
orig_corner_verts,
edge_to_corner_map,
vert_mask,
vert_fans,
new_verts_by_masked_vert);
Array<int> new_corner_verts = calc_updated_corner_verts(
orig_verts_num, orig_corner_verts, vert_fans, new_verts_by_masked_vert);
/* Create new edges. */
Array<int> new_corner_edges(orig_corner_edges.size());