fix: Boolean difference silently does nothing on near-coplanar/step cuts (#11410) - #11411
fix: Boolean difference silently does nothing on near-coplanar/step cuts (#11410)#11411BenJule wants to merge 1 commit into
Conversation
7f0962c to
56c65f4
Compare
ArthurBambulab
left a comment
There was a problem hiding this comment.
Please address the cancel-vs-failure ambiguity before merge, and ideally verify the CGAL path on the reporter's model (Manifold offline success ≠ CGAL in-tree).
Must fix
Cancel is conflated with mcut failure (MeshBoolean.cpp, do_boolean A_NOT_B branch)
do_boolean_single() also returns false when cancel_cb() fires mid-loop (around the connected-component traversal). This PR treats every !ok as a geometric mcut failure and runs CGAL fallback, so a user cancel can:
- still run an expensive
cgal::minus, and/or - complete successfully via CGAL, contradicting cancel semantics.
Suggested minimal guard:
bool ok = do_boolean_single(*src_part, *cut_part, boolean_opts, cancel_cb, temp_progress_cb);
if (cancel_cb && cancel_cb())
return false;
if (boolean_opts == "A_NOT_B") {
// existing fallback...
}56c65f4 to
0c972fd
Compare
|
Thanks for catching this. I added an explicit cancel check immediately after I also built the updated commit successfully with the Linux PR pipeline and tested the resulting Bambu Studio artifact against the reporter's original 3MF from #11410. The subtraction now completes successfully and produces the expected deeper cut, confirming the in-tree CGAL fallback on the actual model. I attempted to exercise cancellation manually as well, but this particular operation completes too quickly to reliably hit the Cancel button. I therefore cannot claim a manual cancellation test, but the cancel/failure ambiguity identified in the review is now explicitly guarded in the code. |
ArthurBambulab
left a comment
There was a problem hiding this comment.
Thanks for the update — the cancel guard is exactly what I asked for and looks correct.
Unfortunately I pulled this patchset locally, built it (Windows / VS2022 / Release) and
tested it against the model from #11410, and the operation now fails instead of
applying the cut. The dialog reports "Operation Failed." Log:
MCUT mcDispatch fails! err=-2
MCUT A_NOT_B produced nothing, CGAL fallback failed: CGAL mesh boolean operation failed.
check error:CGAL mesh boolean operation failed.
[Mesh Boolean] Boolean operation returned empty mesh at step 1
So the CGAL fallback is entered as designed, but it throws, the exception is re-thrown,
and failed_cb() turns the whole operation into a hard failure.
Root cause: the fallback engine rejects exactly the input that makes mcut fail
The model self-intersects. The slicing path says so independently:
check_csgmesh_booleans fails! mesh 0/2 does_self_intersect is true, cannot do boolean!
And cgal::minus ends up in _cgal_diff, which hard-codes the self-intersection guard:
static bool _cgal_diff(CGALMesh &A, CGALMesh &B, CGALMesh &R)
{
const auto &p = CGALParams::throw_on_self_intersection(true);
return CGALProc::corefine_and_compute_difference(A.m, B.m, R.m, p, p);
}
CGAL throws Self_intersection_exception, _cgal_do converts it into
RuntimeError("CGAL mesh boolean operation failed."), and the fallback is dead on arrival.
Self-intersecting / degenerate input is the dominant reason mcut fails in the first place,
so the fallback misses its own target case.
Note also that the actual failure point differs from the PR description: mcut does not
return "zero connected components" here — mcDispatch itself returns err=-2
(MC_INVALID_OPERATION). Your code still routes to the fallback, but the stated root
cause is not what happens on this model.
The guard cannot simply be relaxed
I also tried a fallback variant with throw_on_self_intersection(false): instead of
failing, the application crashes inside corefine_and_compute_difference
(STATUS_BREAKPOINT, 0x80000003). That guard is a crash protection, not incidental
strictness — so the fallback's only two outcomes on this input are "Operation Failed" or a
crash. Swapping engines cannot fix #11410.
Conclusion
As it stands this PR does not appear to fix anything: the reported case goes from a silent
no-op to a hard "Operation Failed", and relaxing the guard only turns that into a crash. I
don't think it should be merged in its current form.
I'd suggest stepping back and investigating the actual root cause first — why the input
mesh is self-intersecting/degenerate and how to repair it before the boolean — rather than
routing mcut failures to a second engine that rejects the same input. Happy to re-test once
there's a root-cause-based approach to try.
997aa4e to
7dbd87b
Compare
…ambulab#11410) The reporter's A_NOT_B operation fails in mcDispatch with MC_INVALID_OPERATION on self-intersecting input. do_boolean_single() returns false for that failure, but do_boolean() previously ignored the result for A_NOT_B and merged the unchanged source part back, so the Boolean operation could silently appear to succeed without applying the cut. A direct CGAL PMP difference is not a suitable fallback for this input: the corefinement path intentionally rejects self-intersections, and disabling that protection can crash inside corefine_and_compute_difference(). Keep the existing do_boolean_single() API and use an internal implementation that additionally reports whether mcDispatch itself failed. For A_NOT_B, only that specific failure path performs recovery: - preserve cancellation semantics before entering recovery - detect self-intersections in the source and cutter - normalize only affected meshes with the existing self_union() path - verify that the repaired meshes are no longer self-intersecting - retry the original mcut difference - report a hard failure if repair or the retry does not succeed Other false-return paths are left unchanged, and UNION / INTERSECTION retain their existing behavior. Closes bambulab#11410
7dbd87b to
94c1a01
Compare
|
@ArthurBambulab Thanks for the detailed investigation and for identifying the actual failure mode. I reworked the fix based on your findings. The direct CGAL difference fallback has been removed. The current implementation now only enters recovery when The cancellation guard remains in place, so a user cancellation is not interpreted as a geometry failure. I also updated the PR description to reflect the actual Most importantly, I tested the current PR head ( Would you mind re-testing the current patch when you have a chance? Thanks again. |
What
Fixes #11410.
The interactive Boolean Difference operation can silently leave the source mesh unchanged when mcut fails on problematic input geometry.
For the reporter's original model, the actual failure occurs in
mcDispatchwithMC_INVALID_OPERATION(err=-2). The affected input contains self-intersections, so directly falling back to CGAL PMP corefinement is not viable either: the existing CGAL path deliberately rejects self-intersecting input, and disabling that protection can crash insidecorefine_and_compute_difference.Root cause
MeshBoolean::mcut::do_boolean()performsA_NOT_Boperations throughdo_boolean_single().On the reporter's model:
mcDispatchfails withMC_INVALID_OPERATION.do_boolean_single()returnsfalse.A_NOT_Bpath did not distinguish this dispatch failure from other false-return paths.A direct
cgal::minus()fallback does not solve the problem because the input is self-intersecting. The CGAL PMP path usesthrow_on_self_intersection(true)for good reason; relaxing that guard can crash during corefinement.Fix
Keep the existing public
do_boolean_single()API and use an internal implementation that additionally reports whethermcDispatchitself failed.For
A_NOT_B, recovery is only attempted for an actualmcDispatchfailure:MeshBoolean::self_union()pathOther false-return paths are intentionally left unchanged.
UNIONandINTERSECTIONbehavior is unchanged.Cancellation
do_boolean_single()can also returnfalsewhen the user cancels an operation.The recovery path therefore checks
cancel_cb()immediately after the mcut call and again around repair/retry. A cancellation returns fromdo_boolean()and is never reinterpreted as a geometry failure.Validation
The current PR head was validated with both Linux and Windows CI builds.
The reporter's original 3MF from #11410 was also tested interactively with the current Windows PR build:
This specifically validates the repair-and-retry path on the same Windows platform and reporter geometry that previously failed with
mcDispatch err=-2.Closes #11410