Fix intersection behavior in CrossSection::BatchBoolean #1247
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1247 +/- ##
==========================================
- Coverage 92.17% 92.07% -0.10%
==========================================
Files 31 31
Lines 5888 5895 +7
==========================================
+ Hits 5427 5428 +1
- Misses 461 467 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Yes, I think you are right. const result = CrossSection.intersection(
[
CrossSection.square(100)
,CrossSection.circle(30,30).translate(-10,30)
,CrossSection.circle(20,30).translate(110,20)
,CrossSection.circle(40,30).translate(50,110)
]).extrude(1) |
I actually think the correct result for your code should be empty. There's no area covered by all four shapes. The result should be equivalent to: const result = CrossSection.intersection([
CrossSection.square(100),
CrossSection.intersection([
CrossSection.circle(30,30).translate(-10,30),
CrossSection.intersection([
CrossSection.circle(20,30).translate(110,20),
CrossSection.circle(40,30).translate(50,110)
])
])
]).extrude(1)or, expressed in 3D: const result = Manifold.intersection(
[
Manifold.cube([100, 100, 1])
,Manifold.cylinder(1, 30).translate(-10,30)
,Manifold.cylinder(1, 20).translate(110,20)
,Manifold.cylinder(1, 40).translate(50,110)
]) |
|
This code makes a Reuleaux triangle: const size = 10.0
const offset = size / Math.sqrt(3)
const result = CrossSection.intersection([
CrossSection.circle(size)
.translate([offset, 0]),
CrossSection.circle(size)
.translate([offset, 0])
.rotate(120),
CrossSection.circle(size)
.translate([offset, 0])
.rotate(240)
]).extrude(1)ExpectedActual |
|
My bad, the output CrossSection indeed should be empty. |
I'm also not sure it's the best way. As far as I can tell, Clipper2 has no direct way to intersect more than two shapes, so Manifold would need to make multiple intersection calls, one way or another. |
|
Actually Clipper2 can handle multi shapes. But the different is Clipper2 handles the input as a whole set, not process one by one with order as we thought. So manifold just keeps this feature. So I think this is not a bug. Because this apparently can be useful under some circumstances, and no other exposed api can replace this. Rather than throw this feature completely, maybe the CrossSection::BatchBoolean can have another name? Like whole boolean or something? // Process CrossSection as a whole, output three parts
const result = CrossSection.intersection(
[
CrossSection.square(100)
,CrossSection.circle(30,30).translate(-10,30)
,CrossSection.circle(20,30).translate(110,20)
,CrossSection.circle(40,30).translate(50,110)
]).extrude(1)
// Process Manifold one by one, output nothing
const result = Manifold.intersection([
CrossSection.square(100).extrude(1)
,CrossSection.circle(30,30).translate(-10,30).extrude(1)
,CrossSection.circle(20,30).translate(110,20).extrude(1)
,CrossSection.circle(40,30).translate(50,110).extrude(1)
]) |
As far as I can tell, the current behavior is the same as an intersection with a union of the tail. For your example, the proper replacement for the same output is: const result = CrossSection.intersection(
[
CrossSection.square(100),
CrossSection.union([
CrossSection.circle(30,30).translate(-10,30),
CrossSection.circle(20,30).translate(110,20),
CrossSection.circle(40,30).translate(50,110)
])
]).extrude(1) |
|
You are right, I think your current way is the best way to fix the misleading behaviour. Good job! |
CrossSection::BatchBooleanbuilds a big list of clip paths from the tail of thecrossSectionslist and then performs the boolean operation with the head. This works fine for union and difference, but when intersecting more than two cross sections, it produces incorrect results.This PR adds a special case for
OpType::IntersectwhereC2::BooleanOpis instead called repeatedly for every part of the tail.I'm not a C++ guy, so take the code with a scoop of salt.