From b65adcf649179a686c32f51873668b7a90c6c566 Mon Sep 17 00:00:00 2001 From: Andy Aragon Date: Tue, 9 Jun 2026 12:33:43 -0700 Subject: [PATCH 1/2] fix(algo): trim plane-plane section curves to mutual face overlap --- crates/algo/src/builder/classify_2d.rs | 42 ++++++++++- crates/algo/src/builder/same_domain.rs | 32 ++++++-- crates/algo/src/classifier/ray_cast.rs | 11 +-- crates/algo/src/diagnostic.rs | 11 +-- crates/algo/src/pave_filler/phase_ff.rs | 98 ++++++++++++++++++------- crates/algo/src/pave_filler/tests.rs | 22 +++--- crates/operations/src/boolean/tests.rs | 1 - crates/operations/tests/examples.rs | 1 - 8 files changed, 162 insertions(+), 56 deletions(-) diff --git a/crates/algo/src/builder/classify_2d.rs b/crates/algo/src/builder/classify_2d.rs index 197326aa..f7c31f82 100644 --- a/crates/algo/src/builder/classify_2d.rs +++ b/crates/algo/src/builder/classify_2d.rs @@ -30,8 +30,12 @@ pub fn sample_interior_point(loop_pts: &[Point2]) -> Point2 { let cy = loop_pts.iter().map(|p| p.y()).sum::() / n; let centroid = Point2::new(cx, cy); - // 2. If centroid is inside, use it. - if point_in_polygon_2d(centroid, loop_pts) { + // 2. If centroid is inside, use it. A centroid that lands on the + // boundary itself (e.g. the reflex corner of an L-shaped loop) passes + // the even-odd test unpredictably and is not a safe interior sample. + if point_in_polygon_2d(centroid, loop_pts) + && distance_to_polygon_boundary(centroid, loop_pts) > boundary_eps(loop_pts) + { return centroid; } @@ -72,6 +76,40 @@ pub fn sample_interior_point(loop_pts: &[Point2]) -> Point2 { centroid } +fn boundary_eps(loop_pts: &[Point2]) -> f64 { + let (mut min_x, mut min_y) = (f64::INFINITY, f64::INFINITY); + let (mut max_x, mut max_y) = (f64::NEG_INFINITY, f64::NEG_INFINITY); + for p in loop_pts { + min_x = min_x.min(p.x()); + min_y = min_y.min(p.y()); + max_x = max_x.max(p.x()); + max_y = max_y.max(p.y()); + } + let diag = ((max_x - min_x).powi(2) + (max_y - min_y).powi(2)).sqrt(); + (diag * 1e-6).max(1e-12) +} + +fn distance_to_polygon_boundary(p: Point2, loop_pts: &[Point2]) -> f64 { + let mut best = f64::INFINITY; + let n = loop_pts.len(); + for i in 0..n { + let a = loop_pts[i]; + let b = loop_pts[(i + 1) % n]; + let ab = Vec2::new(b.x() - a.x(), b.y() - a.y()); + let ap = Vec2::new(p.x() - a.x(), p.y() - a.y()); + let len_sq = ab.x() * ab.x() + ab.y() * ab.y(); + let t = if len_sq < 1e-30 { + 0.0 + } else { + ((ap.x() * ab.x() + ap.y() * ab.y()) / len_sq).clamp(0.0, 1.0) + }; + let dx = p.x() - (a.x() + ab.x() * t); + let dy = p.y() - (a.y() + ab.y() * t); + best = best.min((dx * dx + dy * dy).sqrt()); + } + best +} + /// Test whether a 2D point is inside a closed polygon using ray-casting /// (even-odd rule). pub fn point_in_polygon_2d(p: Point2, polygon: &[Point2]) -> bool { diff --git a/crates/algo/src/builder/same_domain.rs b/crates/algo/src/builder/same_domain.rs index 3df1957f..570cb926 100644 --- a/crates/algo/src/builder/same_domain.rs +++ b/crates/algo/src/builder/same_domain.rs @@ -411,9 +411,9 @@ fn planar_faces_overlap(topo: &Topology, sub_faces: &[SubFace], i: usize, j: usi return false; }; - let wire_points = |face: &brepkit_topology::face::Face| -> Vec { + let wire_points = |wire_id: brepkit_topology::wire::WireId| -> Vec { let mut pts = Vec::new(); - let Ok(wire) = topo.wire(face.outer_wire()) else { + let Ok(wire) = topo.wire(wire_id) else { return pts; }; for oe in wire.edges() { @@ -432,8 +432,8 @@ fn planar_faces_overlap(topo: &Topology, sub_faces: &[SubFace], i: usize, j: usi pts }; - let pts_i = wire_points(face_i); - let pts_j = wire_points(face_j); + let pts_i = wire_points(face_i.outer_wire()); + let pts_j = wire_points(face_j.outer_wire()); if pts_i.len() < 3 || pts_j.len() < 3 { return false; } @@ -459,13 +459,33 @@ fn planar_faces_overlap(topo: &Topology, sub_faces: &[SubFace], i: usize, j: usi .all(|&v| super::classify_2d::point_in_polygon_2d(v, poly)) }; + // A point landing inside one of the container's inner wires sits in a + // hole, not on the face — e.g. a frame face whose hole exactly hosts + // the candidate. Containment through a hole is not overlap. + let in_hole = |p: brepkit_math::vec::Point2, face: &brepkit_topology::face::Face| -> bool { + face.inner_wires().iter().any(|&wid| { + let pts = wire_points(wid); + if pts.len() < 3 { + return false; + } + let poly: Vec<_> = pts.iter().map(|&q| frame.project(q)).collect(); + super::classify_2d::point_in_polygon_2d(p, &poly) + }) + }; + // i fully contained in j: every vertex of i (plus its interior sample) // is inside j's polygon. - if super::classify_2d::point_in_polygon_2d(p_i_2d, &poly_j) && all_inside(&poly_i, &poly_j) { + if super::classify_2d::point_in_polygon_2d(p_i_2d, &poly_j) + && all_inside(&poly_i, &poly_j) + && !in_hole(p_i_2d, face_j) + { return true; } // j fully contained in i. - if super::classify_2d::point_in_polygon_2d(p_j_2d, &poly_i) && all_inside(&poly_j, &poly_i) { + if super::classify_2d::point_in_polygon_2d(p_j_2d, &poly_i) + && all_inside(&poly_j, &poly_i) + && !in_hole(p_j_2d, face_i) + { return true; } false diff --git a/crates/algo/src/classifier/ray_cast.rs b/crates/algo/src/classifier/ray_cast.rs index 271d393a..722132a0 100644 --- a/crates/algo/src/classifier/ray_cast.rs +++ b/crates/algo/src/classifier/ray_cast.rs @@ -333,18 +333,19 @@ mod tests { let e37 = edge(3, 7); let fwd = |eid| OrientedEdge::new(eid, true); + let rev = |eid| OrientedEdge::new(eid, false); let w_bot = - topo.add_wire(Wire::new(vec![fwd(e01), fwd(e12), fwd(e23), fwd(e30)], true).unwrap()); + topo.add_wire(Wire::new(vec![rev(e01), rev(e30), rev(e23), rev(e12)], true).unwrap()); let w_top = topo.add_wire(Wire::new(vec![fwd(e45), fwd(e56), fwd(e67), fwd(e74)], true).unwrap()); let w_front = - topo.add_wire(Wire::new(vec![fwd(e01), fwd(e15), fwd(e45), fwd(e04)], true).unwrap()); + topo.add_wire(Wire::new(vec![fwd(e01), fwd(e15), rev(e45), rev(e04)], true).unwrap()); let w_back = - topo.add_wire(Wire::new(vec![fwd(e23), fwd(e37), fwd(e67), fwd(e26)], true).unwrap()); + topo.add_wire(Wire::new(vec![fwd(e23), fwd(e37), rev(e67), rev(e26)], true).unwrap()); let w_left = - topo.add_wire(Wire::new(vec![fwd(e30), fwd(e04), fwd(e74), fwd(e37)], true).unwrap()); + topo.add_wire(Wire::new(vec![fwd(e30), fwd(e04), rev(e74), rev(e37)], true).unwrap()); let w_right = - topo.add_wire(Wire::new(vec![fwd(e12), fwd(e26), fwd(e56), fwd(e15)], true).unwrap()); + topo.add_wire(Wire::new(vec![fwd(e12), fwd(e26), rev(e56), rev(e15)], true).unwrap()); let mk_face = |w, n: Vec3, d: f64| Face::new(w, vec![], FaceSurface::Plane { normal: n, d }); diff --git a/crates/algo/src/diagnostic.rs b/crates/algo/src/diagnostic.rs index e562c932..baf8780b 100644 --- a/crates/algo/src/diagnostic.rs +++ b/crates/algo/src/diagnostic.rs @@ -201,18 +201,19 @@ mod tests { let e26 = edge(2, 6); let e37 = edge(3, 7); let fwd = |eid| OrientedEdge::new(eid, true); + let rev = |eid| OrientedEdge::new(eid, false); let w_bot = - topo.add_wire(Wire::new(vec![fwd(e01), fwd(e12), fwd(e23), fwd(e30)], true).unwrap()); + topo.add_wire(Wire::new(vec![rev(e01), rev(e30), rev(e23), rev(e12)], true).unwrap()); let w_top = topo.add_wire(Wire::new(vec![fwd(e45), fwd(e56), fwd(e67), fwd(e74)], true).unwrap()); let w_front = - topo.add_wire(Wire::new(vec![fwd(e01), fwd(e15), fwd(e45), fwd(e04)], true).unwrap()); + topo.add_wire(Wire::new(vec![fwd(e01), fwd(e15), rev(e45), rev(e04)], true).unwrap()); let w_back = - topo.add_wire(Wire::new(vec![fwd(e23), fwd(e37), fwd(e67), fwd(e26)], true).unwrap()); + topo.add_wire(Wire::new(vec![fwd(e23), fwd(e37), rev(e67), rev(e26)], true).unwrap()); let w_left = - topo.add_wire(Wire::new(vec![fwd(e30), fwd(e04), fwd(e74), fwd(e37)], true).unwrap()); + topo.add_wire(Wire::new(vec![fwd(e30), fwd(e04), rev(e74), rev(e37)], true).unwrap()); let w_right = - topo.add_wire(Wire::new(vec![fwd(e12), fwd(e26), fwd(e56), fwd(e15)], true).unwrap()); + topo.add_wire(Wire::new(vec![fwd(e12), fwd(e26), rev(e56), rev(e15)], true).unwrap()); let f_bot = topo.add_face(Face::new( w_bot, vec![], diff --git a/crates/algo/src/pave_filler/phase_ff.rs b/crates/algo/src/pave_filler/phase_ff.rs index 88925717..5085aa00 100644 --- a/crates/algo/src/pave_filler/phase_ff.rs +++ b/crates/algo/src/pave_filler/phase_ff.rs @@ -31,9 +31,9 @@ const NURBS_MARCH_STEP: f64 = 0.01; /// Detect face-face intersections between the two solids. /// /// For each face pair (one from each solid), computes intersection -/// curves using surface-type-specific algorithms. Raw intersection -/// curves are stored in the arena without trimming to face boundaries -/// (boundary trimming is a later phase). +/// curves using surface-type-specific algorithms. Plane-plane line +/// curves are trimmed to the mutual overlap of the two faces; other +/// raw curves are stored untrimmed (boundary trimming is a later phase). /// /// Creates topology vertices and edges for each intersection curve /// endpoint, and a pave block spanning the full parameter range. @@ -102,33 +102,29 @@ pub fn perform( let raw_curves = compute_raw_curves(surf_a, surf_b, bbox_a, bbox_b, v_range_a, v_range_b)?; - // For plane-plane Line curves with all-straight-edge faces, filter - // curves whose clipped ranges on each face have a LARGE gap (no - // overlap). A small gap or touching boundary is kept (the face - // splitter handles final trimming). Only curves with a significant - // separation (>10% of curve length) between the two ranges are - // rejected as truly spurious. + // For plane-plane Line curves with all-straight-edge faces, trim + // each curve to the mutual overlap of the two faces' clipped + // ranges. Without this the section curve spans the union of both + // face extents, producing over-long chords that cross face + // boundaries mid-edge and corrupt the downstream face partition. let raw_curves: Vec = if matches!(surf_a, FaceSurface::Plane { .. }) && matches!(surf_b, FaceSurface::Plane { .. }) { raw_curves .into_iter() - .filter(|raw| { + .filter_map(|raw| { if !matches!(raw.curve, EdgeCurve::Line) { - return true; + return Some(raw); } - let range_a = clip_line_to_face(topo, fa, raw); - let range_b = clip_line_to_face(topo, fb, raw); + let range_a = clip_line_to_face(topo, fa, &raw); + let range_b = clip_line_to_face(topo, fb, &raw); match (range_a, range_b) { - (None, None) => false, // Outside both faces - (Some(_), None) | (None, Some(_)) => true, // Inside one face + (None, None) => None, + (Some(_), None) | (None, Some(_)) => Some(raw), (Some(a), Some(b)) => { - // Check gap between ranges - let t_min = a.0.max(b.0); - let t_max = a.1.min(b.1); - let gap = t_min - t_max; - // Keep if gap < 10% of curve length (allows touching) - gap < 0.1 + let f0 = a.0.max(b.0); + let f1 = a.1.min(b.1); + trim_raw_line(&raw, f0, f1, tol) } } }) @@ -1002,6 +998,34 @@ fn nurbs_curve_bbox(curve: &brepkit_math::nurbs::curve::NurbsCurve) -> Aabb3 { // ── FF curve boundary filtering ────────────────────────────────────── +/// Shrink a `Line` raw curve to the fractional sub-range `[f0, f1]` of its +/// current extent, recomputing endpoints, parameter range, and bbox. +/// +/// Returns `None` when the trimmed segment is shorter than `tol.linear` +/// (touching or disjoint clip ranges — no real overlap). +fn trim_raw_line(raw: &RawCurve, f0: f64, f1: f64, tol: Tolerance) -> Option { + let span = raw.t_range.1 - raw.t_range.0; + let t0 = raw.t_range.0 + f0 * span; + let t1 = raw.t_range.0 + f1 * span; + let seg = raw.p_end - raw.p_start; + if (f1 - f0) * seg.length() < tol.linear { + return None; + } + let p0 = raw.p_start + seg * f0; + let p1 = raw.p_start + seg * f1; + let bbox = Aabb3 { + min: Point3::new(p0.x().min(p1.x()), p0.y().min(p1.y()), p0.z().min(p1.z())), + max: Point3::new(p0.x().max(p1.x()), p0.y().max(p1.y()), p0.z().max(p1.z())), + }; + Some(RawCurve { + curve: raw.curve.clone(), + bbox, + t_range: (t0, t1), + p_start: p0, + p_end: p1, + }) +} + /// Clip a Line curve to a planar face's boundary polygon. fn clip_line_to_face(topo: &Topology, face_id: FaceId, raw: &RawCurve) -> Option<(f64, f64)> { let face = topo.face(face_id).ok()?; @@ -1016,14 +1040,36 @@ fn clip_line_to_face(topo: &Topology, face_id: FaceId, raw: &RawCurve) -> Option return Some((0.0, 1.0)); } - let mut verts = Vec::new(); + // Chain edges by shared vertex IDs rather than trusting stored + // orientation flags — wires from external builders may carry + // inconsistent `is_forward` flags, and a mis-ordered polygon makes + // the clip silently truncate the range. + let mut remaining: Vec<( + brepkit_topology::vertex::VertexId, + brepkit_topology::vertex::VertexId, + )> = Vec::new(); for oe in wire.edges() { let edge = topo.edge(oe.edge()).ok()?; - let vid = if oe.is_forward() { - edge.start() - } else { - edge.end() + remaining.push((edge.start(), edge.end())); + } + if remaining.len() < 3 { + return Some((0.0, 1.0)); + } + let (first_start, first_end) = remaining.swap_remove(0); + let mut vert_ids = vec![first_start, first_end]; + while !remaining.is_empty() { + let cur = *vert_ids.last()?; + let Some(pos) = remaining.iter().position(|&(s, e)| s == cur || e == cur) else { + return Some((0.0, 1.0)); }; + let (s, e) = remaining.swap_remove(pos); + vert_ids.push(if s == cur { e } else { s }); + } + if vert_ids.first() == vert_ids.last() { + vert_ids.pop(); + } + let mut verts = Vec::with_capacity(vert_ids.len()); + for vid in vert_ids { verts.push(topo.vertex(vid).ok()?.point()); } if verts.len() < 3 { diff --git a/crates/algo/src/pave_filler/tests.rs b/crates/algo/src/pave_filler/tests.rs index d5d187de..d051739b 100644 --- a/crates/algo/src/pave_filler/tests.rs +++ b/crates/algo/src/pave_filler/tests.rs @@ -55,19 +55,20 @@ fn make_box(topo: &mut Topology, min: [f64; 3], max: [f64; 3]) -> brepkit_topolo let e37 = edge(3, 7); let fwd = |eid| OrientedEdge::new(eid, true); + let rev = |eid| OrientedEdge::new(eid, false); let w_bot = - topo.add_wire(Wire::new(vec![fwd(e01), fwd(e12), fwd(e23), fwd(e30)], true).unwrap()); + topo.add_wire(Wire::new(vec![rev(e01), rev(e30), rev(e23), rev(e12)], true).unwrap()); let w_top = topo.add_wire(Wire::new(vec![fwd(e45), fwd(e56), fwd(e67), fwd(e74)], true).unwrap()); let w_front = - topo.add_wire(Wire::new(vec![fwd(e01), fwd(e15), fwd(e45), fwd(e04)], true).unwrap()); + topo.add_wire(Wire::new(vec![fwd(e01), fwd(e15), rev(e45), rev(e04)], true).unwrap()); let w_back = - topo.add_wire(Wire::new(vec![fwd(e23), fwd(e37), fwd(e67), fwd(e26)], true).unwrap()); + topo.add_wire(Wire::new(vec![fwd(e23), fwd(e37), rev(e67), rev(e26)], true).unwrap()); let w_left = - topo.add_wire(Wire::new(vec![fwd(e30), fwd(e04), fwd(e74), fwd(e37)], true).unwrap()); + topo.add_wire(Wire::new(vec![fwd(e30), fwd(e04), rev(e74), rev(e37)], true).unwrap()); let w_right = - topo.add_wire(Wire::new(vec![fwd(e12), fwd(e26), fwd(e56), fwd(e15)], true).unwrap()); + topo.add_wire(Wire::new(vec![fwd(e12), fwd(e26), rev(e56), rev(e15)], true).unwrap()); let f_bot = topo.add_face(Face::new( w_bot, @@ -561,12 +562,13 @@ fn gfa_fuse_overlapping_boxes_face_count() { let result = crate::gfa::boolean(&mut topo, crate::bop::BooleanOp::Fuse, a, b) .expect("fuse of overlapping boxes"); let faces = brepkit_topology::explorer::solid_faces(&topo, result).unwrap(); - // GFA produces quadrant-split faces (24) at the algo level. - // The operations-level `boolean_gfa` unifies coplanar faces to 10. - // Accept either count here since this tests the algo crate directly. + // Section curves are trimmed to the mutual face overlap, so each cut + // face splits into exactly 2 sub-faces (kept L + discarded square): + // 6 untouched faces + 6 L-faces = 12 at the algo level. The + // operations-level `boolean_gfa` unifies coplanar faces to 10. assert!( - faces.len() == 10 || faces.len() == 24, - "overlapping fuse should have 10 or 24 faces, got {}", + faces.len() == 10 || faces.len() == 12, + "overlapping fuse should have 10 or 12 faces, got {}", faces.len() ); } diff --git a/crates/operations/src/boolean/tests.rs b/crates/operations/src/boolean/tests.rs index 0f36dc44..b8b71c60 100644 --- a/crates/operations/src/boolean/tests.rs +++ b/crates/operations/src/boolean/tests.rs @@ -3275,7 +3275,6 @@ fn d4_shelled_box_fuse_lip() { // must split faces correctly. #[test] -#[ignore = "GFA coplanar face handling not yet complete"] fn coplanar_box_cut_d1a2() { let _ = env_logger::try_init(); let mut topo = Topology::new(); diff --git a/crates/operations/tests/examples.rs b/crates/operations/tests/examples.rs index 41e6ed59..800d39a6 100644 --- a/crates/operations/tests/examples.rs +++ b/crates/operations/tests/examples.rs @@ -355,7 +355,6 @@ fn example_multi_solid_assembly() { /// Create an L-shaped cross-section using boolean cut, then tessellate. #[test] -#[ignore = "GFA pipeline limitation — old boolean pipeline removed"] fn example_custom_profile_extrusion() { use brepkit_operations::boolean::{BooleanOp, boolean}; From 7652053f3ba72795f194bc6b7dddd220e2fce37e Mon Sep 17 00:00:00 2001 From: Andy Aragon Date: Tue, 9 Jun 2026 13:42:35 -0700 Subject: [PATCH 2/2] fix(algo): disambiguate empty vs degraded FF clip, guard non-convex polygons The plane-plane FF mutual-overlap trim treated a `None` clip from one face as a conservative keep regardless of cause. Split the two causes: a built convex polygon that the line lies outside now yields `FaceClip::Empty` and the section curve is dropped, while a polygon that could not be built (or is non-convex) yields `FaceClip::Indeterminate` and keeps the raw curve. Cyrus-Beck clipping is only valid for convex polygons, so gate the active trim with a signed-cross-product convexity check; non-convex outlines fall back to keeping the raw curve. Strengthen `planar_faces_overlap`'s hole suppression: in addition to the single interior-sample test, reject a pair when every sampled point of the candidate inside the container's outer boundary falls inside a hole, so a non-convex face straddling a hole boundary no longer slips through. The common convex case is unchanged. --- crates/algo/src/builder/same_domain.rs | 24 ++++ crates/algo/src/pave_filler/phase_ff.rs | 148 +++++++++++++++++++++--- 2 files changed, 154 insertions(+), 18 deletions(-) diff --git a/crates/algo/src/builder/same_domain.rs b/crates/algo/src/builder/same_domain.rs index 570cb926..6e73110e 100644 --- a/crates/algo/src/builder/same_domain.rs +++ b/crates/algo/src/builder/same_domain.rs @@ -473,11 +473,34 @@ fn planar_faces_overlap(topo: &Topology, sub_faces: &[SubFace], i: usize, j: usi }) }; + // A single interior sample can miss the hole for a non-convex candidate + // straddling a hole boundary: the sample may land on solid material while + // the candidate's footprint actually sits entirely over the container's + // holes. As an additional (not replacement) suppressor, also reject when + // EVERY sampled point of the candidate that lies inside the container's + // outer boundary falls inside one of the container's holes. This keeps + // the common case (interior sample alone) identical and only fires extra + // for footprints fully over holes. + let footprint_in_holes = |sample: brepkit_math::vec::Point2, + verts: &[brepkit_math::vec::Point2], + outer: &[brepkit_math::vec::Point2], + face: &brepkit_topology::face::Face| + -> bool { + if face.inner_wires().is_empty() { + return false; + } + std::iter::once(sample) + .chain(verts.iter().copied()) + .filter(|&p| super::classify_2d::point_in_polygon_2d(p, outer)) + .all(|p| in_hole(p, face)) + }; + // i fully contained in j: every vertex of i (plus its interior sample) // is inside j's polygon. if super::classify_2d::point_in_polygon_2d(p_i_2d, &poly_j) && all_inside(&poly_i, &poly_j) && !in_hole(p_i_2d, face_j) + && !footprint_in_holes(p_i_2d, &poly_i, &poly_j, face_j) { return true; } @@ -485,6 +508,7 @@ fn planar_faces_overlap(topo: &Topology, sub_faces: &[SubFace], i: usize, j: usi if super::classify_2d::point_in_polygon_2d(p_j_2d, &poly_i) && all_inside(&poly_j, &poly_i) && !in_hole(p_j_2d, face_i) + && !footprint_in_holes(p_j_2d, &poly_j, &poly_i, face_i) { return true; } diff --git a/crates/algo/src/pave_filler/phase_ff.rs b/crates/algo/src/pave_filler/phase_ff.rs index 5085aa00..d67410af 100644 --- a/crates/algo/src/pave_filler/phase_ff.rs +++ b/crates/algo/src/pave_filler/phase_ff.rs @@ -107,6 +107,9 @@ pub fn perform( // ranges. Without this the section curve spans the union of both // face extents, producing over-long chords that cross face // boundaries mid-edge and corrupt the downstream face partition. + // When a face's polygon is built but the line lies outside it the + // overlap is empty and the curve is dropped; when a polygon can't + // be built (or is non-convex) the raw curve is kept conservatively. let raw_curves: Vec = if matches!(surf_a, FaceSurface::Plane { .. }) && matches!(surf_b, FaceSurface::Plane { .. }) { @@ -116,16 +119,29 @@ pub fn perform( if !matches!(raw.curve, EdgeCurve::Line) { return Some(raw); } - let range_a = clip_line_to_face(topo, fa, &raw); - let range_b = clip_line_to_face(topo, fb, &raw); - match (range_a, range_b) { - (None, None) => None, - (Some(_), None) | (None, Some(_)) => Some(raw), - (Some(a), Some(b)) => { + let clip_a = clip_line_to_face(topo, fa, &raw); + let clip_b = clip_line_to_face(topo, fb, &raw); + match (clip_a, clip_b) { + // A face's polygon was built but the line lies + // entirely outside it: the mutual overlap is + // empty, so the section curve does not belong on + // this pair — drop it. + (FaceClip::Empty, _) | (_, FaceClip::Empty) => None, + // Both clips produced an interval: trim to the + // mutual overlap. + (FaceClip::Range(a), FaceClip::Range(b)) => { let f0 = a.0.max(b.0); let f1 = a.1.min(b.1); trim_raw_line(&raw, f0, f1, tol) } + // At least one face could not build a usable + // polygon (degenerate wire, non-line edges, or a + // non-convex outline the Cyrus-Beck clip can't + // handle). Conservatively keep the raw curve and + // leave trimming to a later phase. + (FaceClip::Indeterminate, _) | (_, FaceClip::Indeterminate) => { + Some(raw) + } } }) .collect() @@ -1026,18 +1042,39 @@ fn trim_raw_line(raw: &RawCurve, f0: f64, f1: f64, tol: Tolerance) -> Option Option<(f64, f64)> { - let face = topo.face(face_id).ok()?; +fn clip_line_to_face(topo: &Topology, face_id: FaceId, raw: &RawCurve) -> FaceClip { + let Ok(face) = topo.face(face_id) else { + return FaceClip::Indeterminate; + }; let FaceSurface::Plane { normal, .. } = face.surface() else { - return Some((0.0, 1.0)); + return FaceClip::Indeterminate; + }; + let Ok(wire) = topo.wire(face.outer_wire()) else { + return FaceClip::Indeterminate; }; - let wire = topo.wire(face.outer_wire()).ok()?; if !wire.edges().iter().all(|oe| { topo.edge(oe.edge()) .is_ok_and(|e| matches!(e.curve(), EdgeCurve::Line)) }) { - return Some((0.0, 1.0)); + return FaceClip::Indeterminate; } // Chain edges by shared vertex IDs rather than trusting stored @@ -1049,18 +1086,22 @@ fn clip_line_to_face(topo: &Topology, face_id: FaceId, raw: &RawCurve) -> Option brepkit_topology::vertex::VertexId, )> = Vec::new(); for oe in wire.edges() { - let edge = topo.edge(oe.edge()).ok()?; + let Ok(edge) = topo.edge(oe.edge()) else { + return FaceClip::Indeterminate; + }; remaining.push((edge.start(), edge.end())); } if remaining.len() < 3 { - return Some((0.0, 1.0)); + return FaceClip::Indeterminate; } let (first_start, first_end) = remaining.swap_remove(0); let mut vert_ids = vec![first_start, first_end]; while !remaining.is_empty() { - let cur = *vert_ids.last()?; + let Some(&cur) = vert_ids.last() else { + return FaceClip::Indeterminate; + }; let Some(pos) = remaining.iter().position(|&(s, e)| s == cur || e == cur) else { - return Some((0.0, 1.0)); + return FaceClip::Indeterminate; }; let (s, e) = remaining.swap_remove(pos); vert_ids.push(if s == cur { e } else { s }); @@ -1070,10 +1111,13 @@ fn clip_line_to_face(topo: &Topology, face_id: FaceId, raw: &RawCurve) -> Option } let mut verts = Vec::with_capacity(vert_ids.len()); for vid in vert_ids { - verts.push(topo.vertex(vid).ok()?.point()); + let Ok(v) = topo.vertex(vid) else { + return FaceClip::Indeterminate; + }; + verts.push(v.point()); } if verts.len() < 3 { - return Some((0.0, 1.0)); + return FaceClip::Indeterminate; } let frame = @@ -1085,12 +1129,53 @@ fn clip_line_to_face(topo: &Topology, face_id: FaceId, raw: &RawCurve) -> Option (uv.x(), uv.y()) }) .collect(); + // Cyrus-Beck is only correct for convex polygons. A non-convex outline + // would produce a wrong (over-trimmed) interval, so treat it as + // indeterminate and let the caller keep the raw curve. + if !polygon_is_convex(&poly) { + return FaceClip::Indeterminate; + } let s = frame.project(raw.p_start); let e = frame.project(raw.p_end); - clip_line_to_polygon((s.x(), s.y()), (e.x(), e.y()), &poly) + match clip_line_to_polygon((s.x(), s.y()), (e.x(), e.y()), &poly) { + Some(range) => FaceClip::Range(range), + None => FaceClip::Empty, + } +} + +/// Test whether a simple polygon is convex via a signed-cross-product +/// sweep: all consecutive edge turns must share the same sign. +/// +/// Collinear vertices (zero cross product) are tolerated. Degenerate +/// polygons (< 3 vertices) are reported non-convex. +fn polygon_is_convex(polygon: &[(f64, f64)]) -> bool { + let n = polygon.len(); + if n < 3 { + return false; + } + let mut sign: i32 = 0; + for i in 0..n { + let a = polygon[i]; + let b = polygon[(i + 1) % n]; + let c = polygon[(i + 2) % n]; + let cross = (b.0 - a.0) * (c.1 - b.1) - (b.1 - a.1) * (c.0 - b.0); + if cross.abs() < 1e-12 { + continue; + } + let s = if cross > 0.0 { 1 } else { -1 }; + if sign == 0 { + sign = s; + } else if s != sign { + return false; + } + } + true } /// Cyrus-Beck line-polygon clipping. Handles CCW and CW winding. +/// +/// Only correct for convex polygons — callers must gate non-convex +/// outlines (see [`polygon_is_convex`]). fn clip_line_to_polygon( start: (f64, f64), end: (f64, f64), @@ -1174,4 +1259,31 @@ mod tests { assert!((r.0 - 1.0 / 3.0).abs() < 1e-6); assert!((r.1 - 2.0 / 3.0).abs() < 1e-6); } + + #[test] + fn clip_outside_means_drop() { + // A line provably outside a built (convex) polygon yields `None`, + // which the FF trim path maps to `FaceClip::Empty` → drop the curve. + let poly = vec![(0.0, 0.0), (1.0, 0.0), (1.0, 1.0), (0.0, 1.0)]; + assert!(clip_line_to_polygon((2.0, 0.5), (3.0, 0.5), &poly).is_none()); + } + + #[test] + fn convex_square_is_convex() { + let poly = vec![(0.0, 0.0), (1.0, 0.0), (1.0, 1.0), (0.0, 1.0)]; + assert!(polygon_is_convex(&poly)); + } + + #[test] + fn convex_check_tolerates_collinear() { + let poly = vec![(0.0, 0.0), (0.5, 0.0), (1.0, 0.0), (1.0, 1.0), (0.0, 1.0)]; + assert!(polygon_is_convex(&poly)); + } + + #[test] + fn arrow_polygon_is_non_convex() { + // A concave "arrowhead": the reflex vertex flips the turn sign. + let poly = vec![(0.0, 0.0), (2.0, 1.0), (0.0, 2.0), (1.0, 1.0)]; + assert!(!polygon_is_convex(&poly)); + } }