Frame the camera on every body, not just the first (#19) - #11
Merged
Conversation
Closes the camera half of the OCCTSwift#302 multibody ripple, tracked as ecosystem#18. Before OCCTSwift v1.11.3 a multibody STL/BREP came back as one lumped shape, so "the first shape" genuinely was the whole model. Since v1.11.3 the importers return one body per solid, and the framing code kept asking for the first one. Two paths were affected, and only one of them was reported: - `loadFile(from:progress:)` (deprecated) sets `legacyLoadedShape` to `result.shapes.first`, so `focusOnLoadedShape()` zoomed to body 0. This is what OCCTSwiftCADKit#19 and OCCTSwiftUX#12 describe. - `focus(on:)` framed via `shape(id:)`, which returns only an entity's FIRST body. The multi-entity `loadFile(from:id:progress:)` registers a multibody file as ONE entity owning N bodies, and its own documentation tells callers to "call focus(on:) once you've loaded what should be visible". So the documented way to frame a multibody import was the broken one. The issues did not mention this path. The same first-body-only assumption was fixed once before in `applySideBySide`, whose regression test records it as "fine for the roughly-frame-the-camera use in focus(on:)". It was not fine, and that comment is why this stayed open: the earlier fix stopped at the boundary of the reported symptom. Both paths now go through `combinedBounds(ofEntities:)`, which unions over `Entity.bodyIDs` rather than `shape(id:)`. That also collapses the two copies of the centre/max-dimension arithmetic that had drifted into `focus(on:)` and `focusOnLoadedShape()` into one `frameCamera(on:)`. `focusOnLoadedShape()` keeps a `currentSingleShape` fallback for any path that sets `legacyLoadedShape` without installing identity. Nothing does that today; the fallback is there so it stays a non-event rather than an unframed camera. Tested by asserting the union directly rather than the camera: `focus(on:)` animates over 0.3s, so `cameraState` read straight afterwards is mid interpolation and cannot say what was framed. Verified discriminating by reverting the union to `bodyIDs.prefix(1)`, which fails it at 1.0 against the expected 21.0. The single-body companion test compares with a tolerance rather than for exact equality: `load` tessellates the shape, and the bounding box read afterwards is computed over the triangulation, so it lands within a rounding step of the pre-tessellation one rather than on it. 362 tests pass; swift-format --strict and swiftlint --strict are clean.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Reviewed by nemotron-3-ultra-550b-a55b:free · Input: 175.1K · Output: 3.1K · Cached: 125.3K |
gsdali
added a commit
that referenced
this pull request
Aug 20, 2026
… the dedup (#14) * Remove the deprecated API ahead of 2.0.0 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. * Split CADViewportService into per-domain files 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. * docs: write the durable identity cookbook the code already pointed at `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. * Verify the pick-resolver deduplication by execution, not by reading #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.
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.
Closes the camera half of the OCCTSwift#302 multibody ripple (ecosystem#18), filed here as OCCTSwiftCADKit#19 and in the sibling service as OCCTSwiftUX#12.
Before OCCTSwift v1.11.3 a multibody STL/BREP came back as one lumped shape, so "the first shape" genuinely was the whole model and framing on it read correctly. Since v1.11.3 the importers return one body per solid, and the framing code kept asking for the first one.
The reported path, and the one that was not reported
loadFile(from:progress:)(deprecated)focusOnLoadedShape()offlegacyLoadedShape = result.shapes.firstfocus(on:)shape(id:), which returns only an entity's first bodyThe second one is the worse of the two. The multi-entity
loadFile(from:id:progress:)registers a multibody file as one entity owning N bodies, and its own doc comment tells callers:So the documented way to frame a multibody import was the broken one, and the issues do not mention it.
Why it survived a previous fix
This exact assumption was found and fixed once before, in
applySideBySide. Its regression test says so explicitly:It was not fine for framing either. That earlier pass stopped at the boundary of the symptom in front of it, and left a comment asserting the rest was safe. Worth remembering the next time a shared accessor turns out to be wrong for one caller.
The change
Both paths now go through
combinedBounds(ofEntities:), which unions overEntity.bodyIDsinstead ofshape(id:). That also collapses two drifted copies of the same centre and max-dimension arithmetic into oneframeCamera(on:).focusOnLoadedShape()keeps acurrentSingleShapefallback for any path that setslegacyLoadedShapewithout installing identity. Nothing does that today; it is there so that stays a non-event rather than a silently unframed camera.Testing
Asserts the union directly rather than the camera, because
focus(on:)animates toward the framing over 0.3s andcameraStateread straight afterwards is mid-interpolation, so it cannot answer what was framed.Verified discriminating, not merely passing: reverting the union to
bodyIDs.prefix(1)fails it atbox.max.x → 1.0000001against the expected21.0000001.The single-body companion test compares with a tolerance rather than for exact equality, which is a real finding rather than a loosened assertion:
loadtessellates the shape, and the bounding box read afterwards is computed over the triangulation, landing within a rounding step of the pre-tessellation box (-2.0000001000000003against-2.0000001) rather than on it.362 tests pass.
swift-format lint --strictandswiftlint --strictare clean.Not in this PR
OCCTSwiftUX#12 is a separate implementation in
OCCTSwiftUXViewportService, not a consumer of this one, so it needs the same fix applied there. That duplication is itself in scope for the ecosystem#25 audit, and is the argument for that service eventually delegating here rather than carrying a parallel copy.