Skip to content
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

triangulator optimization #723

Closed
3 tasks
pca006132 opened this issue Jan 28, 2024 · 10 comments · Fixed by #830
Closed
3 tasks

triangulator optimization #723

pca006132 opened this issue Jan 28, 2024 · 10 comments · Fixed by #830
Labels
enhancement New feature or request

Comments

@pca006132
Copy link
Collaborator

The triangulator takes O(n^2) time, which performs poorly when the number of points of a polygon increases. For example, for the woodgrain svg we had for #502, there are around 60k points in the polygon. The old triangulator (not as robust) takes 35ms to triangulate the 60k points, while our current triangulator takes around 11500ms (11.5s!). We should try to use bvh to reduce the number of points to check, to avoid quadratic complexity in the general case (for degenerate cases bvh can't help much I guess).

  • Make Collider generic and support 2D Rect. This is not necessary, but will probably help with performance and is not hard to do.
  • Add Remove method to Collider to support point removal. We don't have to care about rebalancing because the height is bounded (at least in terms of complexity it does not matter, not sure about that in practice, and rebalancing is costly).
  • Avoid calculating DelaunayCost. We can just categorize ears into different categories: definitely valid, potentially valid, definitely invalid, according to the max cost over query results.

@elalish what do you think?

@pca006132 pca006132 added the enhancement New feature or request label Jan 28, 2024
@elalish
Copy link
Owner

elalish commented Jan 29, 2024

Yeah, that sounds good. And actually, I think Delaunay cost should work too - we only care within a bounded region: Delaunay only matters for points closer to the edge midpoint than its half-length. So we should still be able to safely screen out more distant points with BHV.

@pca006132
Copy link
Collaborator Author

btw wonder if it is possible to move some of the sort by morton code logic into the collider.

@elalish
Copy link
Owner

elalish commented Jan 30, 2024

btw wonder if it is possible to move some of the sort by morton code logic into the collider.

Or in its own header file. Still, it's important the sort the actual underlying data structures, since that gives us cache coherence. So the Morton codes are for more than just the collider.

@onlynadeem
Copy link

I know how to do triangulator optimization very well, but where to import its source file?

@pca006132
Copy link
Collaborator Author

https://github.com/elalish/manifold/blob/master/src/polygon/src/polygon.cpp

@pca006132
Copy link
Collaborator Author

Adding a comment from #751:

Optimizations related to triangulator is interesting, I am open to adding fast paths, but a more general one (e.g. monotone triangulation, but doesn't handle epsilon), and directly into the triangulator rather than limited to extrude.

@kintel
Copy link
Contributor

kintel commented Jun 4, 2024

This is starting to hurt for some OpenSCAD usage too. It's also surprisingly slow in debug mode:

  • Manifold debug: 5m50s
  • Manifold release: 12s
  • CGAL constrained delaunay triangulator: 0.3s

^ not very scientific, just measuring OpenSCAD e2e processing time.

@elalish
Copy link
Owner

elalish commented Jun 4, 2024

Yeah, debug mode adds a lot of extra checks - we should probably go through and see how many are necessary. And you do mean just MANIFOLD_DEBUG right? Because actual C++ Debug mode is horrifically slow for some reason.

Yeah, we should get this done - it shouldn't even really be so difficult. What kind of example is this where you see the slowdown? Care to make a TEST of it?

@kintel
Copy link
Contributor

kintel commented Jun 5, 2024

The polygon in question is #831 - might be a tad big to add as a performance test

@kintel
Copy link
Contributor

kintel commented Jun 7, 2024

Thx - looks very promising so far!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants