Merge pull request #1904 from tangency-labs/fix/wr619-union-solid-leak

fix: WR-619 opt-in refusal of a union that merges a solid into the outside; WR-614/619 records + strip fixture (supersedes #1868, refs #1794)

crates/solidcore/src/boolean/merge/p4.rs

@@ -32,10 +32,13 @@
 //! its void flags follow the nesting): a union that seals a void left the void's shell
 //! first and failed `SmBrep::AssertValid` test 12 (the fork's fix,
 //! tangency-labs/smlib `abastran/void-shell-check`; WR-265,
-//! `plan/upstream-issues/WR-265--manifold-boolean-void-shell-first.md`).
+//! `plan/upstream-issues/WR-265--manifold-boolean-void-shell-first.md`). With
+//! `boolean_union_refuses_solid_leak` (own design, WR-619), a union whose step 6 would
+//! delete a face between the infinite region and a solid region is refused with `SM_ERR`
+//! (the face in the error) instead of returning a body whose solid merged into the outside.
 
 use super::p1::{self, MergeOptions};
-use crate::error::{Result, SolidError};
+use crate::error::{Result, SolidError, SM_ERR};
 use crate::geom::a_object::BooleanOperation;
 use crate::geom::attribute::{
     ids, Attribute, AttributeStore, Behavior, MergeRegionData, ObjectKey,
@@ -449,6 +452,29 @@ fn region_pair(b: &Brep, fu: FaceuseId) -> Result<(RegionId, RegionId)> {
     ))
 }
 
+/// Not upstream (WR-619): the first of `delete` with the infinite region on one side and a
+/// solid region on the other, whose deletion would merge that solid into the outside.
+fn face_leaking_solid(b: &Brep, delete: &[FaceId]) -> Result<Option<FaceId>> {
+    let Some(infinite) = b.infinite_region() else {
+        return Ok(None);
+    };
+    for &f in delete {
+        let Some(fu) = face::faceuses(b, f)?.0 else {
+            continue;
+        };
+        let (r1, r2) = region_pair(b, fu)?;
+        let inside = match (r1 == infinite, r2 == infinite) {
+            (true, false) => r2,
+            (false, true) => r1,
+            _ => continue,
+        };
+        if void(b, inside)? == 0 {
+            return Ok(Some(f));
+        }
+    }
+    Ok(None)
+}
+
 fn void(b: &Brep, r: RegionId) -> Result<i32> {
     Ok(b.region(r)?.is_void())
 }
@@ -1407,6 +1433,19 @@ fn editing_stages(
             }
         }
     }
+    // Not upstream unless opted in (`fixes.boolean_union_refuses_solid_leak`, WR-619): a
+    // union never deletes a face that bounds a solid against the outside.
+    if kernels.primary.fixes.boolean_union_refuses_solid_leak
+        && operation == BooleanOperation::Union
+    {
+        if let Some(f) = face_leaking_solid(primary, &faces.delete)? {
+            return Err(SolidError::Upstream {
+                operation: OP,
+                code: SM_ERR,
+                entity: Some(EntityRef::Face(f)),
+            });
+        }
+    }
     intersector.delete_faces(
         kernels,
         primary,

crates/solidcore/src/kernel.rs

@@ -521,6 +521,8 @@ pub struct UpstreamFixes {
     pub ellipse_adjust_step_interval_keeps_turns: bool,
     /// `SmBrep::DeleteFilletFace` removes a ring fillet (a round all the way round a boss's foot or rim: two closed tangent rails, one vertex each, and at most one seam) with the fork's `SmRingFilletRemoval`: the sharp edge is the intersection of the two tangent faces' surfaces within their natural domains (an exact circle for a boss), the tangent face's seam runs on to it along its line, and the rails become it; a removal that would need a surface or a curved edge continued past its domain is refused before the B-rep changes. Upstream takes four-sided fillets only, and fails on a ring (WR-605; SolidCraft change, tangency-labs/smlib 52bfef0, with 661f79c under `glue_vertices_joins_every_region`).
     pub fillet_removal_takes_ring_fillets: bool,
+    /// `SmMerge::ManifoldBoolean` refuses a union (`SM_ERR`, the face in the error) when a face its STEP 6 would delete has the infinite region on one side and a solid region on the other: deleting it would merge that solid into the outside. Upstream deletes it: where two operands' faces stay within tolerance of each other across a strip without being coincident (two 4 mm cubes, one turned 5.7 deg about z, within 5e-6 across a 1e-4 strip), the union deletes two such faces of the first operand, both of its solid regions merge into the infinite region, and the union comes back with no solid region and volume 0 (WR-619, #1794; own design, not in the fork).
+    pub boolean_union_refuses_solid_leak: bool,
 }
 
 /// Options for [`Kernel::new`].

crates/solidcore/tests/wr_619_union_solid_leak.rs

@@ -0,0 +1,109 @@
+//! tangency-labs/solidcore#1794 (WR-619): two 4 mm cubes, B turned atan(0.1) (5.71 deg)
+//! about z, whose faces y = 20 (A) and F1 (B's y' = 0) stay within 5e-6 of each other
+//! across the strip x in [1.9999, 2]. SMLib's union deletes two of A's faces (on y = 20 and
+//! on its bottom z = 0) each between a solid region and the outside, so both solids merge
+//! into the infinite region and the union has no solid region (as the pinned C++,
+//! `boolean.api_brep` case `wr614_strip_union_d5e-06`). The opt-in
+//! `Kernel::fixes.boolean_union_refuses_solid_leak` refuses such a union with `SM_ERR`.
+#![allow(
+    clippy::unwrap_used,
+    clippy::expect_used,
+    clippy::panic,
+    clippy::indexing_slicing
+)] // test helpers
+use solidcore::boolean::api_brep::boolean_union;
+use solidcore::error::SM_ERR;
+use solidcore::math::{Point3, Vec3};
+use solidcore::primitive::api_primitives::create_box;
+use solidcore::topology::brep::p1;
+use solidcore::topology::brep::p2::is_manifold_solid;
+use solidcore::topology::proto_topology::p1::api_transform::{
+    api_rotate, api_translate, TransformObject,
+};
+use solidcore::{Brep, EntityRef, Kernel, SolidError};
+
+const SIDE: f64 = 4.0;
+const SLOPE: f64 = 0.1;
+/// How far each strip edge lies off the other cube's face.
+const D: f64 = 5e-6;
+
+/// A: [-2, 2] x [16, 20] x [0, 4]. B: a 4 mm cube turned about z so that its face F1
+/// crosses y = 20 at x = 2 - D / SLOPE, with its corner edge at x = 2 - 2 D / SLOPE,
+/// D below y = 20 (the `wr614_coincident_strip.json` fixture's arithmetic).
+fn operands(k: &mut Kernel) -> (Brep, Brep) {
+    let a = create_box(k, Point3::new(-2.0, 16.0, 0.0), SIDE, SIDE, SIDE).unwrap();
+    let mut b = create_box(k, Point3::new(0.0, 0.0, 0.0), SIDE, SIDE, SIDE).unwrap();
+    let o = Point3::new(0.0, 0.0, 0.0);
+    let deg = SLOPE.atan().to_degrees();
+    api_rotate(
+        k,
+        Some(TransformObject::Brep(&mut b)),
+        o,
+        Vec3::new(0.0, 0.0, 1.0),
+        deg,
+    )
+    .unwrap();
+    let half = D / SLOPE;
+    let shift = Vec3::new((2.0 - half) - half, 20.0 - D, 0.0);
+    api_translate(k, Some(TransformObject::Brep(&mut b)), shift).unwrap();
+    (a, b)
+}
+
+/// The regions other than the infinite one.
+fn solid_regions(b: &Brep) -> usize {
+    p1::regions(b, None)
+        .unwrap()
+        .into_iter()
+        .filter(|&r| Some(r) != b.infinite_region())
+        .count()
+}
+
+#[test]
+fn wr_619_union_refuses_solid_leak_is_off_by_default() {
+    assert!(!Kernel::default().fixes.boolean_union_refuses_solid_leak);
+}
+
+#[test]
+fn wr_619_upstream_union_has_no_solid() {
+    let mut k = Kernel::default();
+    let (a, b) = operands(&mut k);
+    let u = boolean_union(&mut k, a, b).unwrap();
+    assert_eq!(solid_regions(&u), 0);
+    assert!(!is_manifold_solid(&u).unwrap());
+}
+
+#[test]
+fn wr_619_union_with_the_fix_is_refused() {
+    let mut k = Kernel::default();
+    k.fixes.boolean_union_refuses_solid_leak = true;
+    let (a, b) = operands(&mut k);
+    match boolean_union(&mut k, a, b) {
+        Err(SolidError::Upstream {
+            code,
+            entity: Some(EntityRef::Face(_)),
+            ..
+        }) => assert_eq!(code, SM_ERR),
+        other => panic!("refused with SM_ERR at a face, got {other:?}"),
+    }
+}
+
+/// Two overlapping boxes: the fix leaves an ordinary union alone.
+#[test]
+fn wr_619_fix_leaves_an_ordinary_union_alone() {
+    let union = |fix: bool| {
+        let mut k = Kernel::default();
+        k.fixes.boolean_union_refuses_solid_leak = fix;
+        let a = create_box(&mut k, Point3::new(0.0, 0.0, 0.0), 4.0, 4.0, 4.0).unwrap();
+        let b = create_box(&mut k, Point3::new(2.0, 1.0, 1.0), 4.0, 2.0, 2.0).unwrap();
+        let u = boolean_union(&mut k, a, b).unwrap();
+        (
+            p1::faces(&u, None).unwrap().len(),
+            p1::edges(&u, None).unwrap().len(),
+            solid_regions(&u),
+            is_manifold_solid(&u).unwrap(),
+        )
+    };
+    let off = union(false);
+    assert_eq!((off.2, off.3), (1, true));
+    assert_eq!(union(true), off);
+}

plan/upstream-issues/WR-614--crossing-of-planes-within-tolerance-across-a-strip-fails-the-merge.md

@@ -0,0 +1,10 @@
+#### WR-614. The crossing line of two planes within tolerance of each other across a narrow strip fails the merge
+- **Severity:** Wrong result (a boolean of valid bodies fails with `SM_ERR`)
+- **Area:** Booleans
+- **Location:** `source/SMLib/src/SmTopologyIntersector.cpp`, `SmTopologyIntersector::IntersectInsertRelate` (def. `:1092`), which resolves (`ResolveIntersections`, def. `:2251`) every face/face proto edge it found; `source/SMLib/src/SmProtoTopology.cpp`, `SmProtoVertex::GetFinalVertex` (def. `:2143`); the failure is reported by `SmMerge::MakeFaceuse` (`source/SMLib/src/SmMerge.cpp`, `SER(SM_ERR)` when the Brep mate of an edge use's vertex is neither end of the Brep edge, `:7077`).
+- **What happens:** two planar faces crossing at a small angle meet in a line fixed only to `tol / sin(angle)`. Where an edge of one face lies on the other face within tolerance, parallel to the crossing, the faces are within tolerance of each other across the strip between them, yet the crossing still becomes an edge definition. In the rebuilt cubes below, `IntersectTopology` combines it into the proto edge of B's corner edge lying on A's face, about 1e-4 away: the imprint then ties the crossing on A's face to B's edge, whose end vertices are not the crossing's. In the issue's imported bodies, the crossing's end proto vertices (capture distance 0.01) hold two correctly classified vertex definitions, one on either side of it; `GetFinalVertex` picks, per body, the one nearest and connected to that body's source face, and `SetRelationships` relates vertices only within one definition. Either way `MakeFaceuse` finds the edge's vertex mates inconsistent and fails. ProtoFaces only detect coincident surfaces, never non-parallel planes coincident along a strip.
+- **Trigger:** two 4 mm cubes, B turned 5.7 deg about z, whose faces (A's y = 20, B's F1) cross at x = 1.99995 and stay within 5e-6 of each other across x in [1.9999, 2], overlapping by about 1.0e-4 mm (the issue's imported bodies, intersection tolerance 5e-6): Intersect returns `SM_ERR` (1001); both bodies pass the B-rep check. Rebuilt from primitives at the default tolerances, A `[-2, 2] x [16, 20] x [0, 4]` and B turned atan(0.1) about z within 1e-5 of A's face across x in [1.9998, 2], the union fails with `SM_ERR` from `MakeFaceuse`. The same cubes within 5e-6 give WR-619 instead.
+- **How found:** reported in tangency-labs/solidcore#1794 (SolidCraft, from the fork's batch-4 bodies). No fix in the fork (tangency-labs/smlib main and the batch branches) or upstream.
+- **Confirmed:** yes, on the C++ build: `boolean.api_brep` fixture `fixtures/boolean.api_brep/wr614_coincident_strip.json`, cases `wr614_strip_union_d1e-05` (`SmApiBooleanUnion`) and `wr614_strip_measured_union_d1e-05` (`SmApiBooleanWithOptions`, post-processing on), fail with the same status on both sides.
+- **Workaround:** none found. Post-processing does not help: `SmApiBooleanUnion` already merges with `bBooleanPostProcess` TRUE (the `SmMerge` constructor's default), so the plain and the options path are the same merge and both fail.
+- **Rust port:** behaves as upstream. A rule that drops the crossing's edge definition was tried and dropped: it turned the `SM_ERR` into the region-less union of WR-619.

plan/upstream-issues/WR-619--union-across-a-strip-within-tolerance-merges-both-solids-into-the-outside.md

@@ -0,0 +1,9 @@
+#### WR-619. A union of solids within tolerance of each other across a narrow strip merges both solids into the outside
+- **Severity:** Wrong result (a union of two solids returns a body with no solid region, volume 0; reported as success)
+- **Area:** Booleans
+- **Location:** `source/SMLib/src/SmMerge.cpp`, `SmMerge::ManifoldBoolean` (def. `:2510`), STEP 6 (`m_vTI.DeleteFaces(sDelFaces, ..)`, `:4724`, `SmTopologyIntersector::DeleteFaces` def. `source/SMLib/src/SmTopologyIntersector.cpp:456`) and STEP 7 (`:4782`-`:4898`); the region merge in `SmBrep::DeleteFace` (`source/SMLib/src/SmBrep.cpp:7170`-`:7199`).
+- **What happens:** two planar faces of the operands stay within tolerance of each other across a narrow strip without being coincident (they cross at a small angle, so no ProtoFace joins them), while the operands' top and bottom faces are coplanar. STEP 6 deletes two faces of the primary operand A: one on its plane y = 20 (the face that crosses B's F1 in the strip) and one on its bottom plane z = 0 (coplanar with B's bottom face). Each has a solid region on one side and the infinite region on the other, so each `DeleteFace` merges a solid region into the infinite region (`DeleteFace` keeps the infinite or void region of the two it joins): the first A's solid region, the second the region the merge split off when it added B's faces. STEP 7 then marks the one region left solid (`SetIsVoid(FALSE)`, `:4895`), and `FindAndSetInfiniteRegion` makes it the infinite region and void again. The result keeps every face, all of their face uses in one shell of the infinite region, with no solid region: not a manifold solid, volume 0. Why the classification deletes these two faces, where a union deletes only faces inside the other operand, is not traced further here.
+- **Trigger:** `SmApiBooleanUnion` of two 4 mm cubes, A `[-2, 2] x [16, 20] x [0, 4]` and B turned atan(0.1) (5.71 deg) about z, whose faces (A's y = 20, B's F1) cross at x = 1.99995 and stay within 5e-6 of each other across x in [1.9999, 2]. The same cubes within 1e-5 across x in [1.9998, 2] fail instead (WR-614). Both operands are valid manifold solids.
+- **How found:** tangency-labs/solidcore#1794 (WR-614 follow-up): the union that succeeds a hair from the failing one returns no solid. Traced with gdb on both sides.
+- **Confirmed:** yes, on the C++ build. `boolean.api_brep` fixture `fixtures/boolean.api_brep/wr614_coincident_strip.json`, case `wr614_strip_union_d5e-06`, gives the same result on both sides (12 faces, 26 edges, 16 vertices, one region, not a manifold solid). In both engines `ManifoldBoolean`'s STEP 6 deletes two faces, and STEP 7 and `FindAndSetInfiniteRegion` then reset the one region left; in the port (gdb, the deleted faces' planes read from their surfaces) the two `DeleteFace` calls delete A's faces on y = 20 and on z = 0, and each merges a solid region into the infinite region.
+- **Rust port:** behaves as upstream by default. Through the fork's C API (`smlib_boolean`) the body is refused with `SMLIB_ERR_NOT_SOLID` (`finish()`, its result check); a direct caller of the port gets the region-less body. With the opt-in `Kernel::fixes.boolean_union_refuses_solid_leak` (own design), `manifold_boolean` refuses a union with `SM_ERR` (the face in the error) when a face its step 6 would delete has the infinite region on one side and a solid region on the other (`boolean/merge/p4.rs`, `face_leaking_solid`). Only a union is checked: a union deletes the faces of each operand that lie inside the other, which have material on both sides, so it never rightly deletes a face between a solid and the outside; an intersection's step 6 does exactly that on purpose (it deletes A's faces outside B, and merging A's solid there into the outside is how that part becomes void). `crates/solidcore/tests/wr_619_union_solid_leak.rs` checks the cubes' union both ways and that an ordinary union of two overlapping boxes is unchanged.