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

3d Convex Hull #489

Merged
merged 18 commits into from
Jul 18, 2023
Merged

3d Convex Hull #489

merged 18 commits into from
Jul 18, 2023

Conversation

geoffder
Copy link
Collaborator

This adds a 3d convex hull operation to Manifold. Implementation is a port of what I have in OCADml, which is a port of BOSL2. I've made some effort to get the performance reasonable (for this algorithm anyway, there may be smarter ones out there). From my rough measurement, the vast majority of the time is spent on the DistanceToPlane operation, so I'm not sure what can be wrung out.

I've set it up to try and preserve some properties info, but I'm not too familiar with that API, so I'm hoping that what I've missed can get filled in with review.

@codecov
Copy link

codecov bot commented Jul 13, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.09 ⚠️

Comparison is base (da3f323) 90.35% compared to head (aaca0c8) 90.27%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #489      +/-   ##
==========================================
- Coverage   90.35%   90.27%   -0.09%     
==========================================
  Files          35       35              
  Lines        4396     4410      +14     
==========================================
+ Hits         3972     3981       +9     
- Misses        424      429       +5     
Impacted Files Coverage Δ
src/manifold/include/manifold.h 100.00% <ø> (ø)
src/manifold/src/manifold.cpp 95.63% <100.00%> (+0.29%) ⬆️

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

src/manifold/src/manifold.cpp Outdated Show resolved Hide resolved
src/manifold/src/manifold.cpp Show resolved Hide resolved
src/manifold/src/manifold.cpp Outdated Show resolved Hide resolved
src/manifold/src/manifold.cpp Outdated Show resolved Hide resolved
Copy link
Owner

@elalish elalish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess before we go any further we should figure out the performance of this algorithm. If we're not close to what other libraries achieve, we should probably take a dependency, because our n will tend to be quite large.

src/manifold/src/manifold.cpp Outdated Show resolved Hide resolved
src/manifold/src/manifold.cpp Outdated Show resolved Hide resolved
src/manifold/src/manifold.cpp Outdated Show resolved Hide resolved
src/manifold/src/manifold.cpp Outdated Show resolved Hide resolved
test/manifold_test.cpp Show resolved Hide resolved
src/manifold/src/manifold.cpp Outdated Show resolved Hide resolved
src/manifold/src/manifold.cpp Outdated Show resolved Hide resolved
src/manifold/src/manifold.cpp Outdated Show resolved Hide resolved
src/manifold/src/manifold.cpp Outdated Show resolved Hide resolved
src/manifold/src/manifold.cpp Outdated Show resolved Hide resolved
@geoffder
Copy link
Collaborator Author

geoffder commented Jul 13, 2023

RE: libraries this one is the top result, and seems like it'd be easy to vendor and use if we want to just go the dependency route. It's an implementation of QuickHull which I believe is better than what I have here. It also supports multithreading but via OMP, maybe forking and adapting it to Manifold's flexible OMP/TBB use via thrust would be worthwhile.

There are a couple papers that say they have something faster than QHull but I just looked at summaries/abstracts last night.

This was a good experience in imperative-izing and general C++ practice anyways 😅.

This is the other article about 3d hull algorithms with example implementations that I meant to link before (deleted wrong paste for 2d).

While I'm listing options, here are some simpler to consume quickhull implementations that I've come across for reference:

@geoffder
Copy link
Collaborator Author

I'll go ahead try out the header quickhulls implementations that I linked above on then. Not necessary to start with I don't think, but might be interesting to try out a strategy like described here in this video and article in the future. I'm curious how this and another similar paper I've seen along the way actually play out with the kind of inputs we'd have in Manifold, since many of the benchmarks that are shown off in these things are clouds rather than separate sphere meshes (that are already hollow) for example.

@geoffder
Copy link
Collaborator Author

geoffder commented Jul 14, 2023

So far out of the non-qhull options, akuuka performs the best, as in it doesn't blow up and OOM my 32GM completely at circularSegments = 1000 (does at 10000) in the basic tictac hull test, but it is capping out the resolution below even that level (the hull output has lower resolution triangles than the input).

image
image

I guess I'm going to actually have to try qhull next to see if it has similar limitations.

edit: Actually, when using double rather than float it maintains the resolution at 1000.
image
At 5000 (2 * 6250038 vertex spheres) it completed in 140s capping out at about 15GB ram.

@geoffder
Copy link
Collaborator Author

geoffder commented Jul 17, 2023

After trying the above mentioned libs it seems like Akuukka's QuickHull implementation is the way to go, with very few lines of code and CMakeLists.txt required to get it working to boot.

@geoffder
Copy link
Collaborator Author

The time to hull I mentioned before was actually an over-estimate since it included the time to get the huge mesh and export it 😅, here is a rough idea of scaling over vertex count.

TEST(Manifold, TictacBench) {
  const float tictacRad = 100;
  const float tictacHeight = 500;
  const float tictacMid = tictacHeight - 2 * tictacRad;
  const std::vector<int> tictacSegs{36,   360,  720,  1000, 2000,
                                    3000, 4000, 5000, 6000};
  for (int seg : tictacSegs) {
    auto sphere = Manifold::Sphere(tictacRad, seg);
    auto spheres = sphere + sphere.Translate({0, 0, tictacMid});
    auto t0 = std::chrono::system_clock::now();
    auto tictac = spheres.Hull();
    double ms = std::chrono::duration_cast<std::chrono::milliseconds>(
                    std::chrono::system_clock::now() - t0)
                    .count();
    std::printf("segs = %i; verts = %i; time = %f\n", seg, spheres.NumVert(),
                ms);
  }
}
segs = 36; verts = 652; time = 9.000000
segs = 360; verts = 64804; time = 154.000000
segs = 720; verts = 259204; time = 696.000000
segs = 1000; verts = 500004; time = 1453.000000
segs = 2000; verts = 2000004; time = 6134.000000
segs = 3000; verts = 4500004; time = 14119.000000
segs = 4000; verts = 8000004; time = 26975.000000
segs = 5000; verts = 12500004; time = 48744.000000
segs = 6000; verts = 18000004; time = 77166.000000

akuukka_quickhull_bench

Copy link
Owner

@elalish elalish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this library you found, thanks!

src/manifold/src/manifold.cpp Outdated Show resolved Hide resolved
src/manifold/src/manifold.cpp Show resolved Hide resolved

Manifold Manifold::Hull() const {
auto mesh = GetMeshGL();
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is to get 1:1 properties to positions? Of course that means we'll get duplicate positions with different properties, so it's going to be basically random which properties end up in the output. Also, it'll be hard to make sense of them without an originalID, since some of those properties might mean different things. Actually it's worse that than: you could end up with a face interpolating UV coordinates into vertex colors or something, since there's no guarantee the verts are from the same input object.

I wonder if we should just remove properties from the Hull entirely and do just geometry for this reason. I can't think of a way to make it sensical otherwise.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't really sure myself since I don't know much about theesh properties business. I just did it this way to try and preserve properties as you said, hoping that you or pca could point out what more I should be preserving (or not). I did consider triangles ending up with weirdly disparate properties as a possibility, but I don't know/understand what is done with properties during booleans either.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The nice thing about Booleans is we can guarantee a bunch of things: every result triangle is a subset of exactly one input triangle, so it will only every interpolate properties from that triangle. Our decimator is also aware, so it won't merge triangles that don't come from the same original triangle.

Each triangle in a hull is either an input triangle, or a new triangle. I suppose we could try to maintain properties (and faceIDs) for input triangles, but we'd have no choice but to drop them for new triangles. Personally I think it sounds like more trouble than it's worth.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, sounds good 👍. I've stripped out all of the properties stuff and just get the vertices via GetMesh.

src/manifold/src/manifold.cpp Outdated Show resolved Hide resolved
src/third_party/CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Owner

@elalish elalish left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

src/manifold/src/manifold.cpp Outdated Show resolved Hide resolved
@geoffder
Copy link
Collaborator Author

I'll follow up soon with some bindings updates (2d and 3d hull).

@geoffder geoffder merged commit 2f22b92 into elalish:master Jul 18, 2023
22 checks passed
@geoffder geoffder deleted the manifold-hull branch July 18, 2023 00:12
@elalish elalish mentioned this pull request Nov 3, 2023
cartesian-theatrics pushed a commit to SovereignShop/manifold that referenced this pull request Mar 11, 2024
Add convex hull methods to Manifold using newly vendored https://github.com/akuukka/quickhull
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants