Skip to content

fix: chamfer the OD and raised-face rim instead of every edge (#103) - #104

Merged
gsdali merged 2 commits into
mainfrom
fix/103-flange-chamfer
Aug 4, 2026
Merged

fix: chamfer the OD and raised-face rim instead of every edge (#103)#104
gsdali merged 2 commits into
mainfrom
fix/103-flange-chamfer

Conversation

@gsdali

@gsdali gsdali commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator

What & why

Stacks on PR #102 (base branch fix/100-revolve-shell), which fixes recipe 03's
flange to emit a genuine solid instead of a shell. This PR fixes the separate,
pre-existing bug #102's review surfaced: recipe 03's chamfer never applied.
flange.chamfered(distance: 1.0) ?? flange returned nil every single time, and the
?? flange fallback silently hid it, so the recipe shipped an un-chamfered body while
its header, README prose, and README API list all advertised a chamfer.

Closes #103

Diagnosed mechanism

Shape.chamfered(distance:) chamfers every edge in one BRepFilletAPI_MakeChamfer
call. The flange, a solid of revolution, has a seam edge on every periodic cylindrical
face the revolve creates: the bore, the OD, the raised-face wall, and each of the eight
bolt holes (11 seam edges total, out of 33). I isolated every edge with
chamferedWithFullHistory(distance:edges:) on a single edge at a time:

  • Every circular edge chamfers fine alone.
  • Every one of the 11 seam edges (the straight closing edge of a periodic surface)
    fails alone, at every distance tried: 1, 0.5, 0.1, 0.01, and 0.001 mm. Not a size
    problem, a structural one: BRepFilletAPI_MakeChamfer cannot resolve a blend on the
    seam of a periodic surface, where both "adjacent" faces it needs are really the same
    face referenced twice.

chamfered(distance:) bundles all 33 edges, including the 11 unchamferable seams, into
one call, so IsDone() is false and it returns nil, at 1 mm and at every smaller
distance I tried down to 0.01 mm (0.01 mm and below happen to succeed, which is
incidental, not a fix; the recipe still wants a 1 mm chamfer on the real edges).

Fix

Select only the edges that should genuinely be broken: the OD (front + back) and the
raised-face rim, using Shape.edges(where:) filtered by radius-from-axis and
isCircle (which excludes every seam by construction, no seam is a circle). Chamfer
just those with Shape.chamferedWithFullHistory(distance:edges:).

The raised-face step's base (where the disk-front annulus meets the raised-face
wall) is deliberately excluded: it's a reentrant corner, so chamfering it adds material
into the corner instead of breaking one (confirmed by volume: chamfering it alone
increases volume by ~158 mm^3), and chamfering it together with the rim exhausts the
2mm-tall wall and fails outright.

The result is force-unwrapped (!), not ?? flange: a future regression now crashes
loudly instead of silently shipping an un-chamfered body, matching how every other
operation in this recipe already fails.

Volume evidence

volume (mm^3)
before chamfer (baseline, matches the committed pre-fix reference) 228927.86
after this fix 228302.68
delta -625.18 (-0.27%)

A single 1 mm chamfer on the OD alone removes ~pi * R * distance^2 = pi * 75 * 1 =
~235.6 mm^3 (measured in isolation: 234.57 mm^3, matching closely). This fix chamfers
both OD edges plus the raised-face rim, for a combined 625.18 mm^3 removed, well above
the ~235 mm^3 threshold that distinguishes "the chamfer applied" from "reproduced the
bug." shape.subShapeCount(ofType: .solid) is 1 both before and after.

recipes/03-pipe-flange/output.brep and output.png are regenerated to match.

Verification

Checklist

  • New behavior is covered by Scripts/recipe-check.sh in the same PR: the
    regenerated output.brep reference asserts solidCount >= 1 and the volume via
    occtkit metrics, which is this repo's established test mechanism for recipes
    (no unit test framework exists here; see CLAUDE.md).

Notes for the reviewer

  • Do not merge; this PR stacks on the still-open fix: recipes 02 and 03 emit a solid, not a shell (closes #100) #102 (base branch
    fix/100-revolve-shell), which fixes recipe 03's revolve to emit a solid instead of
    a shell. Only the chamfer-related changes here are this PR's own.
  • While diagnosing, I found Shape.convexEdges() / concaveEdges() classify this
    shape's edges in a way that doesn't match what chamfering them actually does
    (volume-increase vs decrease): concaveEdges() returns empty for a shape that has a
    clearly reentrant corner, and convexEdges() includes the reentrant edge while
    excluding the genuinely convex one. I did not rely on either for the selection here,
    used edges(where:) with an explicit geometric predicate instead, and did not file
    anything upstream about it since it's outside this issue's scope; flagging it here in
    case it's useful.

@gsdali

gsdali commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator Author

Review

Scope note: stacked on #102 (fix/100-revolve-shell); git diff base...head is 4 files, so the diff under review is genuinely this PR's own. Not for merge before #102.

Verification run against the branch

  • Reference volumes via occtkit metrics on both blobs: 228927.8567 (base) to 228302.6797 (head), delta -625.18, -0.273%. Matches the PR body exactly. Bounding box unchanged in both, as expected, since the OD cylinder still reaches r=75 between the two chamfers.
  • Read the regenerated output.brep: Surfaces gains exactly three conical surfaces, at (y=1, r=75, +pi/4), (y=14, r=75, -pi/4), (y=16, r=50, +pi/4). That is precisely OD-back, OD-front and raised-face rim, at 1 mm and 45 degrees. The So (solid) record is intact.
  • Predicate math checks out: the revolve axis is Y, so sqrt(x^2 + z^2) is the right radius. The rim gate bounds.min.y > thickness + 1e-3 separates the rim (y=17) from the step base (y=15), and bolt-hole edges top out at y=15 because the holes only pierce the disk (boltCircleRadius 60 > raisedRadius 50), so they cannot match.
  • The force unwrap is well founded: per OCCTSwift docs, chamferedWithFullHistory(distance:edges:) returns nil "on failure or empty edge list", so a predicate that silently stops matching crashes rather than no-ops.
  • Test coverage is real without a new test: recipe-check.sh compares against the reference volume with relative TOL=1e-3, and a regression to un-chamfered is 2.73e-3 drift, so it fails. Together with the base branch's new solidCount >= 1 assert, the checklist claim holds.

Diagnosis, fix and evidence are all sound. The points below are improvements, not blockers.

Suggestions

1. The selector does not do what its comment claims (medium). The comment says selection is geometric "so the recipe still finds the right edges if the parameters change", but nearAxisRadius samples one point at the parametric midpoint and asks "does some point on this circle lie at radius R", not "is this circle centred on the axis with radius R". Bolt-hole edges span radius 53 to 67 today, so no alias exists; shift boltCircleRadius to 55 and that range covers raisedRadius 50, at which point the only thing keeping them out is the y-gate, which is incidental rather than structural. Both APIs needed to state the real condition exist: centerOfCurvature(at: mid) (require |c.x| and |c.z| below tolerance, i.e. centred on the axis) and 1/curvature(at: mid) for the true circle radius.

2. Chamfer distance is a bare literal with undocumented parameter coupling (medium). Every other dimension in this recipe is a named parameter with a documented valid range, but the chamfer is 1.0 inline. Now that the ?? flange fallback is (correctly) gone, raisedHeight: 0.5 or thickness: 1 turns a graceful degrade into a hard crash. Either promote it to chamferDistance in the parameter block and tighten the README table (raisedHeight becomes > chamferDistance, likewise thickness), or state the constraint in Gotchas.

3. No OKF entry (medium, repo convention). CLAUDE.md and the OKF policy require durable decisions to land in okf/, and #102 did exactly that (two okf/decisions/ entries plus okf/log.md lines) for its findings. "chamfered(distance:) is structurally unusable on any full revolve, because every periodic face contributes a seam edge BRepFilletAPI_MakeChamfer cannot blend" is exactly that kind of reusable constraint, and it currently lives only in one recipe's header. A future author hitting it on recipe 04 or 06 will not find it there. Suggest okf/decisions/revolve-seams-cannot-be-chamfered.md plus an okf/log.md line under 2026-08-05.

4. "No seam is a circle" holds only for a polygonal profile (low). The seam edges of a revolve are the profile edges themselves, so they are lines here. Put a fillet or arc in the half-section and the seam becomes a circular arc, where isCircle stops excluding it. Worth a qualifying clause in the README gotcha, since that section is teaching material. OCCTSwift also has an explicit Edge.isSeam(on:) if a future variant needs the direct test.

5. Naming and signature nits (low). nearAxisRadius reads as "radius near the axis" and hides its isCircle gate; radiusFromAxis or axisCircleRadius says what it returns. borrowing Edge is unusual for a copyable value here, and edges(where:) is itself declared (Edge) -> Bool, so a plain _ edge: Edge matches both the library and the rest of the recipe collection. The call could also use trailing-closure form, matching the OCCTSwift doc examples.

6. Out of scope but cheap (low). docs/SCRIPT_WORKFLOW.md:138 still lists shape.chamfered(distance: 0.5) in the API cheatsheet with no caveat. One clause pointing at recipe 03's gotcha closes the loop under docs-current.

Possible lead on #101

The PR notes in passing that convexEdges() / concaveEdges() misclassify this shape. Recipe 01 does prism.filleted(edges: prism.concaveEdges(), radius: filletRadius) ?? prism (recipes/01-mounting-bracket/main.swift:40), which is both the same classifier and the same silent ?? fallback pattern this PR just removed. If concaveEdges() semantics moved under the OCCTSwift 1.17.0 repin, that would produce exactly the kind of quiet volume drift #101 reports. Worth checking before writing #101 off as unrelated drift, and worth giving recipe 01 the same fallback removal in that follow-up.

Style, security, performance

  • No em-dashes in the added prose; writing-style policy respected.
  • No security surface (recipe script, no external input).
  • Strictly faster: three edges chamfered instead of a 33-edge attempt that always failed.

gsdali added a commit that referenced this pull request Aug 4, 2026
Item 1, the selector now states its real condition. It previously sampled one
point at the parametric midpoint and asked "does some point on this edge lie at
radius R", which is weaker than "is this a circle concentric with the revolve
axis of radius R". Now uses centerOfCurvature(at:) to pin the centre to the axis
and 1/curvature(at:) for the true radius, so bolt-hole circles are excluded on
their own geometry rather than incidentally by the y-gate.

Reporting this accurately: I could not construct a case on this shape where the
old and new predicates disagree. The aliasing values the review suggests
(boltCircleRadius 55, 57, 68, 43) all either leave both predicates agreeing at 3
edges, or change the geometry so that the extra matches are genuine concentric
arcs. So this is a correctness-of-intent change, not a latent bug fix, and it is
behaviour-neutral: volume is 228302.67972902392 before and after, identical to
the committed reference.

Item 2, chamferDistance is now a named parameter with its coupling documented:
it must stay below both raisedHeight and thickness, since the chamfer is cut into
those walls and the failure is now a hard crash rather than a graceful degrade.

Item 3, added okf/decisions/revolve-seams-cannot-be-chamfered.md plus index and
log lines. The reusable constraint is that a full revolve makes every periodic
face carry a seam edge, BRepFilletAPI cannot blend a seam because both adjacent
faces are the same face, and the all-edge convenience call therefore fails
entirely rather than skipping it.

Item 4, the README now states that excluding seams via isCircle only holds for a
polygonal profile: a revolve's seams are the profile edges themselves, so an arc
in the half-section makes the seam circular. Points at Edge.isSeam(on:) for that
case, and notes the concentricity test excludes seams regardless.

Item 5, renamed nearAxisRadius to axisCircleRadius, dropped the unusual
`borrowing Edge` for a plain parameter matching edges(where:)'s own signature,
and switched the call to trailing-closure form.

Item 6, docs/SCRIPT_WORKFLOW.md's cheatsheet now carries the seam caveat on both
chamfered(distance:) and the all-edge filleted(radius:), which has the same trap.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@gsdali

gsdali commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator Author

All six addressed in 2189c0b. One of them I have to report as not reproducible, which changes what that change is worth.

1. Selector semantics: fixed, but I could not demonstrate a behavioural difference

You are right that the predicate did not say what its comment claimed. It now uses
centerOfCurvature(at:) to require the centre on the revolve axis and 1 / curvature(at:) for the
true radius, so bolt-hole circles are excluded on their own geometry rather than incidentally by
the y-gate.

But I could not construct a case where the old and new predicates disagree, and I tried the
ones your analysis implies:

boltCircleRadius hole edges span old selects new selects
60 (shipped) 53..67 3 3
55 (your example) 48..62 12 12
57 (aliases raisedRadius at the sampled midpoint) 50..64 3 3
43 (same, from below) 36..50 3 3
68 (aliases outerRadius) 61..75 17 17

The midpoint of a bolt-hole circle lands at axis-radius |bcr - boltRadius|, so 57 and 43 are the
exact aliasing values for raisedRadius. At those values the y-gate excludes them, as you said. At
55 and 68 the holes pierce the raised face or the OD, so the geometry genuinely changes and the
extra matches are real concentric arcs, which I confirmed: at 57 the nine edges where the old
predicate reports radius 50 are matched by the new predicate too, because they genuinely are
concentric r=50 arcs.

So this is a correctness-of-intent change, not a latent bug fix, and it is behaviour-neutral:
228302.67972902392 before and after, identical to the committed reference. I would rather state
that than imply I closed a hole I could not actually open.

2. Chamfer distance parameterised

chamferDistance is now in the parameter block, with the coupling documented where someone
changing raisedHeight or thickness will see it. You are right that removing the fallback turned
a graceful degrade into a crash, which makes the constraint real rather than stylistic.

3. OKF entry

Added okf/decisions/revolve-seams-cannot-be-chamfered.md with index and log.md lines. Your
framing of the durable rule was the right one, so the entry leads with it: every periodic face of a
full revolve carries a seam, BRepFilletAPI cannot blend a seam because both adjacent faces are
the same face, and the all-edge call therefore fails entirely rather than skipping it.

4. The isCircle caveat

Good catch, and it is exactly the kind of thing that quietly stops working later. The README now
says the exclusion holds only for a polygonal profile, since a revolve's seams are the profile
edges, and points at Edge.isSeam(on:). Worth noting the new concentricity test excludes seams
regardless, since a seam lies in a plane through the axis rather than on a circle around it.

5. Naming and signature

axisCircleRadius, plain _ edge: Edge matching edges(where:)'s own signature, trailing-closure
call. All three taken.

6. SCRIPT_WORKFLOW.md

Caveat added, and also on the all-edge filleted(radius: 1.0) two lines above, which has the same
trap.

On the #101 lead

Worth closing that loop: I ran the comparison, and the concaveEdges() theory is refuted.

1.3.1  -> concave=2 convex=16 removed=824.18
1.15.0 -> concave=2 convex=16 removed=824.18
1.17.0 -> concave=2 convex=16 removed=824.18

Identical. The real cause is OCCTSwift#272,
Shape.drilled ignoring direction and hardcoding +Z, closed 2026-07-18. Proven arithmetically: a
+Z-forced bore at y = -1 cuts only the circular segment above the face, 12.3387 mm2 x 7.0 = 86.37 mm3, exactly the per-hole removal at 1.3.1. Current 1.17.0 removes pi*r^2*t = 192.42
exactly, and that holds at 2.0.0-kernel.1 too. Bisected to between 1.12.0 and 1.12.9.

So #101 is a fix surfacing late, not a regression: regenerate the reference. Details on that
issue.

Your instinct about giving recipe 01 the same fallback removal was right for a different reason
though: its fillet has never applied at any version, because filletRadius = 8 exceeds the 5 mm
leg thickness. Filed as #105.

@gsdali
gsdali changed the base branch from fix/100-revolve-shell to main August 4, 2026 22:03
gsdali and others added 2 commits August 5, 2026 08:04
Shape.chamfered(distance:) chamfers every edge, and cannot build a chamfer
at all on this shape: the revolve produces a seam edge on every periodic
cylindrical face it creates (the bore, the OD, the raised-face wall, and
each of the eight bolt holes), and BRepFilletAPI_MakeChamfer cannot resolve
a blend on the seam of a periodic surface, where both "adjacent" faces are
really the same face. Verified this holds at every distance from 1 mm down
to 0.001 mm in isolation, so it is not a size problem the old ?? flange
fallback happened to paper over; it degrades every single time.

Select the OD and the raised-face rim edges geometrically with
Shape.edges(where:), filtering by radius from the axis and isCircle (which
excludes every seam), then chamfer just those with
Shape.chamferedWithFullHistory(distance:edges:). The step's base (where the
disk-front annulus meets the raised-face wall) is left out: it is a
reentrant corner, so chamfering it adds material instead of breaking one,
and chamfering it together with the rim exhausts the 2mm-tall wall and
fails outright.

The chamfer result is force-unwrapped rather than falling back with ??, so
a future regression crashes loudly instead of silently shipping an
un-chamfered body.

Volume: 228927.86 -> 228302.68 mm^3 (-625.18 mm^3, -0.27%), comfortably
above the ~235 mm^3 a single 1mm OD chamfer alone should remove, and
subShapeCount(ofType: .solid) is 1 both before and after. Regenerated
output.brep and output.png to match.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Item 1, the selector now states its real condition. It previously sampled one
point at the parametric midpoint and asked "does some point on this edge lie at
radius R", which is weaker than "is this a circle concentric with the revolve
axis of radius R". Now uses centerOfCurvature(at:) to pin the centre to the axis
and 1/curvature(at:) for the true radius, so bolt-hole circles are excluded on
their own geometry rather than incidentally by the y-gate.

Reporting this accurately: I could not construct a case on this shape where the
old and new predicates disagree. The aliasing values the review suggests
(boltCircleRadius 55, 57, 68, 43) all either leave both predicates agreeing at 3
edges, or change the geometry so that the extra matches are genuine concentric
arcs. So this is a correctness-of-intent change, not a latent bug fix, and it is
behaviour-neutral: volume is 228302.67972902392 before and after, identical to
the committed reference.

Item 2, chamferDistance is now a named parameter with its coupling documented:
it must stay below both raisedHeight and thickness, since the chamfer is cut into
those walls and the failure is now a hard crash rather than a graceful degrade.

Item 3, added okf/decisions/revolve-seams-cannot-be-chamfered.md plus index and
log lines. The reusable constraint is that a full revolve makes every periodic
face carry a seam edge, BRepFilletAPI cannot blend a seam because both adjacent
faces are the same face, and the all-edge convenience call therefore fails
entirely rather than skipping it.

Item 4, the README now states that excluding seams via isCircle only holds for a
polygonal profile: a revolve's seams are the profile edges themselves, so an arc
in the half-section makes the seam circular. Points at Edge.isSeam(on:) for that
case, and notes the concentricity test excludes seams regardless.

Item 5, renamed nearAxisRadius to axisCircleRadius, dropped the unusual
`borrowing Edge` for a plain parameter matching edges(where:)'s own signature,
and switched the call to trailing-closure form.

Item 6, docs/SCRIPT_WORKFLOW.md's cheatsheet now carries the seam caveat on both
chamfered(distance:) and the all-edge filleted(radius:), which has the same trap.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@gsdali
gsdali force-pushed the fix/103-flange-chamfer branch from 2189c0b to f51c061 Compare August 4, 2026 22:06
@gsdali
gsdali merged commit 8b62b25 into main Aug 4, 2026
5 checks passed
@gsdali
gsdali deleted the fix/103-flange-chamfer branch August 4, 2026 22:12
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.

recipe 03 chamfer never applies: chamfered() returns nil and the ?? fallback hides it

1 participant