Skip to content

One selection, held by InteractiveContext, and CADKit projects it - #6

Merged
gsdali merged 1 commit into
mainfrom
fix/3-cadkit-adopts-ais-selection
Aug 19, 2026
Merged

One selection, held by InteractiveContext, and CADKit projects it#6
gsdali merged 1 commit into
mainfrom
fix/3-cadkit-adopts-ais-selection

Conversation

@gsdali

@gsdali gsdali commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

What & why

CADViewportService held an InteractiveContext and ran a second selection alongside it, which its own source described as "a separate, independent selection system this service does not share state with". There is now one store: InteractiveContext.selection. CADViewportService.selection is that store enriched into PickedEntity values, and CADViewportService.selectionModes is interactiveContext.selectionMode rather than a second variable free to disagree with it. By default they did disagree, [.face] here and [.body] there, in a service that owns both. Phase 3 of ecosystem#43, implementing the bakeoff posted on the issue.

Closes #3

Checklist

  • New or changed behavior is covered by a unit test in the same PR (not just manual
    verification), see SecondMouseAU/OCCTReconstruct#397
    for the ecosystem-wide test-coverage standard this is piloting.

What the bakeoff decided

Three collisions, three different verdicts. Only one of them was a merge.

Collision Verdict
select / clearSelection / remove / removeAll Merge, one way. AIS owns the state, CADKit projects it. remove/removeAll keep both implementations: same name, different types, different arguments, and only their selection pruning was shared.
SelectionSummary (CADKit and OCCTSwiftUX) Not a duplicate. Zero shared fields, zero shared logic, zero shared consumers, and OCCTSwiftUX does not depend on OCCTSwiftCADKit at all. Resolved by naming, as phase 1 resolved SelectionFilter.
Axis (AIS and CADKit) Never a collision. AIS's is nested inside ManipulatorWidget, so it is never spelled Axis at file scope; the package already compiles with both. Three statements of shared arithmetic (the unit vector). Left alone.

Measuring the overlapping logic rather than the function length, per the phase 2 correction: the four method bodies plus documentation run to about 120 lines, but only 15 statements of CADKit's selection code were duplicated (8 for scheme combination, 4 for the mode set and its gates, 2 for pruning on removal, 1 for emptying). Quoting the function length would have overstated it eightfold. The duplication that actually mattered was not the statements, it was the two stores.

Applying the "check what a bounds test is testing against" correction found two guards that look alike and are not. selectionMeasurements re-derives Face(info.shape).bounds instead of reading info.bounds, which looks like a missed reuse and is not: FaceBounds is XY only and Float, the aggregate is 3D and Double. And resolveFacePick's guard let faceBounds = face.bounds exists only because PickedFaceInfo.bounds is non-optional, which ties it to the judgement call below: delete the type and that guard goes with it, and a pick on a face with a void bounding box starts resolving where it used to be discarded.

The judgement call: the picked-info types survive

PickedFaceInfo, PickedEdgeInfo and PickedVertexInfo stay, as presentation types, and each now stores a SubShapeRef with shape / uid / faceIndex forwarding to it. Four reasons, in order of weight:

  1. Two of their fields cannot be recovered from a SubShapeRef. scalarValue for a .perTriangle field is resolved from the picked triangle, which does not exist in a per-sub-shape ref; description is formatted prose. Delete the types and that information has no home.
  2. Phase 2 kept the enrichment in CADKit deliberately. These types are its output. Keeping the enrichment and deleting its return type means loose tuples, which is worse than the mapping layer it was meant to avoid.
  3. The consumer cost is real and the architectural gain is zero. PadCAM's entire consumption of CADKit selection is selectedFace and clearSelection(), reading three members. Deleting the types breaks four PadCAM files to remove a projection, after the state those files never touched has already moved.
  4. They stop being a second identity system, which was the actual objection. Each carried a hand-written == commented "Mirrors OCCTSwiftAIS.SubShapeRef.== exactly": three copies of one rule, free to drift. They now share one isSamePick.

No .body case is added to PickedEntity. Whole-body selection is now genuinely reachable, because selectionMode is shared, but it lives in interactiveContext.selection as SubShape.body, where AIS's whole-body fallback and body-level highlight already are. A fourth case would break every consumer switch to surface a concept CADKit expresses better through entities / visibility / focus(on:).

ComesFromDecomposition: still no, and this was the phase meant to change the answer

Phase 2 deferred it here on the grounds that it only becomes load-bearing if SubShape loses .body. Implementing the selection merge produced no site that wanted to ask "did this ref come from decomposition?": every site that cared asked "is this .body?" and got a compiler-checked answer from the sum type. Two points this phase adds:

  • Dropping .body for a flagged ref over the root shape would move the distinction from the compiler to the reader at every consumer switch, including CADKit's own three-way projection. OCCT only takes that trade because SelectMgr_EntityOwner is one C++ type serving both cases.
  • The flag would still be a constant. SubShapePickResolver is still the only way to mint a SubShapeRef, and it still cannot mint one for a whole body.

Closing it rather than deferring it a third time.

What breaks, for whoever migrates OCCTSwiftUX and PadCAM

Seven items, in docs/CHANGELOG-OCCTSwiftCADKit.md in full. Summary:

  1. SelectionSummary renamed to SelectionMeasurements, selectionSummary to selectionMeasurements. Both old spellings still resolve as deprecated aliases, so this is a warning today, not an error.
  2. selection is no longer in selection order. It is ordered by (body id, kind, ordinal): the store is a Set<SubShape>, so insertion order no longer exists. Code reading selection.last as "the latest pick" needs to change.
  3. Assigning selectionModes clears the selection, which is the interactive context's documented behaviour for selectionMode. It was plain storage before. Set the modes before selecting.
  4. selectionModes and interactiveContext.selectionMode are one setting, initialised to [.face], which overrides the context's [.body] default. An app that displays extra geometry into the context and relied on picks against it giving a whole-body selection now gets a face selection: set [.body] or [.face, .body] deliberately.
  5. The two selections are no longer independent. A pick on a model body replaces the whole shared selection; a pick on empty space clears all of it. A pick on a body the context displays itself is left for the context, rather than treated as unresolved and clearing a selection this service does not own.
  6. The picked-info types store a SubShapeRef. Every existing read compiles unchanged and the old memberwise initialisers are kept, so nothing breaks today.
  7. PickedFaceInfo.scalarValue can be nil where it was not, for a .perTriangle field only, and only for a face that reached the selection without a pick (through the context directly, or by area selection): there is no triangle to sample.

Concretely, per consumer:

  • PadCAM reads viewportService.selectedFace at ProjectDetailView.swift:216, :382 and :1405, calls clearSelection() at :217, and declares var selectedFace: PickedFaceInfo? in OperationDetailView.swift:15, OperationInlineDetailView.swift:15 and OperationListView.swift:15. It reads only bounds.{minX,maxX,minY,maxY}, zLevel and description. All of that keeps working unchanged. Its one real exposure is item 4: ProjectDetailView.swift:632 displays a stock box via interactiveContext.display(_:style:) and installs a ManipulatorWidget on it, so picks against that stock body now resolve under [.face] rather than [.body]. It never uses .selection, .selected, .selectionModes, .select(_:scheme:), .selectionSummary, PickedEntity, PickedEdgeInfo or PickedVertexInfo, so items 1, 2, 3, 5, 6 and 7 cost it nothing.
  • OCCTSwiftUX has no dependency on OCCTSwiftCADKit in either direction and no import OCCTSwiftCADKit anywhere. Nothing in this PR reaches it. Its own OCCTSwiftUXKit.SelectionSummary is untouched and stays where it is; the rename is on this side precisely so the two stop sharing a name.

Notes for the reviewer

Why CADKit's bodies are not registered as InteractiveContext entries. The obvious implementation is interactiveContext.display(_:) per model body, letting the context own everything. It is wrong twice: display owns tessellation, which CADKit does itself with its own transforms, caps and comparison state; and the context's updateSelectionVisuals writes triangleStyles, the same array setScalarField(_:forBody:) paints, so registering would silently destroy any scalar field. CADKit mints one InteractiveObject per body id instead (id cached, Shape re-read on each construction, which is safe because InteractiveObject compares and hashes by id alone) and drives the context's selection with those.

Why the $selection sink has no .receive(on: RunLoop.main), unlike the $bodies sink two lines above it. It has to run synchronously, so a caller reading selection right after select(_:scheme:) sees the result, which the existing tests require. The consequence is that it fires during willSet, when interactiveContext.selection still reports the previous value, so syncSelection(with:) takes the new selection as an argument rather than reading it back off the context.

Why select(_:scheme:) on InteractiveContext has no default value. Adding a defaulted scheme: to the existing select(_:) would have silently retuned all of AIS's existing call sites from add to replace. The one-argument form keeps its exact meaning and forwards to .add; the scheme form always says which scheme it means. There is a test for it.

One new public method on InteractiveContext: displaysBody(withID:). Without it, CADKit cannot tell a pick on an AIS-displayed object from a pick it simply failed to resolve, and would clear a selection it never owned on every pick against a manipulator target or a display(_:)ed shape. Also a real bug this PR would otherwise have introduced into PadCAM's stock-box workflow, which is why there is a test named for that exact scenario.

One small internal cleanup that fell out. AreaSelection.swift had its own copy of the four-case scheme switch; both it and select(_:scheme:) now call one internal applySelection(_:scheme:).

The clip-plane pre-filter stayed in CADKit, as phase 2 decided. Worth recording why it cannot become an AIS SelectionFilter: that protocol sees a resolved SubShape, while the clip test needs the picked primitive's world position. A large face straddling a clip plane must be pickable on its visible half and not on its hidden half, and a sub-shape-level filter cannot tell those apart. OCCTSwiftViewport.ViewportController.selectionFilter is the layer-correct long-term home, since it gates the pick stream before either selection system sees it, but it is a single slot a consumer may want for itself, so not here.

A new changelog, docs/CHANGELOG-OCCTSwiftCADKit.md. That target had none. This is the first change to it a consumer has to read before upgrading, so it now has somewhere to be written down.

Em-dashes cleared from the four current-state docs this touches, per the writing-style policy's "clear them from any file you are already editing" (244 of them). The two changelogs and the Tools implementation spec are historical records and are left alone, matching what phase 2 did.

Verification

$ swift build
Build complete! (2.29s)

$ swift build --build-tests
Build complete! (6.86s)

$ OCCT_SERIAL=1 swift test --parallel --num-workers 1
✔ Test run with 343 tests in 30 suites passed after 0.516 seconds.

330 in 28 suites to 343 in 30, all passing. 13 new: 3 in OCCTSwiftAISTests (the four schemes, select(_:) still meaning add, displaysBody(withID:)) and 10 in a new SharedSelectionTests suite (the shared mode set, both directions of the shared selection, on-demand enrichment, ordering, the AIS-body pick case, ref forwarding, cross-body identity, the deprecated alias). No test deleted or weakened; t_sharedFaceBetweenShells_everyPickResolvesToCorrectSubShape is untouched and still green.

swift-format lint --strict and swiftlint lint --strict: 0 violations. The only new build warnings are the 2 the deprecated-alias test deliberately triggers.

…cts it

CADViewportService held an InteractiveContext and ran a second selection
alongside it. Its own source said so: ".body has no effect here; it exists on
SelectionMode for OCCTSwiftAIS.InteractiveContext.selectionMode, a separate,
independent selection system this service does not share state with."

There is now one store. CADViewportService.selection is
interactiveContext.selection enriched into PickedEntity values, and
selectionModes IS interactiveContext.selectionMode rather than a second
variable free to disagree with it (by default they did: [.face] here, [.body]
there, in a service owning both).

InteractiveContext.select gained the four-scheme parameter from CADKit's
version, with select(_:) unchanged and still meaning .add, and a new
displaysBody(withID:) so a host sharing this selection can tell its own
composited bodies from the context's own objects.

Phase 3 of ecosystem#43, implementing the bakeoff posted on the issue. The
bakeoff also found two of the three listed collisions were not duplicates at
all: Axis is nested inside ManipulatorWidget so it never collided, and
OCCTSwiftUX's SelectionSummary shares no field, input or consumer with CADKit's
(and OCCTSwiftUX does not depend on CADKit). That one is resolved by naming, as
phase 1 resolved SelectionFilter: CADKit's becomes SelectionMeasurements, with
a deprecated alias.

PickedFaceInfo, PickedEdgeInfo and PickedVertexInfo survive as presentation
types, now storing a SubShapeRef and forwarding identity to it, which collapses
three hand-written copies of one equality rule into one.

Closes #3

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@kilo-code-bot

kilo-code-bot Bot commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Files Reviewed (17 files)
  • Sources/OCCTSwiftAIS/AreaSelection.swift
  • Sources/OCCTSwiftAIS/InteractiveContext.swift
  • Sources/OCCTSwiftCADKit/CADViewportService.swift
  • Sources/OCCTSwiftCADKit/PickedEntity.swift
  • Sources/OCCTSwiftCADKit/PickedFaceInfo.swift
  • Sources/OCCTSwiftCADKit/SelectionMeasurements.swift
  • Tests/OCCTSwiftAISTests/SelectionTests.swift
  • Tests/OCCTSwiftCADKitTests/SharedSelectionTests.swift
  • Tests/OCCTSwiftCADKitTests/SmokeTests.swift
  • docs/CHANGELOG-OCCTSwiftAIS.md
  • docs/CHANGELOG-OCCTSwiftCADKit.md
  • docs/guides/getting-started-OCCTSwiftCADKit.md
  • docs/index.md
  • docs/module-notes/OCCTSwiftCADKit.md
  • docs/reference/CADViewportService.md
  • docs/reference/InteractiveContext.md
  • okf/components/OCCTSwiftAIS.md
  • okf/components/OCCTSwiftCADKit.md

Reviewed by nemotron-3-ultra-550b-a55b:free · Input: 390.6K · Output: 18.2K · Cached: 816.5K

@gsdali
gsdali merged commit bbeadbe into main Aug 19, 2026
5 checks passed
@gsdali
gsdali deleted the fix/3-cadkit-adopts-ais-selection branch August 19, 2026 12:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

CADKit adopts AIS selection state instead of running a parallel one

1 participant