fix(#613): seven entry points indexed sub-shapes by occurrence, and the bracket's concave edge was invisible - #650
Conversation
…he bracket's concave edge was invisible stayed on a bare TopExp_Explorer, which yields one entry per occurrence: a plain 10mm box has 24 edge occurrences over 12 edges and 48 vertex occurrences over 8 vertices, so the two disagree on any ordinary solid. The end-to-end failure is the composition the API's own docs recommend. edgeConcavities() zips the bridge's per-occurrence answer against edges(), so on an L-bracket the one concave edge (map index 27) was labelled convex and concaveEdges() returned []. filleted(edges: concaveEdges()) rounded nothing and reported success. edgeConcavityCount(.convex) answered 24 on a 12-edge box. Converted: the concavity classifier and its count, OCCTBRepExtremaExtCC, checkSubShape (behind checkEdge/Wire/Shell/Vertex), OCCTLocOpeSplitShapeByVertex, both mesh entry points, and the Edge.index handed back by edgesInFace(at:) and commonEdges(with:). Plus OCCTBiTgteBlend and OCCTBiTgteBlendInfo_, which neither the issue nor its audit named, found by sweeping for the idiom. Meshing uses both enumerations rather than choosing: triangle winding comes from the occurrence's orientation and the stamped faceIndex from the map, via a new occtForEachOrientedFace. Meshing off the map alone would drop a shared wall's second side (measured: +x 2, -x 0). OCCTPolyMergeNodes is deliberately NOT converted for the same reason, and is now pinned by a test that fails if a later sweep converts it. Sites 1-4 were confirmed orientation-insensitive by measurement, not by reading headers: every consumer was handed both orientations of every box edge (12 pairs) and vertex (8 pairs), 0 differing. Closes #613
…me from the fixture it described B1. edgesInFace(at: 3) on a 10mm box: the real indices are 2, 6, 10, 11 and ALL FOUR were wrong, by 10.00, 12.25, 7.07 and 12.25 mm. I had written 2, 8, 10, 11 / 10.00, 16.75, 0.71, 13.80 and "the fourth matched by coincidence", in six places including SEMVER.md, a user-facing contract. Root cause, two compounding errors, both from transcribing an exploratory probe instead of measuring the claim: the offsets came from Edge.point(at: 0.5) -- a raw CURVE PARAMETER, not an arc-length midpoint -- on the origin-centred box spelling, while the text described midpoints; and the "real indices" were never measured at all, only inferred, so the 8 was simply wrong (edges()[8] is at (5,0,0) and is not on face 3). The reviewer's decisive check is right: 16.75 and 13.80 are not achievable between two midpoints of a 10mm box, which is why they could not reproduce. B2. "rounded nothing and reported success" is false. Measured: filleted(edges: [], radius: 2) returns nil -- it reported FAILURE. The defect is still severe but the harm is a misattributed error, not a silent wrong answer: a nil from a fillet reads as "the fillet failed" and nothing pointed at the selection. Corrected in CHANGELOG and SEMVER. B3. A third vacuous assertion, and the class was not exhausted after my own two. The commonEdges array-position check compared against a hardcoded Set(0..<4) while the finder returns 8 entries, so it could never fire. Now Set(0..<common.count); proven by injecting site 6 with the sibling cross-check neutralised. B4. Removed two indices.max()! force-unwraps inside #expect (CLAUDE.md bans this; Swift Testing does not short-circuit, so an empty mesh crashed the process instead of failing). WIRE/SHELL: my "no fixture has one" limitation was a fixture-finding failure, not a real gap. Six constructions give 26 WIRE and 4 SHELL two-orientation pairs, BRepCheck identical in every one, invalid geometry included. Also corrected "cannot move their answer at all" -- the domain does move (12 occurrences to 6 distinct), which SEMVER already recorded. Completeness claim softened and #651 (nbEdges/nbVertices/nbFaces) cited; the unmeasured fixEdgeSameParameter/fixEdgeVertexTolerance pair recorded as a candidate, not converted blind. Nits: test counts are 16+9 not 17+8; removed the dead ShapeType guard from both concavity loops (it would have desynced the result array if it ever fired, since count does not advance); dropped Wire.rectangle force-unwraps; corrected splitBoxCompound's doc. Closes #613
|
All four blockers fixed in B1 — corrected, and how it happenedRe-measured on both box spellings (identical answers):
Face 3's edges are at (0,10,5), (10,10,5), (5,10,0), (5,10,10), all on the y=10 face; Root cause — two compounding errors, both from transcribing an exploratory probe instead of So your impossibility check is the right one: those distances are between two arbitrary sample points B2 — correctedMeasured on base: B3 — fixed, and the class was indeed not exhausted
That is three vacuous assertions across this PR (two caught by my own injections, this one not). B4 — fixedBoth WIRE/SHELL — you were right, and it is stronger than you statedMy "no fixture has one" was a fixture-finding failure. I built your construction plus five more and
Invalid geometry included. And your correction stands: "cannot move their answer at all" was #651 and completenessCompleteness claim softened in both Nits
Verification
|
Closes #613
The defect
#541 put sub-shape indexing on one enumeration:
TopExp::MapShapes, one entry per distinctsub-shape (
TopoDS_Shape::IsSame, orientation ignored). Seven entry points stayed on a bareTopExp_Explorer, which yields one entry per occurrence. Measured on the pinned kernel, a plain10 mm box:
Every edge is reached once per adjacent face, so these disagree on any ordinary solid, not only on
an exotic one.
The end-to-end failure is the composition this branch's own documentation recommends.
Shape.filleted(edges:radius:)carries the snippetbracket.filleted(edges: bracket.concaveEdges(), radius: 3). Measured on an L-bracket (two fusedboxes, inner corner at x = 10, z = 10, map edge 27):
bracket.concaveEdges()[][27]edgeConcavityCount(.concave)box.edgeConcavityCount(.convex)on a 12-edge boxSo the recommended one-liner filleted an empty list. Measured on base,
filleted(edges: [], radius: 2)returns
nil— it reported failure, not success. The harm is a misattributed error ratherthan a silent wrong answer: a
nilfrom a fillet reads as "the fillet failed", and nothing pointed atthe edge selection as the cause. On the fix it returns 28034.3 mm³ against the bracket's 28000.0.
Per-site verdicts — re-derived, and one overturned
The audit comment on #613 was re-verified rather than taken on trust. Sites 1-5 confirmed;
site 6 overturned; one site added that neither the issue nor the audit names.
OCCTShapeAnalyzeEdgeConcavityBRepOffset_Analyse::Typeidentical for both orientations of all 12 box edges, 0 differingOCCTShapeCountEdgeConcavity(sibling, unlisted)OCCTBRepExtremaExtCCedges()to index 8, named a different edge from 9 (closest point 14.14 mm and 10.0 mm away); answered for 12…23checkSubShape(Healing)checkEdge(at: 12)reported valid althoughedge(at: 12)is nil, and kept answering to 23;checkVertexto 47 on an 8-vertex boxOCCTLocOpeSplitShapeByVertexedges()[4], index 11 splitedges()[0]; 12 and 13 split successfullyOCCTShapeCreateMesh/…WithParamsedgesInFacefaceIndexargument, false of theEdge.indexhanded back — the issue's actual complaint.edgesInFace(at: 3)returned 0, 1, 2, 3 for edges at 2, 6, 10, 11 — all four naming a different edge, 10.00, 12.25, 7.07 and 12.25 mm awaycommonEdges(with:)(sibling, unlisted)OCCTPolyMergeNodes(sibling)OCCTBiTgteBlend/OCCTBiTgteBlendInfo_std::vectorfilled from an explorer, subscripted by the caller's indices. No index blended the bracket's concave edge at all. Also silently dropped an unresolvable index (a #568 violation)Sites 1-4 are safe on the map — measured, not argued
The audit reasons from the headers (
BRepOffset_Analyse::myMapEdgeType,LocOpe_SplitShape::myMap/myDblE, andBiTgte_Blend::myEdgesfor site 7, allTopTools_ShapeMapHasher-keyed, and that hasher's equality operator isIsSame,TopTools_ShapeMapHasher.hxx:35-38). That is the reason to check, not the check. A ground-truth C++program handed every consumer both orientations of every box edge (12 pairs) and vertex
(8 pairs):
A plain box has no WIRE or SHELL occurring twice, so six further fixtures were built to reach them —
compound{solid, solid.Reversed()}and the same for a shell, a face, a wire, an open (invalid,non-closed) wire, and a fused two-body solid:
compound{solid, solid.Reversed()}compound{shell, shell.Reversed()}compound{face, face.Reversed()}compound{wire, wire.Reversed()}compound{openWire, …Reversed()}compound{fused, fused.Reversed()}Note this does not make the conversion a no-op for them: their index domain moves, and should —
on
compound{solid, solid.Reversed()}the WIRE enumeration goes 12 occurrences → 6 distinct, whichSEMVER.mdrecords. What is unchanged is the answer for a wire or shell both enumerations name.The winding evidence (site 5 and the
OCCTPolyMergeNodessibling)Meshing is the one site where the two enumerations pull opposite ways. Triangle winding is set by
the face's orientation as it occurs in the parent (
if REVERSED swap(n2, n3)), and that sets thesign of every emitted triangle normal. On a
BRepAlgoAPI_Splittercut of a 20×10×10 block at x = 10:Shared wall at x = 10, triangles by winding:
+x−xThe upper solid's floor simply absent — #614's defect one level down. So neither enumeration alone
is correct, and the fix uses both: winding from the occurrence, index from the map, via a new
occtForEachOrientedFace.FindIndexis theIsSamelookup, so aREVERSEDoccurrence resolves toits
FORWARDtwin's index, and both sides of the shared wall carry the one index (5) that names it —no second traversal.
OCCTPolyMergeNodescarries the same hazard and is deliberately not converted. It emits noindex; the
reversedflag it derives per occurrence goes straight toPoly_MergeNodesTool::AddTriangulation. Converting it measures+x2 /−x0 — the upper solidloses its wall. It is now commented as a do-not-convert site and pinned by a test.
Inject-the-bug results, per site
Each site was reverted alone (file copied aside and restored by copy — never
git checkout --),rebuilt, and the suite re-run.
concave.map(\.index) → []vs[27]; counter 2 vs classifier 0 concave, 42 vs 19 convex, 12 vs 9 tangent;edgeConcavityCount(.convex) → 24vs 12offBy → 14.142and→ 10.0mm; surplus indices 12/17/23 answered with real extremacheckSubShape→ explorercheckEdge(at:)valid for 12/17/23;checkVertex(at:)valid for 8/20/47splitEdge→ explorer7.071,14.142,12.247mm from the target; 12/17/23 succeededplusX > 0 && minusX > 0→true && false;plusX → 2vsminusX → 0face(at: 11) → nil; triangles10.0mm off the plane of the face they name; wall stamped with 2 indicesEdge.index→ array positiondist(a, b) → 10.0/12.247/7.071mm for both finders; the "not array positions" test could find no non-trivial index setOCCTPolyMergeNodes→ mapminusX → 0;plusX → 2vsminusX → 0biTgteBlend→ explorer vectorchanged → []vs[target] → [27]— no index reached the concave edge; surplus and mixed batches acceptedRestored after each, and the full set re-verified green.
Three vacuous assertions were found in this PR — two by my own injections, the third only in
review. All fixed. The lesson taken: an inject-the-bug pass proves a test can fail, but only if
the injection reaches that specific assertion; a test with several assertions can hide a dead one
behind a live sibling, which is the shape of all three.
The first two:
pair.0.index == n, which is true by construction — Swift builds each pair fromedges()[n]whatever the bridge returns. It passed with the defect present. Replaced with a falsifiable
cross-check: the classifier and the per-type counter are two independent bridge computations over
the same enumeration and must agree (2 vs 0 concave under injection).
concaveEdges(), so they failedunder injection 1 and could not attribute a failure. They now locate the inner corner
geometrically.
The third, found in review: the
commonEdgesarray-position check compared against a hardcodedSet(0..<4)while the finder returns 8 entries, so it could never fire. NowSet(0..<common.count), proven by injecting site 6 with the sibling cross-check neutralised:(Set(common.map(\.index)) → [0…7]) != (Set(0..<common.count) → [0…7]).One test is labelled a control rather than a detector, because the injections showed it does not
fail under either mesh injection: "every mesh triangle is wound outward for the solid that owns it".
Losing a facet removes a triangle rather than inverting one, and the copy that survives on this
fixture is stored
FORWARD, outward for its own owner. Said plainly in its doc comment.Tests
Tests/OCCTTopologyTests/Issue613IndexContractTests.swift(16) andTests/OCCTMeshTests/Issue613MeshIndexContractTests.swift(9) — 25 tests. Every fixture is ashape whose explorer count exceeds its map count, and every identity assertion cross-checks against a
geometric lookup that knows nothing about either indexing scheme, so it cannot pass by agreeing
with itself.
swift buildclean;swift testgreen — 5298 tests in 1396 suites.count-operations.py4,304, both documents ✓ (unchanged — no new public entry point).check-bridge-index.py,check-null-handle-guards.py,check-docs-defaults.pyall exit 0.Docs
docs/CHANGELOG.md— Unreleased entry with the measurements (no invented version).docs/SEMVER.md— a recorded exception was needed and is added. Eight silent behaviourchanges, none a compile error, each with what a caller does. Notably: the edge- and vertex-indexed
entry points move on ordinary solids, not only on shapes that share a sub-shape, because every
edge of every solid is shared between two faces. The running count in the intro is updated from
five exceptions to six.
docs/reference/Shape-Measurement.md,Shape-HLR-Geom.md,Mesh.md, plus///snippets on everychanged API.
Shape-HLR-Geom.mddocumentedbiTgteBlend's indices as "as returned by the shape'stopology explorer" — the stale contract, stated outright — now corrected.
Not a complete sweep
Shape.nbEdges/nbVertices/nbFacesreturn per-occurrence counts (box 24 and 48 againstedgeCount12 /vertexCount8; split compoundnbFaces12 againstfaceCount11) while theirown docs assert the deduplicated answers — filed separately as #651 and unchanged here.
OCCTShapeFixEdgeSameParameter/FixEdgeVertexTolerancecount explorer occurrences behind a"number of edges fixed" contract; source-visible but unmeasured (a box returns 0 either way), so
recorded as a candidate rather than converted on a pattern match.
Scope notes
Bridge-only plus two Swift wrappers. No kernel patch, no
OCCT.xcframeworkrebuild.OCCTLocOpeFindEdgesandOCCTLocOpeFindEdgesInFacegained an optionaloutIndicesparameter(
OCCTBridgeis not an SPM product, so this is not consumer-visible Swift API).macOS CI is red branch-wide (#585) and is not affected by this change; the gate is base-vs-PR failure
parity, and this PR has zero test failures.
🤖 Generated with Claude Code