Simplify the CsgOpNodes as we build them, rather than in GetChildren #368

ochafik · 2023-03-14T03:29:08Z

Early simplification (while building boolean op nodes) makes it easier to flatten the tree and avoids... stack overflows in GetChildren.

large_scene_test.cpp in this PR will just segfault without the flattening changes (and could potentially form the basis for some large scene benchmark so I thought it worth to check it in).

Found this crash while testing openscad/openscad#4533 (segfaulted on issue2342.scad)

pca006132

Thanks. I thought that I have already fixed this issue previously but it seems that it is not.

pca006132 · 2023-03-14T03:39:18Z

src/manifold/src/csg_tree.h

@@ -22,13 +22,16 @@ enum class CsgNodeType { Union, Intersection, Difference, Leaf };

 class CsgLeafNode;

-class CsgNode {
+class CsgNode : public std::enable_shared_from_this<CsgNode> {


Thanks! I never know such a thing exists, I wanted to do something similar and have to do it the dumb way.

Yeah it's a nice get out of jail free card 🤣

pca006132 · 2023-03-14T03:41:18Z

src/manifold/src/csg_tree.cpp

+
+  auto handleOperand = [&](const std::shared_ptr<CsgNode> &operand) {
+    if (auto opNode = std::dynamic_pointer_cast<CsgOpNode>(operand)) {
+      if (opNode->IsOp(op) && opNode.use_count() == 1 &&


Maybe we can also have some sort of max recursion level parameter, and force flattening the tree when the recursion level is reached, regardless of whether or not the child is shared? So we can avoid stack overflow in any situation.

So tbh I'm not quite sure in which circumstances the nodes and/or their impl are shared. I've relaxed the test anyway as I didn't realize the operand.use_count()==1 broke the intended fix (there's copies of shared pointers around, and not just because I mistakenly passed it by value in that method - now fixed). Checking that count seems a bit brittle.

I've left the other check (on impl.UseCount()) to keep some mysterious tests happy, though.

Re/ your suggestion to force-flatten in extreme sharing cases, maybe we could explore as follow up? Not sure if geometry from OpenSCAD would trigger that case though.

Sure, I don't think it will happen normally.

codecov · 2023-03-14T03:55:33Z

Codecov Report

Patch coverage: 95.00% and project coverage change: -3.92 ⚠️

Comparison is base (dca9cce) 89.39% compared to head (409dd3f) 85.48%.

❗ Current head 409dd3f differs from pull request most recent head 6fcf61b. Consider uploading reports for the commit 6fcf61b to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #368      +/-   ##
==========================================
- Coverage   89.39%   85.48%   -3.92%     
==========================================
  Files          33       36       +3     
  Lines        3979     4409     +430     
==========================================
+ Hits         3557     3769     +212     
- Misses        422      640     +218

Impacted Files	Coverage Δ
src/manifold/src/csg_tree.h	`75.00% <0.00%> (-25.00%)`	⬇️
src/manifold/src/csg_tree.cpp	`92.30% <97.36%> (+0.67%)`	⬆️
src/manifold/src/manifold.cpp	`92.14% <100.00%> (-0.07%)`	⬇️

... and 16 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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

elalish

@pca006132, you know this code better than I, so I'll leave approval to you.

elalish · 2023-03-14T05:58:59Z

extras/large_scene_test.cpp

+    time ./extras/largeSceneTest 50 )
+*/
+int main(int argc, char **argv) {
+  int n = 20;  // Crashes at n=50


Does it still crash at n=50 with this change, or was that only before?

That was only before, edited this out, thanks!

elalish · 2023-03-14T06:03:34Z

src/manifold/src/csg_tree.cpp

@@ -419,34 +457,16 @@ std::vector<std::shared_ptr<CsgNode>> &CsgOpNode::GetChildren(
    bool finalize) const {
  auto impl = impl_.GetGuard();
  auto &children_ = impl->children_;
-  if (children_.empty() || (impl->simplified_ && !finalize) || impl->flattened_)
-    return children_;
-  impl->simplified_ = true;


Happy to see this simplification, but what was the difference between simplified and flattened?

simplified did not perform flattening if the nodes are shared, iirc.

Flattening (also aliased here to finalization) was forcing leaf nodes, while simplification was flattening 1 level of op nodes nesting (adopting the children of same op children, and in the case of difference, of union op children past the first child).

Have now renamed finalize & flattened_ to force[d]ToLeafNodes[_] to disambiguate, and simplified this method further.

Perfect, thank you!

pca006132 · 2023-03-14T06:10:20Z

This looks fine for me, but can you do a simple benchmark comparing with the current master? Wondering if there is any performance improvement/degradation for our test cases (manifold_test and the python examples, which are extracted from your gist of benchmarks)

…n GetChildren arg naming

…better w/ CsgOpNode::ToLeafNode

ochafik · 2023-03-14T13:19:22Z

src/manifold/src/csg_tree.cpp

+  auto handleOperand = [&](const std::shared_ptr<CsgNode> &operand) {
+    if (auto opNode = std::dynamic_pointer_cast<CsgOpNode>(operand)) {
+      if ((opNode->IsOp(op) ||
+           (op == OpType::Subtract && opNode->IsOp(OpType::Add))) &&


Now doing (a-(b+c) -> (a-b-c) here to play better w/ CsgOpNode::ToLeafNode (which turns it back to (a-batch(b+c))

ochafik · 2023-03-14T16:58:56Z

can you do a simple benchmark comparing with the current master?

@pca006132 performance looks identical for these tests and examples, here's the script I used to benchmark (uses hyperfine)

pca006132 · 2023-03-15T04:20:49Z

I was testing this branch and I found it breaks my existing python code. The output stl is different from the current master. I will try to make a minimal example for it.

pca006132 · 2023-03-15T04:48:09Z

  auto a = Manifold::Cylinder(10, 10);
  auto b = Manifold::Cube({10, 10, 10});
  auto c = Manifold::Cylinder(1, 1);

  float volume1 = (a - c).GetProperties().volume;
  float volume2 = (a + b - c).GetProperties().volume;
  EXPECT_GT(volume2, volume1);

The assertion failed:

Expected: (volume2) > (volume1), actual: 2342.13 vs 3122.84

while this passed with the current master:

3341.423096 > 3122.840088

ochafik · 2023-03-16T03:42:37Z

@pca006132 thanks for the repro! Pushed a fix and some tests based on your snippet

pca006132 · 2023-03-16T09:00:45Z

Interestingly it still break my other code... will try to reproduce it later today.

pca006132

Minimal example that you can add to the test case:

  auto a = (Manifold::Cube({1, 1, 1}) + Manifold::Cube({1, 1, 1})).Translate({1, 0, 0});
  auto b = (Manifold::Cube({1, 1, 1}) + Manifold::Cube({1, 1, 1}));

  EXPECT_FLOAT_EQ((a + b).GetProperties().volume, 2);

Currently this evaluates to 1. Note that if you write it as

  EXPECT_FLOAT_EQ(((Manifold::Cube({1, 1, 1}) + Manifold::Cube({1, 1, 1})).Translate({1, 0, 0}) + (Manifold::Cube({1, 1, 1}) + Manifold::Cube({1, 1, 1}))).GetProperties().volume, 2);

it will not trigger the bug, as the use count is somehow set to 2, perhaps due to how C++ deal with destructors.

pca006132 · 2023-03-16T12:21:26Z

src/manifold/src/csg_tree.cpp

+
+  if (IsOp(op) && (impl_.UseCount() == 1)) {
+    auto impl = impl_.GetGuard();
+    std::copy(impl->children_.begin(), impl->children_.end(),


This copy doesn't apply transform from the parent.

Hadn't realized the transform was around, thanks!

src/manifold/src/csg_tree.cpp

elalish

Nice job working together on this!

elalish · 2023-03-16T15:52:51Z

test/manifold_test.cpp

+  // Define solids which volumes are easy to compute w/ bit arithmetics:
+  // m1, m2, m4 are unique, non intersecting "bits" (of volume 1, 2, 4)
+  // m3 = m1 + m2
+  // m7 = m1 + m2 + m3


Excellent tests, thank you! Technically these should go in boolean_test.cpp now that I've separated them.

Thanks!! Moved them 👌

ochafik · 2023-03-16T17:42:23Z

Thanks a lot guys, appreciate the help & quick turnaround! (aaaaand the amazing library of course!!)

…lalish#368) * Flatten the CsgOpNodes as we build them, rather than in GetChildren * Don't inline reused nodes in CsgNode::Boolean * Reinstate difference -> union rewrite * No need for opportunistic flattening anymore * Fix typo * Remove CsgOpNode::Impl::simplified_ * Don't overskip referenced nodes in simplification * Avoid needless copy of shared pointers * Disambiguate CsgOpNode::Impl::flatten_ -> forcedToLeafNodes_ and align GetChildren arg naming * Simplified CsgOpNode::GetChildren * Simpler (a-(b+c)) -> (a-b-c) transform in CsgOpNode::Boolean to work better w/ CsgOpNode::ToLeafNode * Simpler build hints for large_scene_test.cpp * [flatten] Refine rules about tree flattening + added test case * [manifold] Propagate transforms when flattening op trees * [flatten] move tests from manifold_test to boolean_test

ochafik added 6 commits March 14, 2023 02:56

Flatten the CsgOpNodes as we build them, rather than in GetChildren

a43e337

Don't inline reused nodes in CsgNode::Boolean

f091479

Reinstate difference -> union rewrite

210bb0e

No need for opportunistic flattening anymore

7b488a5

Fix typo

166c603

Remove CsgOpNode::Impl::simplified_

5f91aa9

ochafik changed the title ~~Flatten the CsgOpNodes as we build them, rather than in GetChildren~~ Simplify the CsgOpNodes as we build them, rather than in GetChildren Mar 14, 2023

ochafik mentioned this pull request Mar 14, 2023

Use Manifold for much faster & multithreaded CSG & minkowski operations openscad/openscad#4533

Merged

12 tasks

pca006132 reviewed Mar 14, 2023

View reviewed changes

ochafik added 2 commits March 14, 2023 04:31

Don't overskip referenced nodes in simplification

17a13d6

Avoid needless copy of shared pointers

3bf059c

elalish reviewed Mar 14, 2023

View reviewed changes

ochafik added 3 commits March 14, 2023 12:45

Disambiguate CsgOpNode::Impl::flatten_ -> forcedToLeafNodes_ and alig…

ce85a44

…n GetChildren arg naming

Simplified CsgOpNode::GetChildren

147017f

Simpler (a-(b+c)) -> (a-b-c) transform in CsgOpNode::Boolean to work …

2ef7d4e

…better w/ CsgOpNode::ToLeafNode

ochafik commented Mar 14, 2023

View reviewed changes

Simpler build hints for large_scene_test.cpp

7cb439c

[flatten] Refine rules about tree flattening + added test case

409dd3f

pca006132 requested changes Mar 16, 2023

View reviewed changes

[manifold] Propagate transforms when flattening op trees

1adafe8

pca006132 approved these changes Mar 16, 2023

View reviewed changes

elalish approved these changes Mar 16, 2023

View reviewed changes

[flatten] move tests from manifold_test to boolean_test

6fcf61b

elalish merged commit e17eb03 into elalish:master Mar 16, 2023

ochafik added a commit to ochafik/openscad that referenced this pull request Mar 16, 2023

[manifold] Unskip test now elalish/manifold#368 is merged

f49e8d7

ochafik mentioned this pull request Mar 18, 2023

[DRAFT] Flatten CSG tree to let more lazy unions bubble up to the top openscad/openscad#4561

Draft

elalish mentioned this pull request Nov 3, 2023

V2.2.1 #589

Merged

ochafik mentioned this pull request Jun 18, 2024

Color support for 3D rendering + export to glTF, Collada, STEP, PLY, X3D openscad/openscad#5180

Draft

14 tasks

pca006132 mentioned this pull request Oct 15, 2024

accidental quadratic runtime in csg tree #989

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify the CsgOpNodes as we build them, rather than in GetChildren #368

Simplify the CsgOpNodes as we build them, rather than in GetChildren #368

ochafik commented Mar 14, 2023 •

edited

Loading

pca006132 left a comment

pca006132 Mar 14, 2023

ochafik Mar 14, 2023

pca006132 Mar 14, 2023

ochafik Mar 14, 2023 •

edited

Loading

ochafik Mar 14, 2023

pca006132 Mar 14, 2023

codecov bot commented Mar 14, 2023 •

edited

Loading

elalish left a comment

elalish Mar 14, 2023

ochafik Mar 14, 2023

elalish Mar 14, 2023

pca006132 Mar 14, 2023

ochafik Mar 14, 2023

elalish Mar 14, 2023

pca006132 commented Mar 14, 2023

ochafik Mar 14, 2023 •

edited

Loading

ochafik commented Mar 14, 2023

pca006132 commented Mar 15, 2023

pca006132 commented Mar 15, 2023

ochafik commented Mar 16, 2023

pca006132 commented Mar 16, 2023

pca006132 left a comment

pca006132 Mar 16, 2023

ochafik Mar 16, 2023

elalish left a comment

elalish Mar 16, 2023

ochafik Mar 16, 2023

ochafik commented Mar 16, 2023 •

edited

Loading

Simplify the CsgOpNodes as we build them, rather than in GetChildren #368

Simplify the CsgOpNodes as we build them, rather than in GetChildren #368

Conversation

ochafik commented Mar 14, 2023 • edited Loading

pca006132 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

ochafik Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 14, 2023 • edited Loading

Codecov Report

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

pca006132 commented Mar 14, 2023

ochafik Mar 14, 2023 • edited Loading

Choose a reason for hiding this comment

ochafik commented Mar 14, 2023

pca006132 commented Mar 15, 2023

pca006132 commented Mar 15, 2023

ochafik commented Mar 16, 2023

pca006132 commented Mar 16, 2023

pca006132 left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ochafik commented Mar 16, 2023 • edited Loading

ochafik commented Mar 14, 2023 •

edited

Loading

ochafik Mar 14, 2023 •

edited

Loading

codecov bot commented Mar 14, 2023 •

edited

Loading

ochafik Mar 14, 2023 •

edited

Loading

ochafik commented Mar 16, 2023 •

edited

Loading