Skip to content

fix(algo): trim plane-cone circle sections to exact boundary-crossing arcs#1106

Merged
andymai merged 1 commit into
mainfrom
fix/plane-cone-circle-exact-arc
Jul 17, 2026
Merged

fix(algo): trim plane-cone circle sections to exact boundary-crossing arcs#1106
andymai merged 1 commit into
mainfrom
fix/plane-cone-circle-exact-arc

Conversation

@andymai

@andymai andymai commented Jul 17, 2026

Copy link
Copy Markdown
Owner

Summary

Second half of the snapClip join-edges export root (follows #1104). The export baseplate builds with the simplified tapered pockets, so each deep relief cutter's back corner lands on a pocket corner cone. The cutter should contribute three sections to that cone face: two marched conics (cone × back wall, cone × side wall) and the cone × cutter-top arc that closes the chain.

Root cause. A horizontal plane cuts a cone in an exact EdgeCurve::Circle. The exact-arc path that bypasses the sampling pre-filters (trim_ellipse_to_boundary_crossings — built for the tilted-tread × cylinder family) only accepted Ellipse sections, so the circle fell to the generic 16-sample in-both AABB filter, which cannot find a ~0.11-long in-both arc on an ~18-long circle. With the closing arc missing, the cone face received an open two-conic chain, the internal-loops splitter rejected it (open chains are dropped by design), and the face stayed unsplit. The resulting analytic-but-leaky solid (posBad=8) was accepted by the by-edge-id gate and poisoned the export chain into mesh fallback two cuts later (final bnd=111 nm=6 at export tolerance).

Fix. The trimmer now dispatches over Circle and Ellipse sections through one angular parameterization (SecCurve); emitted arcs keep their exact curve type so downstream circle-calibrated machinery sees real Circle sections. No other behavior changes — Ellipse handling is byte-identical.

Result. The minimal repro (cut(plate-after-op-cut-2, deep-relief-cutter)) goes posBad 8→0 on both the raw GFA and ops paths, and the full 44-hole export-variant join-edges chain replays analytic and watertight: final F=418, posBad=0 (was F=4842, bnd=111, nm=6).

Verification

  • New fixture crates/io/tests/snapclip_export_corner_inmem.rs (tool's serialized operands, 2026-07-17 export-variant capture): edge pairing + analytic-cone assertions, 10× flake gate clean
  • Full io suite green (including the tilted-tread/halfSockets ramp landscapes that calibrate the ellipse path)
  • d4 canary 27/27; brepkit-algo and brepkit-operations suites green
  • Full workspace suite green via pre-push hook

Roadmap updated in the same PR. Known remaining snapClip residuals (out of scope, recorded with repro recipes): the 0.6mm-nozzle export chain's op-cut-3 (posBad=10, different landscape), the by-edge-id acceptance gate's blindness to position-duplicate leaks, and the bed-flat clip volume pin.


Summary by cubic

Fixes trimming for plane×cone intersections by treating circle sections as exact boundary-crossing arcs. Restores the short cutter-top closing arcs, so cone faces split and the snapClip export chain stays analytic and watertight.

  • Bug Fixes
    • Updated trim_ellipse_to_boundary_crossings to handle both EdgeCurve::Circle and EdgeCurve::Ellipse via a shared angular parameterization (SecCurve); emitted arcs keep their exact type.
    • Avoids dropping ~0.11-length circle arcs in the 16-sample in-both filter that left open two-conic chains on cone faces.
    • Added crates/io/tests/snapclip_export_corner_inmem.rs with serialized operands to assert analytic cones and B-Rep edge pairing.
    • Result: minimal repro goes posBad 8→0; full 44-hole export variant replays analytic F=418, posBad=0 (was F=4842, bnd=111, nm=6).

Written for commit ca4ce75. Summary will update on new commits.

Review in cubic

… arcs

The export baseplate's tapered pockets put a corner CONE under each deep
relief cutter's back corner. The cutter contributes three sections to the
cone face: two marched conics (cone x back wall, cone x side wall) and the
cone x cutter-top arc that closes the chain. A horizontal plane cuts the
cone in an exact Circle, and the exact-arc path that bypasses the sampling
pre-filters (trim_ellipse_to_boundary_crossings) only accepted Ellipse
sections - so the ~0.11-long arc (2% of the full circle) died in the
16-sample in-both filter, the cone face received an open two-conic chain,
the internal-loops splitter rejected it, and the face stayed unsplit. The
analytic-but-leaky result was accepted and poisoned the snapClip export
chain into mesh fallback two cuts later (final bnd=111 nm=6).

The trimmer now dispatches over Circle and Ellipse sections (same angular
parameterization); emitted arcs keep their exact curve type. The full
44-hole export-variant join-edges chain replays analytic and watertight
(F=418 posBad=0, was F=4842 bnd=111 nm=6).
Copilot AI review requested due to automatic review settings July 17, 2026 17:30
@github-actions

Copy link
Copy Markdown
Contributor

WASM Binary Size

File main PR Delta
brepkit_wasm.wasm 5.87 MB 5.87 MB -.2 KB (0%)

✅ 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

Fixes a snapClip export-variant regression where plane×cone intersections produced exact EdgeCurve::Circle sections that were not trimmed via the “exact boundary-crossings” path, causing short closing arcs to be dropped and leaving cone faces unsplit (eventually poisoning the chain into mesh fallback). The change extends the exact trimming path to support both Circle and Ellipse sections via a shared angular parameterization and adds an in-memory fixture to prevent regressions.

Changes:

  • Extend trim_ellipse_to_boundary_crossings to also handle EdgeCurve::Circle (via SecCurve dispatch) while preserving exact curve types in emitted arcs.
  • Add a tier-3 .bin fixture integration test asserting cone analyticity and position-quantized edge pairing (watertightness proxy) for the snapClip export corner case.
  • Update roadmap notes documenting the closed root and remaining known snapClip residuals.

Reviewed changes

Copilot reviewed 3 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
crates/algo/src/pave_filler/phase_ff.rs Extends exact boundary-crossing trimming from Ellipse-only to Circle+Ellipse using a shared angular parameterization while keeping arc curve types exact.
crates/io/tests/snapclip_export_corner_inmem.rs Adds a regression test for the plane×cone exact Circle arc case, asserting cone analyticity and edge pairing.
crates/io/tests/data/snapclip_export_corner_plate.bin Adds serialized operand fixture for the post-op-cut-2 plate used by the new regression test.
crates/io/tests/data/snapclip_export_corner_cutter.bin Adds serialized operand fixture for the deep relief cutter used by the new regression test.
.claude/skills/roadmap/SKILL.md Updates roadmap narrative to record the closed root and remaining snapClip residuals.
Files not reviewed (1)
  • crates/io/tests/data/snapclip_export_corner_cutter.bin: Generated file

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

@greptile-apps

greptile-apps Bot commented Jul 17, 2026

Copy link
Copy Markdown

Greptile Summary

This PR fixes a dropped plane×cone EdgeCurve::Circle arc in the trim_ellipse_to_boundary_crossings fast path, which previously only accepted Ellipse sections. A horizontal plane cutting a cone yields an exact Circle, not an Ellipse, so it fell to the 16-sample AABB filter and was lost, leaving the cone face with an open two-conic chain that the internal-loops splitter rejected.

  • phase_ff.rs: introduces a local SecCurve<'a> dispatch enum with project/evaluate/edge_curve methods, extending the existing trimmer to handle both Circle and Ellipse while preserving exact curve types in emitted arcs; Ellipse behaviour is byte-identical.
  • New snapclip_export_corner_inmem.rs integration test and binary fixtures guard the fix with a position-quantised B-Rep edge-pairing check and a cone-count assertion.
  • Roadmap in SKILL.md is updated to record the fix and three remaining out-of-scope residuals.

Confidence Score: 5/5

Safe to merge — the change is a focused extension of one private function's dispatch, with no new enum variants, no new crate dependencies, and no changes to the Ellipse path.

The core change is minimal and surgical: a local SecCurve enum replaces a hard-coded Ellipse-only guard while leaving every downstream call site unchanged. The new test exercises the exact failure scenario with a position-quantized edge-pairing invariant, and the PR reports the full io suite and d4 canary passing. The only observation is a stale function name with no runtime impact.

No files require special attention. phase_ff.rs carries the only logic change, well-contained within the private trim_ellipse_to_boundary_crossings helper.

Important Files Changed

Filename Overview
crates/algo/src/pave_filler/phase_ff.rs Extends trim_ellipse_to_boundary_crossings to dispatch over both EdgeCurve::Circle and Ellipse via a local SecCurve enum; emitted arcs preserve exact curve type; Ellipse path is byte-identical to before
crates/io/tests/snapclip_export_corner_inmem.rs New integration test guarding the Circle-arc fix: performs the plate x cutter boolean, asserts 80+ analytic cone faces, and validates every B-Rep edge appears exactly twice via position-quantized pairing
.claude/skills/roadmap/SKILL.md Roadmap updated to reflect this fix closing the export-variant join-edges chain (F=418, posBad=0) and documenting three remaining residuals out of scope for this PR
crates/io/tests/data/snapclip_export_corner_cutter.bin Binary fixture: serialized relief-cutter solid operand for the new integration test
crates/io/tests/data/snapclip_export_corner_plate.bin Binary fixture: serialized plate-after-op-cut-2 solid operand for the new integration test

Comments Outside Diff (1)

  1. crates/algo/src/pave_filler/phase_ff.rs, line 1272 (link)

    P2 The function name trim_ellipse_to_boundary_crossings is now a misnomer: it dispatches on both Circle and Ellipse sections. Since this is a private helper, the impact is limited to readability, but a future reader may assume it only touches ellipses and be surprised to find circle handling inside.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

    Fix in Claude Code

Fix All in Claude Code

Reviews (1): Last reviewed commit: "fix(algo): trim plane-cone circle sectio..." | Re-trigger Greptile

@andymai
andymai merged commit 5bf9534 into main Jul 17, 2026
22 checks passed
@andymai
andymai deleted the fix/plane-cone-circle-exact-arc branch July 17, 2026 17:39
@brepkit brepkit Bot mentioned this pull request Jul 17, 2026
brepkit Bot added a commit that referenced this pull request Jul 17, 2026
🤖 I have created a release *beep* *boop*
---


##
[2.126.15](v2.126.14...v2.126.15)
(2026-07-17)


### Bug Fixes

* **algo:** trim plane-cone circle sections to exact boundary-crossing
arcs ([#1106](#1106))
([5bf9534](5bf9534))

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

<!-- This is an auto-generated description by cubic. -->
---
## Summary by cubic
Release 2.126.15 for `brepkit-wasm`, fixing plane–cone circle
intersections to trim to exact boundary-crossing arcs. This improves
section precision and prevents arcs extending beyond cone boundaries.

<sup>Written for commit ad4d4dc.
Summary will update on new commits.</sup>

<a
href="https://cubic.dev/pr/andymai/brepkit/pull/1107?utm_source=github"
target="_blank" rel="noopener noreferrer"
data-no-image-dialog="true"><picture><source
media="(prefers-color-scheme: dark)"
srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source
media="(prefers-color-scheme: light)"
srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img
alt="Review in cubic"
src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a>

<!-- End of auto-generated description by cubic. -->

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