avoid recursion in csg tree #1086

pca006132 · 2024-12-01T16:01:07Z

Fixes #989.

This refactors the code into a single loop that does dfs on the csg tree, and perform flattening on-the-fly.

There are still some failures, I guess this is due to shared nodes being evaluated in a weird way... Will try to fix it soon. But for now I think the logic is mostly there and should be ready for review.

For comparison:

// before
n = 20
nTri = 91814, time = 2.28608 sec
n = 30
nTri = 279334, time = 20.6878 sec

// after
n = 20
nTri = 91814, time = 1.18475 sec
n = 30
nTri = 279334, time = 5.67487 sec

elalish · 2024-12-01T17:05:15Z

src/csg_tree.cpp

                ->GetImpl());
      } else {
        std::vector<std::shared_ptr<CsgLeafNode>> tmp;
        for (size_t j : set) {
          tmp.push_back(
-              std::dynamic_pointer_cast<CsgLeafNode>(children_[start + j]));
+              std::dynamic_pointer_cast<CsgLeafNode>(children[start + j]));


why is this cast dynamic while the one above is static?

ah, I should change these to static.

elalish · 2024-12-01T17:09:18Z

src/csg_tree.cpp

+  bool finalize;
+  OpType parent_op;
+  mat3x4 transform;
+  std::vector<std::shared_ptr<CsgLeafNode>> *parent_children;


what's the relationship between parent_children and left_children and right_children? And why is just the parent a pointer?

parent_children is the list of children of the parent (or ancestor), while left_children and right_children are the sets of children of the current node.

Will explain it better with some comment.

I think I might call parent_children siblings, if I'm understanding you correctly?

Probably, the issue is that it is not exactly sibling as they may be in different levels... I need to think about a better naming.

elalish · 2024-12-01T17:17:26Z

src/csg_tree.cpp

+        op_node(op_node) {}
+};
+
+std::shared_ptr<CsgLeafNode> CsgOpNode::ToLeafNode() const {


I never fully understood what the previous version of this algorithm did either - perhaps some comments on the general steps this is recursing through?

elalish

Thanks! Looks like a good direction, though a bit out of my wheelhouse. I'll be curious to understand where the bug is...

pca006132 · 2024-12-02T14:05:43Z

CondensedMatter16 hits #970 after this PR. I guess this is due to the code now performs optimization more aggressively, and there are more things that got feed into BatchUnion...

codecov · 2024-12-02T14:11:22Z

Codecov Report

Attention: Patch coverage is 90.62500% with 9 lines in your changes missing coverage. Please review.

Project coverage is 91.65%. Comparing base (11387fb) to head (c99eb63).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
src/csg_tree.cpp	90.62%	9 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1086      +/-   ##
==========================================
- Coverage   91.72%   91.65%   -0.07%     
==========================================
  Files          30       30              
  Lines        5910     5897      -13     
==========================================
- Hits         5421     5405      -16     
- Misses        489      492       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

pca006132 · 2024-12-02T14:57:07Z

This slows down the overall test slightly, from 26180ms to 26877ms, ~2.5%. This is due to CondensedMatter64 being slower, which is due to BatchUnioning a lot of meshes (1021) being slower than unioning them in batches...

If I tune the BatchUnion threshold from 1000 to 100, it is now taking 25641ms, ~2% faster. Not sure if this is general enough though.

pca006132 · 2024-12-02T15:09:23Z

Will add documentation tmr, a bit tired today.

pca006132 · 2024-12-02T17:15:33Z

Anyway, decided to write the documentation now :)

About the original bug: The idea is that the flattening takes O(n) time in the old implementation, which makes it problematic because in the worst case we can have O(n) flattening operations as well, which gives us O(n^2) time. As an example, consider the following:

Union(a, Union(b, Union(c, Union(d)))
=> Union(a, Union(b, Union(c, d)))       // 1 copy
=> Union(a, Union(b, c, d))              // 2 copy
=> Union(a, b, c, d)                     // 3 copy

The implementation here doesn't attempt to do flattening when we construct the node. In fact, it is impossible to avoid this quadratic behavior if we perform (all) flattening when we construct the nodes, because there will be O(n^2) nodes if we do that. The algorithm only does one pass over the tree, and do flattening on-the-fly. Each node is moved only once. This makes it O(n).

elalish · 2024-12-02T22:44:25Z

src/csg_tree.cpp

+  // `Subtract` is handled differently from `Add` and `Intersect`. It is treated
+  // as two `Add` nodes, `left_children` and `right_children`, that should be
+  // subtracted later. This allows flattening children `Add` nodes. For normal
+  // `Add` and `Intersect`, we only use `left_children`.


Ah! Could these be named positive_children and negative_children? Or children and inverted_children?

elalish · 2024-12-02T22:52:34Z

src/csg_tree.cpp

+    }
+    auto impl = frame->op_node->impl_.GetGuard();
+    if (frame->finalize) {
+      if (frame->op_node->cache_ == nullptr) switch (frame->op_node->op_) {


can cache_ be anything but nullptr here?

Yeah, I forgot to remove this check after adding the check above.

elalish · 2024-12-02T22:57:30Z

src/csg_tree.cpp

+            std::vector<std::shared_ptr<const Manifold::Impl>> impls;
+            for (auto &child : frame->left_children)
+              impls.push_back(child->GetImpl());
+            auto result = BatchBoolean(OpType::Intersect, impls);


Why is batch intersection so different than batch union?

elalish · 2024-12-02T23:04:48Z

src/csg_tree.cpp

+              node->Transform(transform)));
+        else
+          stack.push_back(std::make_shared<CsgStackFrame>(
+              false, op, transform, children,


I don't quite follow this logic: either we add the node to the set of children, or we add a frame to the stack that contains this node and all the children? Do the children belong to this node or not (when the function is called)?

elalish · 2024-12-02T23:08:01Z

src/csg_tree.cpp

+            frame->op_node->cache_->Transform(frame->transform)));
+      stack.pop_back();
+    } else {
+      auto add_children = [&](std::shared_ptr<CsgNode> &node, OpType op,


I prefer to be explicit about what's captured in a lambda - it helps me to understand the scope of the function.

elalish · 2024-12-02T23:12:09Z

src/csg_tree.cpp

+                            frame->op_node.use_count() <= 2 &&
+                            frame->op_node->impl_.UseCount() == 1;
+        if (skipFinalize) stack.pop_back();
+        auto transform =


const? And for short types, I prefer to avoid auto.

elalish · 2024-12-02T23:13:49Z

src/csg_tree.cpp

+                            frame->op_node->op_ == frame->parent_op &&
+                            frame->op_node.use_count() <= 2 &&
+                            frame->op_node->impl_.UseCount() == 1;
+        if (skipFinalize) stack.pop_back();


We just set frame->finalize = true; above - is that still true? Or does this skipping refer to its child or parent?

The frame is removed here, so we will skip the current frame. Anyway, I can make it explicit.

pca006132 added 3 commits December 1, 2024 23:38

csg_tree.cpp: avoid recursion

03b4394

fix cache transform

a23619c

fix compiler error

73ce437

pca006132 requested a review from elalish December 1, 2024 16:01

elalish reviewed Dec 1, 2024

View reviewed changes

pca006132 added 3 commits December 2, 2024 21:59

cleanup

3cd5421

fixed simple bug

2ca3180

disable flaky CondensedMatter16

d07bf04

disable properly

cbca524

documentation and cleanup

c99eb63

elalish reviewed Dec 2, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

avoid recursion in csg tree #1086

avoid recursion in csg tree #1086

pca006132 commented Dec 1, 2024

elalish Dec 1, 2024

pca006132 Dec 2, 2024

elalish Dec 1, 2024

pca006132 Dec 2, 2024

elalish Dec 2, 2024

pca006132 Dec 2, 2024

elalish Dec 1, 2024

elalish left a comment

pca006132 commented Dec 2, 2024

codecov bot commented Dec 2, 2024 •

edited

Loading

pca006132 commented Dec 2, 2024

pca006132 commented Dec 2, 2024

pca006132 commented Dec 2, 2024

elalish Dec 2, 2024

pca006132 Dec 3, 2024

elalish Dec 2, 2024

pca006132 Dec 3, 2024

elalish Dec 2, 2024

elalish Dec 2, 2024

elalish Dec 2, 2024

elalish Dec 2, 2024

elalish Dec 2, 2024

pca006132 Dec 3, 2024

avoid recursion in csg tree #1086

Are you sure you want to change the base?

avoid recursion in csg tree #1086

Conversation

pca006132 commented Dec 1, 2024

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

elalish left a comment

Choose a reason for hiding this comment

pca006132 commented Dec 2, 2024

codecov bot commented Dec 2, 2024 • edited Loading

Codecov Report

pca006132 commented Dec 2, 2024

pca006132 commented Dec 2, 2024

pca006132 commented Dec 2, 2024

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

codecov bot commented Dec 2, 2024 •

edited

Loading