fix: recipes 02 and 03 emit a solid, not a shell (closes #100) - #102
Conversation
Pins the dependency this branch was set up against; no other pin changes.
Shape.revolve(profile: wire, ...) revolves the curve itself and returns a shell (BRepPrimAPI_MakeRevol on a wire), not a solid. That is standard OCCT behaviour, not an OCCTSwift bug, but it is a trap: Shape.extrude(profile: wire, ...) faces the wire for you, so the two neighbouring sweep factories are not symmetric. Face the half-section first with Shape.face(from:), then revolve the face instead of the wire. Regenerated the committed output.brep and output.png: the raw revolve volume is unchanged (247400.42 mm^3 both ways, confirming the old shell was already topologically closed), but the final volume after circularPatternCut drops from 241242.90 to 228927.86 mm^3 (-5.1%). That drop is not the intended fix, it is a second, previously-hidden bug the shell was masking: circularPatternCut's boolean subtract against a non-solid shell only removed 6157.52 mm^3 across the 8 bolt holes instead of the correct 8 * pi * 7^2 * 15 = 18472.56 mm^3 (verified by isolating the raw-revolve and post-cut volumes on both the old and new construction). The new volume matches the expected geometry exactly; the old one did not. Part of #100.
Shape.sweep(profile:along:) wraps BRepOffsetAPI_MakePipe, which never caps the ends of the swept tube: it returns a shell even for a closed circular profile. Switch to Shape.pipeShell(spine:profile:mode:solid:), which wraps BRepOffsetAPI_MakePipeShell and has an explicit solid: flag that caps the ends into a genuine solid. This is the documented canonical spring recipe in the OCCTSwift cookbook (Helices & Springs), reached independently of the recipe 03 fix since this is a pipe sweep, not a revolve. Regenerated the committed output.brep and output.png. Volume drops from 9729.67 to 8575.15 mm^3 (-11.9%). Unlike recipe 03, this is not a hidden second bug: an open (uncapped) shell has no well-defined enclosed volume, so Shape.volume on the old shell was already reading an artifact of the algorithm, not a physical quantity. The new, capped-solid volume matches the analytic round-wire coil formula pi * r^2 * L (r = 2mm, L = coil length over 6 turns at mean radius 18mm and pitch 12mm) to within 0.06%, the old one was off by 13%. Part of #100.
Add solidCount (Shape.subShapes(ofType: .solid).count) as an opt-in metric alongside volume / surfaceArea / boundingBox / principalAxes. shapeType alone cannot distinguish a healthy circularPatternCut result (compound wrapping one solid) from a wire-based revolve or sweep that never got faced/capped (compound or shell wrapping zero solids), both of which can report the same top-level shapeType. Needed by Scripts/recipe-check.sh's hardened solids >= 1 assertion, next commit. Part of #100.
…ss bug Assert the emitted body's solidCount (via the new metrics field) is at least 1, not shapeType == "solid": five of the seven recipes legitimately report compound, since a compound wrapping one solid is the normal result of circularPatternCut. Asserting solidCount is what actually would have caught both recipe 02 and 03 shipping a shell. While wiring this up, found and fixed a pre-existing bug that made every assertion in this script unable to fail. check_one() is always invoked as check_one ... || status=1, and bash disables errexit for every command inside a function while that function is the left-hand side of ||. The Python validation block was a bare statement, so a nonzero exit from any die() call (missing/empty manifest or body, volume <= 0, now solidCount < 1, or a reference drift) fell straight through to the unconditional final echo and "OK" was printed and the function returned 0 regardless. Confirmed on the unmodified script before this fix: a real reference-drift failure on 01-mounting-bracket (unrelated to #100, see #101) printed the die() message and then "OK" and exited 0. Fixed by wrapping the Python invocation in if ! ...; then return 1; fi. Proved the hardened check has teeth: temporarily reverted the recipe 03 face fix back to Shape.revolve(profile: wire, ...), reran Scripts/recipe-check.sh recipes/03-pipe-flange, got a clean failure ("solidCount not >= 1: 0") and exit 1, then restored the fix and reconfirmed a clean pass. make recipes-test now fails on 01-mounting-bracket for an unrelated, pre- existing reason (a 2.27% volume drift against its committed reference, tracked separately as #101) rather than passing silently; 02 through 07 all pass with solidCount == 1 and match their reference output.brep exactly. Closes #100.
ReviewVerified the diff against the OCCTSwift API surface at the declared floor, the regenerated BREP topology, and the volume arithmetic. What I verified independently (all holds up)
Issues to address in this PR1. The same two traps remain documented as correct elsewhere in this repo. This is the significant gap.
Same class,
The 2. No OKF entry. Repo precedent is direct: #82 produced 3. 4. let solidCount: Int? = wants("solidCount") ? shape.subShapeCount(ofType: .solid) : nilOCCTSwift's 5. if os.path.exists(ref):
r = metrics(ref)
if solids != r.get("solidCount"):
die(f"solidCount drift: {solids} vs reference {r.get('solidCount')}")
RisksThe Follow-up worth filing (pre-existing, out of scope)Recipe 03's chamfer has never applied. This PR's own evidence proves it: Nits
Style complianceClean. No em-dashes in any added line (checked across the whole non-generated diff), no banned words. Commit messages are unusually good: each states the OCCT mechanism, the numbers, and the verification. Comment density in VerdictThe geometry work is correct and well evidenced. Items 1 through 3 (cookbook / SCRIPT_WORKFLOW docs, OKF entry, CLAUDE.md) should land before merge under the repo's own |
…des) Item 1, docs drift. The two traps this PR fixes were still documented as correct elsewhere, and the cookbook is what people copy: * docs/guides/cookbook/sweeps-lofts-patterns.md mirrored recipe 02 verbatim, still built the spring with Shape.sweep, and described the result as a solid. Now uses pipeShell(solid: true) and its gotcha explains why sweep returns a shell that still reports a plausible volume. * docs/SCRIPT_WORKFLOW.md's "From profiles" cheat sheet listed extrude, revolve and sweep on consecutive lines with nothing saying only extrude faces the wire for you. That adjacency is where the wrong assumption forms, so each line is now annotated solid or SHELL, with the facing alternative alongside. * The same file's two "Rail solid" sweeps now use pipeShell. Item 2, OKF entries. Two durable rules were carried only in commit messages and a shell comment: * wire-sweep-factories-are-not-symmetric: which factories face a wire for you, and why to assert solidCount >= 1 rather than shapeType or a positive volume. * errexit-is-suppressed-in-or-context: why a function invoked as `f || status=1` must return 1 explicitly, with the recipe-check silent-pass bug as the worked example. Cross-linked to single-source-verb-inventory as the same guard-that-cannot-fail family. Item 3, CLAUDE.md:91 now lists solidCount alongside the rest of what metrics wraps, including the assert-solidCount-not-shapeType guidance. It was the second hand-maintained copy drifting from docs/reference/occtkit-verbs.md. Item 4, Metrics.swift uses subShapeCount(ofType:) rather than subShapes(ofType:).count. The latter allocates a Shape handle per solid only to discard them, and the counting API already existed. Verified behaviour-neutral: solidCount unchanged across recipes 02, 03 and 05. Item 5, recipe-check.sh now compares solidCount against the committed reference, not just >= 1. That catches the other half of the family: a topology regression that keeps a positive volume, such as a compound going from 1 solid to 3 because a boolean stopped fusing. Negative-tested by rewriting a reference to a 2-solid compound, confirming "solidCount drift: 1 vs reference 2", then restoring. Nits: qualified Shape.revolved(...) in recipe 03's API list, and restored the signedVolume / orientedForward pointer to recipe 02's gotchas, scoped to orientation debugging rather than the topology check. Also filed #103: recipe 03's chamfer has never applied. chamfered(distance: 1.0) returns nil and the ?? fallback hides it, confirmed directly and corroborated by this PR's own volume arithmetic. Out of scope here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
All five items addressed in 1. Docs driftThe sharpest catch in the review: the cookbook is what people copy, and it contradicted the recipe it documents.
2. OKF entriesTwo added, 3. CLAUDE.md:91Updated, including the assert- 4.
|
ReviewVerification run against the branch
The negative control in the PR body, and the OKF entry that codifies "break it deliberately, watch it go red, put the evidence in the PR", are the strongest parts of this change. Issues and suggestions1. Merging leaves the 2. The hardened check's own output is misordered under buffering (medium). Python's stdout is block-buffered when piped while The real order is volume, solidCount, then drift. Fix with 3. Reference doc names an API the verb does not call (low). 4. CLAUDE.md's OCCTSwift floor is stale (low). It still says 5. The two OKF texts describe the same number differently (low). 6. 7. Trivial. Observations, no action needed
|
…green Item 1, the red suite. Rather than land a knowingly-failing check, this regenerates recipe 01's reference and closes #101. That is safe now because #101 is fully diagnosed: the 2.27% is OCCTSwift#272, Shape.drilled ignoring direction and hardcoding +Z, fixed between 1.12.0 and 1.12.9. Proven arithmetically, a +Z-forced bore entering at y = -1 removes only the circular segment above the face, 12.3387 mm2 x 7.0 = 86.37 mm3, exactly the per-hole figure the old reference was built from at 1.3.1. Current removes pi*r^2*t = 192.42 exactly and holds at 2.0.0-kernel.1. So the old reference baked in an upstream bug and the new one is correct. Full suite is now green, exit 0. Note the bracket was already un-filleted in the old reference: recipe 01's fillet has never applied at any version, since filletRadius 8 exceeds the 5mm leg thickness. That is #105 and will regenerate this reference again. Item 2, python3 -u. stdout was block-buffered when piped while die() wrote to stderr unbuffered, so failures printed above the passing lines that preceded them. Verified fixed: a forced drift now prints volume, then solidCount, then the failure, in that order. This mattered because the point of the change was making this script's output trustworthy. Item 3, docs/reference/occtkit-verbs.md said subShapes(ofType:).count while Metrics.swift calls subShapeCount(ofType:). Aligned. Item 4, CLAUDE.md's OCCTSwift floor still read >= 1.15.0 and "Floored at v1.15.0" while Package.swift has said from: "1.17.0" since d5d31e8. Updated, including the stale gsdali URL, and noted that the 1.17.0 floor also carries the #272 drilling fix. Item 5, okf/log.md said the flange cut was "removing one third of the correct material", which reads as though a third of the flange vanished. Reworded to match the decision entry: it removed one third of what it should have, leaving two thirds of each bolt hole unfinished. Item 6, added a downstream note to the metrics verb reference: solidCount is in the default-all set, so existing consumers including OCCTMCP's compute_metrics get one extra optional field, and why it is default-all rather than opt-in. Item 7, trailing blank line in okf/decisions/index.md. Closes #101. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
All seven addressed in 1. The red suite: closed #101 insteadYou framed the risk exactly right, that landing a knowingly-failing suite trains people to ignore the check, which is the failure this PR's own OKF entry warns about. So rather than add an That is safe because #101 is now fully diagnosed rather than merely suspected. It is So the old reference baked in an upstream bug, and the new one is correct. Details on #101. One thing worth noting: the bracket was already un-filleted in the old reference, because recipe 01's fillet has never applied at any version (radius 8 on a 5 mm leg). So this regeneration corrects only the drilling. #105 will regenerate it once more. 2. Buffering
Correct order now. Your point that this mattered because the change is about making this script's output trustworthy was the right reason to fix it here rather than later. 3 to 7
On your observationsThe note that The double reference churn across the stack is real and I agree the sequencing is right: #102 regenerates 03 for the shell fix, #104 regenerates it again for the chamfer. Recipe 01 will churn twice too, once here and once for #105. VerificationFull suite green at exit 0, |
1 participant
What & why
Recipes 02 (helical spring) and 03 (pipe flange) each emitted a shell where they should
emit a solid:
Shape.revolve(profile: wire, ...)revolves the curve itself, andShape.sweep(profile:along:)never caps the ends of the swept tube, so both are correctOCCT behaviour on a wire/uncapped pipe, not OCCTSwift bugs, but both are traps relative to
their extrude/pipeShell siblings. Fixed each on its own terms (face-then-revolve for 03,
pipeShell(..., solid: true)for 02, see per-commit messages), regenerated the affectedrecipes' reference
output.brep/output.png, and hardenedScripts/recipe-check.shtoassert
solidCount >= 1on every recipe's emitted body so this class of bug cannot shipsilently again.
Closes #100.
Before / after solid counts
shapeType=shellsolids=0shapeType=solidsolids=1shapeType=compoundsolids=0shapeType=compoundsolids=1Measured via
Shape.subShapes(ofType: .solid).count(now exposed asmetrics's newsolidCountfield), not by readingshapeType, per the issue's own scoping note: five ofthe seven recipes legitimately report
compound(one solid wrapped bycircularPatternCut), soshapeType == "solid"would reject correct recipes.Volume deltas on the regenerated references
Both changed. Investigated both rather than assuming "same geometry, only topology
changed":
identical either way (247400.42 mm^3), confirming the old shell was already a closed,
well-defined surface. The drop happens at the
circularPatternCutstep: cutting aboolean tool against a non-solid shell only removed 6157.52 mm^3 across the 8 bolt
holes, not the correct
8 * pi * 7^2 * 15 = 18472.56 mm^3. That is a second,previously-hidden bug the shell was masking (some bolt holes were not being fully cut
through). The new volume matches the expected geometry exactly.
well-defined enclosed volume, so
Shape.volumeon the old shell was reading an artifactof the GProp algorithm, not a physical quantity. The new capped-solid volume matches the
analytic round-wire coil formula
pi * r^2 * L(r = 2mm, L = the helical length over 6turns at mean radius 18mm / pitch 12mm) to within 0.06%; the old value was off by 13%.
Negative-control evidence that the hardened check has teeth
Temporarily reverted the recipe 03 fix back to
Shape.revolve(profile: wire, ...)andreran the check:
Restored the fix, reconfirmed a clean pass (
solidCount = 1,EXIT=0).Separately, found and fixed a pre-existing bug in
recipe-check.shitself while wiringthis up:
check_one()is always called ascheck_one ... || status=1, and bash disableserrexitfor every command inside a function while that function is the left-hand side of||. The Python validation block was a bare statement, so anydie()failure (missing orempty manifest/body, volume not > 0, now
solidCount < 1, or a reference drift) printedits message and then fell straight through to the unconditional final
echo "... OK", sothe script exited 0 regardless. Confirmed this on the unmodified script using a real,
unrelated failure (see below): it printed the failure line and then "OK" and exited 0.
Fixed by wrapping the Python invocation in
if ! ...; then return 1; fi.make recipes-testresult01-mounting-bracketwas already out of scope for #100 (itssolidCountwas already 1,the issue's own audit confirmed it unaffected) and this PR does not touch it. Its failure
is a real, pre-existing, unrelated 2.27% volume drift against its committed reference,
deterministic across repeated runs, not flaky, and reproduced with only the
Package.resolvedOCCTSwift 1.15.0 to 1.17.0 repin already staged on this branch (noother recipe drifts on that same repin). It was previously invisible because of the
recipe-check.shbug above. Filed separately as#101 rather than folded
into this PR, since it is a numeric fillet-result drift, not a topology bug, and the issue
explicitly scoped recipe 01 as unaffected and not to be changed.
Checklist
unit test framework (
CLAUDE.md: "No tests exist. No linter is configured.");Scripts/recipe-check.sh'ssolidCount >= 1assertion plus the regeneratedreference
output.brepcomparisons are its equivalent, and the negative-controlevidence above demonstrates the assertion actually catches the regression it
targets.
Notes for the reviewer
assume the revolve fix applies to the sweep: recipe 03 needed
Shape.face(from:)beforerevolving; recipe 02 needed
Shape.pipeShell(..., solid: true)in place ofShape.sweep, which is the documented canonical spring recipe in the OCCTSwift cookbookand has no
Shape.sweep-equivalent "face it first" fix,BRepOffsetAPI_MakePipesimplyhas no capping mode.
solidCountto themetricsverb (Shape.subShapes(ofType: .solid).count)rather than building a separate one-off check, since
recipe-check.shalready shellsout to
occtkit metricsfor volume/boundingBox and no existing verb exposed a solidcount. Documented in
docs/reference/occtkit-verbs.md.Package.swiftorPackage.resolvedbeyond what the branch already hadstaged (OCCTSwift 1.15.0 to 1.17.0) per the task setup; included that pre-staged diff in
its own commit so the branch is self-contained and buildable from a fresh checkout.