RFC to make SimplifyTopology() steps optional #603
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #603 +/- ##
=======================================
Coverage 91.41% 91.41%
=======================================
Files 35 35
Lines 4496 4497 +1
=======================================
+ Hits 4110 4111 +1
Misses 386 386 ☔ View full report in Codecov by Sentry. |
src/manifold/src/edge_op.cpp
Outdated
| std::cout << "found " << numFlagged << " duplicate edges to split" | ||
| << std::endl; | ||
| } | ||
| if (ManifoldParams().verbose && numFlagged > 0) { |
src/manifold/src/edge_op.cpp
Outdated
| for (int i = 0; i < nbEdges; ++i) { | ||
| if (halfedge_[i].IsForward()) { | ||
| entries.push_back({halfedge_[i].startVert, halfedge_[i].endVert, i}); | ||
| } |
There was a problem hiding this comment.
splitPinchedVerts and splitDuplicateEdges are not optional - they rarely make a change, but when they do, it's necessary for manifoldness.
Let's just put an early return after this section when ManifoldParams().cleanupTriangles = false and have that be the only new param.
221b09a to
27879d3
Compare
|
I think this should match what you're thinking. I force pushed over the earlier revision. That way, this is two simple changes from mainline rather than a ton of noise which is then immediately reverted. |
Add option to skip cleaning up triangles in SimplifyTopology()
This is not intended for real inclusion.
Each major step in SimplifyTopology() was made separately optional. While this is handy for debugging, it is unlikely the way to production. In particular, some of the steps may depend on one another in ways that this can violate.
However, this gives a base for testing the optionality of steps in SimplifyTopology() and it provides a starting point for the discussion of how these options should be implemented in production.