fix(#943): make Shape.bounds Optional for v3.0.0 breaking change - #944
Conversation
- OCCTShapeGetBounds: add explicit IsVoid() check, return nil for void shapes - Shape.bounds: return Optional tuple (min, max)? instead of non-optional - Shape.size, Shape.center: return SIMD3<Double>? derived from bounds - Wire.bounds, Edge.bounds, Face.bounds: return Optionals for consistency - Update all test files to handle optional bounds/size/center - docs/SEMVER.md: add v3.0.0 break table entry for this change All 5614 tests pass, all 6 gate scripts clean, clang-format compliant.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| /// `(0,0,0)-(0,0,0)` for void shapes, this now correctly signals "no geometry". | ||
| /// Use ``boundingBox`` for the pre-v3.0.0 Optional-returning variant with the same semantics. | ||
| public var bounds: (min: SIMD3<Double>, max: SIMD3<Double>)? { | ||
| var minX: Double = 0, minY: Double = 0, minZ: Double = 0 |
There was a problem hiding this comment.
CRITICAL: Heuristic-based void detection incorrectly treats legitimate zero-size shapes at origin as void
The check min == .zero && max == .zero ? nil : (min: min, max: max) cannot distinguish between:
- A void/empty shape (should return
nil) - A legitimate point-vertex or degenerate shape at the origin (should return
(0,0,0)-(0,0,0))
This is a known issue (#900). The test pointVertexAtOriginBoundingBoxIsNotNil verifies boundingBox and boundingBoxOptimal return non-nil for a point-vertex at origin, but bounds will now incorrectly return nil for the same shape.
The boundingBox property (in Shape+Analysis.swift) correctly handles this by using OCCTShapeBoundingBox which returns a boolean indicating void vs. valid zero-size.
Fix options:
- Change
OCCTShapeGetBoundsto returnbool(likeOCCTShapeBoundingBox) - Have
boundsdelegate toboundingBoxinternally - Add a separate
OCCTShapeIsVoidbridge function
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| var maxX: Double = 0, maxY: Double = 0, maxZ: Double = 0 | ||
| OCCTEdgeGetBounds(handle, &minX, &minY, &minZ, &maxX, &maxY, &maxZ) | ||
| return (min: SIMD3(minX, minY, minZ), max: SIMD3(maxX, maxY, maxZ)) | ||
| let min = SIMD3(minX, minY, minZ) |
There was a problem hiding this comment.
CRITICAL: Same heuristic void detection bug as Shape.bounds
The check min == .zero && max == .zero ? nil : (min: min, max: max) incorrectly returns nil for a legitimate zero-length edge at the origin.
OCCTEdgeGetBounds doesn't check IsVoid() and doesn't return a boolean for void detection.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| ) -> (SIMD3<Double>, SIMD3<Double>) { | ||
| unwrapAxisComponents { fn(handle, $0, $1, $2, $3, $4, $5) } | ||
| ) -> (min: SIMD3<Double>, max: SIMD3<Double>)? { | ||
| let (min, max) = unwrapAxisComponents { fn(handle, $0, $1, $2, $3, $4, $5) } |
There was a problem hiding this comment.
CRITICAL: Same heuristic void detection bug in boundsVia
The check min == .zero && max == .zero ? nil : (min: min, max: max) incorrectly returns nil for a legitimate face with zero bounds at origin.
OCCTFaceGetBounds and OCCTFaceGetBoundsExact don't check IsVoid() and don't return a boolean for void detection. This affects both Face.bounds and Face.exactBounds which both use boundsVia.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| // tessellation state, so its bounds must not move depending on whether the caller | ||
| // happened to call mesh() first. | ||
| bounds: face.exactBounds | ||
| bounds: face.exactBounds! |
There was a problem hiding this comment.
CRITICAL: Force unwrap of exactBounds can crash
face.exactBounds! will crash if exactBounds returns nil (which happens for void faces AND for legitimate zero-size faces at origin due to the heuristic bug).
Since exactBounds is now Optional, this should be safely unwrapped:
guard let bounds = face.exactBounds else { continue }
// use boundsReply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (28 files)
Detailed FindingsPreviously CRITICAL issues now FIXED:
Remaining WARNING issues:
Positive Notes
Fix these issues in Kilo Cloud Previous Review Summary (commit 6c4377c)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 6c4377c)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Files Reviewed (34 files)
Detailed FindingsCRITICAL: Heuristic void detection is fundamentally flawed The check
This is a known issue (#900). The existing test The Fix options:
CRITICAL: Force unwrap in FeatureRecognition.swift
WARNING: Documentation gaps
Reviewed by nemotron-3-ultra-550b-a55b:free · Input: 900.1K · Output: 32.8K · Cached: 14.4M |
The first cut of this PR made bounds Optional but decided "void" in Swift by comparing the six returned doubles against zero. That is the same fabrication the issue exists to remove, moved rather than fixed: it cannot tell a void shape from a shape whose box genuinely measures zero. Four bridge entry points now return bool, matching OCCTShapeBoundingBox's established convention, and all six bounds entry points share one helper (occtComputeBoundingBox, moved to OCCTBridge_Internal.h) which is the single place that reads Bnd_Box::IsVoid(). OCCTShapeGetBounds lives in a different .mm and could not reach the old file-static, which is how it ended up as the one bounds entry point with no guard at all (#834). AAG no longer force-unwraps Face.exactBounds: buildGraph() drops a face with no box before anything derives from the face list, so nodes, faceOccurrences and adjacencyList stay index-aligned by construction. Tests: Issue943BoundsVoidTests pins void, point-vertex-at-origin, zero-length edge and face/AAG. Injection matrix in the PR body. Style: the seven Swift files and two bridge files this PR touches are brought swift-format/clang-format clean and removed from the exemption manifests (#876). Shape+Modeling.swift is left alone: its doc-comment violations are #942's, not this PR's. Closes #943
…nto fix/issue-943-bounds-optional # Conflicts: # Sources/OCCTSwift/Shape+Modeling.swift
swift-format's BeginDocumentationCommentWithOneLineSummary wants the summary alone in the first paragraph, so splitting one mid-line leaves the remainder wrapped at the old column. Rewrapped to 100. Comment-only. The first attempt at this collapsed a ```swift block in Wire.swift into prose, because it looked for a fence inside the paragraph rather than tracking fences across the whole doc comment; reverted and redone with the tracking, and the diff now contains no line that is code. Also adds the #943 repro README next to the probe.
2 participants
What & why
Breaking change for v3.0.0.
Shape.bounds(andsize,center,Wire.bounds,Edge.bounds,Face.bounds,Face.exactBounds) fabricated(0,0,0)-(0,0,0)for a shape with no bounding box,indistinguishable from a genuine zero-size shape at the origin.
Shape.boundingBoxalready answerednilfor the same input, so the two disagreed on exactly this case (#834 documented the divergenceand deliberately did not fix it).
All of those accessors are now Optional, and the void-versus-measured verdict comes from OCCT,
not from inspecting the values.
The design, and the alternative rejected
The first cut of this PR decided "void" in Swift:
That is the same fabrication moved rather than removed: a value a real measurement can produce is
being read as a failure signal. It is the failure class #609/#726/#900/#605 are all instances of, so
it is rejected outright rather than traded off.
What shipped instead: the four bridge entry points that had no way to report a void box now
return
bool, matchingOCCTShapeBoundingBox's established convention, and all six bounds entrypoints share one helper,
occtComputeBoundingBox, moved fromOCCTBridge_Topology.mm(where itwas file-static) into
OCCTBridge_Internal.h. That helper is now the single place that readsBnd_Box::IsVoid().OCCTShapeGetBoundslives inOCCTBridge_Properties.mmand could not reach afile-static in another translation unit, which is exactly how it ended up as the one bounds entry
point with no guard at all.
OCCTShapeGetBoundsBRepBndLib::Add(s, b, useTriangulation=true)Shape.bounds(andsize,center,Wire.bounds)void, zeroes on voidboolOCCTFaceGetBoundsBRepBndLib::Add(f, b, useTriangulation=true)Face.boundsvoid, noIsVoid()at allboolOCCTFaceGetBoundsExactBRepBndLib::Add(f, b, useTriangulation=false)Face.exactBoundsvoid, noIsVoid()at allboolOCCTEdgeGetBoundsBRepBndLib::Add(e, b, useTriangulation=true)Edge.boundsvoid, noIsVoid()at allboolOCCTShapeBoundingBoxBRepBndLib::Add(s, b, useTriangulation=true)Shape.boundingBoxbool(#900)bool, now via the shared helperOCCTShapeBoundingBoxOptimalBRepBndLib::AddOptimal(s, b, true, useShapeTolerance)Shape.boundingBoxOptimal(useShapeTolerance:)bool(#900)bool, now via the shared helperTwo alternatives the review also offered were rejected. Having
boundsdelegate toboundingBoxfixes one of the four accessors and leaves
Edge/Face(which have noboundingBoxsibling) on thesentinel, and it would orphan
OCCTShapeGetBounds(#506: an orphaned bridge function freezes itscontract). A separate
OCCTShapeIsVoidwould compute the box twice and put the verdict and themeasurement in two calls that can disagree.
On the Swift side,
unwrapAxisComponentsIfSuccessful(_:)joins the existingunwrapVectorComponentsIfSuccessful(_:)inSIMD3Unpacking.swift, so the six-out-param-plus-boolunwrap is written once for all six accessors rather than six times.
sizeandcenterBoth become
SIMD3<Double>?, derived frombounds. #943 left this open, so, stated as a decision:they are measurements of the same box, they have no value that means "no box", and leaving them
non-optional would keep exactly the fabrication this PR removes in the two properties most likely to
be read without thinking.
.zerofrom either is now a measurement of a zero-size shape at theorigin, and that is what the doc comments say. They are the only
bounds-derived public API;AAGNode.boundsis a stored property, andDrawing.bounds()/Interval.bounds/LawFunction.boundsare unrelated members that this PR does not touch.
What the measurement says, including where the review was wrong
Scripts/repro/943-bounds-void-vs-zero/(probe + transcript) measures the collision directly ratherthan assuming it:
BRepBndLib::Addmeasures(-1e-7 … +1e-7), notall-zero:
BRep_Tool::Tolerancefloors every vertex/edge/face tolerance atPrecision::Confusion()andAddalways enlarges by it. So the exact case the review named,"a point-vertex at the origin now returns nil", is not reachable through
boundstoday, andthe sentinel would not have misfired on it.
BRepBndLib::AddOptimalmeasures exactly(0,0,0)-(0,0,0)withgap=0, which is byte-identical to what every failure path writes. That is boundingBox/boundingBoxOptimal misreport a degenerate/point shape at the origin as unmeasurable (nil) #900's live repro,re-measured here.
So the sentinel's safety on the
Addpath rests entirely on a kernel constant this package neithercontrols nor pins, while one flag away in the same family it is demonstrably wrong. With all six
entry points now sharing one decision point, a value sentinel would be wrong for all six at once.
That is the argument for the
bool, and it is stronger than the one the review gave.Tests, and the injection matrix
New:
Tests/OCCTAnalysisTests/Issue943BoundsVoidTests.swift(4 tests). The pre-existingvoidShapeBoundingBoxIsNilButBoundsFabricatesZerois renamedvoidShapeReportsNoBoxFromAnyAccessor,since its old name now describes behaviour that no longer exists.
Every new test was run with its subject broken (
okf/policies/prove-the-test-fails.md), oneinjection at a time, rebuilt each time:
if (box.IsVoid()) return false;Bnd_Box::Get()catch (...)returns false anyway. Reported, not hidden: the guard is not what carries the void verdict on its own.if (box.IsVoid()) return true;(the pre-#943 contract verbatim)voidShapeHasNoBoundsSizeOrCenter(5 issues) +voidShapeReportsNoBoxFromAnyAccessor(4)pointVertexAtOriginReportsAMeasuredBox+ #900's ownpointVertexAtOriginBoundingBoxIsNotNilEdge.boundsAddpath onlyAddnever returns exact zerosOCCTFaceGetBoundsExactreports no boxfaceBoundsAreMeasuredAndAAGKeepsEveryFaceonly (both halves:exactBoundsand the AAG node count, 0 vs 6)OCCTEdgeGetBoundsreports no boxzeroLengthEdgeAtOriginReportsAMeasuredBoxonlyD and E each fail exactly one test and no other, so they isolate rather than coincide. C failing
nothing is the measured result, not an oversight, and it is why B is the injection that matters.
Fixtures:
Shape.compound([])cannot be built at all (the bridge requires at least one member), sothe void fixture is a far-disjoint intersection, matching
BRepBndLibTests' own. The zero-lengthedge is a sphere's polar degenerate edge, centred so that pole sits at the origin.
The force unwrap, and the AAG contract
FeatureRecognition.swift'sbounds: face.exactBounds!would crash onnil.AAGNode.boundsis astored, non-optional property and node indices must stay aligned with
faceOccurrencesandadjacencyList, sobuildGraph()now filters faces with no box before anything derives from theface list, keeping all three index-for-index aligned by construction. No explorer-derived face in
this tree's fixtures is in that state, so this is the contract rather than a live case.
One pre-existing latent mismatch fixed in passing:
detectHoles()re-derivedshape.orientedFaces()and indexed it by node index, which is only aligned when nothing wasfiltered. It now reads the
faceOccurrencescachenodeswas actually built from, which is what#753 added that cache for.
Style manifests (#876)
Nine files brought clean and removed from the exemption manifests, since this PR touches them:
DrawingAutoDimensions.swift,Edge.swift,FeatureRecognition.swift,Shape+Topology.swift,Shape.swift,ThreadFeatures.swift,Wire.swift.OCCTBridge_Properties.mm,OCCTBridge_Properties.h.Most of it was mechanical (indentation, line length, one-variable-per-line, semicolons, trailing
periods on summary lines). Real content had to be written for 33 missing
- Returns:/- Throws:sections (21 of them inShape.swiftalone, mostly the primitivefactories and the boolean ops), 6 singular
- Parameterblocks that needed expanding to cover everyparameter, 2
parameter lists that had drifted from their signatures (
Wire.line,Shape+Topology.repeated), and7 over-long trailing comments moved above the code they annotate.
Shape+Modeling.swiftis deliberately not part of that: its doc-comment violations are #942/#945's.The deconflict merge on this branch had dropped two of #945's fixes (a paragraph break in
splitWithFullHistoryand a period plus paragraph break inGlueMode.asBooleanGlue); this branchnow carries
origin/main's copy of that file verbatim, and those two were the whole of theswift-formatfailure.Verification
swift testfrom source (OCCTSWIFT_LOCAL=1, bridge built from source, never the prebuiltxcframework, local kernel checksum verified equal to
Package.swift's pin):5618 tests in 1461 suites, 0 failures, against a 5614/1460 baseline on
main. The delta isthis PR's one new suite of four tests; no other test's result moved.
code-stylejob reproduced locally, all six steps:swift-format lint --strictoverevery non-manifest file,
swiftlint --strict,clang-format --dry-run --Werrorover everynon-manifest bridge file,
check-style-manifest.py --self-test(6/6) and its real run againstorigin/main,comment-ratio-check.py --self-test(5/5).--self-test.Scripts/repro/943-bounds-void-vs-zero/.CHANGELOG entry
bounds,sizeandcenterare Optional, and "no bounding box" now comes from OCCT (#943)Shape.bounds,Shape.size,Shape.center,Wire.bounds,Edge.boundsandFace.boundsreturnnilfor a shape with no bounding box instead of fabricating(0,0,0)-(0,0,0), which wasindistinguishable from a genuine zero-size shape at the origin (#834 documented this and left it in
place).
Shape.boundingBoxalready behaved this way, so the two no longer disagree.The verdict comes from OCCT's own
Bnd_Box::IsVoid(), reported across the bridge as aBool:OCCTShapeGetBounds,OCCTFaceGetBounds,OCCTFaceGetBoundsExactandOCCTEdgeGetBoundsnowreturn
boollikeOCCTShapeBoundingBoxalready did, and all six share one helper. A Swift-sidecomparison of the returned coordinates against zero would be the same fabrication in a new place: a
vertex at the world origin measures exactly
(0,0,0)-(0,0,0)throughBRepBndLib::AddOptimal(measured,
Scripts/repro/943-bounds-void-vs-zero/).Migration: unwrap.
shape.bounds.maxbecomesshape.bounds?.max, orguard let b = shape.bounds.AAGdrops any face with no bounding box rather than force-unwrapping it.SemVer impact
MAJOR. Six public accessors change type, so a consumer taking this blindly will not compile:
Shape.bounds(min: SIMD3<Double>, max: SIMD3<Double>)(min: SIMD3<Double>, max: SIMD3<Double>)?Shape.sizeSIMD3<Double>SIMD3<Double>?Shape.centerSIMD3<Double>SIMD3<Double>?Wire.bounds(min: SIMD3<Double>, max: SIMD3<Double>)(min: SIMD3<Double>, max: SIMD3<Double>)?Edge.bounds(min: SIMD3<Double>, max: SIMD3<Double>)(min: SIMD3<Double>, max: SIMD3<Double>)?Face.bounds(min: SIMD3<Double>, max: SIMD3<Double>)(min: SIMD3<Double>, max: SIMD3<Double>)?Bridge C surface, for anyone linking
OCCTBridgedirectly:OCCTShapeGetBounds,OCCTFaceGetBounds,OCCTFaceGetBoundsExactandOCCTEdgeGetBoundsreturnboolinstead ofvoid. Source-compatible for callers that ignore the return value; the six out-params are unchangedand are still zeroed on every failure path.
Row for
docs/SEMVER.md's v3.0.0 break table, to be assembled at release (this PR does not edit thatfile, per
semver-at-release):Shape.bounds,Shape.size,Shape.center,Wire.bounds,Edge.boundsandFace.boundsbecome Optional (#943)(0,0,0)-(0,0,0), indistinguishable from a real zero-size shape at the origin. "No bounding box" is now OCCT's ownBnd_Box::IsVoid(), carried across the bridge as aBool, not inferred from the returned coordinates.shape.bounds?.max, orguard let b = shape.bounds else { … }.Shape.boundingBoxalready had this contract and is unchanged.Closes #943