Repository navigation
fix(booleans): three band-redesign regressions from #758 (CI 6h hang, bore winding, pinch wedges) - #771
Conversation
`parse_band` decomposed a face's outer loop into its two chains by scanning
the vertex list linearly (`0..n-1`), so it could only see a direction change
that fell strictly inside the list — the change carried by the closing edge
(v[n-1] → v[0]) was invisible. A loop whose starting half-edge sits mid-chain
therefore presented just one visible flip, the decomposition handed the tail
a single vertex, and the parse bailed ("short chains N 1").
`reversed_winding` already compensated for exactly this ("judging by which run
came first breaks when the loop's starting half-edge sits mid-chain"); the
decomposition itself did not.
The cost was not just a refused face. Every refusal made
`split_cylindrical_face_by_sampled` return the face unsplit, so the boolean
kept geometry it should have trimmed and the next operand split the leftovers
again — fragmentation compounding across a pattern's instances.
On the doc10 catalogue case (23-blade pattern − r45 annulus):
faces 1856 → 593
time 450s → 92s (release; ~4h16m → minutes in CI's debug build)
volume 6295 → 3769 (expected 3365)
That regression was eating the full 6h GitHub job limit on every PR, on both
the stable and nightly Rust jobs.
Fix: when the linear parse fails, re-anchor the loop at each true cyclic
turning point (computed over the full cycle, including the closing edge) and
retry. Loops that already start on a boundary take the identical path as
before, so passing cases are untouched.
Also here:
- Cache the `VCAD_BAND_DEBUG` lookup in a `OnceLock`, matching what #758 did
for `VCAD_SPLIT_DEBUG`. `band_dbg!` sits inside `try_parse_two_chain`, which
runs per-face and up to four times per face, and `env::var` allocates and
takes the environment lock on every call.
- Report which stage refused a sampled split. The old message always blamed
the intersection curve ("not a closed single-valued profile") even when the
curve was fine and the FACE was rejected — every failure in this case was
the latter, which is what made the bug hard to see. It now names the face,
its loop size, and its hole count.
- Cap the Rust CI jobs at 75 minutes. A single pathological test otherwise
burns the full 6h platform limit — 12 runner-hours per PR across the matrix
— before anyone learns anything.
Known remaining, both pre-existing on main and both from #758's band redesign:
doc10 still reads ~12% high (down from ~87%) because one collapsed-pinch wedge
comes through as a 3-vertex loop, which `parse_band` refuses by construction
at `n < 4`; and `half_annuli_union_full_annulus` /
`c2_extrude_union_extrude_half_annuli` fail identically without this change.
Admitting 3-vertex pinch wedges needs the per-chain machinery to tolerate a
1-vertex chain, which is the band redesign #758 flagged as its prerequisite.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Choji review — Looks good The three targeted regressions are correctly diagnosed and fixed — the cyclic-anchor retry, the double-winding replay removal, and the collapsed-pinch wedge path all look sound. No correctness defects found in the new code. No findings — looks good. Verified by ChojiChoji ran your change live — checks failed. 3 checks.
Checks marked environment failed in Choji's sandbox, not against your PR. They never block. No live preview was run for this Rust/kernel change. Both No issues found in the running app. Reviewed |
![chojiai[bot]](https://avatars.githubusercontent.com/in/3490784?s=60&v=4){kind=small}
Two more defects from #758's band redesign, both found by bisecting the regressions its own catalogue cases exposed against 2c251ee. 1. realize_bands replayed `band.reversed_winding` Emitting bore-convention loops (lower chain descending, upper ascending) double-applies an orientation the sewing stage already establishes. On the two-half-annuli union it inverted the r20 bore wall, inflating that wall's area 628 → 890 mm² and cutting the solid 7777 → 6031 mm³ (`half_annuli_union_full_annulus`, passing before #758, failing after). The replay earns nothing anywhere measurable: the 752-case torture corpus scores identically with and without it (643 pass, no regressions), as does every other boolean and tessellate test, as does the whole torr catalogue. Zero demonstrated benefit, one demonstrated regression — so stop replaying it. The field stays for diagnostics, with the evidence recorded on it; restoring the replay needs a case that shows it is required. Drops the now-dead `VCAD_NO_WINDING` escape hatch. 2. parse_band refused collapsed-pinch wedges Where a band's two chains meet at a shared vertex, sewing leaves a THREE- vertex loop: the pinch corner plus the two chain endpoints at the opposite column. `try_parse_two_chain` cannot express the resulting one-vertex chain, and the `n < 4` floor refused the loop outright — so every such wedge came back from `split_cylindrical_face_by_sampled` unsplit and the boolean kept material it should have trimmed. Build the band directly instead; the shape is one the downstream machinery already expects (the end-slack close in the same function manufactures exactly it). doc10 (23-blade pattern − r45 annulus), cumulative with the cyclic parse fix: volume 6295 → 3769 → 3346 (expected 3365, tolerance ±16) binary 6h kill → 93s → 17.6s Verified: vcad-kernel-booleans + vcad-kernel-tessellate fully green (126 tests, `half_annuli_union_full_annulus` among them); torture corpus 643 pass, no regressions, +2 improvements (rand-052, rand-180 — not baselined, the baseline is x86_64 and this ran on aarch64); clippy -D warnings clean. Still failing, both pre-existing on main and left alone here: - doc10 now undershoots by 19.4 mm³ against a ±16 tolerance — roughly 0.84 mm³ per blade, the size of the chord-vs-arc sliver lost where each blade meets the r45 wall at the boolean's segment count while the oracle integrates the true cylinder at 256. A discretization gap, no longer a topology defect. - c2_extrude_union_extrude_half_annuli reads +64%. Localized but NOT fixed: the culprit is `weld_boundary_vertices` in repair.rs, which merges boundary vertices at 1.5e-3 without regard for whether the two are already joined by an edge. Welding across such a pair does not join two boundaries, it COLLAPSES that edge — and on a thin overlap wedge it eats the wedge. With the weld off, the union reads 7777.29 against 7854 expected, matching pre-758 exactly, and operand order stops mattering (12861/12886 → 7777 both ways). The obvious guard — skip pairs already joined by a half-edge — is wrong: it costs 7 torture crashes ("half-edge has no next") and 3 unit failures, so some adjacent welds are load-bearing. Tried and reverted; a correct fix needs to separate "this edge is genuinely degenerate" from "this edge is a thin feature" rather than refusing all adjacent pairs. Note the eval path is also wrong at 0° and 135° overlap, which the weld does not explain — a self-union should be idempotent and reads 5376 vs 4104. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ecto
changed the title
Dismissing prior approval to re-evaluate 4c64e8d.
…entity retry Review nit on #771: `cyclic_turning_points` skips any vertex whose own outgoing step is vertical, which reads like it could miss vertex 0. It can, and that is intentional on both counts: - A vertex with a vertical outgoing step is the foot of a connector. Anchoring there would open the rotated list with the connector instead of with a chain; the vertex at the connector's far end — the first that actually leaves in the reversed direction — is the anchor the linear parse wants, and that one IS reported. Mirrors the existing `split_at` walk-back, which likewise pushes connector vertices onto the chain after them. - Anchoring at vertex 0 is the identity rotation, which the linear pass just tried and failed. Now skipped explicitly rather than silently wasted: it saves four parse attempts per face on every loop whose vertex 0 happens to be a turning point, in a function that runs per-face. Both comments added. No behavior change beyond dropping those redundant parses: booleans + tessellate green, torture 643 pass / no regressions, catalogue byte-identical (doc10 3345.944, 17.2s). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
@chojiai Addressed the turning-point nit in f574cd7. You were right that it's harmless, and for the reason you gave — anchoring at vertex 0 is the identity rotation the linear pass already tried. I made that explicit rather than incidental: the retry loop now skips Also documented the broader No behavior change beyond the dropped redundant parses: booleans + tessellate green, torture 643 pass / no regressions, catalogue byte-identical (doc10 3345.944, 17.2s). On the sandbox check failures — agreed those are environmental. 🤖 Addressed by Claude Code |
|
@ecto the change looks solid. The reasoning is tight:
On the sandbox failures: documented-prerequisites explanation is believable and matches what I see in the diff ( LGTM once CI confirms green. No further nits from me. - Choji |
Dismissing prior approval to re-evaluate f574cd7.
|
CI results are in, and they match the local runs exactly — no surprises.
Both Rust jobs report The torture pass on x86_64 is the meaningful confirmation: my local run was aarch64 against an x86_64 baseline, so its "+2 improvements" weren't trustworthy. CI ran it on the baseline's own platform — clean across all 752 cases. One more negative result on the c2 weld defectI took a second run at It fails identically — same 7 torture crashes ( That rules out the whole tolerance-tuning family: the load-bearing welds live in the same tolerance band as the wedge-destroying ones, so no threshold separates them. A correct fix needs a different discriminator entirely — something topological (would collapsing this edge eliminate real face area? is this loop a thin wedge or a degenerate stub?) rather than metric. Also still unexplained and not the weld: the eval path is wrong at 0° overlap, where a self-union should be idempotent and reads 5376 vs 4104. Likely a fourth defect in the same redesign. Where this leaves the PRThree real regressions fixed and the 6h burn stopped, but CI is not green and this PR doesn't claim it will be. The two remaining failures are documented in the body with reproductions. Merging trades a 12-runner-hour-per-PR blackout for a 36-minute red — worth it on its own, but the catalogue stays red until someone lands the weld fix. 🤖 Posted by Claude Code |
Why
CI had been burning the full 6h GitHub job limit on every PR since #758 — 12 runner-hours per run across the stable + nightly matrix. Both Rust jobs were killed mid-test, so no PR had a real Rust signal since 2026-07-31 17:22.
One test caused it:
a2_boolean_over_pattern_doc10went from 2m53s to 4h16m and started returning wrong geometry. The only kernel commit in that window isc3c884d1(#758, chain-based CylBand redesign). Bisecting against its parent2c251ee0turned up three distinct defects.1.
parse_bandassumed the loop started on a chain boundaryIt decomposed a face's outer loop into its two chains by scanning the vertex list linearly (
for i in 0..n-1), so it never saw the direction change carried by the closing edgev[n-1] → v[0]. A loop whose starting half-edge sits mid-chain presents only one visible flip; the decomposition hands the tail a single vertex and bails (short chains N 1).reversed_windingin the same function already compensated for exactly this — "judging by which run came first breaks when the loop's starting half-edge sits mid-chain" — but the decomposition itself never did.The damage wasn't one refused face. Each refusal made
split_cylindrical_face_by_sampledreturn the face unsplit, so the boolean kept geometry it should have trimmed and the next operand split the leftovers again. Fragmentation compounded across the pattern's instances.Fix: when the linear parse fails, re-anchor at each true cyclic turning point and retry. Loops that already start on a boundary take the identical path, so passing cases are untouched.
2.
realize_bandsreplayedband.reversed_windingEmitting bore-convention loops double-applies an orientation the sewing stage already establishes. On the two-half-annuli union it inverted the r20 bore wall — that wall's area 628 → 890 mm², the solid 7777 → 6031 mm³ (
half_annuli_union_full_annulus, passing before #758, failing after).The replay earns nothing anywhere measurable: the 752-case torture corpus scores identically with and without it, as does every other boolean and tessellate test, as does the whole catalogue. Zero demonstrated benefit, one demonstrated regression — so stop replaying it. The field stays for diagnostics with the evidence recorded on it.
3.
parse_bandrefused collapsed-pinch wedgesWhere a band's two chains meet at a shared vertex, sewing leaves a three-vertex loop.
try_parse_two_chaincan't express the resulting one-vertex chain and then < 4floor refused it outright, so every such wedge came back unsplit. Built directly instead — the shape is one the downstream machinery already expects.Results
Also here
VCAD_BAND_DEBUGlookup — the same bug WIP: chain-based CylBand redesign + boolean seam-freeze — 3 of 4 torr seam targets pass #758 fixed forVCAD_SPLIT_DEBUGand missed here.band_dbg!sits insidetry_parse_two_chain, which runs per-face and up to four times per face, andenv::varallocates and takes the environment lock on every call.timeout-minuteson any job, which is why one slow test could cost 12 runner-hours.Testing
vcad-kernel-booleans+vcad-kernel-tessellate: green, 126 tests, includinghalf_annuli_union_full_annulus.torr_boolean_catalogue: 24 passed, 2 failed, 6 ignored — completes instead of hanging.clippy -D warningsclean,fmtapplied.Still failing — reviewer please note
This does not make CI green. Two catalogue cases remain, both pre-existing on main:
doc10 undershoots by 19.4 mm³ against a ±16 tolerance — about 0.84 mm³ per blade, the size of the chord-vs-arc sliver lost where each blade meets the r45 wall at the boolean's segment count while the oracle integrates the true cylinder at 256. A discretization gap, no longer a topology defect.
c2_extrude_union_extrude_half_annulireads +64%. Localized but not fixed. The culprit isweld_boundary_verticesinrepair.rs, which merges boundary vertices at 1.5e-3 without checking whether the two are already joined by an edge. Welding across such a pair doesn't join two boundaries, it collapses that edge — and on a thin overlap wedge it eats the wedge. With the weld off the union reads 7777.29 vs 7854 expected, matching pre-758 exactly, and operand order stops mattering (12861/12886 → 7777 both ways).The obvious guard — skip pairs already joined by a half-edge — is wrong: it costs 7 torture crashes (
half-edge has no next) and 3 unit failures, so some adjacent welds are load-bearing. Tried and reverted. A correct fix has to separate "genuinely degenerate edge" from "thin feature". Note the eval path is also wrong at 0° and 135° overlap, which the weld does not explain — a self-union should be idempotent and reads 5376 vs 4104.🤖 Generated with Claude Code