fix(geometry): defer to sequential path when an accept gate rejects the roof-clip union subtract

rust/geometry/src/csg/topology_diagnostic.rs

@@ -214,4 +214,24 @@ impl ClippingProcessor {
         let topology_rejected = self.topology_gate_reject(op, mesh);
         manifold_rejected | topology_rejected
     }
+
+    /// #3919: whether any failure recorded since `since` (a prior
+    /// `failure_count()`) was an accept-gate rejection
+    /// (`OpenTopologyRejected` / `NonManifoldRejected`). A rejection hands
+    /// back the operand UN-CUT — the same `Ok(host_mesh.clone())` shape
+    /// `subtract_mesh` uses for "nothing to cut here" — so a caller that only
+    /// checks emptiness can't tell the two apart. A caller that treats a gate
+    /// rejection like a kernel error (deferring to a fallback path) must
+    /// check this too.
+    pub(crate) fn has_accept_gate_rejection_since(&self, since: usize) -> bool {
+        let failures = self.failures.borrow();
+        let since = since.min(failures.len());
+        failures[since..].iter().any(|f| {
+            matches!(
+                f.reason,
+                BoolFailureReason::OpenTopologyRejected
+                    | BoolFailureReason::NonManifoldRejected { .. }
+            )
+        })
+    }
 }

rust/geometry/src/processors/boolean/chain_cycle_tests.rs

@@ -896,3 +896,67 @@ fn an_operand_shared_between_two_branches_is_not_a_cycle() {
         );
     }
 }
+
+/// A #3922-review hypothesis: `solo_step` was `spine.len() == 1`, applied
+/// uniformly to every node in `spine`. But a longer chain's top-level batch
+/// can fail for a reason specific to its OUTERMOST cutter while the very
+/// next suffix batches cleanly, leaving `spine` holding only that outermost
+/// node — `spine.len() == 1` — even though its cutter has siblings (the ones
+/// the suffix already folded into the mesh). If that lone node then hits an
+/// accept-gate rejection, the old `solo_step = true` sent it to the riskier
+/// unbounded `FallThrough` instead of the safer `KeepUncut` — exactly the
+/// over-cut `!solo_step` exists to prevent.
+///
+/// House.ifc wall #2152's real chain (fixture: issue #960) reproduces the
+/// shape without any synthetic geometry: entity #2146 (8 PBHS cutters) is
+/// the node whose own top-level batch fails (its accept-gate rejects), while
+/// the very next level, #2145 (7 cutters), batches cleanly. In the full
+/// wall-#2152 chain (topmost #2149, 11 cutters) that leaves #2146 as one of
+/// 4 nodes still in `spine`, so `solo_step` was already correctly `false`
+/// there. Entering the SAME real geometry graph directly at #2146 — a
+/// perfectly valid `IfcBooleanClippingResult` node; nothing in the IFC
+/// schema requires the chain above it to exist — simulates an authored
+/// element whose own representation root IS #2146: `spine` then holds only
+/// `[#2146]`, so the old code set `solo_step = true` even though #2146's
+/// cutter has the same 7 siblings, already batched, right below it.
+#[cfg(any(feature = "csg_manifold_gate", feature = "csg_topology_gate"))]
+#[test]
+fn solo_step_accounts_for_a_batched_suffix_not_just_spine_length() {
+    let path = std::path::PathBuf::from(env!("CARGO_MANIFEST_DIR"))
+        .join("../../tests/models/issues/960_house_segmented_roof_clip.ifc");
+    let content = match std::fs::read_to_string(&path) {
+        Ok(s) if !s.starts_with("version https://git-lfs.github.com/spec/") => s,
+        _ => {
+            eprintln!(
+                "skipping: fixture issues/960_house_segmented_roof_clip.ifc not present \
+                 (or an LFS pointer) — run `pnpm fixtures`"
+            );
+            return;
+        }
+    };
+    let mut decoder = EntityDecoder::new(&content);
+    let entity = decoder.decode_by_id(2146).expect("decode #2146");
+    let processor = BooleanClippingProcessor::new();
+    let schema = IfcSchema::new();
+    let mesh = processor
+        .process(&entity, &mut decoder, &schema, TessellationQuality::Medium)
+        .expect("process #2146 chain");
+    assert!(!mesh.is_empty(), "#2146's chain must not render as empty");
+    let (_, mx) = mesh.bounds();
+    // Before the fix: `solo_step` was wrongly `true` here, so the accept-gate
+    // rejection on #2146's own cutter fell through to the unbounded plane
+    // clip and OVER-cut the already-batched 7-cutter result down to max
+    // Z ~= 2735.6 mm. After the fix, `solo_step` is correctly `false` (the
+    // 7-cutter suffix batched below #2146 means it is not alone), so the
+    // rejection keeps that step's host un-cut instead, landing at
+    // max Z ~= 4475.3 mm — the un-cut 7-cutter-batched mesh, not a
+    // secondary over-cut of it.
+    assert!(
+        (mx.z - 4475.3).abs() < 1.0,
+        "#2146 max Z = {:.1} mm, expected ~4475.3 mm (KeepUncut, not an \
+         unbounded-fallback over-cut). A value near 2735.6 means solo_step \
+         was miscomputed from spine.len() alone again, ignoring that a \
+         nested batch already folded this cutter's siblings into the mesh.",
+        mx.z,
+    );
+}

rust/geometry/src/processors/boolean/mod.rs

@@ -27,6 +27,8 @@ mod failures;
 mod operand;
 mod halfspace_cap;
 mod polygonal_prism;
+mod single_cutter_gate;
+use single_cutter_gate::SingleCutterSubtract;
 use cut_heuristics::{
     cutter_below_skip_ratio, plane_is_coincident_with_host_face, quality_skips_small_cuts,
 };
@@ -397,19 +399,13 @@ impl BooleanClippingProcessor {
         let mut tight_min = Point3::new(f32::NEG_INFINITY, f32::NEG_INFINITY, f32::NEG_INFINITY);
         let mut tight_max = Point3::new(f32::INFINITY, f32::INFINITY, f32::INFINITY);
         for prism in &prisms {
-            let trial = match clipper.subtract_mesh(&base_mesh, prism) {
-                Ok(m) if !m.is_empty() => m,
-                // Empty or errored single cut — the sequential path's per-cutter
-                // fallback handles it better than a batched union would.
-                _ => {
-                    let _ = clipper.take_failures();
-                    return self.defer_after(mark);
-                }
-            };
-            if ClippingProcessor::difference_result_looks_degenerate(&base_mesh, &trial) {
+            // `subtract_checked` folds in the #3919 accept-gate check: a
+            // rejection hands back the base UN-CUT, which would wrongly widen
+            // tight_min/tight_max, so it is treated like empty/errored/degenerate.
+            let Some(trial) = Self::subtract_checked(&clipper, &base_mesh, prism) else {
                 let _ = clipper.take_failures();
                 return self.defer_after(mark);
-            }
+            };
             let (tmn, tmx) = trial.bounds();
             tight_min = Point3::new(
                 tight_min.x.max(tmn.x),
@@ -448,18 +444,17 @@ impl BooleanClippingProcessor {
                 return Ok(None);
             }
         };
-        let result = clipper.subtract_mesh(&base_mesh, &combined);
+        // `subtract_checked` folds in the #3919 accept-gate check: a rejected
+        // gate hands back the base UN-CUT — the same shape as "nothing to
+        // cut", which `difference_result_looks_degenerate` can't catch — so
+        // it is treated like a kernel error and defers to the sequential
+        // per-cutter path (whose own accept-gate + #635 fallback handle it).
+        // Uncaught, the full-height base used to be accepted here and the
+        // issue-#960 seam sliver silently regrew.
+        let checked = Self::subtract_checked(&clipper, &base_mesh, &combined);
         self.absorb_failures(clipper.take_failures());
-        let clipped = match result {
-            Ok(m)
-                if !m.is_empty()
-                    && !ClippingProcessor::difference_result_looks_degenerate(&base_mesh, &m) =>
-            {
-                m
-            }
-            // Kernel error or a degenerate union result — fall back to the
-            // sequential per-cutter path.
-            _ => return self.defer_after(mark),
+        let Some(clipped) = checked else {
+            return self.defer_after(mark);
         };
 
         // Reject a silently under-removing union: the result must fit inside the
@@ -620,6 +615,14 @@ impl BooleanClippingProcessor {
         let mut spine: Vec<DecodedEntity> = Vec::new();
         let mut spine_seen: std::collections::HashSet<u32> = std::collections::HashSet::new();
         let mut current = entity.clone();
+        // Whether `mesh` (below) already carries a successfully BATCHED set of
+        // PBHS cutters (`try_union_polygonal_chain`), as opposed to being the
+        // untouched base solid. A leftover `spine` of length 1 is only the
+        // true #3923 single-cutter shape (no sibling cutter anywhere) when
+        // this is false; if a nested batch succeeded first, that node's own
+        // cutter has siblings already folded into `mesh`, even though it is
+        // the only node left in `spine` — see the `solo_step` comment below.
+        let mut based_on_batch = false;
         let mut mesh = loop {
             if !spine_seen.insert(current.id) {
                 // Cyclic FirstOperand chain (malformed input). The recursive
@@ -644,6 +647,7 @@ impl BooleanClippingProcessor {
                 {
                     // Batched PBHS resolution handled this node and everything
                     // below it (see the comment on the sequential step).
+                    based_on_batch = true;
                     break result;
                 }
             }
@@ -657,15 +661,25 @@ impl BooleanClippingProcessor {
             current = first;
         };
 
-        // Apply each spine node's operator + SecondOperand, innermost-first —
-        // exactly the order the recursive walk produced.
+        // Apply each spine node's operator + SecondOperand, innermost-first.
+        // `spine.len() == 1 && !based_on_batch` is the true #3923
+        // single-cutter shape (no other node shares the job, and the mesh it
+        // is cutting is the untouched base). `> 1` means a longer chain's
+        // batching failed at every level, so each node here is a
+        // one-cutter-at-a-time fallback. `spine.len() == 1 && based_on_batch`
+        // is the same "one cutter at a time" shape: a nested batch already
+        // succeeded on the levels below, so this lone leftover node's cutter
+        // has siblings (the batched ones) even though `spine` holds only it —
+        // see `single_cutter_gate.rs` for why that distinction matters to the
+        // gate-rejection fallback.
+        let solo_step = spine.len() == 1 && !based_on_batch;
         for node in spine.iter().rev() {
             if mesh.is_empty() {
                 // An emptied intermediate ends the chain, matching the old
                 // per-level early-out (for every operator, UNION included).
                 return Ok(mesh);
             }
-            mesh = self.apply_boolean_step(node, mesh, decoder, depth, quality, visited)?;
+            mesh = self.apply_boolean_step(node, mesh, decoder, depth, quality, visited, solo_step)?;
         }
         Ok(mesh)
     }
@@ -703,6 +717,14 @@ impl BooleanClippingProcessor {
     /// (`build_cutter_union`, the exact kernel's N-ary `union_many`); when it
     /// can't produce one, the chain falls through to this path — never worse
     /// than pre-#960 (841_house_stack_overflow.ifc).
+    ///
+    /// `solo_step`: true when this is the ONLY node the caller's spine walk
+    /// deferred to (a genuine single-PBHS-cutter DIFFERENCE, #3923's target
+    /// shape); false when it's one of several nodes from a longer authored
+    /// chain that couldn't be batched at any level and is now being applied
+    /// one cutter at a time. See the `IfcPolygonalBoundedHalfSpace` branch
+    /// below for why that distinction gates the accept-gate-rejection
+    /// fallback.
     fn apply_boolean_step(
         &self,
         entity: &DecodedEntity,
@@ -711,6 +733,7 @@ impl BooleanClippingProcessor {
         depth: u32,
         quality: TessellationQuality,
         visited: &mut OperandPath,
+        solo_step: bool,
     ) -> Result<Mesh> {
         let operator = Self::boolean_operator(entity);
 
@@ -784,30 +807,20 @@ impl BooleanClippingProcessor {
                     plane_normal,
                     agreement,
                 ) {
-                    let clipper = ClippingProcessor::new();
-                    let subtract_result = clipper.subtract_mesh(&mesh, &bound_mesh);
-                    self.absorb_failures(clipper.take_failures());
-                    if let Ok(clipped) = subtract_result {
-                        // The bounded-prism subtract is fragile on coincident
-                        // faces: when the clip polygon spans the full host
-                        // cross-section, the prism's in-plane side walls land
-                        // exactly on the host's side faces and the CSG kernel
-                        // can collapse the host to a near-empty sliver
-                        // (duplex.ifc "Party Wall" segments #4287/#4399 —
-                        // 12-tri box → 2-tri quad on the deleted legacy BSP
-                        // kernel). When the result looks degenerate
-                        // we fall through to the robust unbounded plane clip
-                        // below: a strict superset of the bounded cut that is
-                        // exactly correct whenever the polygon already covers
-                        // the host's projected cross-section.
-                        if !ClippingProcessor::difference_result_looks_degenerate(&mesh, &clipped) {
+                    // See `single_cutter_gate.rs` for the #3919/#3923
+                    // accept-gate check and why a rejection's fallback
+                    // depends on `solo_step`.
+                    match self.resolve_single_cutter_subtract(&mesh, &bound_mesh, solo_step) {
+                        SingleCutterSubtract::Clipped(clipped) => {
                             return Ok(self.guard_against_full_host_removal(
                                 mesh,
                                 clipped,
                                 plane_point,
                                 plane_normal,
                             ));
                         }
+                        SingleCutterSubtract::KeepUncut => return Ok(mesh),
+                        SingleCutterSubtract::FallThrough => {}
                     }
                 }
 

rust/geometry/src/processors/boolean/polygonal_prism.rs

@@ -4,7 +4,9 @@
 
 use super::super::helpers::parse_axis2_placement_3d;
 use super::BooleanClippingProcessor;
-use crate::{calculate_normals, Error, Mesh, Point2, Point3, Profile2D, Result, Vector3};
+use crate::{
+    calculate_normals, ClippingProcessor, Error, Mesh, Point2, Point3, Profile2D, Result, Vector3,
+};
 use ifc_lite_core::{DecodedEntity, EntityDecoder, IfcType};
 
 impl BooleanClippingProcessor {
@@ -375,4 +377,24 @@ impl BooleanClippingProcessor {
         calculate_normals(&mut mesh);
         Ok(mesh)
     }
+
+    /// Subtract `cutter` from `base`; an accept-gate rejection (#3919) hands
+    /// back `base` UN-CUT (same shape as "nothing to cut", which
+    /// `difference_result_looks_degenerate` can't catch), so it is treated
+    /// like empty/errored/degenerate: `None`. Used by
+    /// `try_union_polygonal_chain`'s per-cutter trials and final subtract so a
+    /// rejected gate always defers to the sequential path. Leaves any
+    /// failures in `clipper` for the caller to drain.
+    pub(super) fn subtract_checked(
+        clipper: &ClippingProcessor,
+        base: &Mesh,
+        cutter: &Mesh,
+    ) -> Option<Mesh> {
+        let mark = clipper.failure_count();
+        let m = match clipper.subtract_mesh(base, cutter) {
+            Ok(m) if !m.is_empty() && !clipper.has_accept_gate_rejection_since(mark) => m,
+            _ => return None,
+        };
+        (!ClippingProcessor::difference_result_looks_degenerate(base, &m)).then_some(m)
+    }
 }