Fix face removal (#1191) #1192
Fix face removal (#1191) #1192
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1192 +/- ##
==========================================
- Coverage 92.21% 92.09% -0.13%
==========================================
Files 31 31
Lines 5959 5982 +23
==========================================
+ Hits 5495 5509 +14
- Misses 464 473 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
src/impl.cpp
Outdated
| #endif | ||
|
|
||
| if (!removedHalfedges.empty()) { | ||
| // Remove the marked halfedges |
There was a problem hiding this comment.
Okay, so the logic I had been using was that I could mark any halfedge as removed by setting it to -1 and any vertex by setting it to NAN, then Finish() would take care of actually removing them because that's where the vectors get sorted and reindexed anyway. I was figuring that's an expensive step, so better to do it once. This bug sounds like I messed that up somewhere.
Still, isn't this big chunk of new code a lot like Finish? Is there a way we can share it? Or a way we can organize things so that removals are still lazy, but safer?
There was a problem hiding this comment.
Yeah make sense, I just added the checks elsewhere and keep the removed halfedges in-place.
|
Weird, I can't reproduce the failure. |
|
OK some uninitialized memory stuff. Here we go again. |
src/impl.cpp
Outdated
| if (h0.startVert != h1.endVert || h0.endVert != h1.startVert) break; | ||
| if (halfedge_[NextHalfedge(pair0)].endVert == | ||
| halfedge_[NextHalfedge(pair1)].endVert) { | ||
| h0 = {-1, -1, -1}; | ||
| h1 = {-1, -1, -1}; | ||
| h0.pairedHalfedge = h1.pairedHalfedge = -2; |
There was a problem hiding this comment.
perhaps we should start using named constants?
| @@ -42,7 +42,7 @@ TEST(SDF, SphereShell) { | |||
| }, | |||
| {vec3(-1.1), vec3(1.1)}, 0.01, 0, 0.0001); | |||
|
|
|||
| EXPECT_NEAR(sphere.Genus(), 11500, 1000); | |||
| EXPECT_NEAR(sphere.Genus(), 14235, 1000); | |||
There was a problem hiding this comment.
Does this still look good, or look better? I recall working to remove little bits of infinitely thin geometry on this one.
There was a problem hiding this comment.
I don't see infinitely thin stuff, but I see something that looks like self-intersection:
This is probably related to the other thing I mentioned, as there are many non-2-manifold edges here.
There was a problem hiding this comment.
Ah, right - I think my slick pre-edge collapse thing in the SDF can create even manifold edges. We should probably look at that again and see if there's a better way. I believe without that it's all guaranteed 2-manifold.
pca006132
force-pushed
the
fix-edge-removal
branch
from
b2427e6 to
5500e0b
Compare
Closes #1191.
The bulk of the code is handling removed faces. Instead of letting them sit in the vector, I remove them and reorder properties dependent on this. This avoids the need to make other functions accept removed halfedges.
While writing this, something that comes to my mind is that we are not really trying our best to handle cases that are not 2-manifold. The current matching method may produce pairs that cross-cross together. I think one way to handle this is to sort the duplicated halfedges according to the face normals, so we will not produce any criss-crossing pairs. Probably for another issue, though.