CAMP running-task view P2: map overlay + two-way selection sync#135
Merged
Conversation
Two new classes (TaskOverlayItem, RunningTasksOverlay) wire RunningTasksView signals to per-task GeoGraphicsItems on the CAMP scene; hasTask guard added to RunningTasksView.
Adds TaskOverlayItem (GeoGraphicsItem subclass) and RunningTasksOverlay (ROSWidget glue) to render running tasks on the QGraphicsScene and provide two-way selection sync with RunningTasksView. - RunningTasksModel: add hasTaskPoses() predicate - RunningTasksView: guard taskSelected with hasTaskPoses(); emit tasksUpdated after each model rebuild (no second ROS subscription) - TaskOverlayItem: single pose→circle, multi-pose→polyline; visual states for current/selected/done/pending; emits clicked(id) - RunningTasksOverlay: rebuild-all on tasksUpdated, idempotent selection-sync with QSignalBlocker-style guard - GeoGraphicsItem: add RunningTaskType to enum - Platform: construct overlay in onNodeUpdated using scene() directly - CMakeLists: add new .cpp files; add test_running_tasks_model gtest - test/test_running_tasks_model.cpp: 5 passing tests for hasTaskPoses Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Multi-pose shape() returned an open, zero-area polyline, so Qt's shape().contains() hit-test never matched and survey_line/transit overlay items were unclickable (map->tree selection broken). Stroke the polyline into a clickable band. Marker radii were fixed scene units (6.0/3.0), so they were sub-pixel when zoomed out and oversized when zoomed in. Add sceneRadius() to size markers and the hit-stroke to a constant display-pixel footprint, per the ADR-0003 constant-pixel convention (mirrors ShipTrack::drawTriangle).
- Guard against scene/overlay teardown double-free: scene_ is now a QPointer, and clearItems() only deletes items while the scene is alive (a destroyed scene has already deleted its child items) — fixes the use-after-free / double-free when the QGraphicsScene outlives nothing and is torn down first. (must-fix) - rebuildItems() now also guards node_ (not just transform_buffer_): getGeoCoordinate's stale-stamp fallback logs via node_->get_logger(). - Clear selected_id_ when the selected task disappears from a republish so the overlay and tree don't drift. - Correct the onItemClicked comment: it uses an id-equality guard, not the QSignalBlocker pattern.
The overlay was only constructed inside onNodeUpdated when scene() was already non-null; if the scene wasn't attached on the first (and only) node update the overlay was silently never created. Factor creation into ensureRunningTasksOverlay() and also call it from the periodic update(), so the overlay is created on whichever call first sees both node_ and scene().
…129) - Guard rebuildItems against a duplicate task id overwriting items_[id] and orphaning the prior scene item (leak). - Rename TaskOverlayItem::setSelected -> setHighlighted (+ is_selected_ -> is_highlighted_) to avoid shadowing non-virtual QGraphicsItem::setSelected.
Root fix for a pre-existing rendering bug (flagged during #129 review): on a zoom change, updateMapScale only stored the scalar; constant-pixel glyphs (Waypoint markers/arrows, etc.) change item-coordinate size but the scene kept their stale cached boundingRect, clipping them when zooming out. updateMapScale now calls prepareGeometryChange() on every GeoGraphicsItem in the scene before the scale changes, so the scene re-indexes/repaints at the new size. This fixes the existing mission glyphs AND the new running-task overlay items (which live in the same scene), replacing the need for an item-specific zoom hook.
- sceneRadius: floor against a collapsed projection (zero metresPerPixel / coincident probe pixels) so a multi-pose item never gets a zero-width, unclickable hit stroke. - onItemClicked: apply the highlight locally before delegating to the tree, so a map click is reflected immediately even if the tree can't resolve the id.
This was referenced Jun 28, 2026
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.
Closes #129
Summary
P2 of the running-task display (umbrella #123): put the running tasks on the
chart and sync selection both ways with the P1 tree view.
TaskOverlayItem(GeoGraphicsItemsubclass) — renders one task's poses onthe Web-Mercator scene: filled circle for single-pose (goto), polyline for
multi-pose (survey_line/transit). Current / pending / done visual states.
Markers are constant-pixel (ADR-0003
metresPerPixel); the multi-poseshape()is stroked so the line is hit-testable. Emits
clicked(id).RunningTasksOverlay(camp_ros::ROSWidgetglue) — owns the overlay items,does the
getGeoCoordinate(ROS frame → earth → scene) conversion, and wirestwo-way selection. Refreshes off
RunningTasksView::tasksUpdated(no second ROSsubscription); rebuild-all per
TaskFeedback(~1 Hz). Idempotent selection sync.RunningTasksViewgains atasksUpdatedsignal and ahasTaskPosesguard sotaskSelectedonly fires for rows that actually have geometry.Platformviascene();RunningTaskTypeadded to theGeoGraphicsItemenum.Included: a pre-existing zoom-geometry bug fix
While reviewing, we found a real bug in shared rendering code: on a zoom change
updateMapScaleonly stored the scalar, so constant-pixel glyphs (Waypointmarkers/arrows, etc.) changed item-coordinate size but the scene kept their stale
cached
boundingRectand clipped them when zooming out.updateMapScalenow callsprepareGeometryChange()on everyGeoGraphicsItemin the scene before the scalechanges — fixing the existing mission glyphs and the new task-overlay items in
one place (they share the scene).
scaleChangedonly fires on wheel-zoom, so thisruns on discrete zoom steps, not on pan.
Testing
colcon buildofcampclean; 128 tests, 0 failures (incl. 5 new gtests forthe
hasTaskPosesguard).item double-ownership UAF, non-zoom-scaled markers) — all fixed and re-verified.
Rounds 2–3 surfaced suggestions; the robustness ones (dup-id leak guard,
setSelected→setHighlightedrename,sceneRadiusfloor, local highlight onmap click) are folded in. Final verdict: approved, 0 must-fix.
Tracked follow-ups (minor, from review)
deselect so the panes don't drift.
updateMapScalecould pre-filter ontype() >= UserTypebeforedynamic_castif scenes grow very large (negligible today).
Next (umbrella #123)
P3 editing / nudge-a-waypoint; P4 mission progress; the additive rqt wrapper.
Authored-By:
Claude Code AgentModel:
Claude Opus 4.8 (1M context)