diff --git a/crates/algo/src/builder/face_splitter/mod.rs b/crates/algo/src/builder/face_splitter/mod.rs index 430a47f6..67df9627 100644 --- a/crates/algo/src/builder/face_splitter/mod.rs +++ b/crates/algo/src/builder/face_splitter/mod.rs @@ -31,7 +31,8 @@ use conversion::{ use edge_splitting::split_boundary_edges_at_3d_points; use sampling::{sample_wire_loop_uv, sample_wire_loop_uv_periodic}; use special_cases::{ - split_face_with_internal_loops, split_noseam_face_direct, try_split_crossing_plane_face, + split_face_with_internal_loops, split_noseam_face_direct, split_periodic_face_into_bands, + try_split_crossing_plane_face, }; /// Split a face by its section edges, producing sub-faces. @@ -99,15 +100,23 @@ pub fn split_face_2d( .iter() .filter_map(|&iw_id| { let iw_pts = collect_wire_points(topo, iw_id); - if iw_pts.len() < 3 { - return None; - } let edges = if is_plane { boundary_edges_to_pcurve(topo, iw_id, &surface, &iw_pts, Some(frame)) } else { boundary_edges_to_pcurve(topo, iw_id, &surface, &iw_pts, None) }; - if edges.is_empty() { None } else { Some(edges) } + // A hole bounded by closed curved edges (e.g. a single full + // circle) has fewer than 3 distinct wire points but is a valid + // inner wire; only polyline-style wires need 3+ points. + let has_closed_curve = edges.iter().any(|e| { + !matches!(e.curve_3d, EdgeCurve::Line) + && (e.start_3d - e.end_3d).length() < tol.linear * 100.0 + }); + if edges.is_empty() || (iw_pts.len() < 3 && !has_closed_curve) { + None + } else { + Some(edges) + } }) .collect(); @@ -147,6 +156,24 @@ pub fn split_face_2d( ); } + // Band shortcut: closed section circles on a u-periodic face split it + // into stacked bands, not discs. Requires seam-anchored circles (see + // the seam-anchor pre-pass in fill_images_faces); falls through to the + // generic paths when preconditions don't hold. + if u_periodic && !is_plane && original_inner_wires.is_empty() { + if let Some(bands) = split_periodic_face_into_bands( + &surface, + &boundary_edges, + sections, + rank, + reversed, + face_id, + tol.linear, + ) { + return bands; + } + } + // Internal section edge shortcut: when section edges form closed loops // entirely within the face (not connecting to boundary edges), the wire // builder struggles with periodic UV and 4-way junctions. Instead, group @@ -182,6 +209,7 @@ pub fn split_face_2d( return split_face_with_internal_loops( &surface, &boundary_edges, + &original_inner_wires, sections, rank, reversed, diff --git a/crates/algo/src/builder/face_splitter/special_cases.rs b/crates/algo/src/builder/face_splitter/special_cases.rs index 7ea0f03e..5a079b73 100644 --- a/crates/algo/src/builder/face_splitter/special_cases.rs +++ b/crates/algo/src/builder/face_splitter/special_cases.rs @@ -144,66 +144,224 @@ pub(super) fn split_noseam_face_direct( } // --------------------------------------------------------------------------- -// Known gap: disc-loop interpretation is wrong for periodic surfaces. -// -// `split_face_with_internal_loops` below treats each closed section curve -// as a planar disc (loop bounds a disc-shaped sub-face). This is correct -// for plane faces, but on a u-periodic surface (cylinder/cone/sphere/torus -// lateral) a single closed circle does NOT bound a disc — it splits the -// periodic surface into bands. The disc-pair output produces invalid -// topology (Euler=3, non-manifold) for box-cylinder cuts; see the pinning -// tests: -// - `gfa_cut_box_cylinder_through_produces_valid_topology` (PR #534) -// - `gfa_cut_box_cylinder_grid_through_sequential_produces_valid_topology` -// (PR #537) -// -// The operations layer's mesh-boolean fallback rescues most callers, but -// at the cost of falling out of GFA for any cylinder cut. -// -// The proper fix is a `split_periodic_face_at_circles` post-pass that -// emits N+1 band sub-faces (each: bot_circle + seam + top_circle_rev + -// seam_rev) for a u-periodic face cut by N closed section circles. -// -// Investigation notes (2026-04-30, ultimately reverted before merge): -// -// 1. **Topology piece works.** A band-builder gated on -// `cylinder_face_count == 1` (pre-registering closed-circle section -// endpoints in `pb_vertex_registry` so cross-face vertex sharing -// via `merge_duplicate_edges` fires) un-ignores the single-cyl pin: -// F=7 E=15 V=10 Euler=2 manifold. -// -// 2. **Geometry piece is the blocker.** The band-builder reused the -// existing section-circle endpoint chosen by `phase_ff` (some -// arbitrary u-angle, e.g. (5,4,0) rather than the seam (6,5,0)) as -// the band's wire endpoint. The seam-segment Line then connects -// two off-seam points, crossing the cylinder interior. This is -// topologically valid but geometrically inconsistent. Downstream -// ops that use face geometry (volume, intersection in subsequent -// compound_cut iterations, tessellation) hang or produce wrong -// results — `compound_cut_honeycomb` (9 cyls in a 3×3 grid) hangs -// indefinitely on iter 2 even with the band-builder gated off for -// that iter, because iter 1's corrupt geometry propagates. -// -// 3. **What the next attempt needs.** A pre-pass that, for each closed -// section circle on a u-periodic face: (a) creates a vertex at -// `surface.evaluate(seam_u, v_section)`, (b) re-parameterizes the -// section's pcurve to start at `u = seam_u`, (c) splits the existing -// seam Line edge at `v_section` so the band's seam-segment endpoints -// land on real topology vertices. Without (a)+(b)+(c) the band's -// seam doesn't lie on the cylinder surface and downstream ops break. -// `pb_vertex_registry` is the right place to wire the new vertices — -// `resolve_edge_vertices` already consults it. -// -// 4. **Multi-cylinder gating is a separate hard problem.** Even with -// correct seam geometry, multi-tool scenarios (compound_cut with -// tools sharing section planes — e.g. 4×4 grid) confuse BOP face- -// image classification. PR #535's body recommends gating on -// "BOP can reliably classify this band's interior" — easier to -// derive empirically than design a priori. -// -// See PRs #534, #535, #537 for the pinning test history. +// Periodic faces take `split_periodic_face_into_bands` below; the disc-loop +// interpretation in `split_face_with_internal_loops` applies to plane faces +// (and to periodic faces only as a fallback when the band preconditions +// don't hold). The band path depends on the seam-anchor pre-pass in +// `fill_images_faces`, which re-parameterizes closed section circles to +// start at the face's seam — without it, the band's seam segments would +// connect arbitrary section angles and cut through the surface interior. // --------------------------------------------------------------------------- +/// Split a u-periodic face (cylinder/cone lateral) into stacked bands at +/// its closed section circles. +/// +/// A closed circle on a u-periodic surface does not bound a disc — it +/// separates the surface into bands. For N internal circles sorted by v, +/// emits N+1 band sub-faces, each bounded by: +/// lower circle + seam segment up + upper circle reversed + seam segment +/// down. The end bands reuse the face's original boundary circle edges. +/// +/// Preconditions (returns `None` so the caller can fall back otherwise): +/// - surface is a cylinder or cone +/// - boundary is exactly 2 closed circle edges plus seam Line edges, all +/// seam endpoints at the same u +/// - every section is a full closed circle whose start point sits on the +/// seam (guaranteed by the seam-anchor pre-pass) at a v strictly between +/// the boundary circles, with no two circles at the same v +#[allow( + clippy::too_many_lines, + clippy::too_many_arguments, + clippy::items_after_statements +)] +pub(super) fn split_periodic_face_into_bands( + surface: &FaceSurface, + boundary_edges: &[OrientedPCurveEdge], + sections: &[SectionEdge], + rank: Rank, + reversed: bool, + face_id: FaceId, + tol: f64, +) -> Option> { + use brepkit_math::curves2d::{Curve2D, Line2D}; + use brepkit_math::vec::{Point2, Vec2}; + use std::f64::consts::{PI, TAU}; + + if !matches!(surface, FaceSurface::Cylinder(_) | FaceSurface::Cone(_)) { + return None; + } + let close_tol = tol * 100.0; + + // Partition boundary into closed circle edges and seam Line edges. + let mut boundary_circles: Vec<&OrientedPCurveEdge> = Vec::new(); + let mut seam_edges: Vec<&OrientedPCurveEdge> = Vec::new(); + for e in boundary_edges { + let is_closed = (e.start_3d - e.end_3d).length() < close_tol; + match (&e.curve_3d, is_closed) { + (EdgeCurve::Circle(_), true) => boundary_circles.push(e), + (EdgeCurve::Line, false) => seam_edges.push(e), + _ => return None, + } + } + if boundary_circles.len() != 2 || seam_edges.is_empty() { + return None; + } + + // Seam u — shared by every seam edge endpoint (mod 2π). + let (seam_u, _) = surface.project_point(seam_edges[0].start_3d)?; + for e in &seam_edges { + for p in [e.start_3d, e.end_3d] { + let (u, _) = surface.project_point(p)?; + let du = (u - seam_u + PI).rem_euclid(TAU) - PI; + if du.abs() > 1e-6 { + return None; + } + } + } + + // Every circle must start on the seam; collect (v, lower_fwd, edge). + let circle_v = |e: &OrientedPCurveEdge| -> Option { + let (_, v) = surface.project_point(e.start_3d)?; + let on_seam = surface.evaluate(seam_u, v)?; + ((on_seam - e.start_3d).length() < close_tol).then_some(v) + }; + + let v0 = circle_v(boundary_circles[0])?; + let v1 = circle_v(boundary_circles[1])?; + let (v_bot, bot_edge, v_top, top_edge) = if v0 < v1 { + (v0, boundary_circles[0], v1, boundary_circles[1]) + } else { + (v1, boundary_circles[1], v0, boundary_circles[0]) + }; + if v_top - v_bot < close_tol { + return None; + } + + // Reference traversal tangent: how the original bottom circle is + // traversed at the seam. Section circles in the lower role must + // traverse the same way; in the upper role, the opposite way. + let traversal_tangent = |e: &OrientedPCurveEdge| -> Option { + let EdgeCurve::Circle(c) = &e.curve_3d else { + return None; + }; + let t = c.tangent(c.project(e.start_3d)); + Some(if e.forward { t } else { -t }) + }; + let ref_tan = traversal_tangent(bot_edge)?; + + // Collect section circles with their v and natural-direction alignment. + struct BandCircle { + v: f64, + lower: OrientedPCurveEdge, + upper: OrientedPCurveEdge, + } + let mut mids: Vec = Vec::with_capacity(sections.len()); + for s in sections { + if (s.start - s.end).length() > close_tol { + return None; + } + let EdgeCurve::Circle(c) = &s.curve_3d else { + return None; + }; + let (_, v) = surface.project_point(s.start)?; + let on_seam = surface.evaluate(seam_u, v)?; + if (on_seam - s.start).length() > close_tol { + return None; + } + // A section circle at a boundary circle's v duplicates the existing + // boundary edge (flush cap configuration) — no split there. + if (v - v_bot).abs() < close_tol || (v_top - v).abs() < close_tol { + continue; + } + if v < v_bot || v > v_top { + return None; + } + let natural_tan = c.tangent(c.project(s.start)); + let lower_fwd = natural_tan.dot(ref_tan) > 0.0; + let pcurve = match rank { + Rank::A => &s.pcurve_a, + Rank::B => &s.pcurve_b, + }; + let mk = |forward: bool| OrientedPCurveEdge { + curve_3d: s.curve_3d.clone(), + pcurve: pcurve.clone(), + start_uv: Point2::new(seam_u, v), + end_uv: Point2::new(seam_u, v), + start_3d: s.start, + end_3d: s.start, + forward, + source_edge_idx: None, + pave_block_id: s.pave_block_id, + }; + mids.push(BandCircle { + v, + lower: mk(lower_fwd), + upper: mk(!lower_fwd), + }); + } + if mids.is_empty() { + // Every section coincided with a boundary circle (fully flush + // tool): no band split applies — let the generic paths handle the + // coplanar-cap interaction. + return None; + } + mids.sort_by(|a, b| a.v.partial_cmp(&b.v).unwrap_or(std::cmp::Ordering::Equal)); + if mids.windows(2).any(|w| w[1].v - w[0].v < close_tol) { + return None; + } + + let mk_seam = |va: f64, vb: f64| -> Option { + let pa = surface.evaluate(seam_u, va)?; + let pb = surface.evaluate(seam_u, vb)?; + let dir = Vec2::new(0.0, if vb > va { 1.0 } else { -1.0 }); + let pcurve = Curve2D::Line(Line2D::new(Point2::new(seam_u, va), dir).ok()?); + Some(OrientedPCurveEdge { + curve_3d: EdgeCurve::Line, + pcurve, + start_uv: Point2::new(seam_u, va), + end_uv: Point2::new(seam_u, vb), + start_3d: pa, + end_3d: pb, + forward: true, + source_edge_idx: None, + pave_block_id: None, + }) + }; + + // Assemble bands bottom-to-top. Levels: bot boundary, sections, top + // boundary. Each band: lower circle, seam up, upper circle, seam down. + let mut levels: Vec<(f64, OrientedPCurveEdge, OrientedPCurveEdge)> = Vec::new(); + levels.push((v_bot, bot_edge.clone(), bot_edge.clone())); + for m in mids { + levels.push((m.v, m.lower, m.upper)); + } + levels.push((v_top, top_edge.clone(), top_edge.clone())); + + let mut bands = Vec::with_capacity(levels.len() - 1); + for w in levels.windows(2) { + let (va, lower, _) = &w[0]; + let (vb, _, upper) = &w[1]; + let (va, vb) = (*va, *vb); + let wire = vec![ + lower.clone(), + mk_seam(va, vb)?, + upper.clone(), + mk_seam(vb, va)?, + ]; + let interior = surface.evaluate((seam_u + PI).rem_euclid(TAU), f64::midpoint(va, vb))?; + bands.push(SplitSubFace { + surface: surface.clone(), + outer_wire: wire, + inner_wires: Vec::new(), + reversed, + parent: face_id, + rank, + precomputed_interior: Some(interior), + }); + } + Some(bands) +} + /// Split a face when ALL section edges are interior (don't touch the boundary). /// /// Groups section edges into closed loops by chaining shared 3D endpoints. @@ -214,13 +372,15 @@ pub(super) fn split_noseam_face_direct( /// The "outside" sub-face has the original boundary as outer wire with all /// loops as holes. /// -/// **Known limitation**: applies the planar disc-loop interpretation to -/// periodic surfaces (cylinder/cone/sphere/torus laterals), which is -/// wrong — see the module-level investigation notes above. +/// The disc-loop interpretation is only correct for plane faces (and +/// sphere caps). Cylinder/cone laterals are routed to +/// [`split_periodic_face_into_bands`] first and only reach this fallback +/// when the band preconditions don't hold. #[allow(clippy::too_many_arguments)] pub(super) fn split_face_with_internal_loops( surface: &FaceSurface, boundary_edges: &[OrientedPCurveEdge], + original_inner_wires: &[Vec], sections: &[SectionEdge], rank: Rank, reversed: bool, @@ -443,6 +603,10 @@ pub(super) fn split_face_with_internal_loops( } // The "outside" sub-face: original boundary with all loops as holes. + // Pre-existing holes (from earlier boolean operations) stay with the + // outside sub-face — dropping them would leave the faces ringing those + // holes with free edges. + all_holes.extend(original_inner_wires.iter().cloned()); result.push(SplitSubFace { surface: surface.clone(), outer_wire: boundary_edges.to_vec(), @@ -726,3 +890,31 @@ pub(super) fn try_split_crossing_plane_face( } Some(result) } + +#[cfg(test)] +mod tests { + #![allow(clippy::unwrap_used)] + use brepkit_math::surfaces::CylindricalSurface; + use brepkit_math::vec::{Point3, Vec3}; + use brepkit_topology::face::FaceSurface; + use std::f64::consts::{PI, TAU}; + + #[test] + fn band_interior_antipode_wraps_into_domain() { + let cyl = + CylindricalSurface::new(Point3::new(0.0, 0.0, 0.0), Vec3::new(0.0, 0.0, 1.0), 2.0) + .unwrap(); + let surface = FaceSurface::Cylinder(cyl); + + // Seam in (π, 2π): the unwrapped antipode seam_u + π exceeds 2π. + for &seam_u in &[1.1 * PI, 1.5 * PI, 1.9 * PI] { + let wrapped = (seam_u + PI).rem_euclid(TAU); + assert!((0.0..TAU).contains(&wrapped)); + // The wrap is behavior-preserving on a periodic surface: the + // in-domain parameter evaluates to the same 3D interior point. + let a = surface.evaluate(seam_u + PI, 3.0).unwrap(); + let b = surface.evaluate(wrapped, 3.0).unwrap(); + assert!((a - b).length() < 1e-9); + } + } +} diff --git a/crates/algo/src/builder/fill_images_faces.rs b/crates/algo/src/builder/fill_images_faces.rs index 5a7f75d2..3da87f22 100644 --- a/crates/algo/src/builder/fill_images_faces.rs +++ b/crates/algo/src/builder/fill_images_faces.rs @@ -227,7 +227,23 @@ pub fn fill_images_faces( // (pb_vertex_registry and CB pre-pass moved above rank pool) // Pre-compute which faces have section edges from which curves - let section_map = build_section_map(arena); + let section_map = build_section_map(topo, arena); + + // ── Periodic seam anchor pre-pass ─────────────────────────────── + // Closed circle intersection curves on a u-periodic face (cylinder / + // cone lateral) are re-anchored so the circle's start/end vertex sits + // on the face's seam. The band splitter connects consecutive circles + // with seam segments; without re-anchoring, those segments would join + // arbitrary section angles and cut through the surface interior. + // Pre-registering the anchor vertices makes the periodic face's band + // wires and the opposing plane faces' hole wires resolve to the same + // VertexId, so merge_duplicate_edges can share the circle edge. + let seam_anchors = compute_seam_anchors(topo, arena); + for &anchor in seam_anchors.values() { + pb_vertex_registry + .entry(qpos(anchor)) + .or_insert_with(|| topo.add_vertex(Vertex::new(anchor, tol.linear))); + } // No boundary edge cache — each face creates its own edges with its own // vertices. Cross-face edge sharing is handled by merge_duplicate_edges @@ -277,7 +293,14 @@ pub fn fill_images_faces( } // Build SectionEdge entries from pave block data - let sections = build_section_edges(topo, arena, face_id, §ion_map, tol.linear); + let sections = build_section_edges( + topo, + arena, + face_id, + §ion_map, + &seam_anchors, + tol.linear, + ); log::debug!( "fill_images_faces: face {:?} has_sections={} sections={}", @@ -958,7 +981,88 @@ enum SectionSource { PaveBlock(PaveBlockId), } -fn build_section_map(arena: &GfaArena) -> HashMap> { +/// Tolerance on `|t_range| - 2π` for treating a circle edge as a full circle. +/// Angular (radian) comparison on a parameter span. +const FULL_CIRCLE_T_TOL: f64 = 1e-9; + +/// Minimum 3D length for a seam Line edge to count as non-degenerate. +const SEAM_DEGENERATE_TOL: f64 = 1e-10; + +/// Radial/planar tolerance for accepting the seam point as lying on the +/// circle. Looser than the linear default (1e-7) because the anchor is built +/// from two `project_point` + `evaluate` round-trips whose float error +/// accumulates; tightening it would spuriously reject valid anchors. +const SEAM_ON_CIRCLE_TOL: f64 = 1e-6; + +/// Compute seam-anchored start points for closed circle intersection curves. +/// +/// For each full-circle FF curve whose face pair includes a u-periodic +/// surface (cylinder/cone) with a seam Line edge, returns the point on the +/// circle at the seam's u parameter. Keyed by the curve's arena index. +fn compute_seam_anchors(topo: &Topology, arena: &GfaArena) -> BTreeMap { + use std::f64::consts::TAU; + + let mut anchors = BTreeMap::new(); + for (idx, curve_ds) in arena.curves.iter().enumerate() { + let EdgeCurve::Circle(circle) = &curve_ds.curve else { + continue; + }; + let (t0, t1) = curve_ds.t_range; + if ((t1 - t0).abs() - TAU).abs() > FULL_CIRCLE_T_TOL { + continue; + } + for fid in [curve_ds.face_a, curve_ds.face_b] { + let Ok(face) = topo.face(fid) else { continue }; + let surface = face.surface(); + if !matches!(surface, FaceSurface::Cylinder(_) | FaceSurface::Cone(_)) { + continue; + } + let Some(anchor) = seam_anchor_on_circle(topo, face, circle) else { + continue; + }; + anchors.insert(idx, anchor); + break; + } + } + anchors +} + +/// Find the point on `circle` at the seam u of `face`'s periodic surface. +/// +/// Returns `None` if the face has no non-degenerate seam Line edge, the +/// projections fail, or the seam point at the circle's v does not actually +/// lie on the circle (e.g. the circle is not a constant-v iso-curve). +fn seam_anchor_on_circle( + topo: &Topology, + face: &Face, + circle: &brepkit_math::curves::Circle3D, +) -> Option { + let surface = face.surface(); + let wire = topo.wire(face.outer_wire()).ok()?; + let mut seam_pt = None; + for oe in wire.edges() { + let Ok(edge) = topo.edge(oe.edge()) else { + continue; + }; + if matches!(edge.curve(), EdgeCurve::Line) { + let sp = topo.vertex(edge.start()).ok()?.point(); + let ep = topo.vertex(edge.end()).ok()?.point(); + if (sp - ep).length() > SEAM_DEGENERATE_TOL { + seam_pt = Some(sp); + break; + } + } + } + let (seam_u, _) = surface.project_point(seam_pt?)?; + let (_, v_circle) = surface.project_point(circle.evaluate(0.0))?; + let anchor = surface.evaluate(seam_u, v_circle)?; + let radial = anchor - circle.center(); + let on_circle = (radial.length() - circle.radius()).abs() < SEAM_ON_CIRCLE_TOL + && radial.dot(circle.normal()).abs() < SEAM_ON_CIRCLE_TOL; + on_circle.then_some(anchor) +} + +fn build_section_map(topo: &Topology, arena: &GfaArena) -> HashMap> { let mut map: HashMap> = HashMap::new(); // Section edges from FF intersection curves. // For non-Line curves (Circle, Ellipse, NURBS): use one Curve entry per curve @@ -1008,7 +1112,21 @@ fn build_section_map(arena: &GfaArena) -> HashMap> { if faces_with_curved_ff.contains(&face_id.index()) { continue; // Face has curved FF curve — IN PBs are redundant. } + // A plane cap disc (outer boundary is a single closed circle, e.g. + // the cutting tool's own cap lying flush on a wall) needs no interior + // splitting: its boundary already trims it. IN edges projected onto + // such a cap from the wall's rectangle and other holes are spurious + // arcs that fragment the clean disc into a many-sided polygon, which + // then fails edge-set same-domain pairing with the wall's matching + // cap disc and survives the cut as a stray face. Keep only IN edges + // strictly inside the disc; drop those on or outside its boundary. + let cap_disc = cap_disc_circle(topo, face_id); for &pb_id in &fi.pave_blocks_in { + if let Some(circle) = &cap_disc { + if !pb_strictly_inside_circle(topo, arena, pb_id, circle) { + continue; + } + } map.entry(face_id) .or_default() .push(SectionSource::PaveBlock(pb_id)); @@ -1017,6 +1135,52 @@ fn build_section_map(arena: &GfaArena) -> HashMap> { map } +/// If `face_id` is a plane whose outer wire is a single closed circular edge +/// (a cap disc), return that circle; otherwise `None`. +fn cap_disc_circle(topo: &Topology, face_id: FaceId) -> Option { + let face = topo.face(face_id).ok()?; + if !matches!(face.surface(), FaceSurface::Plane { .. }) { + return None; + } + let wire = topo.wire(face.outer_wire()).ok()?; + if wire.edges().len() != 1 { + return None; + } + let edge = topo.edge(wire.edges()[0].edge()).ok()?; + match edge.curve() { + EdgeCurve::Circle(c) => Some(c.clone()), + _ => None, + } +} + +/// Whether the pave block's edge midpoint lies strictly inside (not on) the +/// disc bounded by `circle` — its in-plane distance from the centre is below +/// the radius by more than tolerance. +fn pb_strictly_inside_circle( + topo: &Topology, + arena: &GfaArena, + pb_id: crate::ds::PaveBlockId, + circle: &brepkit_math::curves::Circle3D, +) -> bool { + let Some(pb) = arena.pave_blocks.get(pb_id) else { + return false; + }; + let Ok(edge) = topo.edge(pb.original_edge) else { + return false; + }; + let (Ok(sv), Ok(ev)) = (topo.vertex(edge.start()), topo.vertex(edge.end())) else { + return false; + }; + let (sp, ep) = (sv.point(), ev.point()); + let (t0, t1) = edge.curve().domain_with_endpoints(sp, ep); + let mid = edge + .curve() + .evaluate_with_endpoints(0.5 * (t0 + t1), sp, ep); + let radial = mid - circle.center(); + let in_plane = radial - circle.normal() * radial.dot(circle.normal()); + in_plane.length() < circle.radius() - SEAM_ON_CIRCLE_TOL +} + /// Convert section sources to `SectionEdge` entries. /// /// For intersection curves, uses the complete curve geometry (not individual @@ -1027,6 +1191,7 @@ fn build_section_edges( arena: &GfaArena, face_id: FaceId, section_map: &HashMap>, + seam_anchors: &BTreeMap, tol: f64, ) -> Vec { use brepkit_math::vec::Point3; @@ -1076,7 +1241,24 @@ fn build_section_edges( // it lies entirely on the boundary. Feeding it to the face // splitter would corrupt the wire (the boundary circle gets // re-split against its own geometry). - if closed_curve_coincides_with_boundary(topo, face_id, &curve_ds.curve, tol) { + // + // This only holds when the circle is genuinely redundant: it + // bounds a region that lies fully on the partner FF face (a + // flush coplanar cap sitting inside a wall). When the circle + // is a real section larger than the partner — e.g. a cylinder + // rim where another, narrower cylinder's cap plane slices the + // lateral — keeping the lateral whole drops the split the + // cut/fuse needs and yields invalid topology. Require the + // partner face to host the full circle within its extent + // before treating the coincidence as a pure self-boundary. + let partner = if curve_ds.face_a == face_id { + curve_ds.face_b + } else { + curve_ds.face_a + }; + if closed_curve_coincides_with_boundary(topo, face_id, &curve_ds.curve, tol) + && circle_inside_face(topo, partner, &curve_ds.curve, tol) + { continue; } @@ -1088,8 +1270,33 @@ fn build_section_edges( _ => continue, }; + // Seam-anchored closed circles: re-parameterize so the + // circle starts at the periodic face's seam point. Both + // faces of the pair receive the same anchored geometry. + let (curve_3d, start, end) = match seam_anchors.get(curve_idx) { + Some(&anchor) => { + let reanchored = if let EdgeCurve::Circle(c) = &curve_ds.curve { + brepkit_math::curves::Circle3D::new_with_ref( + c.center(), + c.normal(), + c.radius(), + anchor - c.center(), + ) + .ok() + .map(EdgeCurve::Circle) + } else { + None + }; + match reanchored { + Some(c) => (c, anchor, anchor), + None => (curve_ds.curve.clone(), start, end), + } + } + None => (curve_ds.curve.clone(), start, end), + }; + let pcurve = super::pcurve_compute::compute_pcurve_on_surface( - &curve_ds.curve, + &curve_3d, start, end, face.surface(), @@ -1109,12 +1316,9 @@ fn build_section_edges( let start_uv = face.surface().project_point(start); if let Some((su, sv)) = start_uv { // Sample the curve at t=0.25 to determine winding direction. - let (t0, t1) = curve_ds.curve.domain_with_endpoints(start, end); - let mid_3d = curve_ds.curve.evaluate_with_endpoints( - t0 + (t1 - t0) * 0.25, - start, - end, - ); + let (t0, t1) = curve_3d.domain_with_endpoints(start, end); + let mid_3d = + curve_3d.evaluate_with_endpoints(t0 + (t1 - t0) * 0.25, start, end); let mid_uv = face.surface().project_point(mid_3d); if let Some((mu, _mv)) = mid_uv { // Determine winding: does the curve go in +u or -u @@ -1141,7 +1345,7 @@ fn build_section_edges( }; sections.push(SectionEdge { - curve_3d: curve_ds.curve.clone(), + curve_3d, pcurve_a: pcurve.clone(), pcurve_b: pcurve, start, @@ -1240,6 +1444,73 @@ fn build_section_edges( sections } +/// Whether the closed `curve` (a circle/ellipse) is contained within +/// `face`'s 3D extent (its outer-wire bounding box, expanded by tolerance). +/// +/// Distinguishes a flush coplanar cap whose rim sits inside a wall (the +/// circle fits the wall → redundant, skip-safe) from a rim that extends +/// beyond a smaller partner face (e.g. a cylinder rim slicing a narrower +/// cylinder's cap → a real section that must split the partner, never skip). +fn circle_inside_face( + topo: &Topology, + face_id: FaceId, + curve: &brepkit_topology::edge::EdgeCurve, + tol: f64, +) -> bool { + let Ok(face) = topo.face(face_id) else { + return false; + }; + let Ok(wire) = topo.wire(face.outer_wire()) else { + return false; + }; + let mut min = Point3::new(f64::MAX, f64::MAX, f64::MAX); + let mut max = Point3::new(f64::MIN, f64::MIN, f64::MIN); + let mut have = false; + for oe in wire.edges() { + let Ok(edge) = topo.edge(oe.edge()) else { + return false; + }; + let (Ok(sv), Ok(ev)) = (topo.vertex(edge.start()), topo.vertex(edge.end())) else { + return false; + }; + let (sp, ep) = (sv.point(), ev.point()); + let (t0, t1) = edge.curve().domain_with_endpoints(sp, ep); + for k in 0..=8 { + #[allow(clippy::cast_precision_loss)] + let frac = k as f64 / 8.0; + let p = edge + .curve() + .evaluate_with_endpoints((t1 - t0).mul_add(frac, t0), sp, ep); + min = Point3::new(min.x().min(p.x()), min.y().min(p.y()), min.z().min(p.z())); + max = Point3::new(max.x().max(p.x()), max.y().max(p.y()), max.z().max(p.z())); + have = true; + } + } + if !have { + return false; + } + + // Closed Circle/Ellipse: evaluate_with_endpoints ignores the reference + // points and dispatches to the parametric domain directly. + let origin = Point3::new(0.0, 0.0, 0.0); + let (t0, t1) = curve.domain_with_endpoints(origin, origin); + for k in 0..16 { + #[allow(clippy::cast_precision_loss)] + let frac = k as f64 / 16.0; + let p = curve.evaluate_with_endpoints((t1 - t0).mul_add(frac, t0), origin, origin); + if p.x() < min.x() - tol + || p.x() > max.x() + tol + || p.y() < min.y() - tol + || p.y() > max.y() + tol + || p.z() < min.z() - tol + || p.z() > max.z() + tol + { + return false; + } + } + true +} + /// Check whether a closed section curve (Circle/Ellipse) coincides with /// one of the face's own closed boundary edges. fn closed_curve_coincides_with_boundary( diff --git a/crates/algo/src/builder/same_domain.rs b/crates/algo/src/builder/same_domain.rs index 6e73110e..a7dd284d 100644 --- a/crates/algo/src/builder/same_domain.rs +++ b/crates/algo/src/builder/same_domain.rs @@ -411,7 +411,14 @@ fn planar_faces_overlap(topo: &Topology, sub_faces: &[SubFace], i: usize, j: usi return false; }; + // Sample each edge into several points along its curve, not just the + // start vertex. A closed wire built from a single circular edge (a + // circular hole left by an earlier cut) has one start vertex, so a + // vertex-only polygon collapses to a single point and the hole + // containment test silently treats the hole as absent — letting a + // coincident coplanar face be wrongly cancelled through the hole. let wire_points = |wire_id: brepkit_topology::wire::WireId| -> Vec { + let samples_per_edge: usize = 8; let mut pts = Vec::new(); let Ok(wire) = topo.wire(wire_id) else { return pts; @@ -420,13 +427,20 @@ fn planar_faces_overlap(topo: &Topology, sub_faces: &[SubFace], i: usize, j: usi let Ok(edge) = topo.edge(oe.edge()) else { continue; }; - let vid = if oe.is_forward() { - edge.start() - } else { - edge.end() + let (Ok(sv), Ok(ev)) = (topo.vertex(edge.start()), topo.vertex(edge.end())) else { + continue; }; - if let Ok(v) = topo.vertex(vid) { - pts.push(v.point()); + let (sp, ep) = (sv.point(), ev.point()); + let (t0, t1) = edge.curve().domain_with_endpoints(sp, ep); + for k in 0..samples_per_edge { + #[allow(clippy::cast_precision_loss)] + let frac = k as f64 / samples_per_edge as f64; + let t = if oe.is_forward() { + (t1 - t0).mul_add(frac, t0) + } else { + (t0 - t1).mul_add(frac, t1) + }; + pts.push(edge.curve().evaluate_with_endpoints(t, sp, ep)); } } pts diff --git a/crates/algo/src/classifier/ray_cast.rs b/crates/algo/src/classifier/ray_cast.rs index 722132a0..b393ad37 100644 --- a/crates/algo/src/classifier/ray_cast.rs +++ b/crates/algo/src/classifier/ray_cast.rs @@ -16,6 +16,31 @@ use brepkit_topology::solid::SolidId; use crate::builder::FaceClass; use crate::error::AlgoError; +/// Per-face geometry used for ray crossing tests. +enum FaceGeom { + /// A planar (or planar-approximated) face: boundary polygon, hole + /// polygons, and the supporting plane. + Planar { + verts: Vec, + holes: Vec>, + normal: Vec3, + d: f64, + }, + /// A full-period cylindrical face (e.g. a bore lateral). Crossings are + /// computed analytically — a flat polygon approximation counts one + /// crossing where the real surface has two, flipping the parity. + /// + /// `hole_bands` are full-circumference v-ranges carved out of the lateral + /// (a flush-cap interaction can leave such a holed lateral). A crossing + /// whose axial parameter falls inside a hole band is excluded. + Cylinder { + surface: brepkit_math::surfaces::CylindricalSurface, + v_min: f64, + v_max: f64, + hole_bands: Vec<(f64, f64)>, + }, +} + /// Classify a point by ray casting against the solid's faces. /// /// Shoots 3 rays (+Z, +X, +Y) and uses majority vote. A point is @@ -30,7 +55,7 @@ pub fn classify_ray_cast( solid: SolidId, point: Point3, ) -> Result { - let face_data = collect_face_polygons(topo, solid)?; + let face_data = collect_face_geoms(topo, solid)?; if face_data.is_empty() { return Err(AlgoError::ClassificationFailed( @@ -48,8 +73,8 @@ pub fn classify_ray_cast( let mut inside_votes = 0u8; for ray_dir in &ray_dirs { let mut crossings = 0i32; - for &(ref verts, normal, d) in &face_data { - crossings += ray_face_crossing(point, *ray_dir, verts, normal, d, tol); + for geom in &face_data { + crossings += ray_geom_crossings(point, *ray_dir, geom, tol); } if crossings % 2 != 0 { inside_votes += 1; @@ -63,43 +88,159 @@ pub fn classify_ray_cast( } } -/// Collect face polygons from a solid for ray-cast testing. +/// Sample a wire into a polygon by geometrically chaining its edges. /// -/// For each face, samples boundary edges to get polygon vertices. For -/// curved edges, samples at the midpoint for better coverage. -fn collect_face_polygons( +/// Wires are not guaranteed to list edges in traversal order (primitive +/// builders store edge sets), so each edge is sampled into a polyline and +/// the polylines are chained by endpoint matching. Closed curved edges +/// (full circles) get dense sampling; open curved edges get interior +/// samples for better coverage. +fn wire_polygon( topo: &Topology, - solid: SolidId, -) -> Result, Vec3, f64)>, AlgoError> { + wire_id: brepkit_topology::wire::WireId, +) -> Result, AlgoError> { + let wire = topo.wire(wire_id)?; + + // Sample each edge into a polyline in its oriented direction. + let mut polylines: Vec> = Vec::with_capacity(wire.edges().len()); + for oe in wire.edges() { + let edge = topo.edge(oe.edge())?; + let raw_start = topo.vertex(edge.start())?.point(); + let raw_end = topo.vertex(edge.end())?.point(); + let mut pts = vec![raw_start]; + if !matches!(edge.curve(), brepkit_topology::edge::EdgeCurve::Line) { + let (t0, t1) = edge.curve().domain_with_endpoints(raw_start, raw_end); + let is_closed = (raw_start - raw_end).length() < 1e-9; + let n_samples = if is_closed { 16_i32 } else { 3_i32 }; + for k in 1..=n_samples { + let t = t0 + (t1 - t0) * f64::from(k) / f64::from(n_samples + 1); + pts.push(edge.curve().evaluate_with_endpoints(t, raw_start, raw_end)); + } + } + pts.push(raw_end); + if !oe.is_forward() { + pts.reverse(); + } + polylines.push(pts); + } + + // Chain polylines by endpoint proximity. + let join_tol = 1e-6; + let mut used = vec![false; polylines.len()]; + let mut verts: Vec = Vec::new(); + let Some(first) = polylines.first() else { + return Ok(verts); + }; + verts.extend_from_slice(first); + used[0] = true; + for _ in 1..polylines.len() { + let tail = match verts.last() { + Some(p) => *p, + None => break, + }; + let next = polylines.iter().enumerate().find_map(|(i, pl)| { + if used[i] { + return None; + } + let s = *pl.first()?; + let e = *pl.last()?; + if (s - tail).length() < join_tol { + Some((i, false)) + } else if (e - tail).length() < join_tol { + Some((i, true)) + } else { + None + } + }); + let Some((idx, rev)) = next else { break }; + used[idx] = true; + let mut pl = polylines[idx].clone(); + if rev { + pl.reverse(); + } + verts.extend_from_slice(&pl[1..]); + } + // Append any unchained polylines so no geometry is silently lost + // (matches the previous behavior of emitting all edge samples). + for (i, pl) in polylines.iter().enumerate() { + if !used[i] { + verts.extend_from_slice(pl); + } + } + // Drop the duplicated closing point. + if verts.len() >= 2 { + let first_pt = verts[0]; + if let Some(last) = verts.last() { + if (*last - first_pt).length() < join_tol { + verts.pop(); + } + } + } + Ok(verts) +} + +/// Collect per-face ray-cast geometry from a solid. +fn collect_face_geoms(topo: &Topology, solid: SolidId) -> Result, AlgoError> { let faces = brepkit_topology::explorer::solid_faces(topo, solid)?; let mut result = Vec::with_capacity(faces.len()); for fid in faces { let face = topo.face(fid)?; - let wire = topo.wire(face.outer_wire())?; - - let mut verts = Vec::new(); - for oe in wire.edges() { - let edge = topo.edge(oe.edge())?; - let start_pos = topo.vertex(edge.start())?.point(); - let end_pos = topo.vertex(edge.end())?.point(); - verts.push(start_pos); - // For curved edges, add a midpoint sample. - if !matches!(edge.curve(), brepkit_topology::edge::EdgeCurve::Line) { - let (t0, t1) = edge.curve().domain_with_endpoints(start_pos, end_pos); - let t_mid = 0.5_f64.mul_add(t1 - t0, t0); - let mid = edge - .curve() - .evaluate_with_endpoints(t_mid, start_pos, end_pos); - verts.push(mid); + // Full-period cylindrical faces: the outer wire contains a closed + // circle edge, so the face wraps the entire circumference and the + // analytic crossing test applies. Inner wires are accepted only when + // each is a full-circumference v-band (the shape a flush-cap + // interaction carves out); any non-banded hole forces the polygon + // fallback. Partial cylinder patches also fall through. + if let brepkit_topology::face::FaceSurface::Cylinder(cyl) = face.surface() { + let wire = topo.wire(face.outer_wire())?; + let mut has_closed_circle = false; + for oe in wire.edges() { + let edge = topo.edge(oe.edge())?; + if matches!(edge.curve(), brepkit_topology::edge::EdgeCurve::Circle(_)) + && edge.start() == edge.end() + { + has_closed_circle = true; + break; + } + } + if has_closed_circle { + let verts = wire_polygon(topo, face.outer_wire())?; + let mut v_min = f64::INFINITY; + let mut v_max = f64::NEG_INFINITY; + for p in &verts { + let (_, v) = cyl.project_point(*p); + v_min = v_min.min(v); + v_max = v_max.max(v); + } + let hole_bands = cylinder_hole_bands(topo, face, cyl)?; + let holes_banded = hole_bands.len() == face.inner_wires().len(); + if v_min.is_finite() && v_max > v_min && holes_banded { + result.push(FaceGeom::Cylinder { + surface: cyl.clone(), + v_min, + v_max, + hole_bands, + }); + continue; + } } } + let verts = wire_polygon(topo, face.outer_wire())?; if verts.len() < 3 { continue; } + let mut holes = Vec::with_capacity(face.inner_wires().len()); + for &iw in face.inner_wires() { + let hole = wire_polygon(topo, iw)?; + if hole.len() >= 3 { + holes.push(hole); + } + } + // Compute face normal. let raw_normal = if let brepkit_topology::face::FaceSurface::Plane { normal, .. } = face.surface() { @@ -114,20 +255,87 @@ fn collect_face_polygons( }; let d = dot_normal_point(normal, verts[0]); - result.push((verts, normal, d)); + result.push(FaceGeom::Planar { + verts, + holes, + normal, + d, + }); } Ok(result) } +/// Collect full-circumference v-band holes carved out of a cylindrical face. +/// +/// Each inner wire is sampled and projected into `(u, v)`. A wire is treated +/// as a band only when its u-samples wrap the full circumference; the band's +/// `[v_lo, v_hi]` is the projected axial span. Returns one entry per qualifying +/// inner wire — a count short of `face.inner_wires().len()` signals a +/// non-banded hole, which the caller uses to force the polygon fallback. +fn cylinder_hole_bands( + topo: &Topology, + face: &brepkit_topology::face::Face, + cyl: &brepkit_math::surfaces::CylindricalSurface, +) -> Result, AlgoError> { + use std::f64::consts::TAU; + + let mut bands = Vec::with_capacity(face.inner_wires().len()); + for &iw in face.inner_wires() { + let pts = wire_polygon(topo, iw)?; + if pts.len() < 3 { + continue; + } + let mut v_lo = f64::INFINITY; + let mut v_hi = f64::NEG_INFINITY; + let mut u_min = f64::INFINITY; + let mut u_max = f64::NEG_INFINITY; + for p in &pts { + let (u, v) = cyl.project_point(*p); + v_lo = v_lo.min(v); + v_hi = v_hi.max(v); + u_min = u_min.min(u); + u_max = u_max.max(u); + } + // Only full-circumference bands qualify: a partial-arc hole would be + // over-excluded by a v-band test. + let wraps_full = (u_max - u_min) >= TAU - 1e-3; + if wraps_full && v_hi > v_lo { + bands.push((v_lo, v_hi)); + } + } + Ok(bands) +} + +/// Count ray crossings against a face geometry. +#[inline] +fn ray_geom_crossings(origin: Point3, ray_dir: Vec3, geom: &FaceGeom, tol: Tolerance) -> i32 { + match geom { + FaceGeom::Planar { + verts, + holes, + normal, + d, + } => ray_face_crossing(origin, ray_dir, verts, holes, *normal, *d, tol), + FaceGeom::Cylinder { + surface, + v_min, + v_max, + hole_bands, + } => ray_cylinder_crossings(origin, ray_dir, surface, *v_min, *v_max, hole_bands, tol), + } +} + /// Test a single face polygon against a ray for crossing parity. /// -/// Returns +1 for a crossing, 0 for no intersection. +/// Returns +1 for a crossing, 0 for no intersection. Hits inside a hole +/// polygon do not count. #[inline] fn ray_face_crossing( origin: Point3, ray_dir: Vec3, verts: &[Point3], + holes: &[Vec], normal: Vec3, d: f64, tol: Tolerance, @@ -146,11 +354,72 @@ fn ray_face_crossing( origin.y() + ray_dir.y() * t, origin.z() + ray_dir.z() * t, ); - if point_in_face_3d(hit, verts, &normal) { - 1 - } else { - 0 + if !point_in_face_3d(hit, verts, &normal) { + return 0; + } + if holes.iter().any(|h| point_in_face_3d(hit, h, &normal)) { + return 0; + } + 1 +} + +/// Count ray crossings with a bounded full-period cylindrical face. +/// +/// Solves the ray/infinite-cylinder quadratic and counts roots whose axial +/// parameter falls within the face's v-range but outside any `hole_bands` +/// (full-circumference v-ranges carved out of the lateral). Tangent grazes +/// (discriminant ≈ 0) count as zero crossings, which preserves parity. +fn ray_cylinder_crossings( + origin: Point3, + ray_dir: Vec3, + surface: &brepkit_math::surfaces::CylindricalSurface, + v_min: f64, + v_max: f64, + hole_bands: &[(f64, f64)], + tol: Tolerance, +) -> i32 { + let axis = surface.axis(); + let m = origin - surface.origin(); + let d_perp = ray_dir - axis * ray_dir.dot(axis); + let m_perp = m - axis * m.dot(axis); + + let a = d_perp.dot(d_perp); + if a < 1e-14 { + return 0; + } + let b = 2.0 * m_perp.dot(d_perp); + let c = surface + .radius() + .mul_add(-surface.radius(), m_perp.dot(m_perp)); + let disc = b.mul_add(b, -4.0 * a * c); + // Treat near-tangent rays as misses: counting one graze flips parity. + if disc < 1e-12 * a * surface.radius() * surface.radius() { + return 0; + } + let sqrt_disc = disc.sqrt(); + let mut crossings = 0; + for t in [(-b - sqrt_disc) / (2.0 * a), (-b + sqrt_disc) / (2.0 * a)] { + if t <= tol.linear { + continue; + } + let hit = Point3::new( + origin.x() + ray_dir.x() * t, + origin.y() + ray_dir.y() * t, + origin.z() + ray_dir.z() * t, + ); + let v = axis.dot(hit - surface.origin()); + if v < v_min - tol.linear || v > v_max + tol.linear { + continue; + } + if hole_bands + .iter() + .any(|&(lo, hi)| v > lo + tol.linear && v < hi - tol.linear) + { + continue; + } + crossings += 1; } + crossings } /// Test if a 3D point lies inside a planar face polygon by projecting to 2D. diff --git a/crates/algo/src/pave_filler/tests.rs b/crates/algo/src/pave_filler/tests.rs index fb8bc459..c1fa0ac4 100644 --- a/crates/algo/src/pave_filler/tests.rs +++ b/crates/algo/src/pave_filler/tests.rs @@ -1277,15 +1277,6 @@ fn solid_topology_summary( } #[test] -#[ignore = "Gap: GFA `split_face_with_internal_loops` applies a disc-loop \ - interpretation to closed section circles, which is correct only \ - for plane faces. On a cylinder lateral, a single closed circle \ - doesn't bound a disc — two parallel circles split the cylinder \ - into 3 bands. Fixing this requires a `split_periodic_face_into_bands` \ - that constructs band sub-faces (bottom circle + seam + top circle \ - reversed + seam reversed) AND avoids regressing compound_cut grids \ - (where multiple cylinders share section planes). Tracked: \ - gridfinity-layout-tool #260 / #270."] fn gfa_cut_box_cylinder_through_produces_valid_topology() { // Box [0,10]^3 with a cylinder r=1 at (5,5) piercing fully through // (z=-2 to z=12). The result should be a closed manifold solid: @@ -1313,7 +1304,7 @@ fn gfa_cut_box_cylinder_through_produces_valid_topology() { // Manifold check: every edge must be shared by exactly 2 faces. let s = topo.solid(result).unwrap(); let sh = topo.shell(s.outer_shell()).unwrap(); - let manifold = brepkit_topology::validation::validate_shell_manifold(sh, &topo); + let manifold = brepkit_topology::validation::validate_shell_closed(sh, &topo); assert!( manifold.is_ok(), "result shell must be manifold, got {manifold:?}" @@ -1327,21 +1318,6 @@ fn gfa_cut_box_cylinder_through_produces_valid_topology() { } #[test] -#[ignore = "Gap: GFA-direct sequential cuts on a 4×4 cylinder grid degrade \ - after the first iteration. As of PR #535: all 16 boolean calls \ - return Ok, but the final topology is F=8 E=17 V=12 (Euler=3, \ - non-manifold — edge shared by 3 faces) instead of the \ - expected F=22 Euler=2. Only the first cylinder's geometry \ - survives in the result; subsequent cuts silently no-op or \ - corrupt the prior topology. The operations-layer companion \ - `compound_cut_matches_sequential_4x4_grid` masks this because \ - both compound and sequential paths fall back to mesh-boolean \ - (boolean/tests.rs:1542). This is the regression carrier PR #535 \ - cited when reverting the band-merger attempt: any \ - `merge_periodic_discs_into_bands` post-pass must repair the \ - single-cyl case (see sibling test) WITHOUT activating in a way \ - that lets BOP misclassify band interiors across grid cylinders \ - sharing section planes. Tracked: gridfinity-layout-tool #260 / #270."] fn gfa_cut_box_cylinder_grid_through_sequential_produces_valid_topology() { // Slab [0,20]×[0,20]×[0,2] cut by a 4×4 grid of cylinders r=0.5, // z = -1 to +3 (piercing fully through), positions (2 + col*4, 2 + row*4) @@ -1349,10 +1325,7 @@ fn gfa_cut_box_cylinder_grid_through_sequential_produces_valid_topology() { // // This mirrors `compound_cut_matches_sequential_4x4_grid` in // `operations/src/boolean/tests.rs`, but calls `crate::gfa::boolean` - // directly — the operations-layer test passes only because both - // compound and sequential paths fall through to the mesh-boolean - // fallback (see comment at boolean/tests.rs:1542). This test bypasses - // the fallback so the GFA-direct failure surfaces. + // directly so no mesh-boolean fallback can mask a GFA failure. // // Expected result (closed manifold): // - 4 box side faces @@ -1362,11 +1335,6 @@ fn gfa_cut_box_cylinder_grid_through_sequential_produces_valid_topology() { // reversed + seam reversed) // // Total: 22 faces. Euler V-E+F = 2 for a closed manifold of genus 0. - // - // Companion to PR #534 (single-cyl pinning). Resolution likely shares - // a `merge_periodic_discs_into_bands` post-pass with that test — - // see PR #535 follow-up notes for the architectural sketch and the - // gating problem (this grid case is the regression carrier). let mut topo = Topology::default(); let mut target = make_box(&mut topo, [0.0, 0.0, 0.0], [20.0, 20.0, 2.0]); @@ -1402,7 +1370,7 @@ fn gfa_cut_box_cylinder_grid_through_sequential_produces_valid_topology() { let s = topo.solid(target).unwrap(); let sh = topo.shell(s.outer_shell()).unwrap(); - let manifold = brepkit_topology::validation::validate_shell_manifold(sh, &topo); + let manifold = brepkit_topology::validation::validate_shell_closed(sh, &topo); assert!( manifold.is_ok(), "result shell must be manifold, got {manifold:?}" @@ -1542,3 +1510,47 @@ fn gfa_cut_box_cylinder_coplanar_caps_produces_valid_topology() { "volume {vol:.6} should be within 0.1% of {expected_vol:.6} (rel={rel:.5})" ); } + +/// Sequential coplanar-cap cuts: a second flush-cap cylinder cut onto a box +/// wall that already carries a circular hole from the first cut. The wall's +/// existing circular hole must stay visible to the same-domain hole test +/// (sampled, not vertex-only) so the wall face is not cancelled, and the +/// second tool's flush cap must not fragment into a many-sided polygon that +/// survives the cut. Expected: 4 sides + 2 holed caps + 2 lateral walls. +#[test] +fn gfa_cut_box_two_coplanar_cap_cylinders_sequential_valid() { + let mut topo = Topology::default(); + let mut target = make_box(&mut topo, [0.0, 0.0, 0.0], [4.0, 4.0, 2.0]); + for (cx, cy) in [(1.0, 1.0), (3.0, 3.0)] { + let cyl = make_cylinder(&mut topo, cx, cy, 0.0, 0.3, 2.0); + target = crate::gfa::boolean(&mut topo, crate::bop::BooleanOp::Cut, target, cyl) + .expect("sequential coplanar-cap cut should succeed"); + } + + let (f, e, v, euler) = solid_topology_summary(&topo, target); + assert_eq!(f, 8, "expected 4 sides + 2 caps + 2 laterals, got {f}"); + assert_eq!( + euler, 2, + "Euler V-E+F should be 2 for a closed genus-2 manifold, got V={v} E={e} F={f}" + ); + + let s = topo.solid(target).unwrap(); + let sh = topo.shell(s.outer_shell()).unwrap(); + let manifold = brepkit_topology::validation::validate_shell_closed(sh, &topo); + assert!( + manifold.is_ok(), + "result must be manifold, got {manifold:?}" + ); + + let expected_vol = 32.0 - 2.0 * std::f64::consts::PI * 0.3 * 0.3 * 2.0; + let options = brepkit_check::properties::PropertiesOptions { + gauss_order: 16, + ..Default::default() + }; + let vol = brepkit_check::properties::solid_volume(&topo, target, &options).unwrap(); + let rel = (vol - expected_vol).abs() / expected_vol; + assert!( + rel < 0.001, + "volume {vol:.6} should be within 0.1% of {expected_vol:.6} (rel={rel:.5})" + ); +} diff --git a/crates/operations/src/boolean/mod.rs b/crates/operations/src/boolean/mod.rs index 03fd11b0..b1151c64 100644 --- a/crates/operations/src/boolean/mod.rs +++ b/crates/operations/src/boolean/mod.rs @@ -515,7 +515,15 @@ pub fn boolean( let (f, e, v) = brepkit_topology::explorer::solid_entity_counts(topo, result)?; #[allow(clippy::cast_possible_wrap)] let euler = (v as i64) - (e as i64) + (f as i64); - if euler == 2 && validate_boolean_result(topo, result).is_ok() { + // Free edges in an Intersect result mean faces were dropped + // (e.g. a tolerance-thin sliver kept only some of its + // bounding faces) — reject even when Euler accidentally + // balances. Cut and Fuse keep the legacy lenient gate: some + // coplanar cut/fuse results carry boundary edges yet are + // still the best available output (the mesh fallback loses + // more volume than the open GFA shell does). + let open_shell_ok = op != BooleanOp::Intersect || !has_free_edges(topo, result)?; + if euler == 2 && open_shell_ok && validate_boolean_result(topo, result).is_ok() { log::info!( "GFA boolean succeeded in {:.1}ms ({result_faces} faces)", timer_elapsed_ms(gfa_start) @@ -2232,6 +2240,28 @@ fn is_closed_manifold(topo: &Topology, solid: SolidId) -> Result Result { + use std::collections::HashMap; + + let s = topo.solid(solid)?; + let shell = topo.shell(s.outer_shell())?; + let mut counts: HashMap = HashMap::new(); + for &fid in shell.faces() { + let face = topo.face(fid)?; + for wid in std::iter::once(face.outer_wire()).chain(face.inner_wires().iter().copied()) { + let wire = topo.wire(wid)?; + for oe in wire.edges() { + *counts.entry(oe.edge().index()).or_insert(0) += 1; + } + } + } + Ok(counts.values().any(|&c| c == 1)) +} + /// For each vertex position (quantized at tolerance), picks one canonical /// vertex. Rebuilds all edges and wires to use canonical vertices. /// Creates new edges (doesn't mutate existing ones) to avoid corrupting diff --git a/crates/operations/src/boolean/tests.rs b/crates/operations/src/boolean/tests.rs index 3967ea13..35c3fa0e 100644 --- a/crates/operations/src/boolean/tests.rs +++ b/crates/operations/src/boolean/tests.rs @@ -1337,7 +1337,6 @@ fn profile_cylinder_cylinder_intersect() { /// Verify that `cut(box, cylinder)` produces a reasonable edge count /// with proper Circle edges (not tessellated into N line segments). #[test] -#[ignore = "GFA pipeline limitation — old boolean pipeline removed"] fn box_cut_cylinder_edge_count() { let mut topo = Topology::new(); @@ -1550,7 +1549,6 @@ fn compound_cut_empty_tools_returns_target() { } #[test] -#[ignore = "GFA pipeline limitation — old boolean pipeline removed"] fn compound_cut_single_tool_matches_boolean() { use brepkit_math::mat::Mat4; @@ -1569,7 +1567,6 @@ fn compound_cut_single_tool_matches_boolean() { } #[test] -#[ignore = "GFA pipeline limitation — old boolean pipeline removed"] fn compound_cut_two_disjoint_cylinders() { use brepkit_math::mat::Mat4; @@ -1607,12 +1604,6 @@ fn compound_cut_all_tools_disjoint_returns_unchanged_volume() { } #[test] -#[ignore = "Gap: cuts after the first lose the cap faces — the face splitter \ - drops faces that carry pre-existing internal hole loops when a new \ - coplanar closed section arrives, so cuts 2-4 fall back to the mesh \ - path and the absolute volume lands ~27% low. The first coplanar-cap \ - cut is fixed (seam adoption); re-enable when internal-loop face \ - splitting lands."] fn compound_cut_matches_sequential_2x2_grid() { use brepkit_math::mat::Mat4; @@ -2722,7 +2713,6 @@ fn sequential_boolean_face_count_bounded() { /// Regression test for #270: without the mesh boolean threshold, the /// chord-based path preserves `FaceSurface::Cylinder` variants. #[test] -#[ignore = "GFA pipeline limitation — old boolean pipeline removed"] fn sequential_cut_preserves_surface_types() { let mut topo = Topology::new(); let base = crate::primitives::make_box(&mut topo, 10.0, 10.0, 5.0).unwrap(); diff --git a/crates/operations/src/measure/mod.rs b/crates/operations/src/measure/mod.rs index be78c897..4ee87ce4 100644 --- a/crates/operations/src/measure/mod.rs +++ b/crates/operations/src/measure/mod.rs @@ -575,7 +575,6 @@ mod tests { /// /// Regression test for the cylinder band classification bug. #[test] - #[ignore = "GFA pipeline limitation -- old boolean pipeline removed"] fn cut_box_cylinder_volume_decreases() { use crate::boolean::{BooleanOp, boolean}; use crate::primitives::{make_box, make_cylinder}; diff --git a/crates/operations/tests/boolean_stress.rs b/crates/operations/tests/boolean_stress.rs index 9f4d6950..1f71219c 100644 --- a/crates/operations/tests/boolean_stress.rs +++ b/crates/operations/tests/boolean_stress.rs @@ -465,7 +465,6 @@ fn disjoint_intersect_empty() { // =========================================================================== #[test] -#[ignore = "GFA pipeline limitation — old boolean pipeline removed"] fn cut_cylinder_from_box() { let mut topo = Topology::new(); let base = box_at(&mut topo, 0.0, 0.0, 0.0, 4.0, 4.0, 4.0); @@ -858,7 +857,6 @@ fn volume_large_boxes() { // =========================================================================== #[test] -#[ignore = "GFA pipeline limitation — old boolean pipeline removed"] fn cut_cylinder_from_box_volume() { // Box 4×4×4 at origin (z=0..4), cylinder r=1, h=6 at z=0..6 // (make_cylinder places base at z=0), translated to (2,2,-1) → z=-1..5. diff --git a/crates/operations/tests/golden_regression.rs b/crates/operations/tests/golden_regression.rs index 162d591f..c3b8af88 100644 --- a/crates/operations/tests/golden_regression.rs +++ b/crates/operations/tests/golden_regression.rs @@ -178,7 +178,6 @@ fn golden_cone_r2_r0_h5() { } #[test] -#[ignore = "GFA pipeline limitation — old boolean pipeline removed"] fn golden_boolean_box_minus_cylinder() { let mut topo = Topology::new(); let box_solid = make_box(&mut topo, 10.0, 10.0, 10.0).unwrap(); diff --git a/crates/topology/src/validation.rs b/crates/topology/src/validation.rs index f8843300..33483838 100644 --- a/crates/topology/src/validation.rs +++ b/crates/topology/src/validation.rs @@ -108,6 +108,48 @@ pub fn validate_shell_manifold(shell: &Shell, topo: &Topology) -> Result<(), Top Ok(()) } +/// Validates that a shell is a closed 2-manifold. +/// +/// Stricter than [`validate_shell_manifold`]: every edge must be used by +/// exactly two oriented-edge occurrences across the shell's wires. Edges +/// used once (free/boundary edges of an open shell) are rejected, not just +/// edges shared by 3+ faces. +/// +/// # Errors +/// +/// Returns [`TopologyError::NonManifold`] if any edge usage count differs +/// from two. Returns entity-not-found errors if any referenced ID is +/// invalid. +pub fn validate_shell_closed(shell: &Shell, topo: &Topology) -> Result<(), TopologyError> { + let mut edge_counts: HashMap = HashMap::new(); + + for &face_id in shell.faces() { + let face = topo.face(face_id)?; + count_wire_edges(face.outer_wire(), topo, &mut edge_counts)?; + for &inner_wire_id in face.inner_wires() { + count_wire_edges(inner_wire_id, topo, &mut edge_counts)?; + } + } + + for (&edge_index, &count) in &edge_counts { + if count != 2 { + let kind = if count == 1 { + "free edge" + } else { + "over-shared" + }; + return Err(TopologyError::NonManifold { + reason: format!( + "edge index {edge_index} is used by {count} wires ({kind}; closed manifold \ + requires exactly 2)" + ), + }); + } + } + + Ok(()) +} + #[cfg(test)] mod tests { #![allow(clippy::unwrap_used)] @@ -201,6 +243,61 @@ mod tests { assert!(validate_shell_manifold(&shell, &topo).is_ok()); } + #[test] + fn closed_validation_rejects_free_edges() { + // A single triangular face: every edge is used once -> open shell. + let mut topo = Topology::new(); + let wid = make_triangle(&mut topo); + let normal = Vec3::new(0.0, 0.0, 1.0); + let f = topo.add_face(Face::new( + wid, + vec![], + FaceSurface::Plane { normal, d: 0.0 }, + )); + let shell = crate::shell::Shell::new(vec![f]).unwrap(); + + assert!(validate_shell_manifold(&shell, &topo).is_ok()); + let err = validate_shell_closed(&shell, &topo).unwrap_err(); + assert!( + matches!(err, TopologyError::NonManifold { .. }), + "expected NonManifold for free edges, got {err:?}" + ); + } + + #[test] + fn closed_validation_accepts_two_sided_triangle() { + // Two faces over the same three edges (front + back): every edge + // is used exactly twice. + let mut topo = Topology::new(); + let wid = make_triangle(&mut topo); + let back_oes: Vec = topo + .wire(wid) + .unwrap() + .edges() + .iter() + .rev() + .map(|oe| OrientedEdge::new(oe.edge(), !oe.is_forward())) + .collect(); + let back_wid = topo.add_wire(Wire::new(back_oes, true).unwrap()); + let normal = Vec3::new(0.0, 0.0, 1.0); + let f0 = topo.add_face(Face::new( + wid, + vec![], + FaceSurface::Plane { normal, d: 0.0 }, + )); + let f1 = topo.add_face(Face::new( + back_wid, + vec![], + FaceSurface::Plane { + normal: -normal, + d: 0.0, + }, + )); + let shell = crate::shell::Shell::new(vec![f0, f1]).unwrap(); + + assert!(validate_shell_closed(&shell, &topo).is_ok()); + } + #[test] fn non_manifold_three_face_shared_edge() { // Three faces sharing a single edge -> non-manifold. diff --git a/crates/wasm/src/bindings/gridfinity_tests.rs b/crates/wasm/src/bindings/gridfinity_tests.rs index 58132556..e378c517 100644 --- a/crates/wasm/src/bindings/gridfinity_tests.rs +++ b/crates/wasm/src/bindings/gridfinity_tests.rs @@ -444,7 +444,6 @@ fn compound_cut_then_measure() { /// /// Solids: 0=box, 1=cylinder, 2-4=copies, 5-7=cut results #[test] -#[ignore = "volume error 7% — boolean pipeline coplanar face classification (#260)"] fn sequential_booleans_volume_accuracy() { let mut k = BrepKernel::new(); let result = k.execute_batch( diff --git a/tests/golden/data/boolean_box_minus_cylinder.golden b/tests/golden/data/boolean_box_minus_cylinder.golden index 57edc856..0fc630c0 100644 --- a/tests/golden/data/boolean_box_minus_cylinder.golden +++ b/tests/golden/data/boolean_box_minus_cylinder.golden @@ -1,6 +1,6 @@ # box 10x10x10 minus cylinder r=3 h=10 at center -vertices: 113 -triangles: 191 +vertices: 80 +triangles: 156 volume: 717.762071 bbox_min: (0.000000, 0.000000, 0.000000) bbox_max: (10.000000, 10.000000, 10.000000)