fix: chamfer the OD and raised-face rim instead of every edge (#103) - #104
Conversation
ReviewScope note: stacked on #102 ( Verification run against the branch
Diagnosis, fix and evidence are all sound. The points below are improvements, not blockers. Suggestions1. 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 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 3. No OKF entry (medium, repo convention). 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 5. Naming and signature nits (low). 6. Out of scope but cheap (low). Possible lead on #101The PR notes in passing that Style, security, performance
|
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>
|
All six addressed in 1. Selector semantics: fixed, but I could not demonstrate a behavioural differenceYou are right that the predicate did not say what its comment claimed. It now uses But I could not construct a case where the old and new predicates disagree, and I tried the
The midpoint of a bolt-hole circle lands at axis-radius So this is a correctness-of-intent change, not a latent bug fix, and it is behaviour-neutral: 2. Chamfer distance parameterised
3. OKF entryAdded 4. The
|
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>
2189c0b to
f51c061
Compare
What & why
Stacks on PR #102 (base branch
fix/100-revolve-shell), which fixes recipe 03'sflange 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) ?? flangereturned nil every single time, and the?? flangefallback silently hid it, so the recipe shipped an un-chamfered body whileits header, README prose, and README API list all advertised a chamfer.
Closes #103
Diagnosed mechanism
Shape.chamfered(distance:)chamfers every edge in oneBRepFilletAPI_MakeChamfercall. 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: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_MakeChamfercannot resolve a blend on theseam 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, intoone call, so
IsDone()is false and it returns nil, at 1 mm and at every smallerdistance 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 andisCircle(which excludes every seam by construction, no seam is a circle). Chamferjust 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 crashesloudly instead of silently shipping an un-chamfered body, matching how every other
operation in this recipe already fails.
Volume evidence
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.brepandoutput.pngare regenerated to match.Verification
Scripts/recipe-check.sh: all recipes pass except the pre-existing, out-of-scope01-mounting-bracketdrift (recipe 01 (mounting-bracket) volume drifts 2.27% from committed reference under OCCTSwift 1.17.0 #101, 2.27% volume drift under the OCCTSwift 1.17.0repin, unrelated to this change and not made worse by it).
Scripts/policy-check.sh: pass.Scripts/verb-check.sh: pass.Checklist
Scripts/recipe-check.shin the same PR: theregenerated
output.brepreference assertssolidCount >= 1and the volume viaocctkit metrics, which is this repo's established test mechanism for recipes(no unit test framework exists here; see CLAUDE.md).
Notes for the reviewer
fix/100-revolve-shell), which fixes recipe 03's revolve to emit a solid instead ofa shell. Only the chamfer-related changes here are this PR's own.
Shape.convexEdges()/concaveEdges()classify thisshape'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 aclearly reentrant corner, and
convexEdges()includes the reentrant edge whileexcluding 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 fileanything upstream about it since it's outside this issue's scope; flagging it here in
case it's useful.