fixed csg_tree bugs (#226)

1. Due to the shared cache between op nodes, the children list can have only 1 child and the original code will access a dangling pointer. 2. Users may supply empty/singleton lists to BatchBoolean, which CsgOpNode cannot handle the empty case correctly. The new test is for bug 1.
elalish · Oct 4, 2022 · add3a04 · add3a04
1 parent a860854
commit add3a04
Show file tree

Hide file tree

Showing 3 changed files with 46 additions and 32 deletions.
diff --git a/src/manifold/src/csg_tree.cpp b/src/manifold/src/csg_tree.cpp
@@ -256,38 +256,40 @@ std::shared_ptr<CsgLeafNode> CsgOpNode::ToLeafNode() const {
   // turn the children into leaf nodes
   GetChildren();
   auto &children_ = impl_->children_;
-  switch (impl_->op_) {
-    case CsgNodeType::UNION:
-      BatchUnion();
-      break;
-    case CsgNodeType::INTERSECTION: {
-      std::vector<std::shared_ptr<const Manifold::Impl>> impls;
-      for (auto &child : children_) {
-        impls.push_back(
-            std::dynamic_pointer_cast<CsgLeafNode>(child)->GetImpl());
-      }
-      BatchBoolean(Manifold::OpType::INTERSECT, impls);
-      children_.clear();
-      children_.push_back(std::make_shared<CsgLeafNode>(impls.front()));
-      break;
-    };
-    case CsgNodeType::DIFFERENCE: {
-      // take the lhs out and treat the remaining nodes as the rhs, perform
-      // union optimization for them
-      auto lhs = std::dynamic_pointer_cast<CsgLeafNode>(children_.front());
-      children_.erase(children_.begin());
-      BatchUnion();
-      auto rhs = std::dynamic_pointer_cast<CsgLeafNode>(children_.front());
-      children_.clear();
-      Boolean3 boolean(*lhs->GetImpl(), *rhs->GetImpl(),
-                       Manifold::OpType::SUBTRACT);
-      children_.push_back(
-          std::make_shared<CsgLeafNode>(std::make_shared<Manifold::Impl>(
-              boolean.Result(Manifold::OpType::SUBTRACT))));
-    };
-    case CsgNodeType::LEAF:
-      // unreachable
-      break;
+  if (children_.size() > 1) {
+    switch (impl_->op_) {
+      case CsgNodeType::UNION:
+        BatchUnion();
+        break;
+      case CsgNodeType::INTERSECTION: {
+        std::vector<std::shared_ptr<const Manifold::Impl>> impls;
+        for (auto &child : children_) {
+          impls.push_back(
+              std::dynamic_pointer_cast<CsgLeafNode>(child)->GetImpl());
+        }
+        BatchBoolean(Manifold::OpType::INTERSECT, impls);
+        children_.clear();
+        children_.push_back(std::make_shared<CsgLeafNode>(impls.front()));
+        break;
+      };
+      case CsgNodeType::DIFFERENCE: {
+        // take the lhs out and treat the remaining nodes as the rhs, perform
+        // union optimization for them
+        auto lhs = std::dynamic_pointer_cast<CsgLeafNode>(children_.front());
+        children_.erase(children_.begin());
+        BatchUnion();
+        auto rhs = std::dynamic_pointer_cast<CsgLeafNode>(children_.front());
+        children_.clear();
+        Boolean3 boolean(*lhs->GetImpl(), *rhs->GetImpl(),
+                         Manifold::OpType::SUBTRACT);
+        children_.push_back(
+            std::make_shared<CsgLeafNode>(std::make_shared<Manifold::Impl>(
+                boolean.Result(Manifold::OpType::SUBTRACT))));
+      };
+      case CsgNodeType::LEAF:
+        // unreachable
+        break;
+    }
   }
   // children_ must contain only one CsgLeafNode now, and its Transform will
   // give CsgLeafNode as well

diff --git a/src/manifold/src/manifold.cpp b/src/manifold/src/manifold.cpp
@@ -474,6 +474,10 @@ Manifold Manifold::Boolean(const Manifold& second, OpType op) const {
 
 Manifold Manifold::BatchBoolean(const std::vector<Manifold>& manifolds,
                                 OpType op) {
+  if (manifolds.size() == 0)
+    return Manifold();
+  else if (manifolds.size() == 1)
+    return manifolds[0];
   std::vector<std::shared_ptr<CsgNode>> children;
   children.reserve(manifolds.size());
   for (const auto& m : manifolds) children.push_back(m.pNode_);

diff --git a/test/mesh_test.cpp b/test/mesh_test.cpp
@@ -1058,3 +1058,11 @@ TEST(Boolean, Close) {
 
   PolygonParams().processOverlaps = false;
 }
+
+TEST(Boolean, UnionDifference) {
+  Manifold block = Manifold::Cube({1, 1, 1}, true) - Manifold::Cylinder(1, 0.5);
+  Manifold result = block + block.Translate({0, 0, 1});
+  float resultsize = result.GetProperties().volume;
+  float blocksize = block.GetProperties().volume;
+  EXPECT_NEAR(resultsize, blocksize * 2, 0.0001);
+}