Skip to content

fix(algo): trim plane-plane section curves to mutual face overlap#754

Merged
andymai merged 2 commits into
mainfrom
fix/gfa-ff-mutual-overlap-trim
Jun 9, 2026
Merged

fix(algo): trim plane-plane section curves to mutual face overlap#754
andymai merged 2 commits into
mainfrom
fix/gfa-ff-mutual-overlap-trim

Conversation

@andymai

@andymai andymai commented Jun 9, 2026

Copy link
Copy Markdown
Owner

Plane-plane FF section curves were trimmed to the combined extent of the two faces (min-of-mins/max-of-maxes in trim_t_range_to_aabb) instead of their mutual overlap. The per-face clip ranges were already computed by clip_line_to_face but only used to reject disjoint curves. Over-long chords crossed mid-edge with no vertices at the crossings, so coplanar cuts produced overlapping band sub-faces instead of a proper partition (e.g. coplanar_box_cut_d1a2: volume 0.667 vs 0.75 and zero same-domain pairs).

Changes

  • New trim_raw_line shrinks each plane-plane section curve to the intersection of the two faces' clip ranges, recomputing endpoints/bbox/t_range; existing vertex snapping preserved.
  • clip_line_to_face builds the face boundary polygon by chaining edges on shared vertex IDs instead of trusting stored is_forward flags (mis-ordered polygons silently truncated clip ranges).
  • classify_2d::sample_interior_point rejects centroids lying on the opposing face boundary.
  • Un-ignores coplanar_box_cut_d1a2 (correct frame+square partition, 2 SD pairs, volume 0.75) and example_custom_profile_extrusion (3/3).

Remaining coplanar failures (coincident_planes non-manifold cases, thin-shell fuse) trace to a separate defect — collinear split-edge images are not propagated to unsplit neighbor faces — and stay ignored.

Verification

Full corpus green: algo 94, operations 630 lib + 18 integration targets, wasm 206; fmt, clippy -D warnings, boundary check.

Copilot AI review requested due to automatic review settings June 9, 2026 20:21
@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

WASM Binary Size

File main PR Delta
brepkit_wasm.wasm 4.71 MB 4.71 MB -5.4 KB (-.1%)

✅ Size decreased — nice!

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes incorrect trimming of plane–plane face/face (FF) section curves by shrinking Line intersection curves to the mutual overlap of the two faces’ clipped ranges, improving downstream face partitioning and enabling previously-ignored coplanar/operations examples to pass.

Changes:

  • Trim plane–plane Line FF curves to the intersection of per-face clip ranges (trim_raw_line), recomputing endpoints/bbox/t-range.
  • Make clip_line_to_face build boundary polygons by chaining edges via shared vertex IDs (instead of trusting potentially inconsistent is_forward flags).
  • Improve coplanar/same-domain robustness: reject boundary-centroid “interior points”, treat containment-through-holes as non-overlap, and un-ignore two tests now passing.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
crates/operations/tests/examples.rs Un-ignores example_custom_profile_extrusion.
crates/operations/src/boolean/tests.rs Un-ignores coplanar_box_cut_d1a2.
crates/algo/src/pave_filler/tests.rs Updates box wire orientations; updates expected overlapping-fuse face counts.
crates/algo/src/pave_filler/phase_ff.rs Implements plane–plane Line trimming to mutual overlap; rebuilds clip polygon via vertex chaining.
crates/algo/src/diagnostic.rs Adjusts test box wire orientations.
crates/algo/src/classifier/ray_cast.rs Adjusts test box wire orientations.
crates/algo/src/builder/same_domain.rs Refines planar overlap logic to exclude containment through holes.
crates/algo/src/builder/classify_2d.rs Makes interior-point sampling reject centroids that land on polygon boundaries.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread crates/algo/src/pave_filler/phase_ff.rs Outdated
Comment on lines 121 to 124
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)) => {
Comment on lines +105 to 110
// 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<RawCurve> = if matches!(surf_a, FaceSurface::Plane { .. })
@greptile-apps

greptile-apps Bot commented Jun 9, 2026

Copy link
Copy Markdown

Greptile Summary

Fixes plane-plane face-face section curves being trimmed to the union of the two faces' extents instead of their intersection, which caused over-long chords that split faces incorrectly during coplanar boolean operations. The trimming is now done up-front in phase_ff to the mutual clip-range overlap, and both same_domain and classify_2d gain correctness guards for hole-boundary and boundary-touching centroid cases.

  • phase_ff.rs: replaces the old gap-filter with a FaceClip-based mutual-overlap trim (trim_raw_line); adds a polygon-chaining wire builder that avoids relying on stored is_forward flags, and a polygon_is_convex guard to protect Cyrus-Beck clipping from non-convex outlines.
  • same_domain.rs: refactors wire_points to accept a WireId, adds in_hole / footprint_in_holes closures to reject containment claims where the candidate sits inside a hole of the container face.
  • classify_2d.rs: sample_interior_point now rejects centroids that land on the polygon boundary via an adaptive distance check, preventing unpredictable even-odd results at reflex corners.

Confidence Score: 5/5

The fix is logically sound and all corpus tests pass; the two observations are edge cases (sub-millimetre geometry scale and inner-wire flag inconsistency) unlikely to fire in the existing test suite.

The core trimming logic, the polygon-chaining wire builder, the FaceClip enum, and the hole-containment guards are all correct for standard CAD geometry. The polygon_is_convex absolute threshold and the is_forward reliance in in_hole are edge-case concerns that do not affect currently tested inputs.

No files require blocking attention; the two observations are in phase_ff.rs (polygon_is_convex threshold) and same_domain.rs (wire_points is_forward reliance for inner wires).

Important Files Changed

Filename Overview
crates/algo/src/pave_filler/phase_ff.rs Core fix: replaces the old over-long section curve with mutual-overlap trimming. New trim_raw_line, FaceClip enum, polygon-chaining wire builder, and polygon_is_convex helper are all logically correct for normal-scale geometry; the convex-check threshold is absolute (1e-12) and can misfire on sub-millimetre faces.
crates/algo/src/builder/same_domain.rs Adds in_hole / footprint_in_holes guards for frame-face holes and refactors wire_points to accept a WireId; guards are logically sound but wire_points still uses is_forward for both outer and inner wires, leaving the same potential mis-ordering issue the PR fixed in clip_line_to_face.
crates/algo/src/builder/classify_2d.rs Adds adaptive boundary-distance guard to sample_interior_point; new helpers boundary_eps and distance_to_polygon_boundary are correct and use a scale-relative epsilon (diag * 1e-6).
crates/algo/src/classifier/ray_cast.rs Test-only change: corrects oriented-edge direction flags in the box fixture to match the chained-vertex ordering; no production code touched.
crates/algo/src/diagnostic.rs Test-only: same oriented-edge direction fix as in ray_cast.rs for the diagnostic crate's box fixture.
crates/algo/src/pave_filler/tests.rs Updates the fuse face-count assertion from (10 or 24) to (10 or 12) to match the trimmed-curve output; narrows the accepted range, which is the correct expectation after the fix.
crates/operations/src/boolean/tests.rs Un-ignores coplanar_box_cut_d1a2 — straightforward now that the root cause is fixed.
crates/operations/tests/examples.rs Un-ignores example_custom_profile_extrusion — no logic change, just removes the ignore flag.

Fix All in Claude Code

Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 2
crates/algo/src/pave_filler/phase_ff.rs:1157-1173
**`polygon_is_convex` uses an absolute cross-product threshold that breaks at small scale**

The hardcoded `1e-12` tolerance for the 2D cross product in `polygon_is_convex` is an area-unit quantity (UV length²). For faces whose projected UV coordinates are in the sub-millimetre range (e.g. a 10μm × 10μm planar patch), edge vectors are ~1e-5 long and the cross product for any non-collinear turn is ~1e-10 — below this threshold — so every vertex is treated as collinear and every polygon passes the convex test. Cyrus-Beck clipping then runs on a non-convex outline and can return an over-trimmed interval, producing a section curve that's too short rather than discarded.

### Issue 2 of 2
crates/algo/src/builder/same_domain.rs:465-474
**`in_hole` still relies on `is_forward` for hole-wire vertex ordering**

The new `in_hole` closure calls `wire_points(wid)` for each inner-wire ID. `wire_points` uses `oe.is_forward()` to pick the vertex that starts each oriented edge, which is precisely the ordering path that `clip_line_to_face` was refactored away from in this PR (with the comment "wires from external builders may carry inconsistent `is_forward` flags"). If an inner-wire's oriented edges have inconsistent flags the collected vertices form a self-intersecting polygon, and the even-odd `point_in_polygon_2d` test will give unpredictable results — a point in a hole might be reported as outside it, and a face pair that should be rejected as overlapping-through-hole passes through to the SD list.

Reviews (2): Last reviewed commit: "fix(algo): disambiguate empty vs degrade..." | Re-trigger Greptile

Comment thread crates/algo/src/pave_filler/phase_ff.rs Outdated
Comment thread crates/algo/src/builder/same_domain.rs
…olygons

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.
@andymai andymai enabled auto-merge (squash) June 9, 2026 20:43
@andymai andymai merged commit e692c9c into main Jun 9, 2026
18 checks passed
@andymai andymai deleted the fix/gfa-ff-mutual-overlap-trim branch June 9, 2026 20:47
@brepkit brepkit Bot mentioned this pull request Jun 9, 2026
brepkit Bot added a commit that referenced this pull request Jun 9, 2026
🤖 I have created a release *beep* *boop*
---


##
[2.102.0](v2.101.3...v2.102.0)
(2026-06-09)


### Features

* **algo:** split u-periodic faces into bands at internal section
circles ([#756](#756))
([39e9425](39e9425))


### Bug Fixes

* **algo:** adopt existing boundary vertices as seams for closed section
curves ([#755](#755))
([3342271](3342271))
* **algo:** trim plane-plane section curves to mutual face overlap
([#754](#754))
([e692c9c](e692c9c))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: brepkit[bot] <265643962+brepkit[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants