Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 18 additions & 10 deletions .claude/skills/roadmap/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -371,16 +371,24 @@ nozzle nm 1→15, clip volume 46.78→46.70 vs 46.6±0.05). Dovetail residuals:
within the 1e-5 weld band, span > 1e-6·domain) returns the projected
trimmed `[t₀, t₁]`. On the RAWN=1 raw repro this cleaned one of the two
mirrored junction signatures (the use=3 triple + micro-edge chain at
y=−39.4). RESIDUAL: the mirrored twin needs REVERSED sub-spans (SCDOM
instrumentation showed on-curve reversed pairs at 1e-14), but accepting
them (returning `t₀ > t₁`, even gated to open curves) makes the
arrangement mint a DEGENERATE single-edge closed loop (endpoint gap ~4e-9
— a phantom point-hole at the micro-junction, below vertex-merge reach)
as an inner wire on the cone wall, which trips the curved-lens
no-contained-interior abort (wall Id(1137) in the repro). Next step is
the arrangement/wire-builder side: collapse or reject degenerate
(sub-weld-extent) closed loops before they become inner wires, then
re-attempt reversed acceptance. Repro: cached replay_scplate.rs, RAWN=1,
y=−39.4). SECOND LANDING (same day): REVERSED sub-spans accepted on
clearly-open curves (`t₀ > t₁`, start→end interpolation stays truthful;
closed curves keep the full-domain fallback — a reversed pair there is
usually a seam-crossing forward sub-arc). The "degenerate phantom loop"
that blocked reversed acceptance was a MISREAD: the single-edge closed
NURBS inner wire on the aborting cone wall (seam gap 5e-9, source_edge_idx
Some) is a LEGITIMATE pre-existing notch outline; the real defect was a
COVERAGE hole — walls reaching `fill_images_faces` through split paths
that never run the dedicated `cylinder_cone_remainder_interior` search
aborted unconditionally on the lens flag. Fixed by running that search as
a last resort at the consumption point (fill_images_faces; abort only if
even the dense grid finds nothing). Together: RAWN=1 posBad 10 → 6, both
mirrored micro-edge chains resolved. REMAINING 6 (one root): the CONE
face keeps its un-split rim circle A→B while the notch's wall planes
carry the true route A→C→B — the marched cone×plane sections never split
the cone face itself (the pave-machinery-bypass row; sections carry
pave_block_id=None). Fix at phase-FF/make_blocks altitude or the curved
face splitter's section intake. Repro: cached replay_scplate.rs, RAWN=1,
capture capture-snapclip-plate-fresh. Dig provenance: memory
project_snapclip-plate-bore.md.
- **Mesh-boolean fallback emits OPEN meshes that get CONSUMED — OPEN
Expand Down
1 change: 1 addition & 0 deletions crates/algo/src/builder/face_splitter/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ mod conversion;
mod edge_splitting;
mod sampling;
mod special_cases;
pub(in crate::builder) use special_cases::cylinder_cone_remainder_interior;

pub use conversion::collect_wire_points;

Expand Down
2 changes: 1 addition & 1 deletion crates/algo/src/builder/face_splitter/special_cases.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1582,7 +1582,7 @@ pub(super) fn split_face_with_internal_loops(
/// distance to every hole loop is largest, which is guaranteed to lie on the
/// kept wall well clear of every lens. Returns `None` if the surface evaluator
/// or boundary v-range is unusable, so the caller keeps the unset interior.
fn cylinder_cone_remainder_interior(remainder: &SplitSubFace) -> Option<Point3> {
pub fn cylinder_cone_remainder_interior(remainder: &SplitSubFace) -> Option<Point3> {
use std::f64::consts::{PI, TAU};
// Densely sample every hole loop into 3D points AND project each to a (u, v)
// polyline so candidate interior points can be tested for face containment
Expand Down
8 changes: 7 additions & 1 deletion crates/algo/src/builder/fill_images_faces.rs
Original file line number Diff line number Diff line change
Expand Up @@ -461,7 +461,13 @@ pub fn fill_images_faces<S: BuildHasher, S2: BuildHasher>(
resolved_face_id,
) =>
{
None
// Not every split path that can leave a closed curved hole
// on a cylinder/cone wall runs the dedicated remainder
// search (e.g. a wall keeping a pre-existing lens hole
// through the generic split). Run it here as a last resort;
// only when even the dense grid finds nothing does the
// classifier abort to mesh.
super::face_splitter::cylinder_cone_remainder_interior(split)
}
None => Some(super::face_splitter::interior_point_3d(
split,
Expand Down
51 changes: 40 additions & 11 deletions crates/topology/src/edge.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,17 +116,20 @@ impl EdgeCurve {
}
let proj = |p| brepkit_math::nurbs::projection::project_point_to_curve(n, p, 1e-9);
if let (Ok(pa), Ok(pb)) = (proj(start), proj(end)) {
// Accept only a forward, non-degenerate, on-curve span.
// Everything else — reversed pairs (on a closed curve these
// are usually seam-crossing forward sub-arcs, where
// interpolating backward would trace the complement arc;
// downstream arrangement consumers also mint degenerate
// sliver loops from reversed spans), degenerate spans,
// off-curve endpoints — keeps the historical full-domain
// behaviour.
// Accept a non-degenerate on-curve span. A reversed pair
// (`t₀ > t₁`, interpolation still traces start→end) is
// trusted only on a clearly OPEN curve: on a (nearly)
// closed curve a reversed projection pair is usually a
// seam-crossing forward sub-arc, and interpolating
// backward would trace the complement arc. Degenerate
// spans and off-curve endpoints keep the historical
// full-domain behaviour.
let dt = pb.parameter - pa.parameter;
let curve_open = (p0 - p1).length() >= WELD_EPS;
if pa.distance < WELD_EPS
&& pb.distance < WELD_EPS
&& pb.parameter - pa.parameter > 1e-6 * (d1 - d0)
&& dt.abs() > 1e-6 * (d1 - d0)
&& (dt > 0.0 || curve_open)
Comment on lines +127 to +132

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 WELD_EPS used for two distinct roles without comment

WELD_EPS = 1e-5 is introduced as the "NURBS sub-span on-curve band" distance, i.e., how far a projected endpoint may sit off the curve before being distrusted. It is then reused on line 128 to determine whether the underlying curve itself is open or nearly-closed — a conceptually different test. The two roles happen to share the same numeric scale, but the coincidence is not explained. A reader maintaining this later may tighten the on-curve band (e.g., to match vertex tolerance) without realising it also changes the open/closed classification boundary, or vice versa. A short inline comment stating why the same value is appropriate for both would prevent this drift.

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

{
return (pa.parameter, pb.parameter);
}
Expand Down Expand Up @@ -386,7 +389,7 @@ mod tests {
}

#[test]
fn nurbs_domain_reversed_sub_span_falls_back_to_full_domain() {
fn nurbs_domain_reversed_sub_span_on_open_curve_trims_backward() {
let curve = open_nurbs();
let EdgeCurve::NurbsCurve(n) = &curve else {
unreachable!()
Expand All @@ -396,7 +399,33 @@ mod tests {
let tb = d0 + 0.7 * (d1 - d0);
let pa = brepkit_math::traits::ParametricCurve::evaluate(n, ta);
let pb = brepkit_math::traits::ParametricCurve::evaluate(n, tb);
assert_full_domain(curve.domain_with_endpoints(pb, pa), d0, d1);
// Edge runs pb -> pa (reversed relative to curve parameterization):
// the trimmed domain keeps t0 > t1 so t0->t1 interpolation still
// traces start -> end.
let (t0, t1) = curve.domain_with_endpoints(pb, pa);
assert!(t0 > t1, "expected reversed span, got ({t0}, {t1})");
assert!((curve.evaluate_with_endpoints(t0, pb, pa) - pb).length() < 1e-6);
assert!((curve.evaluate_with_endpoints(t1, pb, pa) - pa).length() < 1e-6);
}

#[test]
fn nurbs_domain_reversed_pair_on_closed_curve_falls_back_to_full_domain() {
// A closed fitted loop: a reversed projection pair here is ambiguous
// (usually a seam-crossing forward sub-arc), so the full domain wins.
let pts: Vec<Point3> = (0..=12)
.map(|k| {
let a = std::f64::consts::TAU * f64::from(k) / 12.0;
Point3::new(a.cos(), a.sin(), 0.0)
})
.collect();
let n = brepkit_math::nurbs::fitting::interpolate(&pts, 3).unwrap();
let (d0, d1) = brepkit_math::traits::ParametricCurve::domain(&n);
let ta = d0 + 0.6 * (d1 - d0);
let tb = d0 + 0.2 * (d1 - d0);
let pa = brepkit_math::traits::ParametricCurve::evaluate(&n, ta);
let pb = brepkit_math::traits::ParametricCurve::evaluate(&n, tb);
let curve = EdgeCurve::NurbsCurve(n);
assert_full_domain(curve.domain_with_endpoints(pa, pb), d0, d1);
}

#[test]
Expand Down
Loading