Lazy boolean #131

pca006132 · 2022-06-04T11:47:02Z

Implemented lazy boolean operation as mentioned in #114.

All manifold APIs are now immutable. Though we may want to discuss this about some APIs such as SetAsOriginal, and warp, as they need to copy the underlying Impl.
Copying manifolds and transforms are essentially free now. Manifolds that only differ in transform will now share the underlying impl until the transform has to be applied.
Boolean operations on a (large) set of objects should generally be faster now.

However, there are 3 test failures that I have no idea about how to fix.
1.~~Boolean.Precision~~ Fixed.
2. Samples.Bracelet triggered triangulation failure. Probably related to #102.
3. Samples.Sponge4 reported a bunch of Tri ... does not match normal and with a large number of NumDegenerateTris. ~~Might be related to my changes to the compose function but I am not sure about this.~~

Also will have to fix some of the outdated documentation. Just submit it earlier for review as this patch is rather complicated.

pca006132 · 2022-06-04T11:59:22Z

maybe we should disable running actions on draft PRs as well, it is expected to fail anyway

elalish

Do you have a sample that demonstrates the improved performance here? I'm curious how much difference it makes.

elalish · 2022-06-04T15:41:06Z

manifold/src/csg_tree.h

+  glm::mat4x3 GetTransform() const override;
+
+  static Manifold::Impl Compose(
+      const std::vector<std::shared_ptr<CsgLeafNode>> &nodes);


I wonder if Compose should be another Op type (OpenSCAD calls it a lazy union, not to be confused with your lazy boolean) rather than a unique function? I don't actually have a strong opinion on this, just curious what you think.

I'm fine with this, but I can't do any reordering to this because it seems that decompose depends on this and would not work if the operation is replaced with union for example.

I don't think we need to worry about that. Technically I don't guarantee anything for input that's self-overlapping anyway, so I don't really consider a Compose of intersecting objects to be valid. Ideally I'll make a function to remove self-overlaps eventually, but that's non-trivial.

elalish · 2022-06-04T15:46:22Z

manifold/src/impl.cpp

-  precision_ *= glm::max(1.0f, newScale / oldScale) *
-                glm::max(glm::length(transform_[0]),
-                         glm::max(glm::length(transform_[1]),
-                                  glm::length(transform_[2])));


Hmm, I was going to ask why you removed these lines, but now I see this looks like a bug of mine. I should have used the transform size before I reset it.

I think this is somehow causing the Precision test to fail. That's a real failure; it's testing that objects below the floating-point precision limit get collapsed to nothing. This could easily be causing the other failures on the big tests too.

Yes, I think I tried using the transform size before resetting it but got some errors, I will try to change this and see how it goes.

I found an error in my patch: forgot to use result.precision_ in result.SetPrecision.

Adding these lines back triggered a few more errors (Boolean.Precision2, Manifold.Precision, Samples.TetPuzzle).

Manifold::Impl Manifold::Impl::Transform(const glm::mat4x3 &transform_) const { if (transform_ == glm::mat4x3(1.0f)) return *this; auto policy = autoPolicy(NumVert()); @@ -589,13 +558,15 @@ Manifold::Impl Manifold::Impl::Transform(const glm::mat4x3 &transform_) const { result.CalculateBBox(); const float newScale = result.bBox_.Scale(); - result.precision_ *= glm::max(1.0f, newScale / oldScale); + result.precision_ *= glm::max(1.0f, newScale / oldScale) * + glm::max(glm::length(transform_[0]), + glm::max(glm::length(transform_[1]), + glm::length(transform_[2]))); // Maximum of inherited precision loss and translational precision loss. - result.SetPrecision(precision_); + result.SetPrecision(result.precision_); return result; }

Not sure how clear my code is here, but the idea is that for geometric intersections, floating point precision doesn't make a lot of sense. Fixed point would be better, but that's not how modern chips are made, so I just keep track of a fixed precision based on worst-case rounding errors. I use this as my tolerance for collapsing edges, since I can't trust geometry smaller than this tolerance. Looking at the old version of this function, I have the feeling I had a bug already. Would be great to have another set of eyes on this.

newScale ~ oldScale * transform_. It seems to me that you are multiplying the factor twice here. Maybe you should multiply precision by just newScale/oldScale or max length?

Perhaps so; I think the transform size was always one before since it was getting reset, so maybe that's how it effectively was.

manifold/src/impl.cpp

elalish · 2022-06-04T15:56:51Z

manifold/src/manifold.cpp


 /**
 * This returns a Mesh of simple vectors of vertices and triangles suitable for
 * saving or other operations outside of the context of this library.
 */
 Mesh Manifold::GetMesh() const {
-  pImpl_->ApplyTransform();
+  const Impl& impl = *GetCsgLeafNode().GetImpl();


Is this where the Boolean operations are actually triggered? What if, say, we union/difference a few things together into a sub-module, then union a bunch of those submodules to make the final output? Will those submodule nodes (portions of the tree) know they are related and only evaluate the submodule once? Or will it happen over and over?

Ah, I see the cache_ now in the OpNode, which sounds like it answers my question. Looks good!

Actually it depends on the tree. For example, consider A + (B + C + D) and A' + (B + C + D), in this case B + C + D will be evaluated 2 times because it will try to use compose to perform A + B + C + D and A' + B + C + D. Whether this is faster or combining B + C + D -> TMP and do A + TMP and A' + TMP is faster depends on the bounding box and the size of the manifolds.

I think it should be possible to tune this to make it combine the operands when the number of objects or the complexity (measured by number of vertices for example) exceeded a certain threshold. But this would be complicated, depends on the test case and build configuration. If users are really concerned about this, they can use some methods that depends on the Impl object to force the CSG tree to evaluate and cache the result.

Would that be GetMesh? It kind of reminds me of OpenSCAD's render method, which really meant cache here. I wonder if an explicit method like that would be helpful? No rush, but something to ponder.

manifold/src/manifold.cpp

samples/src/bracelet.cpp

elalish · 2022-06-04T16:54:52Z

samples/src/bracelet.cpp

@@ -52,7 +52,7 @@ Manifold Base(float width, float radius, float decorRadius, float twistRadius,
  }

  base = Manifold::Extrude(stretch, width) ^ base;
-  base.SetAsOriginal();
+  base = base.SetAsOriginal().second;


I wonder if this function should have a different name now that it's const. Maybe just AsOriginal?

Yes it sounds better

elalish · 2022-06-04T16:58:00Z

samples/src/menger_sponge.cpp

-  result.SetAsOriginal();
+  hole = hole.Rotate(90);
+  result -= hole;
+  hole = hole.Rotate(0, 0, 90);


It's not actually important to me that these rotations are sequential. We can probably put it back to how it was and it should still work, though we should probably wait until we've debugged this test first.

pca006132 · 2022-06-05T01:57:21Z

For the performance improvement example, you can try the python tests. The idea is that the more the objects (without using compose), the larger the performance difference.

Previously (CPP backend):

Took 63.3ms for bricks
Took 223.4ms for cube_with_dents

Lazy boolean (CPP backend):

Exported model to bricks.glb
Took 36.2ms for bricks
Exported model to cube_with_dents.glb
Took 62.1ms for cube_with_dents

(I need to export the models or they won't be actually evaluated)

Update: By flattening the tree on the fly, I managed to further strip ~10ms for these test cases

Exported model to bricks.glb
Took 29.4ms for bricks
Exported model to cube_with_dents.glb
Took 49.7ms for cube_with_dents

pca006132 · 2022-06-05T06:29:16Z

OK I know the reason why Boolean.Precision is failing: The meshes are disjoint, so the union is converted to compose which does not track precision correctly. However, setting precision alone won't fix the issue as the vertices are still there. (Note: Decompose also does not handle precision properly)

Fixed via running SimplifyTopology before calling Finish in Compose.

This allows lazy-boolean to reuse Imp for meshes with different transformations.

so it becomes not noticeable when running in precommit hook

pca006132 · 2022-06-05T15:51:24Z

Wondering if we can incrementally construct the collider. That way the performance of Union can be improved by not having to evaluate O(n^2) overlaps for checking disjoint objects.

elalish · 2022-06-05T15:56:24Z

Is this still a draft?

elalish · 2022-06-05T16:01:34Z

Not quite sure what you mean by incrementally construct the collider; it involves sorting, so I'm guessing that'll be tricky. For disjointness, perhaps just check for bounding box overlap? If no intersections are detected in the boolean, it ought to reduce to being pretty close to Compose anyway; it's possible there are some optimizations left there.

pca006132 · 2022-06-05T16:06:59Z

Probably not a draft now, but still not mergeable until the test failures are fixed I guess.

For union, yes I'm doing overlap check for now (https://github.com/elalish/manifold/pull/131/files#diff-3c6caf3e903fbd0bbc1b99ece9f47113782f7493fb54ba7cfca5eafc005c35c2R344). Just wondering if I can replace the vector of boxes with collider for potentially faster overlap detection, but the current implementation should already be pretty fast right now.

elalish · 2022-06-05T16:12:59Z

Oh, yeah, that's a good point. The collider is intended to be a fairly generic BVH implementation, so yes, you can probably construct a higher-level one for whole meshes instead of triangles.

pca006132 · 2022-06-05T16:47:03Z

But it seems that there is no method to append new boxes to the collider. Maybe I will have a look at it later to see how to implement that, this PR is already complicated enough I guess.

pca006132 · 2022-06-05T23:36:32Z

Interestingly #137 did not fix the test failures here, but introduced another failure for Samples.FrameReduced. I guess this lazy boolean operation is really good at doing permutation and give us some weird combinations.

elalish · 2022-06-06T15:33:47Z

Let's go ahead and mark this as not a draft so I can see the test failures more easily.

pca006132 · 2022-06-06T15:36:41Z

Not sure how can I trigger the checks without committing anything

pca006132 · 2022-06-10T16:15:14Z

Just got some time to have a look at this. Calling SimplifyTopology() in Manifold::AsOriginal() and adding

diff --git a/manifold/src/csg_tree.cpp b/manifold/src/csg_tree.cpp
index f767d30..e70bd5c 100644
--- a/manifold/src/csg_tree.cpp
+++ b/manifold/src/csg_tree.cpp
@@ -324,8 +324,14 @@ void CsgOpNode::BatchBoolean(
     results.pop_back();
     // boolean operation
     Boolean3 boolean(*a, *b, operation);
-    results.push_back(
-        std::make_shared<const Manifold::Impl>(boolean.Result(operation)));
+    auto result = std::make_shared<Manifold::Impl>(
+        boolean.Result(operation));
+    ALWAYS_ASSERT(result->IsManifold(), topologyErr, "batch boolean produced "
+                                                     "non-manifold result");
+    result->InitializeNewReference();
+    result->SimplifyTopology();
+
+    results.push_back(result);
     std::push_heap(results.begin(), results.end(), cmpFn);
   }
 }

fixes the test failure for Samples.FrameReduced but causes other tests to fail with

[ RUN      ] Samples.TetPuzzle
/home/pca006132/code/manifold/test/samples_test.cpp:128: Failure
Expected: (puzzle.NumDegenerateTris()) <= (2), actual: 8 vs 2
numEdge: 3201 halfedge: 6420
unknown file: Failure
C++ exception with description "Error in file: /home/pca006132/code/manifold/manifold/src/shared.h (164): 'numEdge == halfedge.size() / 2' is false: Not oriented!" thrown in the test body.
[  FAILED  ] Samples.TetPuzzle (12 ms)
[ RUN      ] Samples.FrameReduced
[       OK ] Samples.FrameReduced (7 ms)
[ RUN      ] Samples.Frame
numEdge: 1656 halfedge: 3318
unknown file: Failure
C++ exception with description "Error in file: /home/pca006132/code/manifold/manifold/src/shared.h (164): 'numEdge == halfedge.size() / 2' is false: Not oriented!" thrown in the test body.
[  FAILED  ] Samples.Frame (20 ms)
[ RUN      ] Samples.Bracelet
numEdge: 16725 halfedge: 33894
unknown file: Failure
C++ exception with description "Error in file: /home/pca006132/code/manifold/manifold/src/shared.h (164): 'numEdge == halfedge.size() / 2' is false: Not oriented!" thrown in the test body.
[  FAILED  ] Samples.Bracelet (62 ms)
[ RUN      ] Samples.Sponge1
numEdge: 90 halfedge: 192
unknown file: Failure
C++ exception with description "Error in file: /home/pca006132/code/manifold/manifold/src/shared.h (164): 'numEdge == halfedge.size() / 2' is false: Not oriented!" thrown in the test body.
[  FAILED  ] Samples.Sponge1 (1 ms)
[ RUN      ] Samples.Sponge4

Although I shouldn't call InitializeNewReference and SimplifyTopology for BatchBoolean, I think it should not produce such a result? I guess this suggests that there is something wrong in SimplifyTopology.

elalish · 2022-06-10T16:56:09Z

I think this PR might just be a little too big. It's using Compose quite heavily, which hasn't been tested as thoroughly as other functions. How about we split this and remove the Boolean reordering optimizations and put those in the next PR. This PR will be about updating the API and adding the CSG tree, while leaving the operations all in the same order, which should make it easier to test. Then we can work on reordering a bit at a time and figure out where the bugs are. How does that sound?

pca006132 · 2022-06-10T17:02:13Z

Sure, it would be nice to get the APIs updated first.

pca006132 · 2022-06-11T14:04:41Z

Now the csg tree will not perform any reordering, so the performance may not be great. I tried disabling compose only and the tests still failed, so using fuzzing to reproduce the errors should not be too hard.

I just added a commit to disable reordering in csg tree, which allows us to enable it easily later. Let me know if you want me to do a rebase instead for a cleaner history.

elalish

Looks great, thanks! Just a couple of nits.

elalish · 2022-06-12T05:18:20Z

manifold/src/csg_tree.h

+  glm::mat4x3 GetTransform() const override;
+
+  static Manifold::Impl Compose(
+      const std::vector<std::shared_ptr<CsgLeafNode>> &nodes);


I don't think we need to worry about that. Technically I don't guarantee anything for input that's self-overlapping anyway, so I don't really consider a Compose of intersecting objects to be valid. Ideally I'll make a function to remove self-overlaps eventually, but that's non-trivial.

elalish · 2022-06-12T05:22:45Z

manifold/src/manifold.cpp


 /**
 * This returns a Mesh of simple vectors of vertices and triangles suitable for
 * saving or other operations outside of the context of this library.
 */
 Mesh Manifold::GetMesh() const {
-  pImpl_->ApplyTransform();
+  const Impl& impl = *GetCsgLeafNode().GetImpl();


Would that be GetMesh? It kind of reminds me of OpenSCAD's render method, which really meant cache here. I wonder if an explicit method like that would be helpful? No rush, but something to ponder.

manifold/src/manifold.cpp

manifold/src/csg_tree.h

elalish · 2022-06-12T05:40:58Z

manifold/src/csg_tree.cpp

+/**
+ * Flatten the children to a list of leaf nodes and return them.
+ * If finalize is true, the list will be guaranteed to be a list of leaf nodes
+ * (i.e. no ops). Otherwise, the list may contain ops.


This seems odd; it says it will flatten the list, but if finalize is not true, then it might not? I try to avoid functions taking boolean inputs if I can, but maybe this is just an issue with the comment?

The idea is that if finalize is false, we will try to flatten the tree if it is not expensive (if we don't have to evaluate the boolean expressions), and we will perform the evaluation if finalize is true. But yes, this is a bit confusing.

Actually this comment is outdated because we don't do any reordering for now, so finalize is kind of useless.

Originally it will call this function with finalize=false when constructing the CSG tree, so unions will be flattened into a large union node with many children, which improves performance a bit. This is not necessary though.

Okay, well let's deal with this in the next PR.

elalish · 2022-06-12T05:43:18Z

manifold/src/csg_tree.cpp

+ * Efficient union operation on a set of nodes by doing Compose as much as
+ * possible.
+ */
+void CsgOpNode::BatchUnion() const {


Okay, so these functions are still here, but just currently unused? Sounds good for now.

pca006132 · 2022-06-12T13:09:17Z

~~Note that if SimplifyTopology is called after InitializeNewReference in Manifold::AsOriginal, it will fail the tests with~~

manifold/src/shared.h (162): 'numEdge == halfedge.size() / 2' is false: Not oriented!" thrown in the test body.

Edit: It turns out I just forgot to call Finish.

pca006132 · 2022-06-12T14:35:55Z

FYI: with reordering enabled and not using compose for union, the only test that fails is the Samples.Sponge4 test:

/home/pca006132/code/manifold/test/samples_test.cpp:25: Failure
Value of: manifold.MatchesTriNormals()
  Actual: false
Expected: true
/home/pca006132/code/manifold/test/samples_test.cpp:183: Failure
Expected equality of these values:
  sponge.NumDegenerateTris()
    Which is: 24259
  0

The number of tri that does not match normal depends on the backend, 1 for CPP backend and many for TBB backend. The generated mesh looks pretty good to my untrained eyes, but it seems that there are a lot of sliver triangles there:

elalish · 2022-06-13T04:48:22Z

The reordering thing with Sponge4 is interesting; can you repro that without the change, but simply by manually switching the order? There's only 3 ops after all.

Lazy boolean

elalish reviewed Jun 4, 2022

View reviewed changes

pca006132 added 3 commits June 5, 2022 21:56

moved transform_ from Impl to Manifold

4a21b58

This allows lazy-boolean to reuse Imp for meshes with different transformations.

lazy boolean implementation

a40fe42

every manifold operation now returns a new manifold object

817e34b

pca006132 force-pushed the lazy-boolean branch from 46376d5 to fb9cfe9 Compare June 5, 2022 14:02

pca006132 added 5 commits June 5, 2022 22:05

test updates

9c55fb9

disabled CI for draft PRs

c7de234

removed ApplyTransform

16b1183

fixed precision bug for compose/decompose

573adb6

fixed perf_test error

2b601ea

pca006132 force-pushed the lazy-boolean branch from fb9cfe9 to 2b601ea Compare June 5, 2022 14:05

flatten the tree on the fly if possible

b36d08b

pca006132 force-pushed the lazy-boolean branch from ecfdcd6 to b36d08b Compare June 5, 2022 14:17

pca006132 added 2 commits June 5, 2022 22:25

renamed SetAsOriginal to AsOriginal

5bf95d6

faster clang-format check

3baf39c

so it becomes not noticeable when running in precommit hook

Merge remote-tracking branch 'upstream/master' into lazy-boolean

c07f2b4

pca006132 marked this pull request as ready for review June 6, 2022 15:34

added license to csg_tree.cpp

ecb6b97

csg_tree: disabled reordering

ecd9329

pca006132 added 2 commits June 11, 2022 22:23

fix cmake error

e115f5e

fixed compilation errors

90a4b22

elalish requested changes Jun 12, 2022

View reviewed changes

cleanup

8197398

elalish merged commit 999a035 into elalish:master Jun 13, 2022

pca006132 mentioned this pull request Jun 17, 2022

RecursiveEdgeSwap invalid memory access #139

Closed

elalish mentioned this pull request Aug 14, 2022

re-enabled reordering #171

Merged

pca006132 mentioned this pull request Sep 25, 2022

use compose for disjoint union #217

Merged

pca006132 deleted the lazy-boolean branch December 22, 2022 05:35

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

Merge pull request elalish#131 from pca006132/lazy-boolean

b8b29ed

Lazy boolean

Lazy boolean #131

Lazy boolean #131

Conversation

pca006132 commented Jun 4, 2022 • edited

pca006132 commented Jun 4, 2022

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pca006132 commented Jun 5, 2022 • edited

pca006132 commented Jun 5, 2022 • edited

pca006132 commented Jun 5, 2022

elalish commented Jun 5, 2022

elalish commented Jun 5, 2022

pca006132 commented Jun 5, 2022

elalish commented Jun 5, 2022

pca006132 commented Jun 5, 2022

pca006132 commented Jun 5, 2022

elalish commented Jun 6, 2022

pca006132 commented Jun 6, 2022

pca006132 commented Jun 10, 2022

elalish commented Jun 10, 2022

pca006132 commented Jun 10, 2022

pca006132 commented Jun 11, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pca006132 commented Jun 12, 2022 • edited

pca006132 commented Jun 12, 2022 • edited

elalish commented Jun 13, 2022

pca006132 commented Jun 4, 2022 •

edited

pca006132 commented Jun 5, 2022 •

edited

pca006132 commented Jun 5, 2022 •

edited

pca006132 commented Jun 12, 2022 •

edited

pca006132 commented Jun 12, 2022 •

edited