Preventing crash with self intersection #501

stephomi · 2023-07-21T04:41:46Z

Issue: #495

The code snippet in the issue makes Samples.Sponge4 fails:

Expected: (sponge.NumDegenerateTris()) <= (40000), actual: 42386 vs 40000

If I change the logic a bit the test passes and it doesn't crash on my example, but

No idea if the fix is robust, as it's hard to replicate the self-intersection crash
Using ImportMesh to test the crash scenario but not sure relative path is the way to go for the test

pca006132 · 2023-07-21T07:03:32Z

I think the relative path should be ../../samples/models/self_intersectA.glb, as the directory we execute the test is in build/test.

stephomi · 2023-07-21T12:43:11Z

It will fail with the README build instruction for example(test/manifold_test, so executed from build/)
Ideally it should be relative to the executable folder (or from where it's executed).
For now I'll simply make relative to FILE, should for all cases.

pca006132 · 2023-07-21T12:56:12Z

To make the test pass, you can add

manifold::PolygonParams().processOverlaps = true;

before your test case, and set it back to false afterwards.

stephomi · 2023-07-21T14:11:33Z

To make the test pass, you can add
manifold::PolygonParams().processOverlaps = true;
before your test case, and set it back to false afterwards.

Thanks!
For some reason, even without it the test was running on my machine™

codecov · 2023-07-21T14:25:16Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.01% 🎉

Comparison is base (047159e) 90.34% compared to head (c95f83d) 90.36%.
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #501      +/-   ##
==========================================
+ Coverage   90.34%   90.36%   +0.01%     
==========================================
  Files          35       35              
  Lines        4414     4431      +17     
==========================================
+ Hits         3988     4004      +16     
- Misses        426      427       +1

Files Changed	Coverage Δ
src/manifold/src/impl.h	`72.72% <ø> (ø)`
src/manifold/src/edge_op.cpp	`96.17% <100.00%> (+0.11%)`	⬆️

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

elalish

Thanks!

elalish · 2023-07-23T04:38:31Z

src/manifold/src/edge_op.cpp

@@ -222,9 +222,10 @@ void Manifold::Impl::SimplifyTopology() {
                     precision_};
    for_each_n(policy, countAt(0), nbEdges,
               [=] __host__ __device__(int i) { bflagsPtr[i] = se(i); });
+    std::vector<uint32_t> visited(halfedge_.size(), -1);


Setting a uint to -1 feels a little awkward...

elalish · 2023-07-23T04:41:13Z

src/manifold/src/edge_op.cpp

  VecDH<TriRef>& triRef = meshRelation_.triRef;

  if (edge < 0) return;
  const int pair = halfedge_[edge].pairedHalfedge;
  if (pair < 0) return;
+  if (visited[edge] == pair) return;
+  visited[edge] = pair;


Would it be safer to mark these as booleans and set both the [edge] and [pair] to visited?

It will likely output more degenerate tris, but I’ll try.
Otherwise maybe tagging visited[pair] = edge.
Ultimately, slight changes in the logic gives different result for the ‘Sponge4’ test

Yeah, I'm not super worried about the variations in Sponge4 - really I need to come back to it and figure out why it has so many in the first place - I had managed to get rid of most of them at one time.

Curious what you find here - I need to look into this logic more deeply at some point. It's been awhile.

Found a way to get the same number of degenerate Tris as master, while avoiding recursion.
Instead of booleans I use tag that I reset when there is a CollapseEdge.

Alternative version: (maybe a bit more robust, to avoid skipping valid cases)
Using a stack and hash pair of edges instead of checking both edges separately

diff --git a/src/manifold/src/edge_op.cpp b/src/manifold/src/edge_op.cpp index 26fd983..e1c2e73 100644 --- a/src/manifold/src/edge_op.cpp +++ b/src/manifold/src/edge_op.cpp @@ -222,12 +222,11 @@ void Manifold::Impl::SimplifyTopology() { precision_}; for_each_n(policy, countAt(0), nbEdges, [=] __host__ __device__(int i) { bflagsPtr[i] = se(i); }); - std::vector<int> visited(halfedge_.size(), -1); - int tag = 0; + std::vector<glm::ivec2> visited(halfedge_.size(), glm::ivec2(-1)); for (int i = 0; i < nbEdges; ++i) { if (bflags[i]) { - tag++; - RecursiveEdgeSwap(i, visited, tag); + visited.resize(0); + RecursiveEdgeSwap(i, visited); numFlagged++; } } @@ -487,7 +486,7 @@ void Manifold::Impl::CollapseEdge(const int edge) { } void Manifold::Impl::RecursiveEdgeSwap(const int edge, - std::vector<int>& visited, int& tag) { + std::vector<glm::ivec2>& visited) { VecDH<TriRef>& triRef = meshRelation_.triRef; if (edge < 0) return; @@ -495,7 +494,9 @@ void Manifold::Impl::RecursiveEdgeSwap(const int edge, if (pair < 0) return; // avoid infinite recursion - if (visited[edge] == tag && visited[pair] == tag) return; + for (int i = 0; i < visited.size(); ++i) { + if (visited[i] == glm::ivec2(edge, pair)) return; + } const glm::ivec3 tri0edge = TriOf(edge); const glm::ivec3 tri1edge = TriOf(pair); @@ -575,15 +576,14 @@ void Manifold::Impl::RecursiveEdgeSwap(const int edge, SwapEdge(); const glm::vec2 e23 = v[3] - v[2]; if (glm::dot(e23, e23) < precision_ * precision_) { - tag++; + visited.resize(0); CollapseEdge(tri0edge[2]); } else { - visited[edge] = tag; - visited[pair] = tag; - RecursiveEdgeSwap(tri0edge[0], visited, tag); - RecursiveEdgeSwap(tri0edge[1], visited, tag); - RecursiveEdgeSwap(tri1edge[0], visited, tag); - RecursiveEdgeSwap(tri1edge[1], visited, tag); + visited.emplace_back(glm::ivec2(edge, pair)); + RecursiveEdgeSwap(tri0edge[0], visited); + RecursiveEdgeSwap(tri0edge[1], visited); + RecursiveEdgeSwap(tri1edge[0], visited); + RecursiveEdgeSwap(tri1edge[1], visited); } return; } else if (CCW(v[0], v[3], v[2], precision_) <= 0 || @@ -592,10 +592,9 @@ void Manifold::Impl::RecursiveEdgeSwap(const int edge, } // Normal path SwapEdge(); - visited[edge] = tag; - visited[pair] = tag; - RecursiveEdgeSwap(halfedge_[tri0edge[1]].pairedHalfedge, visited, tag); - RecursiveEdgeSwap(halfedge_[tri1edge[0]].pairedHalfedge, visited, tag); + visited.emplace_back(glm::ivec2(edge, pair)); + RecursiveEdgeSwap(halfedge_[tri0edge[1]].pairedHalfedge, visited); + RecursiveEdgeSwap(halfedge_[tri1edge[0]].pairedHalfedge, visited); } void Manifold::Impl::SplitPinchedVerts() { diff --git a/src/manifold/src/impl.h b/src/manifold/src/impl.h index dc4f57b..cb890df 100644 --- a/src/manifold/src/impl.h +++ b/src/manifold/src/impl.h @@ -121,7 +121,7 @@ struct Manifold::Impl { void SimplifyTopology(); void DedupeEdge(int edge); void CollapseEdge(int edge); - void RecursiveEdgeSwap(int edge, std::vector<int>& visited, int& tag); + void RecursiveEdgeSwap(int edge, std::vector<glm::ivec2>& visited); void RemoveIfFolded(int edge); void PairUp(int edge0, int edge1); void UpdateVert(int vert, int startEdge, int endEdge);

Interesting, I tend to prefer to simpler version you have now, though I don't quite grok the difference. If you want, it's probably best to put this as a follow-on PR and add a test that shows the difference.

elalish · 2023-07-23T04:44:17Z

test/samples_test.cpp

+  manifold::PolygonParams().processOverlaps = true;
+  std::string file = __FILE__;
+  std::string dir = file.substr(0, file.rfind('/'));
+  Manifold m1 = ImportMesh(dir + "/../samples/models/self_intersectA.glb");


Is there a chance these are simple enough they can be generated by e.g. an Extrude and Warp? Still, there is value in being able to add test models - this may be nice as a helper function. I think I'd prefer to put them in a dedicated models directory in test rather than samples though.

Probably not, it’s a bit sensitive and I couldn’t replicate when I tried with other self intersected tubes,
Maybe if you managed to output the exact same sweep but probably lot of trial & error, and I’m lazy.

Okay, no prob - but let's at least change the directory.

elalish

Nice solution, thanks!

* Prevent infinite recursion * Add test for self intersection

stephomi force-pushed the nomad branch from 743f77b to 8de0bb4 Compare July 21, 2023 12:41

stephomi force-pushed the nomad branch from 8de0bb4 to bfc0858 Compare July 21, 2023 13:59

elalish mentioned this pull request Jul 22, 2023

Difference of Cube and Complex SVG Results in Incomplete Render (openscad/openscad#4679) #502

Closed

elalish requested changes Jul 23, 2023

View reviewed changes

Prevent infinite recursion

d66df2b

stephomi force-pushed the nomad branch 2 times, most recently from da8bbc4 to 7c3730b Compare July 26, 2023 02:19

Add test for self intersection

c95f83d

stephomi force-pushed the nomad branch from 7c3730b to c95f83d Compare July 26, 2023 02:30

elalish approved these changes Jul 26, 2023

View reviewed changes

elalish merged commit 5f9b484 into elalish:master Jul 27, 2023
22 checks passed

elalish mentioned this pull request Nov 3, 2023

V2.2.1 #589

Merged

cartesian-theatrics pushed a commit to SovereignShop/manifold that referenced this pull request Mar 11, 2024

Preventing crash with self intersection (elalish#501)

c596657

* Prevent infinite recursion * Add test for self intersection

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preventing crash with self intersection #501

Preventing crash with self intersection #501

stephomi commented Jul 21, 2023

pca006132 commented Jul 21, 2023

stephomi commented Jul 21, 2023

pca006132 commented Jul 21, 2023

stephomi commented Jul 21, 2023

codecov bot commented Jul 21, 2023 •

edited

Loading

elalish left a comment

elalish Jul 23, 2023

elalish Jul 23, 2023

stephomi Jul 24, 2023

elalish Jul 24, 2023

stephomi Jul 26, 2023 •

edited

Loading

elalish Jul 26, 2023

elalish Jul 23, 2023

stephomi Jul 24, 2023

elalish Jul 24, 2023

elalish left a comment

Preventing crash with self intersection #501

Preventing crash with self intersection #501

Conversation

stephomi commented Jul 21, 2023

pca006132 commented Jul 21, 2023

stephomi commented Jul 21, 2023

pca006132 commented Jul 21, 2023

stephomi commented Jul 21, 2023

codecov bot commented Jul 21, 2023 • edited Loading

Codecov Report

elalish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephomi Jul 26, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elalish left a comment

Choose a reason for hiding this comment

codecov bot commented Jul 21, 2023 •

edited

Loading

stephomi Jul 26, 2023 •

edited

Loading