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 +7 -5
1 changed files with 7 additions and 5 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit 415aebc3d5 - Show all commits

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

@ -342,14 +342,16 @@ static Array<int2> calc_new_edges(const OffsetIndices<int> faces,
MutableSpan<int> corner_edges,
MutableSpan<int> r_new_edge_offsets)
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).
{
/* First we count the number of deduplicated new edges, then accumulate the counts as offsets. */
MutableSpan<int> num_new_edges_per_edge = r_new_edge_offsets;
/* Calculate the offset of new edges assuming no new edges are identical and are merged. */
int no_merge_count = 0;
Array<int> offsets_no_merge(selected_edges.size() + 1);
Array<int> offsets_no_merge(selected_edges.size());
selected_edges.foreach_index([&](const int edge, 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.
offsets_no_merge[mask] = no_merge_count;
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).
no_merge_count += std::max<int>(edge_to_corner_map[edge].size() - 1, 0);
});
offsets_no_merge.last() = no_merge_count;
Array<int2> no_merge_new_edges(no_merge_count);
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.
@ -375,7 +377,7 @@ static Array<int2> calc_new_edges(const OffsetIndices<int> faces,
}
}
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!
if (deduplication.is_empty()) {
r_new_edge_offsets[mask] = 0;
num_new_edges_per_edge[mask] = 0;
return;
}
edges[edge] = int2(deduplication.first().v_low, deduplication.first().v_high);
@ -384,13 +386,13 @@ static Array<int2> calc_new_edges(const OffsetIndices<int> faces,
const int dst_offset = offsets_no_merge[mask];
uninitialized_copy_n(new_edges.data(), new_edges.size(), &no_merge_new_edges[dst_offset]);
}
r_new_edge_offsets[mask] = new_edges.size();
num_new_edges_per_edge[mask] = new_edges.size();
});
/* Use the merged offsets (potentially different from the first offsets) to globalize the new
* edge indices per selected edge. */
const OffsetIndices merged_offsets = offset_indices::accumulate_counts_to_offsets(
r_new_edge_offsets);
num_new_edges_per_edge);
selected_edges.foreach_index(GrainSize(1024), [&](const int edge, const int mask) {
const int new_edge_offset = merged_offsets[mask].start() + edges.size();
for (const int corner : edge_to_corner_map[edge]) {