One selection, held by InteractiveContext, and CADKit projects it - #6
Merged
Conversation
…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>
3 tasks
Contributor
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (17 files)
Reviewed by nemotron-3-ultra-550b-a55b:free · Input: 390.6K · Output: 18.2K · Cached: 816.5K |
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
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.
What & why
CADViewportServiceheld anInteractiveContextand 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.selectionis that store enriched intoPickedEntityvalues, andCADViewportService.selectionModesisinteractiveContext.selectionModerather 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
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.
select/clearSelection/remove/removeAllremove/removeAllkeep both implementations: same name, different types, different arguments, and only their selection pruning was shared.SelectionSummary(CADKit and OCCTSwiftUX)SelectionFilter.Axis(AIS and CADKit)ManipulatorWidget, so it is never spelledAxisat 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.
selectionMeasurementsre-derivesFace(info.shape).boundsinstead of readinginfo.bounds, which looks like a missed reuse and is not:FaceBoundsis XY only andFloat, the aggregate is 3D andDouble. AndresolveFacePick'sguard let faceBounds = face.boundsexists only becausePickedFaceInfo.boundsis 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,PickedEdgeInfoandPickedVertexInfostay, as presentation types, and each now stores aSubShapeRefwithshape/uid/faceIndexforwarding to it. Four reasons, in order of weight:SubShapeRef.scalarValuefor a.perTrianglefield is resolved from the picked triangle, which does not exist in a per-sub-shape ref;descriptionis formatted prose. Delete the types and that information has no home.selectedFaceandclearSelection(), reading three members. Deleting the types breaks four PadCAM files to remove a projection, after the state those files never touched has already moved.==commented "MirrorsOCCTSwiftAIS.SubShapeRef.==exactly": three copies of one rule, free to drift. They now share oneisSamePick.No
.bodycase is added toPickedEntity. Whole-body selection is now genuinely reachable, becauseselectionModeis shared, but it lives ininteractiveContext.selectionasSubShape.body, where AIS's whole-body fallback and body-level highlight already are. A fourth case would break every consumerswitchto surface a concept CADKit expresses better throughentities/visibility/focus(on:).ComesFromDecomposition: still no, and this was the phase meant to change the answerPhase 2 deferred it here on the grounds that it only becomes load-bearing if
SubShapeloses.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:.bodyfor a flagged ref over the root shape would move the distinction from the compiler to the reader at every consumerswitch, including CADKit's own three-way projection. OCCT only takes that trade becauseSelectMgr_EntityOwneris one C++ type serving both cases.SubShapePickResolveris still the only way to mint aSubShapeRef, 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.mdin full. Summary:SelectionSummaryrenamed toSelectionMeasurements,selectionSummarytoselectionMeasurements. Both old spellings still resolve as deprecated aliases, so this is a warning today, not an error.selectionis no longer in selection order. It is ordered by (body id, kind, ordinal): the store is aSet<SubShape>, so insertion order no longer exists. Code readingselection.lastas "the latest pick" needs to change.selectionModesclears the selection, which is the interactive context's documented behaviour forselectionMode. It was plain storage before. Set the modes before selecting.selectionModesandinteractiveContext.selectionModeare 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.SubShapeRef. Every existing read compiles unchanged and the old memberwise initialisers are kept, so nothing breaks today.PickedFaceInfo.scalarValuecan benilwhere it was not, for a.perTrianglefield 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:
viewportService.selectedFaceatProjectDetailView.swift:216,:382and:1405, callsclearSelection()at:217, and declaresvar selectedFace: PickedFaceInfo?inOperationDetailView.swift:15,OperationInlineDetailView.swift:15andOperationListView.swift:15. It reads onlybounds.{minX,maxX,minY,maxY},zLevelanddescription. All of that keeps working unchanged. Its one real exposure is item 4:ProjectDetailView.swift:632displays a stock box viainteractiveContext.display(_:style:)and installs aManipulatorWidgeton it, so picks against that stock body now resolve under[.face]rather than[.body]. It never uses.selection,.selected,.selectionModes,.select(_:scheme:),.selectionSummary,PickedEntity,PickedEdgeInfoorPickedVertexInfo, so items 1, 2, 3, 5, 6 and 7 cost it nothing.import OCCTSwiftCADKitanywhere. Nothing in this PR reaches it. Its ownOCCTSwiftUXKit.SelectionSummaryis 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
InteractiveContextentries. The obvious implementation isinteractiveContext.display(_:)per model body, letting the context own everything. It is wrong twice:displayowns tessellation, which CADKit does itself with its own transforms, caps and comparison state; and the context'supdateSelectionVisualswritestriangleStyles, the same arraysetScalarField(_:forBody:)paints, so registering would silently destroy any scalar field. CADKit mints oneInteractiveObjectper body id instead (id cached,Shapere-read on each construction, which is safe becauseInteractiveObjectcompares and hashes by id alone) and drives the context's selection with those.Why the
$selectionsink has no.receive(on: RunLoop.main), unlike the$bodiessink two lines above it. It has to run synchronously, so a caller readingselectionright afterselect(_:scheme:)sees the result, which the existing tests require. The consequence is that it fires duringwillSet, wheninteractiveContext.selectionstill reports the previous value, sosyncSelection(with:)takes the new selection as an argument rather than reading it back off the context.Why
select(_:scheme:)onInteractiveContexthas no default value. Adding a defaultedscheme:to the existingselect(_:)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 adisplay(_:)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.swifthad its own copy of the four-case scheme switch; both it andselect(_:scheme:)now call one internalapplySelection(_: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 resolvedSubShape, 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.selectionFilteris 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
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 newSharedSelectionTestssuite (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_everyPickResolvesToCorrectSubShapeis untouched and still green.swift-format lint --strictandswiftlint lint --strict: 0 violations. The only new build warnings are the 2 the deprecated-alias test deliberately triggers.