feat(map): coincident-node disambiguation picker (#2049)#2108
Open
bruschill wants to merge 2 commits into
Open
feat(map): coincident-node disambiguation picker (#2049)#2108bruschill wants to merge 2 commits into
bruschill wants to merge 2 commits into
Conversation
Replace the coincident-node fan-out (computeSpreadOverrides) with an on-demand "Select a Node" picker so stacked pins stay on their true GPS location instead of being scattered into a ring. - Remove the per-refresh O(n) spatial-clustering fan-out and the per-pin spreadOverrides dictionary lookup in the annotation-sync coordinate closure. - ClusterMapView: a tapped cluster whose members span < 5 m routes to onColocatedStack (guarded to members > 1); wider clusters still zoom-to-fit. - MeshMapMK: with clustering OFF (no cluster annotation), a pin tap groups the tapped node with coincident neighbors via the pure MeshMapPositionSnapshot.colocated(with:in:withinMeters:) so occluded nodes stay reachable. Picker input is de-duplicated by nodeNum and sorted by name. - Picker reuses the standard node-list cell (NodeListItem) with a "Select a Node (N)" count title; a row tap opens that node's detail. - MapColocation.spreadMeters is the single source of truth for the 5 m threshold. - DEBUG-only QA seed (--meshtastic-cluster-demo: pairs/colocatedN/stacks, plus --cluster-demo-no-clustering) to reproduce every case in the simulator. - Docs: update docs/user/map.md (+ regenerated bundle). Tests: MeshtasticTests/MapColocatedNodesTests.swift (16 swift-testing cases).
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds deterministic cluster-demo seed data and replaces map pin fan-out with picker-based disambiguation for coincident nodes. The change updates MapKit selection handling, introduces colocated-node helpers and state, adds coverage, and documents overlapping-pin behavior. ChangesCoincident Node Handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MeshMapMK
participant ClusterMapView
participant MeshMapPositionSnapshot
participant ColocatedNodeStack
participant NodeDetailSheet
MeshMapMK->>ClusterMapView: register selection callbacks
ClusterMapView->>MeshMapPositionSnapshot: identify colocated members
ClusterMapView->>MeshMapMK: send colocated items
MeshMapMK->>ColocatedNodeStack: present node picker
ColocatedNodeStack->>MeshMapMK: return selected node
MeshMapMK->>NodeDetailSheet: present node details
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Meshtastic/Views/Nodes/Helpers/Map/ClusterMapView.swift`:
- Around line 763-770: Update the spread calculation in the cluster member
annotation flow to measure diagonal distance rather than max(rect.size.width,
rect.size.height), so a bounding box’s corner-to-corner separation matches the
CLLocation.distance behavior used by the pin path. Preserve the existing
Self.colocatedSpreadMeters comparison and colocated-stack routing.
In `@Meshtastic/Views/Nodes/MeshMapMK.swift`:
- Around line 846-866: Update presentNodeSelection to create the picker-ready
array using MeshMapPositionSnapshot.dedupedByNodeNumSortedByName(coincident)
before branching. Use that deduplicated array to decide whether to call
presentColocatedStack or selectNode, and pass the same array to the picker
without re-deduplicating.
- Around line 891-902: Update the demo activation gate in the framing logic
around PerformanceSeedData.configuration and the --meshtastic-cluster-demo
argument to also honor MESHTASTIC_CLUSTER_DEMO environment activation. Ensure
environment-launched QA runs enter the existing coordinate-centroid and camera
framing path without changing its behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 118a280d-9078-4d3a-a033-bc6a89ba4e2c
📒 Files selected for processing (9)
Meshtastic/Persistence/PerformanceSeedData.swiftMeshtastic/Resources/docs/index.jsonMeshtastic/Resources/docs/markdown/user/map.mdMeshtastic/Resources/docs/user/map.htmlMeshtastic/Views/Nodes/Helpers/Map/ClusterMapView.swiftMeshtastic/Views/Nodes/Helpers/Map/MapContent/MeshMapContent.swiftMeshtastic/Views/Nodes/MeshMapMK.swiftMeshtasticTests/MapColocatedNodesTests.swiftdocs/user/map.md
- ClusterMapView: measure the coincident-cluster span as the bounding-box diagonal (hypot) instead of max(width,height), so the clustering-on path agrees with the CLLocation.distance metric used by the clustering-off pin path. - MeshMapMK.presentNodeSelection: de-duplicate by nodeNum before branching, so duplicate snapshots for one node can't open a one-row picker. - MeshMapMK: gate the DEBUG tight-frame on PerformanceSeedData.configuration style == .clusterDemo so env-var-activated (MESHTASTIC_CLUSTER_DEMO) QA runs also get the demo framing, not just the launch-arg path. - Add a test covering coincident duplicates collapsing to a single entry.
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.
What changed?
Reworks how the mesh map presents coincident (stacked) nodes, meaning nodes that report the same or nearly the same location, which fixed-size pins can never separate by zooming (#2049).
computeSpreadOverridesfanned coincident pins outward into a roughly 14 m ring, moving every pin off its real GPS location.NodeListItem), and tapping a row opens that node's detail.The picker is reachable whether map clustering is on (tap the count badge) or off (tap the overlapping pins, since the top pin's tap now surfaces the whole occluded stack, so a covered node is never untappable).
Implementation notes:
computeSpreadOverridesandspreadOverrides(the fan-out) and the per-pin dictionary lookup in the annotation-synccoordinate:closure.ClusterMapView.didSelect: a cluster whose members span less than 5 m routes toonColocatedStack(guarded to more than one member); wider clusters still zoom to fit.MeshMapMK.presentNodeSelection: clustering-off grouping via the pureMeshMapPositionSnapshot.colocated(with:in:withinMeters:). Picker input is de-duplicated bynodeNumand sorted by name.MapColocation.spreadMetersis the single source of truth for the 5 m threshold, shared by both tap paths and the tests.--meshtastic-cluster-demo(pairs,colocated10|30|50,stacks) plus--cluster-demo-no-clustering.Why did it change?
The fan-out ring fixed the "stuck cluster" symptom but moved pins off their real location, which is misleading on a map, and it recomputed a spatial-clustering pass on every position refresh. Moving to an on-demand picker keeps pin positions honest and shifts the cost off the hot path:
computeSpreadOverridesclustering, plus the per-pinspreadOverrides[...]dictionary lookup in thesynccoordinate:closure, so steady-state map cost goes down.colocated(...)is a single O(n) scan bounded by the existingdenseMapPositionLimit = 700pin cap, run once per tap, never per frame or per packet.dedupedByNodeNumSortedByNameis O(n) plus O(n log n), once per picker open.In short, the change removes recurring O(n) work from the map and adds only bounded, per-tap O(n) work.
How is this tested?
MeshtasticTests/MapColocatedNodesTests.swift, 16 swift-testing cases for the grouping policy: membership (lone, pair, 10-stack, sub-meter GPS jitter), strict less-than threshold boundary, custom threshold, neighborhood scoping (adjacent stacks don't merge), empty input, and picker de-dupe-by-nodeNumplus sort-by-name. All pass.swiftlintclean on changed files (no new violations); build succeeds.Screenshots/Videos (when applicable)
All captured on the iPhone 17 simulator via the DEBUG
--meshtastic-cluster-demoseed.Before and after
computeSpreadOverridesused to scatter coincident nodes into a ring, pushing every pin off its real GPS location. Now they stay put on their true coordinate, and you tap to disambiguate.Tapping a stack: pick a node, open it
Works with clustering turned off
With clustering off there is no count badge, so the nodes render as overlapping pins. A tap on the stack still opens the picker, so an occluded node is never stranded under the top pin.
More scenarios
Boundary-straddling pairs, scale (30 and 50 nodes), and multiple independent stacks
Checklist
docs/user/ordocs/developer/, and updated accordingly. Updateddocs/user/map.md(Node Pins section, "Overlapping nodes at the same spot") and regenerated the bundled docs.Summary by CodeRabbit