Prepare 2.0.0: remove the deprecated API, split the blob file, verify the dedup - #14
Merged
Conversation
Ten deprecated public declarations go, along with the machinery that existed only to back them. A major version is when removal is free; carrying them into 1.x would have owed them support for the whole line. Removed from OCCTSwiftCADKit: loadedShape -> loadedShapes / shape(id:) selected -> selection selectedFace -> selection selectionSummary -> selectionMeasurements SelectionSummary (typealias) -> SelectionMeasurements loadFile(from:progress:) -> loadFile(from:id:progress:) loadShape(_:id:) -> load(_:id:transform:) loadFromData(_:filename:progress:) -> loadFromData(_:filename:id:progress:) CADViewportView.init x2 -> the selection: initialiser The removals cascade further than the declarations themselves, which is the point. The three deprecated loaders were the only callers of focusOnLoadedShape(), and the only writers of legacyLoadedShape and legacyLoadedShapeEntityID. With them gone that state was written only as nil and read only by a branch that could never be taken, so currentSingleShape now has a single source of truth: a shape it reports is always an entity that `entities` actually lists. remove(id:) no longer has to invalidate a parallel copy by hand. Nothing auto-focuses now. The deprecated loaders were the only callers of focusOnLoadedShape, and the surviving multi-entity loaders have always documented that the camera is not auto-focused and that you call focus(on:). The multibody framing fix from #11 lives on in focus(on:) via combinedBounds(ofEntities:), which is where it mattered: that was the path the docs told callers to use. Test changes are not all mechanical, so they are worth reading: - Ten call sites moved from loadShape to load(_:id:), same behaviour at one body. - Three tests existed only to cover deprecated semantics and are deleted: the loadedShape single-entity case, the selected single-selection case, and the SelectionSummary alias resolving. - One asserted that a single-shape load wipes a prior multi-entity load's selection. That behaviour no longer exists, and the invariant that does survive (selection drops only entries referencing a removed entity) is already covered by its neighbour, so it is deleted rather than rewritten into a duplicate. - Two are rewritten rather than deleted, because their invariant outlived their vehicle and nothing else asserted it: removeAll() must leave no entity, body or selection behind, and remove(id:) must clear an entity from every collection rather than only from modelBodies. Both now drive the surviving API, and both assert a precondition first so the removal proves something. 358 tests pass. swift-format --strict and swiftlint --strict clean. CADViewportService 2,497 to 2,306 lines; the package 8,439 to 8,214. Not removed: OCCTSwiftAIS/CompatibilityAliases.swift. Those three typealiases are source-compatibility shims from the three-repo merge, deliberately never marked deprecated because a same-module typealias shadows the type it aliases and would warn at ~75 internal uses. Whether they survive 2.0.0 is a separate call.
2,306 lines in one file becomes a 365-line core plus seven extension files, none over 572. This is the code-structure policy applied to the repo's largest outstanding violation (OCCTSwiftInteraction#13), and it is much cheaper now than after 2.0.0 freezes the public surface. CADViewportService.swift 365 stored state, init, shape info CADViewportService+Selection.swift 572 CADViewportService+Clipping.swift 445 clipping, capping, clip-aware picking CADViewportService+Loading.swift 418 file import and multi-entity loading CADViewportService+Comparison.swift 270 CADViewportService+ScalarFields.swift 171 CADViewportService+Escalation.swift 155 CADViewportService+Overlays.swift 48 It is a move: the method bodies are byte-identical, split at the `// MARK: -` seams the file already had. Two things did have to change, and both are worth knowing about. **Access levels.** A Swift extension in another file cannot see `private`, so 31 of the type's 64 private members widened to `internal`; 33 stayed private. That widening is target-scoped, not module-wide: nothing outside OCCTSwiftCADKit can reach them. Stored state stays declared once, in the core file, because an extension cannot hold stored properties, which is why `pendingEscalation` and its continuation stayed behind while its behaviour moved. **Three properties needed `internal(set)`, not plain widening.** `selection`, `comparison` and `pendingEscalation` were `public private(set)`. Blanket-widening made them publicly settable, which would have handed consumers write access to state the service owns, in the release that freezes the API. `public internal(set)` keeps them read-only to consumers exactly as before while letting the split-out files write them. Verified rather than assumed: the public API of `CADViewportService` and `CADViewportView` is byte-identical to the pre-split commit, compared declaration by declaration. 358 tests pass, swift-format --strict and swiftlint --strict clean.
`FaceIdentityTable` closed with "See the durable identity cookbook (`topology-graph-uids.md`)". That page did not exist, and it was the only dangling doc reference in Sources. Writing it is also the answer to the comment-ratio item on #13, though not the answer I predicted. I had recorded the three identity tables as carrying "three near-identical explanations". Reading them, that is wrong and worth correcting: each carries its own genuine difference. Face has the IsSame decision and the raw-versus-deduplicated enumeration history; Edge and Vertex exist largely to say their ordinals were never ambiguous, because both were always built from one TopTools_IndexedMapOfShape, so there was no split to reconcile; Vertex explains why it holds [Shape] where its siblings hold [Shape?]. What was genuinely duplicated is the shared reasoning underneath: why the tables exist at all, what identity means, and the pre-v2.0.0 divergence a consumer may still carry assumptions about. That now lives in one page all three point at, which takes FaceIdentityTable from 4.38x comment:code to 2.69x. The page also records two things that were only in commit messages and issues: the index-space rule behind [Shape?] (a nil keeps its ordinal's slot, because compacting silently renames every later sub-shape, which was #9), and why GraphUID resolution was unaffected by the face-enumeration divergence (it goes through graph.findNode(for:), an identity lookup, never index correspondence). Also clears the six em-dashes from the cookbook index, per the writing-style policy's "clear them from any file you are already editing". The ratio check still lists these files, and should. They are doc-heavy because they encode a decision that is expensive to reconstruct, which is the difference between a comment that earns its place and one that restates the code.
#12's duplication audit was a static read of the call graph. That confirms OCCTSwiftAIS *calls* SubShapePickResolver, which is not the same as confirming the two agree: a wrapper can call a resolver and still hand back something different, by passing it different inputs. Item 4 of #13 asked for the difference, and this is it. Five tests. Three drive a real pick through InteractiveContext.handlePick and independently resolve the same pick by rebuilding the resolver's inputs the way display(_:style:) does, then compare the ordinal. Two cover what AIS legitimately adds on top: selection-mode gating must suppress a pick the resolver would still resolve, and with .body on and .face off a face pick must fall back to the whole body. That fallback stays in AIS deliberately, because "the pick names the object rather than one of its faces" is a selection decision rather than an identity one, and OCCT draws the same line at SelectMgr_EntityOwner::ComesFromDecomposition(). The first version of these tests did not earn its pass, and the second version exists because of it. Comparing a single pick on triangle 0, they all passed even with `triangleIndex + 1` injected into the AIS face wrapper: a box puts two triangles on every face, so an off-by-one in the triangle index lands on the same face and the comparison cannot see it. They now sweep every primitive, and the same injection fails the face test with six issues. Each sweep also asserts it compared more than one pick, so a future change that makes the resolver return nil everywhere degrades to a failure rather than to a vacuous pass over an empty loop. 363 tests. swift-format --strict and swiftlint --strict clean.
Contributor
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (19 files)
Reviewed by nemotron-3-ultra-550b-a55b:free · Input: 264.5K · Output: 21.1K · Cached: 410.4K |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Everything #13 asked for before a stable release, in four commits that are worth reading separately.
1. Remove the deprecated API (
072103e's parent)Ten deprecated public declarations, plus the machinery that existed only to back them. A major version is when removal is free.
loadedShapeloadedShapes/shape(id:)selected,selectedFaceselectionselectionSummary,SelectionSummaryselectionMeasurements,SelectionMeasurementsloadFile(from:progress:)loadFile(from:id:progress:)loadShape(_:id:)load(_:id:transform:)loadFromData(_:filename:progress:)loadFromData(_:filename:id:progress:)CADViewportView.initx2selection:initialiserThe cascade is the interesting part. Those three loaders were the only callers of
focusOnLoadedShape()and the only writers oflegacyLoadedShape. With them gone that state was written only asniland read only by an unreachable branch, socurrentSingleShapenow has one source of truth andremove(id:)no longer maintains a parallel copy by hand.Nothing auto-focuses now, and that is correct: the surviving multi-entity loaders have always documented that you call
focus(on:). The multibody framing fix from #11 lives on there, which is the path the docs point at.Three tests that existed only to cover deprecated semantics are deleted. Two are rewritten rather than deleted, because their invariant outlived their vehicle and nothing else asserted it:
removeAll()must leave no entity, body or selection behind, andremove(id:)must clear an entity from every collection rather than only frommodelBodies.2. Split
CADViewportService(072103e)2,306 lines in one file becomes a 365-line core plus seven extension files, none over 572, split at the
// MARK: -seams the file already had. Method bodies are byte-identical.Two things did change. 31 of the type's 64 private members widened to
internal(target-scoped; nothing outside OCCTSwiftCADKit can reach them), because a Swift extension in another file cannot seeprivate. And three properties neededinternal(set)rather than plain widening:selection,comparisonandpendingEscalationwerepublic private(set), and blanket-widening would have handed consumers write access to service-owned state in the release that freezes the API.Verified rather than asserted: the public API of
CADViewportServiceandCADViewportViewis byte-identical to the pre-split commit, compared declaration by declaration.3. Write the cookbook page the code already pointed at (
b0a92a8)FaceIdentityTableclosed with "See the durable identity cookbook (topology-graph-uids.md)". That page did not exist. It was the only dangling doc reference inSources.A correction to what #13 said, since it shaped the plan: I had recorded the three identity tables as carrying "three near-identical explanations". Reading them, that is wrong. Each carries its own genuine difference. What was duplicated is the shared reasoning underneath, and that now lives in one page all three point at, taking
FaceIdentityTablefrom 4.38x comment:code to 2.69x.The ratio check still lists these files, and should. They are doc-heavy because they encode a decision that is expensive to reconstruct.
4. Verify the deduplication by execution (
d7da8e9)#12's audit was a static read of the call graph. That confirms AIS calls
SubShapePickResolver; it does not confirm the two agree, because a wrapper can call a resolver and still return something different by passing different inputs.Three tests drive a real pick through
handlePickand independently resolve the same pick, then compare. Two cover what AIS legitimately adds: mode gating, and the whole-body fallback that stays in AIS because it is a selection decision rather than an identity one.The first version of these tests did not earn its pass. Comparing a single pick on triangle 0, they passed even with
triangleIndex + 1injected into the AIS face wrapper, because a box puts two triangles on every face. They now sweep every primitive, and the same injection fails the face test with six issues. Each sweep also asserts it compared more than one pick, so a resolver that returnednileverywhere would fail rather than pass vacuously.Result
363 tests in 33 suites.
swift-format lint --strictandswiftlint --strictclean.What this does not do
OCCTSwiftAIS/CompatibilityAliases.swiftstays. Those three typealiases are source-compatibility shims from the three-repo merge, deliberately never marked deprecated because a same-module typealias shadows the type it aliases and would warn at ~75 internal uses. Whether they survive 2.0.0 is a separate call.Ready for
2.0.0-rc1once merged.