-
Notifications
You must be signed in to change notification settings - Fork 123
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
New decimator #1175
New decimator #1175
Conversation
@@ -324,9 +324,7 @@ struct Manifold::Impl { | |||
|
|||
// face_op.cpp | |||
void Face2Tri(const Vec<int>& faceEdge, const Vec<TriRef>& halfedgeRef); | |||
PolygonsIdx Face2Polygons(VecView<Halfedge>::IterC start, | |||
VecView<Halfedge>::IterC end, | |||
mat2x3 projection) const; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I split Face2Polygons into two non-member functions: AssembleHalfedges and ProjectPolygons.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see anything wrong with synchronization here. Probably need more time to debug.
For |
Ah, thanks for noticing that - I'd say it's actually desired behavior. Ideally I'd like any mesh with effectively zero volume (not an easy thing to check in practice, but conceptually) to get decimated down to empty. I think that means any flattish-mesh with a thickness less than 2x tolerance will find only two faces, and no verts (since a vert is an intersection of 3 or more faces). I'll toss in some logic to early-out in that case. |
src/face_op.cpp
Outdated
if (numEdge > 0) { | ||
std::copy(polys2.begin(), polys2.end(), newHalfedge2.begin() + start); | ||
} | ||
faceEdge2[face + 1] = start + numEdge; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found the parallel problem - can't think of the correct way to do this right now...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the order in newHalfedge2
important? Can we fix them later instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh ok I get it, faceEdge requires following the order. I guess we could just modify the definition a bit...
Ugly hack to make it work: diff --git i/src/boolean_result.cpp w/src/boolean_result.cpp
index afc0a6ee..7493b81c 100644
--- i/src/boolean_result.cpp
+++ w/src/boolean_result.cpp
@@ -844,7 +844,7 @@ Manifold::Impl Boolean3::Result(OpType op) const {
if (ManifoldParams().intermediateChecks)
DEBUG_ASSERT(outR.IsManifold(), logicErr, "polygon mesh is not manifold!");
- outR.Face2Tri(faceEdge, halfedgeRef);
+ outR.Face2Tri(faceEdge, halfedgeRef, {});
halfedgeRef.clear();
faceEdge.clear();
diff --git i/src/face_op.cpp w/src/face_op.cpp
index 62c8d026..230a0cd2 100644
--- i/src/face_op.cpp
+++ w/src/face_op.cpp
@@ -91,7 +91,8 @@ using AddTriangle = std::function<void(int, ivec3, vec3, TriRef)>;
* faceNormal_ values are retained, repeated as necessary.
*/
void Manifold::Impl::Face2Tri(const Vec<int>& faceEdge,
- const Vec<TriRef>& halfedgeRef) {
+ const Vec<TriRef>& halfedgeRef,
+ const Vec<int>& faceEdgeStart) {
ZoneScoped;
Vec<ivec3> triVerts;
Vec<vec3> triNormal;
@@ -99,7 +100,7 @@ void Manifold::Impl::Face2Tri(const Vec<int>& faceEdge,
triRef.clear();
auto processFace = [&](GeneralTriangulation general, AddTriangle addTri,
int face) {
- const int firstEdge = faceEdge[face];
+ const int firstEdge = faceEdgeStart.empty() ? faceEdge[face] : faceEdgeStart[face];
const int lastEdge = faceEdge[face + 1];
const int numEdge = lastEdge - firstEdge;
if (numEdge == 0) return;
@@ -181,7 +182,7 @@ void Manifold::Impl::Face2Tri(const Vec<int>& faceEdge,
const vec3 normal = faceNormal_[face];
const mat2x3 projection = GetAxisAlignedProjection(normal);
const PolygonsIdx polys = ProjectPolygons(
- AssembleHalfedges(halfedge_.cbegin() + faceEdge[face],
+ AssembleHalfedges(halfedge_.cbegin() + (faceEdgeStart.empty() ? faceEdge[face] : faceEdgeStart[face]),
halfedge_.cbegin() + faceEdge[face + 1]),
vertPos_, projection);
return TriangulateIdx(polys, epsilon_);
@@ -196,7 +197,7 @@ void Manifold::Impl::Face2Tri(const Vec<int>& faceEdge,
// triangulate complex faces
for_each(autoPolicy(faceEdge.size(), 1e5), countAt(0_uz),
countAt(faceEdge.size() - 1), [&](size_t face) {
- triCount[face] = faceEdge[face + 1] - faceEdge[face] - 2;
+ triCount[face] = faceEdge[face + 1] - (faceEdgeStart.empty() ? faceEdge[face] : faceEdgeStart[face]) - 2;
DEBUG_ASSERT(triCount[face] >= 1, topologyErr,
"face has less than three edges.");
if (triCount[face] > 2)
@@ -307,6 +308,7 @@ void Manifold::Impl::FlattenFaces() {
std::atomic<int> startFace(0);
Vec<int> faceEdge2(faceEdge.size());
+ Vec<int> faceEdgeStart(faceEdge.size() - 1);
faceEdge2[0] = 0;
Vec<Halfedge> newHalfedge2(newHalfedge.size());
for_each_n(policy, countAt(0_uz), numFace, [&](size_t face) {
@@ -334,6 +336,7 @@ void Manifold::Impl::FlattenFaces() {
if (numEdge > 0) {
std::copy(polys2.begin(), polys2.end(), newHalfedge2.begin() + start);
}
+ faceEdgeStart[face] = start;
faceEdge2[face + 1] = start + numEdge;
const int oldFace = newHalf2Old[faceEdge[face]] / 3;
@@ -350,7 +353,7 @@ void Manifold::Impl::FlattenFaces() {
newHalfedge2.resize(startFace);
halfedge_ = std::move(newHalfedge2);
- Face2Tri(faceEdge2, halfedgeRef);
+ Face2Tri(faceEdge2, halfedgeRef, faceEdgeStart);
Finish();
}
diff --git i/src/impl.h w/src/impl.h
index 8f9f537f..5d1c765f 100644
--- i/src/impl.h
+++ w/src/impl.h
@@ -323,7 +323,7 @@ struct Manifold::Impl {
void GatherFaces(const Impl& old, const Vec<int>& faceNew2Old);
// face_op.cpp
- void Face2Tri(const Vec<int>& faceEdge, const Vec<TriRef>& halfedgeRef);
+ void Face2Tri(const Vec<int>& faceEdge, const Vec<TriRef>& halfedgeRef, const Vec<int>& faceEdgeStart);
void FlattenFaces();
Polygons Slice(double height) const;
Polygons Project() const; I don't push it because it is too ugly :P |
Okay, how's this? It's 2x faster than the old decimator, so that's something at least. I still need to see if I can make these hexagons to triangulate nicer. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1175 +/- ##
==========================================
+ Coverage 91.60% 91.62% +0.02%
==========================================
Files 32 32
Lines 5989 6067 +78
==========================================
+ Hits 5486 5559 +73
- Misses 503 508 +5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I think the black formatter version may be automatically updated, causing the format CI error. |
@@ -235,6 +235,9 @@ void Manifold::Impl::CleanupTopology() { | |||
* meshes, thus decreasing the Genus(). It only increases when meshes that have | |||
* collapsed to just a pair of triangles are removed entirely. | |||
* | |||
* Verts with index less than firstNewVert will be left uncollapsed. This is | |||
* zero by default so that everything can be collapsed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When do we use this zero default? We should probably explain this mesh simplification behavior in the public documentation as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everywhere except the Boolean - mesh ingestion, AsOriginal, etc. Hmm, come to think of it, the issue of decimating input planar geometry that doesn't intersect anything isn't really fixed, since we're still doing it on mesh import and other places. Perhaps those should all be restricted to short edge collapse? I think I'll do this in a separate PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good.
src/face_op.cpp
Outdated
thisEdge = startEdge; | ||
polys.push_back({}); | ||
if (numFace < 4) { | ||
MarkFailure(Error::NoError); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we change the name of this function? Done
or something? Otherwise it is a bit weird to mark something as failure even though it has no error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was thinking the same thing. MarkEmpty sounds good.
src/face_op.cpp
Outdated
|
||
const int numEdge = polys2.size(); | ||
if (numEdge < 3) return; | ||
const uint64_t inc = (1LL << 32) + numEdge; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably document this so that whoever looks at this later can figure out the pair encoding at a glance. Or maybe just make a helper function because we use this below as well.
@@ -936,16 +940,21 @@ namespace manifold { | |||
* references back to the original vertices. | |||
* @param epsilon The value of ε, bounding the uncertainty of the | |||
* input. | |||
* @param allowConvex If true (default), the triangulator will use a fast |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where do we use this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was how I fixed Simplify() - the surface looks like garbage with convex triangulation (almost the same as the edge collapse version); ear clipping sorts by quality so it does quite well. Figured others might need it too, so probably best in the public API anyway.
My idea here is to speed up our
Simplify()
function by triangulating the identified faces in parallel rather than collapsing the edges serially. I was also hoping this would produce a better triangulation with fewer slivers, since that makes more difference in a situation like this where the verts of a face are not entirely coplanar.That part doesn't seem to be true so far, but this may point toward ways to improve the triangulator.The problem was the fast convex triangulator, which makes poorer quality triangles.We went from this in 3 seconds (8 million triangles to 2,500):

To doing the same thing in 1.5 seconds with this result:

At the moment this appears to be working in serial mode, but is failing in parallel, so I've probably messed something up with my atomics.Fixed.