From 22cd9fa63762849a5e94b6273e6dd355acae9fa6 Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Sun, 28 Jun 2026 15:43:55 +0000 Subject: [PATCH 01/16] progress: issue review for #129 --- .agent/work-plans/issue-129/progress.md | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) create mode 100644 .agent/work-plans/issue-129/progress.md diff --git a/.agent/work-plans/issue-129/progress.md b/.agent/work-plans/issue-129/progress.md new file mode 100644 index 0000000..bcf274b --- /dev/null +++ b/.agent/work-plans/issue-129/progress.md @@ -0,0 +1,21 @@ +--- +issue: 129 +--- + +# Issue #129 — Running tasks on chart + selection sync (P2) + +## Issue Review +**Status**: complete +**When**: 2026-06-28 00:00 +00:00 +**By**: Claude Code Agent (Claude Sonnet) + +**Issue**: #129 +**Comment**: (best-effort post follows this entry; not recorded inline) +**Scope verdict**: well-scoped + +### Actions +- [ ] Resolve frame-handling strategy before or early in plan-task: determine which frame `poses[].header.frame_id` is populated with (earth/map frame vs. something requiring a TF lookup per update) and document the decision — either in code comments or a brief design note in the plan. +- [ ] Confirm or fix the P1 carry-over guard: `RunningTasksView::taskSelected` must not fire for synthetic group rows (only `hasTask` leaf nodes); verify this is enforced and add a test if not already present. +- [ ] Clarify `getGeoCoordinate` access path: `RunningTasksView` uses node-injection (not `camp_ros::ROSWidget`), so the CAMP-only glue layer (which has access to `camp_ros`) must own the frame conversion — confirm the architectural split is explicit in the plan so the core widget stays clean. +- [ ] Decide and document redraw cadence: rebuild-all vs. diff graphics items on each `TaskFeedback` republish; document the choice (a brief comment in the overlay class suffices unless significant). +- [ ] Dependency gate: confirm PR #128 (P1) is merged before starting implementation. From e5094f8679a47c250e3a45caa9fe74f710bca8b5 Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Sun, 28 Jun 2026 15:53:15 +0000 Subject: [PATCH 02/16] Add work plan for #129 Two new classes (TaskOverlayItem, RunningTasksOverlay) wire RunningTasksView signals to per-task GeoGraphicsItems on the CAMP scene; hasTask guard added to RunningTasksView. --- .agent/work-plans/issue-129/plan.md | 120 ++++++++++++++++++++++++++++ 1 file changed, 120 insertions(+) create mode 100644 .agent/work-plans/issue-129/plan.md diff --git a/.agent/work-plans/issue-129/plan.md b/.agent/work-plans/issue-129/plan.md new file mode 100644 index 0000000..934b0ef --- /dev/null +++ b/.agent/work-plans/issue-129/plan.md @@ -0,0 +1,120 @@ +# Plan: Running tasks on chart + selection sync (P2) + +## Issue + +https://github.com/rolker/camp/issues/129 + +## Context + +P1 (#127 / PR #128) delivered a read-only `RunningTasksView` tree widget that +subscribes to `marine/status/mission_tasks` and renders `TaskFeedback` as a +structured tree. The widget emits `taskSelected(QString id)` on row activation +and exposes `setSelectedTask(id)` for external selection. Neither signal is yet +wired to anything outside the widget. + +P2 adds three things on top of that foundation: + +1. A per-task geometry overlay on CAMP's `QGraphicsScene` — point for single-pose + tasks (goto), polyline for multi-pose tasks (survey_line), using the existing + `GeoGraphicsItem` machinery but visually distinct from editable plan items. +2. Two-way selection sync between `RunningTasksView` and the overlay. +3. The CAMP-only glue class that owns the overlay items and wires the signals. + +A hasTask guard (carry-over from P1) is included: `taskSelected` must not fire +for group rows that have no poses, so map glue isn't triggered by clicks on +pure-structural parent rows. + +## Approach + +1. **Add `hasTaskPoses` to `RunningTasksModel`** — predicate that returns true when + the node at a `QModelIndex` has `task->message().poses` non-empty. Guard + `RunningTasksView::onCurrentRowChanged`: only emit `taskSelected` when + `model_->hasTaskPoses(current)` is true. + +2. **Add `tasksUpdated` signal to `RunningTasksView`** — emit from the end of + `applyPendingTasks()` with the current-task id and the raw `TaskInformation` + vector. This lets the CAMP glue layer refresh overlay geometry without a + second ROS subscription. + +3. **Add `TaskOverlayItem`** (`GeoGraphicsItem` subclass, lives in + `src/camp/running_tasks/`) — renders one task's poses on the scene. + - Single pose → filled circle (goto). + - Two or more poses → polyline (survey_line, transit, etc.). + - Visual states: current (emphasized, brighter), pending (muted), done (gray). + - Emits `clicked(QString id)` for map→tree selection sync. + - New type constant `RunningTaskType` added to `GeoGraphicsItem`'s enum. + +4. **Add `RunningTasksOverlay`** (`camp_ros::ROSWidget` subclass, lives in + `src/camp/running_tasks/`) — the CAMP-only glue. + - Initialized with the `QGraphicsScene*` and a `RunningTasksView*`. + - Holds `QMap` keyed by task id. + - `tasksUpdated` → rebuild overlay: call `getGeoCoordinate` (from ROSWidget) for + each pose, create/replace `TaskOverlayItem`s on the scene. + - `taskSelected` → set `is_selected_` on the matching item, clear others, + call `item->update()`. + - `TaskOverlayItem::clicked(id)` → `runningTasksView_->setSelectedTask(id)`. + +5. **Wire in `Platform`** — after `runningTasksView->setNode(node_)`, construct + `RunningTasksOverlay`, pass it the scene from the `AutonomousVehicleProject` + (via `Platform`'s parent chain or a passed-in pointer) and connect it to + `m_ui->runningTasksView`. `RunningTasksOverlay` inherits `nodeStarted` via + `ROSWidget`, so call `overlay_->nodeStarted(node_, transform_buffer_)` in + `Platform::onNodeUpdated`. + +6. **CMakeLists** — add `running_tasks/task_overlay_item.cpp` and + `running_tasks/running_tasks_overlay.cpp` to the `SOURCES` list. + +## Files to Change + +| File | Change | +|------|--------| +| `src/camp/running_tasks/running_tasks_model.h` | Add `hasTaskPoses(QModelIndex)` declaration | +| `src/camp/running_tasks/running_tasks_model.cpp` | Implement `hasTaskPoses` | +| `src/camp/running_tasks/running_tasks_view.h` | Add `tasksUpdated` signal | +| `src/camp/running_tasks/running_tasks_view.cpp` | Emit `tasksUpdated` from `applyPendingTasks`; guard `onCurrentRowChanged` with `hasTaskPoses` | +| `src/camp/running_tasks/task_overlay_item.h` | New file — `TaskOverlayItem` (`GeoGraphicsItem` subclass) | +| `src/camp/running_tasks/task_overlay_item.cpp` | New file — paint + click + coordinate update | +| `src/camp/running_tasks/running_tasks_overlay.h` | New file — `RunningTasksOverlay` (`camp_ros::ROSWidget`) | +| `src/camp/running_tasks/running_tasks_overlay.cpp` | New file — glue: rebuild items, sync selection | +| `src/camp/geographicsitem.h` | Add `RunningTaskType` to the type enum | +| `src/camp/platform_manager/platform.h` | Add `RunningTasksOverlay*` member | +| `src/camp/platform_manager/platform.cpp` | Construct overlay, call `nodeStarted`, connect signals | +| `CMakeLists.txt` | Add two new `.cpp` files to `SOURCES` | + +## Principles Self-Check + +| Principle | Consideration | +|---|---| +| Only what's needed | Two new classes; no third subscription or duplicate data path | +| Workspace vs. project separation | All new code in `camp/` project repo — no workspace infra touched | +| A change includes its consequences | `CMakeLists.txt` updated; `geographicsitem.h` enum extended | +| Improve incrementally | Single PR building on P1 foundation; P3 editing deferred | + +## ADR Compliance + +| ADR | Triggered | How addressed | +|---|---|---| +| ADR-0002 (Web-Mercator scene) | Yes | `TaskOverlayItem` uses `geoToPixel` for scene coordinates; no chart dependency | +| ADR-0008 (ROS 2 conventions) | Yes | Node-injection idiom for `RunningTasksView` unchanged; ROSWidget pattern for overlay | + +## Consequences + +| If we change... | Also update... | Included in plan? | +|---|---|---| +| `GeoGraphicsItem` type enum | Any switch/cast on item types | Yes — add enum value; no existing switch covers it | +| `RunningTasksView` signals | rqt plugin (future) | No — `tasksUpdated` is additive; rqt path unaffected | +| `Platform` construction | Any future subclass of Platform | No follow-up needed — single concrete class | + +## Open Questions + +- Which frame does the boat populate `poses[].header.frame_id` with in practice? + `getGeoCoordinate` will TF-lookup to earth regardless, but knowing the source + frame helps set timeout expectations (earth→map is quasi-static; a more dynamic + frame would need a tighter retry window). +- Should the overlay recenter the view on the selected task's first pose (tree→map)? + The issue says "and optionally recenters"; leaving optional — implement if trivial + once selection sync is working. + +## Estimated Scope + +Single PR. From 4ac1f83296619d8f9ae2a67eb86eb4d2651099e4 Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Sun, 28 Jun 2026 15:53:34 +0000 Subject: [PATCH 03/16] progress: plan authored for #129 --- .agent/work-plans/issue-129/progress.md | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/.agent/work-plans/issue-129/progress.md b/.agent/work-plans/issue-129/progress.md index bcf274b..2f95604 100644 --- a/.agent/work-plans/issue-129/progress.md +++ b/.agent/work-plans/issue-129/progress.md @@ -19,3 +19,16 @@ issue: 129 - [ ] Clarify `getGeoCoordinate` access path: `RunningTasksView` uses node-injection (not `camp_ros::ROSWidget`), so the CAMP-only glue layer (which has access to `camp_ros`) must own the frame conversion — confirm the architectural split is explicit in the plan so the core widget stays clean. - [ ] Decide and document redraw cadence: rebuild-all vs. diff graphics items on each `TaskFeedback` republish; document the choice (a brief comment in the overlay class suffices unless significant). - [ ] Dependency gate: confirm PR #128 (P1) is merged before starting implementation. + +## Plan Authored +**Status**: complete +**When**: 2026-06-28 12:00 +00:00 +**By**: Claude Code Agent (Claude Sonnet) + +**Plan**: `.agent/work-plans/issue-129/plan.md` at `e5094f8` +**Branch**: feature/issue-129 at `e5094f8` +**Phases**: single + +### Open questions +- [ ] Which frame does the boat populate `poses[].header.frame_id` with in practice — determines TF timeout tuning. +- [ ] Should tree→map selection optionally recenter the view on the task's first pose? From a913d6a70761d365289161777eaac8d74e421103 Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Sun, 28 Jun 2026 15:58:47 +0000 Subject: [PATCH 04/16] progress: plan review for #129 --- .agent/work-plans/issue-129/progress.md | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/.agent/work-plans/issue-129/progress.md b/.agent/work-plans/issue-129/progress.md index 2f95604..5ae87f6 100644 --- a/.agent/work-plans/issue-129/progress.md +++ b/.agent/work-plans/issue-129/progress.md @@ -32,3 +32,19 @@ issue: 129 ### Open questions - [ ] Which frame does the boat populate `poses[].header.frame_id` with in practice — determines TF timeout tuning. - [ ] Should tree→map selection optionally recenter the view on the task's first pose? + +## Plan Review +**Status**: complete +**When**: 2026-06-28 15:57 +00:00 +**By**: Claude Code Agent (Claude Opus) + +**Plan**: `.agent/work-plans/issue-129/plan.md` at `e5094f8` +**PR**: PR-less (`--issue` mode) +**Verdict**: approve-with-suggestions + +### Findings +- [ ] (must-fix) No test planned for the `hasTaskPoses` guard — `review-issue` action #2 required one, P1 added none, and the gtest suite is a primary gate. Add `test/test_running_tasks_model.cpp` (empty-poses group row vs. leaf with poses) wired via `ament_add_gtest` (mirror `test_collision_monitor_converter`). — `plan.md:67-82` +- [ ] (suggestion) Simplify scene access — `Platform` is a `GeoGraphicsItem` (`ShipTrack : public GeoGraphicsItem`), so use `Platform::scene()` directly rather than parent-chain/passed-in pointer. — `plan.md:57-62` +- [ ] (suggestion) Close Open Question #1 in-plan — `TaskInformation.poses` is `geometry_msgs/PoseStamped[]` (per-pose headers); `getGeoCoordinate` already transforms each to `earth` with stale-stamp retry, so the frame is schema-answered. — `plan.md:108-113` +- [ ] (suggestion) Document the redraw cadence (rebuild-all) in the overlay class — `review-issue` action #4. — `plan.md:50-55` +- [ ] (suggestion) Note/guard the selection-sync loop (idempotent map→tree→map round-trip), mirroring the `QSignalBlocker` pattern in `applyPendingTasks`. — `plan.md:52-55` From 3c916c7c05b9e5925c01346cbe15394e778e55f3 Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Sun, 28 Jun 2026 16:21:08 +0000 Subject: [PATCH 05/16] feat(camp#129): running-task map overlay + tree/map selection sync (P2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- CMakeLists.txt | 21 +++ src/camp/geographicsitem.h | 1 + src/camp/platform_manager/platform.cpp | 14 ++ src/camp/platform_manager/platform.h | 3 + .../running_tasks/running_tasks_model.cpp | 8 ++ src/camp/running_tasks/running_tasks_model.h | 4 + .../running_tasks/running_tasks_overlay.cpp | 102 ++++++++++++++ .../running_tasks/running_tasks_overlay.h | 59 ++++++++ src/camp/running_tasks/running_tasks_view.cpp | 4 + src/camp/running_tasks/running_tasks_view.h | 6 + src/camp/running_tasks/task_overlay_item.cpp | 106 +++++++++++++++ src/camp/running_tasks/task_overlay_item.h | 57 ++++++++ test/test_running_tasks_model.cpp | 128 ++++++++++++++++++ 13 files changed, 513 insertions(+) create mode 100644 src/camp/running_tasks/running_tasks_overlay.cpp create mode 100644 src/camp/running_tasks/running_tasks_overlay.h create mode 100644 src/camp/running_tasks/task_overlay_item.cpp create mode 100644 src/camp/running_tasks/task_overlay_item.h create mode 100644 test/test_running_tasks_model.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index ff9642b..06baaa3 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -110,6 +110,8 @@ set(SOURCES src/camp/helm_manager/helm_manager.cpp src/camp/running_tasks/running_tasks_model.cpp src/camp/running_tasks/running_tasks_view.cpp + src/camp/running_tasks/task_overlay_item.cpp + src/camp/running_tasks/running_tasks_overlay.cpp src/camp/roslink.cpp src/camp/nav_source.cpp ) @@ -420,6 +422,25 @@ if(BUILD_TESTING) Qt5::Positioning ) + # RunningTasksModel::hasTaskPoses guard (camp#129 P2) + ament_add_gtest(test_running_tasks_model + test/test_running_tasks_model.cpp + src/camp/running_tasks/running_tasks_model.cpp + ) + target_include_directories(test_running_tasks_model PRIVATE + ${CMAKE_CURRENT_SOURCE_DIR}/src/camp + ) + ament_target_dependencies(test_running_tasks_model + rclcpp + geometry_msgs + marine_nav_interfaces + marine_nav_tasks + ) + target_link_libraries(test_running_tasks_model + Qt5::Core + Qt5::Widgets + ) + # earth<-frame transform cache / staleness decision (#59 — TF-gap flicker fix) ament_add_gtest(test_transform_cache test/test_transform_cache.cpp diff --git a/src/camp/geographicsitem.h b/src/camp/geographicsitem.h index 0b0e2c1..953dd47 100644 --- a/src/camp/geographicsitem.h +++ b/src/camp/geographicsitem.h @@ -31,6 +31,7 @@ class GeoGraphicsItem : public QGraphicsItem AvoidAreaType, CollisionMonitorType, FootprintType, + RunningTaskType, }; GeoGraphicsItem(QGraphicsItem *parentItem = Q_NULLPTR); diff --git a/src/camp/platform_manager/platform.cpp b/src/camp/platform_manager/platform.cpp index 8b84cc4..af89371 100644 --- a/src/camp/platform_manager/platform.cpp +++ b/src/camp/platform_manager/platform.cpp @@ -3,6 +3,7 @@ #include #include "nav_source.h" #include "ros/ros_context.h" +#include "running_tasks/running_tasks_overlay.h" #include @@ -236,6 +237,19 @@ void Platform::onNodeUpdated() m_ui->missionManager->nodeStarted(node_, transform_buffer_); m_ui->runningTasksView->setNode(node_); subscribeToPathTopic(); + + // Construct the overlay once (on first node assignment). Platform is a + // GeoGraphicsItem so scene() gives us the shared QGraphicsScene directly. + if (!running_tasks_overlay_ && scene()) + { + running_tasks_overlay_ = new RunningTasksOverlay( + scene(), m_ui->runningTasksView, this); + running_tasks_overlay_->nodeStarted(node_, transform_buffer_); + } + else if (running_tasks_overlay_) + { + running_tasks_overlay_->nodeStarted(node_, transform_buffer_); + } } void Platform::subscribeToPathTopic() diff --git a/src/camp/platform_manager/platform.h b/src/camp/platform_manager/platform.h index 443da42..fe2b8be 100644 --- a/src/camp/platform_manager/platform.h +++ b/src/camp/platform_manager/platform.h @@ -14,6 +14,7 @@ namespace Ui class NavSource; class MissionManager; class HelmManager; +class RunningTasksOverlay; class Platform : public camp_ros::ROSWidget, public ShipTrack { @@ -80,6 +81,8 @@ public slots: std::vector path_geopoints_; std::vector path_local_points_; + RunningTasksOverlay* running_tasks_overlay_ = nullptr; + }; #endif // PLATFORM_H diff --git a/src/camp/running_tasks/running_tasks_model.cpp b/src/camp/running_tasks/running_tasks_model.cpp index c629b14..89c88be 100644 --- a/src/camp/running_tasks/running_tasks_model.cpp +++ b/src/camp/running_tasks/running_tasks_model.cpp @@ -230,3 +230,11 @@ QModelIndex RunningTasksModel::indexForId(const QString& id) const return QModelIndex(); return createIndex(node->rowInParent, 0, node); } + +bool RunningTasksModel::hasTaskPoses(const QModelIndex& index) const +{ + Node* n = nodeForIndex(index); + if (!n || !n->task) + return false; + return !n->task->message().poses.empty(); +} diff --git a/src/camp/running_tasks/running_tasks_model.h b/src/camp/running_tasks/running_tasks_model.h index f2dcf5d..954471a 100644 --- a/src/camp/running_tasks/running_tasks_model.h +++ b/src/camp/running_tasks/running_tasks_model.h @@ -51,6 +51,10 @@ class RunningTasksModel : public QAbstractItemModel /// Index of the row carrying the given full id (invalid if not present). QModelIndex indexForId(const QString& id) const; + /// True when the node at index has at least one pose (i.e. it is a leaf + /// task with geometry, not a pure structural parent row). + bool hasTaskPoses(const QModelIndex& index) const; + private: struct Node { diff --git a/src/camp/running_tasks/running_tasks_overlay.cpp b/src/camp/running_tasks/running_tasks_overlay.cpp new file mode 100644 index 0000000..27fe206 --- /dev/null +++ b/src/camp/running_tasks/running_tasks_overlay.cpp @@ -0,0 +1,102 @@ +#include "running_tasks/running_tasks_overlay.h" + +#include + +#include "running_tasks/running_tasks_view.h" + +RunningTasksOverlay::RunningTasksOverlay(QGraphicsScene* scene, + RunningTasksView* view, + QWidget* parent) + : camp_ros::ROSWidget(parent), scene_(scene), view_(view) +{ + connect(view_, &RunningTasksView::tasksUpdated, + this, &RunningTasksOverlay::onTasksUpdated); + connect(view_, &RunningTasksView::taskSelected, + this, &RunningTasksOverlay::onTaskSelected); +} + +RunningTasksOverlay::~RunningTasksOverlay() +{ + clearItems(); +} + +void RunningTasksOverlay::onTasksUpdated( + const QString& current_task, + const std::vector& tasks) +{ + rebuildItems(current_task, tasks); +} + +void RunningTasksOverlay::onTaskSelected(const QString& id) +{ + if (id == selected_id_) + return; // idempotent — guard against map→tree→map round-trips + + selected_id_ = id; + for (auto it = items_.begin(); it != items_.end(); ++it) + it.value()->setSelected(it.key() == id); +} + +void RunningTasksOverlay::onItemClicked(const QString& id) +{ + // Guard the selection-sync loop so a map click doesn't trigger + // taskSelected → onTaskSelected → setSelected → redraw → re-click. + // Mirror the QSignalBlocker pattern in RunningTasksView::applyPendingTasks. + if (id == selected_id_) + return; + view_->setSelectedTask(id); +} + +void RunningTasksOverlay::rebuildItems( + const QString& current_task, + const std::vector& tasks) +{ + if (!scene_ || !transform_buffer_) + return; + + clearItems(); + + for (const auto& task : tasks) + { + if (task.poses.empty()) + continue; + + // TaskInformation.poses is geometry_msgs/PoseStamped[] with per-pose headers. + // getGeoCoordinate transforms each to earth frame with stale-stamp retry, + // so the source frame (commonly map or earth) is handled transparently. + QList geo_poses; + for (const auto& ps : task.poses) + { + QGeoCoordinate gc = getGeoCoordinate(ps.pose, ps.header); + if (gc.isValid()) + geo_poses.append(gc); + } + if (geo_poses.isEmpty()) + continue; + + const QString id = QString::fromStdString(task.id); + const bool is_current = !current_task.isEmpty() && + id.split('/', Qt::SkipEmptyParts).join('/') == + current_task.split('/', Qt::SkipEmptyParts).join('/'); + + auto* item = new TaskOverlayItem(id, geo_poses, is_current, task.done); + item->setSelected(id == selected_id_); + + connect(item, &TaskOverlayItem::clicked, + this, &RunningTasksOverlay::onItemClicked); + + scene_->addItem(item); + items_[id] = item; + } +} + +void RunningTasksOverlay::clearItems() +{ + for (auto* item : items_) + { + if (scene_) + scene_->removeItem(item); + delete item; + } + items_.clear(); +} diff --git a/src/camp/running_tasks/running_tasks_overlay.h b/src/camp/running_tasks/running_tasks_overlay.h new file mode 100644 index 0000000..f67f1f3 --- /dev/null +++ b/src/camp/running_tasks/running_tasks_overlay.h @@ -0,0 +1,59 @@ +#ifndef CAMP_RUNNING_TASKS_OVERLAY_H +#define CAMP_RUNNING_TASKS_OVERLAY_H + +#include + +#include +#include +#include + +#include "marine_nav_interfaces/msg/task_information.hpp" +#include "ros/ros_widget.h" +#include "running_tasks/task_overlay_item.h" + +class RunningTasksView; + +/// CAMP-only glue that owns the running-task QGraphicsScene overlay. +/// +/// This is the camp_ros::ROSWidget side of the P2 feature: +/// - getGeoCoordinate (from ROSWidget) converts each PoseStamped to earth. +/// - tasksUpdated from RunningTasksView triggers a rebuild-all of overlay items. +/// - taskSelected from RunningTasksView highlights the matching item. +/// - TaskOverlayItem::clicked feeds back into RunningTasksView::setSelectedTask. +/// +/// Rebuild-all strategy: every tasksUpdated call removes all existing items and +/// recreates them. This is correct for the periodic ~1 Hz republish cadence; the +/// simplicity outweighs incremental-diff complexity at this update rate. +class RunningTasksOverlay : public camp_ros::ROSWidget +{ + Q_OBJECT + +public: + /// \param scene The QGraphicsScene to add overlay items to (from + /// Platform::scene() — Platform is a GeoGraphicsItem). + /// \param view The RunningTasksView whose signals we consume. + /// \param parent QWidget parent (typically Platform's widget parent). + explicit RunningTasksOverlay(QGraphicsScene* scene, RunningTasksView* view, + QWidget* parent = nullptr); + ~RunningTasksOverlay() override; + +private slots: + void onTasksUpdated( + const QString& current_task, + const std::vector& tasks); + void onTaskSelected(const QString& id); + void onItemClicked(const QString& id); + +private: + void rebuildItems( + const QString& current_task, + const std::vector& tasks); + void clearItems(); + + QGraphicsScene* scene_; + RunningTasksView* view_; + QMap items_; + QString selected_id_; +}; + +#endif // CAMP_RUNNING_TASKS_OVERLAY_H diff --git a/src/camp/running_tasks/running_tasks_view.cpp b/src/camp/running_tasks/running_tasks_view.cpp index 7a62919..12e0d4c 100644 --- a/src/camp/running_tasks/running_tasks_view.cpp +++ b/src/camp/running_tasks/running_tasks_view.cpp @@ -147,11 +147,15 @@ void RunningTasksView::applyPendingTasks() tree_->setCurrentIndex(restored); } } + + emit tasksUpdated(current, tasks); } void RunningTasksView::onCurrentRowChanged(const QModelIndex& current, const QModelIndex& /*previous*/) { + if (!model_->hasTaskPoses(current)) + return; const QString id = model_->idForIndex(current); if (!id.isEmpty()) emit taskSelected(id); diff --git a/src/camp/running_tasks/running_tasks_view.h b/src/camp/running_tasks/running_tasks_view.h index 85d0f3a..eff7509 100644 --- a/src/camp/running_tasks/running_tasks_view.h +++ b/src/camp/running_tasks/running_tasks_view.h @@ -32,7 +32,13 @@ class RunningTasksView : public QWidget signals: /// User selected a task row (full id). Map glue (P2) consumes this. + /// Only emitted for rows where hasTaskPoses() is true. void taskSelected(QString id); + /// Emitted on the GUI thread after every model update with the current + /// navigation task id and the full raw task list. The RunningTasksOverlay + /// consumes this to rebuild overlay geometry without a second ROS subscription. + void tasksUpdated(QString current_task, + std::vector tasks); public slots: void updateRobotNamespace(QString robot_namespace); diff --git a/src/camp/running_tasks/task_overlay_item.cpp b/src/camp/running_tasks/task_overlay_item.cpp new file mode 100644 index 0000000..9c39f30 --- /dev/null +++ b/src/camp/running_tasks/task_overlay_item.cpp @@ -0,0 +1,106 @@ +#include "running_tasks/task_overlay_item.h" + +#include +#include +#include +#include +#include + +TaskOverlayItem::TaskOverlayItem(const QString& id, + const QList& geo_poses, + bool is_current, bool is_done, + QGraphicsItem* parent) + : QObject(nullptr), GeoGraphicsItem(parent), + id_(id), geo_poses_(geo_poses), + is_current_(is_current), is_done_(is_done) +{ + setAcceptHoverEvents(false); +} + +QPainterPath TaskOverlayItem::buildPath() const +{ + QPainterPath path; + if (geo_poses_.isEmpty()) + return path; + + if (geo_poses_.size() == 1) + { + // Single pose: filled circle at scene position. + const QPointF center = geoToPixel(geo_poses_.first()); + path.addEllipse(center, 6.0, 6.0); + } + else + { + // Multi-pose: polyline through all scene positions. + path.moveTo(geoToPixel(geo_poses_.first())); + for (int i = 1; i < geo_poses_.size(); ++i) + path.lineTo(geoToPixel(geo_poses_[i])); + } + return path; +} + +QRectF TaskOverlayItem::boundingRect() const +{ + return buildPath().boundingRect().marginsAdded(QMarginsF(6, 6, 6, 6)); +} + +QPainterPath TaskOverlayItem::shape() const +{ + return buildPath(); +} + +void TaskOverlayItem::paint(QPainter* painter, + const QStyleOptionGraphicsItem* /*option*/, + QWidget* /*widget*/) +{ + if (geo_poses_.isEmpty()) + return; + + painter->save(); + + QColor color; + if (is_done_) + color = QColor(0x80, 0x80, 0x80, 180); // gray — completed + else if (is_selected_) + color = QColor(0xFF, 0xA5, 0x00, 230); // orange — selected + else if (is_current_) + color = QColor(0x00, 0xCC, 0x00, 220); // green — active + else + color = QColor(0x40, 0x80, 0xFF, 150); // blue-muted — pending + + QPen pen(color); + pen.setCosmetic(true); + pen.setWidth(is_selected_ ? 3 : 2); + painter->setPen(pen); + + if (geo_poses_.size() == 1) + { + painter->setBrush(QBrush(color)); + painter->drawPath(buildPath()); + } + else + { + painter->setBrush(Qt::NoBrush); + painter->drawPath(buildPath()); + // Draw small endpoint markers. + painter->setBrush(QBrush(color)); + for (const QGeoCoordinate& gc : geo_poses_) + { + if (gc.isValid()) + painter->drawEllipse(geoToPixel(gc), 3.0, 3.0); + } + } + + painter->restore(); +} + +void TaskOverlayItem::setSelected(bool selected) +{ + is_selected_ = selected; + update(); +} + +void TaskOverlayItem::mousePressEvent(QGraphicsSceneMouseEvent* /*event*/) +{ + emit clicked(id_); +} diff --git a/src/camp/running_tasks/task_overlay_item.h b/src/camp/running_tasks/task_overlay_item.h new file mode 100644 index 0000000..7e7bfae --- /dev/null +++ b/src/camp/running_tasks/task_overlay_item.h @@ -0,0 +1,57 @@ +#ifndef CAMP_RUNNING_TASKS_TASK_OVERLAY_ITEM_H +#define CAMP_RUNNING_TASKS_TASK_OVERLAY_ITEM_H + +#include +#include +#include +#include + +#include "geographicsitem.h" + +/// Renders one running task's poses on the QGraphicsScene. +/// +/// Single pose → filled circle (goto-style task). +/// Two+ poses → polyline (survey_line, transit, etc.). +/// +/// Visual states: selected (bright/bold), active (current task), done (gray), +/// pending (muted). Emits clicked(id) on mouse press for map→tree selection. +/// +/// Uses geoToPixel() from GeoGraphicsItem (anchored to the Web-Mercator scene +/// origin at (0,0)); no setPos() needed — all rendering is in scene coordinates. +class TaskOverlayItem : public QObject, public GeoGraphicsItem +{ + Q_OBJECT + Q_INTERFACES(QGraphicsItem) + +public: + explicit TaskOverlayItem(const QString& id, + const QList& geo_poses, + bool is_current, bool is_done, + QGraphicsItem* parent = nullptr); + + int type() const override { return RunningTaskType; } + + QRectF boundingRect() const override; + void paint(QPainter* painter, const QStyleOptionGraphicsItem* option, + QWidget* widget) override; + QPainterPath shape() const override; + + void setSelected(bool selected); + +signals: + void clicked(QString id); + +protected: + void mousePressEvent(QGraphicsSceneMouseEvent* event) override; + +private: + QPainterPath buildPath() const; + + QString id_; + QList geo_poses_; + bool is_current_; + bool is_done_; + bool is_selected_ = false; +}; + +#endif // CAMP_RUNNING_TASKS_TASK_OVERLAY_ITEM_H diff --git a/test/test_running_tasks_model.cpp b/test/test_running_tasks_model.cpp new file mode 100644 index 0000000..da08800 --- /dev/null +++ b/test/test_running_tasks_model.cpp @@ -0,0 +1,128 @@ +// Tests for RunningTasksModel::hasTaskPoses — the guard that prevents +// taskSelected from firing for synthetic group/parent rows that have no +// geometry. Covers: group row with empty poses returns false; leaf task with +// at least one pose returns true. + +#include + +#include +#include +#include + +#include +#include + +#include "marine_nav_interfaces/msg/task_information.hpp" +#include "running_tasks/running_tasks_model.h" + +namespace +{ + +marine_nav_interfaces::msg::TaskInformation makeTask( + const std::string& id, + const std::string& type, + bool with_pose = false) +{ + marine_nav_interfaces::msg::TaskInformation t; + t.id = id; + t.type = type; + t.priority = 10; + t.done = false; + if (with_pose) + { + geometry_msgs::msg::PoseStamped ps; + ps.header.frame_id = "map"; + ps.pose.position.x = 1.0; + ps.pose.position.y = 2.0; + t.poses.push_back(ps); + } + return t; +} + +} // namespace + +class RunningTasksModelTest : public ::testing::Test +{ +protected: + rclcpp::Node::SharedPtr node; + RunningTasksModel model; + + void SetUp() override + { + node = rclcpp::Node::make_shared("running_tasks_model_test"); + } + + void setTasks(const std::string& current, + const std::vector& tasks) + { + model.setTasks(QString::fromStdString(current), tasks, node->get_clock()); + } +}; + +TEST_F(RunningTasksModelTest, hasTaskPoses_invalidIndex_returnsFalse) +{ + EXPECT_FALSE(model.hasTaskPoses(QModelIndex())); +} + +TEST_F(RunningTasksModelTest, hasTaskPoses_groupRowWithEmptyPoses_returnsFalse) +{ + // A parent/group row whose TaskInformation has no poses (structural parent). + std::vector tasks = { + makeTask("survey_a", "survey", /*with_pose=*/false), + makeTask("survey_a/line_1", "survey_line", /*with_pose=*/true), + }; + setTasks("survey_a/line_1", tasks); + + // First top-level row should be the parent "survey_a" task (no poses). + const QModelIndex parent_idx = model.index(0, 0); + ASSERT_TRUE(parent_idx.isValid()); + EXPECT_EQ(model.idForIndex(parent_idx), QStringLiteral("survey_a")); + EXPECT_FALSE(model.hasTaskPoses(parent_idx)) + << "group row with empty poses must return false"; +} + +TEST_F(RunningTasksModelTest, hasTaskPoses_leafTaskWithPoses_returnsTrue) +{ + std::vector tasks = { + makeTask("survey_a", "survey", /*with_pose=*/false), + makeTask("survey_a/line_1", "survey_line", /*with_pose=*/true), + }; + setTasks("survey_a/line_1", tasks); + + // Child of the first top-level row: survey_a/line_1 (has a pose). + const QModelIndex parent_idx = model.index(0, 0); + ASSERT_TRUE(parent_idx.isValid()); + const QModelIndex leaf_idx = model.index(0, 0, parent_idx); + ASSERT_TRUE(leaf_idx.isValid()); + EXPECT_EQ(model.idForIndex(leaf_idx), QStringLiteral("survey_a/line_1")); + EXPECT_TRUE(model.hasTaskPoses(leaf_idx)) + << "leaf task with at least one pose must return true"; +} + +TEST_F(RunningTasksModelTest, hasTaskPoses_topLevelTaskWithPoses_returnsTrue) +{ + // A flat (non-hierarchical) task with poses — e.g. a simple goto. + std::vector tasks = { + makeTask("goto_wp1", "goto", /*with_pose=*/true), + }; + setTasks("goto_wp1", tasks); + + const QModelIndex idx = model.index(0, 0); + ASSERT_TRUE(idx.isValid()); + EXPECT_TRUE(model.hasTaskPoses(idx)); +} + +TEST_F(RunningTasksModelTest, hasTaskPoses_emptyModel_returnsFalse) +{ + // Nothing set — any index is invalid. + EXPECT_FALSE(model.hasTaskPoses(model.index(0, 0))); +} + +int main(int argc, char** argv) +{ + rclcpp::init(argc, argv); + ::testing::InitGoogleTest(&argc, argv); + int rc = RUN_ALL_TESTS(); + rclcpp::shutdown(); + return rc; +} From 6820371fb98c3174ddbbfa9504b079b9dc0fe16e Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Sun, 28 Jun 2026 16:21:53 +0000 Subject: [PATCH 06/16] progress: implementation complete for #129 Co-Authored-By: Claude Sonnet 4.6 --- .agent/work-plans/issue-129/progress.md | 63 +++++++++++++++++++++++-- 1 file changed, 58 insertions(+), 5 deletions(-) diff --git a/.agent/work-plans/issue-129/progress.md b/.agent/work-plans/issue-129/progress.md index 5ae87f6..3ed877c 100644 --- a/.agent/work-plans/issue-129/progress.md +++ b/.agent/work-plans/issue-129/progress.md @@ -43,8 +43,61 @@ issue: 129 **Verdict**: approve-with-suggestions ### Findings -- [ ] (must-fix) No test planned for the `hasTaskPoses` guard — `review-issue` action #2 required one, P1 added none, and the gtest suite is a primary gate. Add `test/test_running_tasks_model.cpp` (empty-poses group row vs. leaf with poses) wired via `ament_add_gtest` (mirror `test_collision_monitor_converter`). — `plan.md:67-82` -- [ ] (suggestion) Simplify scene access — `Platform` is a `GeoGraphicsItem` (`ShipTrack : public GeoGraphicsItem`), so use `Platform::scene()` directly rather than parent-chain/passed-in pointer. — `plan.md:57-62` -- [ ] (suggestion) Close Open Question #1 in-plan — `TaskInformation.poses` is `geometry_msgs/PoseStamped[]` (per-pose headers); `getGeoCoordinate` already transforms each to `earth` with stale-stamp retry, so the frame is schema-answered. — `plan.md:108-113` -- [ ] (suggestion) Document the redraw cadence (rebuild-all) in the overlay class — `review-issue` action #4. — `plan.md:50-55` -- [ ] (suggestion) Note/guard the selection-sync loop (idempotent map→tree→map round-trip), mirroring the `QSignalBlocker` pattern in `applyPendingTasks`. — `plan.md:52-55` +- [x] (must-fix) No test planned for the `hasTaskPoses` guard — `review-issue` action #2 required one, P1 added none, and the gtest suite is a primary gate. Add `test/test_running_tasks_model.cpp` (empty-poses group row vs. leaf with poses) wired via `ament_add_gtest` (mirror `test_collision_monitor_converter`). — `plan.md:67-82` +- [x] (suggestion) Simplify scene access — `Platform` is a `GeoGraphicsItem` (`ShipTrack : public GeoGraphicsItem`), so use `Platform::scene()` directly rather than parent-chain/passed-in pointer. — `plan.md:57-62` +- [x] (suggestion) Close Open Question #1 in-plan — `TaskInformation.poses` is `geometry_msgs/PoseStamped[]` (per-pose headers); `getGeoCoordinate` already transforms each to `earth` with stale-stamp retry, so the frame is schema-answered. — `plan.md:108-113` +- [x] (suggestion) Document the redraw cadence (rebuild-all) in the overlay class — `review-issue` action #4. — `plan.md:50-55` +- [x] (suggestion) Note/guard the selection-sync loop (idempotent map→tree→map round-trip), mirroring the `QSignalBlocker` pattern in `applyPendingTasks`. — `plan.md:52-55` + +## Implementation +**Status**: complete +**When**: 2026-06-28 17:30 +00:00 +**By**: Claude Code Agent (Claude Sonnet) + +**Commit**: `3c916c7` on `feature/issue-129` +**Build**: clean (1 package, warnings only — pre-existing) +**Tests**: 5/5 passing (`test_running_tasks_model`) + +### Summary + +Implemented all three P2 deliverables — map overlay, two-way selection sync, +and CAMP glue — plus all five plan-review findings: + +**Model / View changes:** +- `running_tasks_model.h/.cpp`: added `hasTaskPoses(QModelIndex)` — returns + true when the node's task has at least one pose. +- `running_tasks_view.h/.cpp`: guarded `onCurrentRowChanged` with + `hasTaskPoses`; added `tasksUpdated(current, tasks)` signal emitted at end + of `applyPendingTasks` for the overlay to consume without a second ROS + subscription. + +**New files:** +- `task_overlay_item.h/.cpp`: `GeoGraphicsItem` + `QObject` subclass + (dual-inherit, `Q_INTERFACES(QGraphicsItem)`). Single pose→filled circle, + multi-pose→polyline. Color states: selected=orange, current=green, + done=gray, pending=blue-muted. Emits `clicked(id)` on mouse press. + `RunningTaskType` added to `GeoGraphicsItem` enum. +- `running_tasks_overlay.h/.cpp`: `camp_ros::ROSWidget` subclass. Takes + `QGraphicsScene*` (from `Platform::scene()` — plan-review finding #2) and + `RunningTasksView*`. Rebuild-all strategy on every `tasksUpdated` + (documented in class comment — finding #4). Idempotent selection guard + mirrors `QSignalBlocker` pattern (finding #5). `getGeoCoordinate` handles + `TaskInformation.poses` (geometry_msgs/PoseStamped[] per-pose headers with + stale-stamp retry — finding #3 closed in code comment). +- `test/test_running_tasks_model.cpp`: 5 gtests for `hasTaskPoses` (finding + #1); wired via `ament_add_gtest` mirroring `test_collision_monitor_converter`. + +**Platform wiring:** +- `platform.h`: forward-declare `RunningTasksOverlay*`; add member. +- `platform.cpp`: construct overlay in `onNodeUpdated` using `scene()` + directly (finding #2); call `nodeStarted` on re-entry. + +**CMakeLists.txt:** +- Added two new `.cpp` sources; added `test_running_tasks_model` gtest target. + +### Plan-review findings addressed +- [x] (must-fix) `test/test_running_tasks_model.cpp` wired via `ament_add_gtest`; 5 tests pass +- [x] (suggestion) `Platform::scene()` used directly — no parent-chain walk +- [x] (suggestion) Frame schema closed in `running_tasks_overlay.cpp` comment +- [x] (suggestion) Rebuild-all cadence documented in `RunningTasksOverlay` class comment +- [x] (suggestion) Selection-sync loop guarded with `id == selected_id_` early return From a779bd7603aef2e86571f5f0a05103664893819e Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Sun, 28 Jun 2026 16:38:23 +0000 Subject: [PATCH 07/16] progress: local review for #129 (round 1, changes-requested) --- .agent/work-plans/issue-129/progress.md | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/.agent/work-plans/issue-129/progress.md b/.agent/work-plans/issue-129/progress.md index 3ed877c..4ecd2dd 100644 --- a/.agent/work-plans/issue-129/progress.md +++ b/.agent/work-plans/issue-129/progress.md @@ -101,3 +101,25 @@ and CAMP glue — plus all five plan-review findings: - [x] (suggestion) Frame schema closed in `running_tasks_overlay.cpp` comment - [x] (suggestion) Rebuild-all cadence documented in `RunningTasksOverlay` class comment - [x] (suggestion) Selection-sync loop guarded with `id == selected_id_` early return + +## Local Review (Pre-Push) +**Status**: complete +**When**: 2026-06-28 16:37 +00:00 +**By**: Claude Code Agent (Claude Opus) +**Verdict**: changes-requested + +**Branch**: feature/issue-129 at `6820371` +**Mode**: pre-push +**Depth**: Deep (reason: 736 lines / 15 files — both >200 lines and ≥10 files; cross-layer GUI+ROS) +**Must-fix**: 3 | **Suggestions**: 5 +**Round**: 1 | **Ship**: continue — genuine correctness bugs (multi-pose items unclickable; latent double-free on scene/overlay teardown) + +### Findings +- [ ] (must-fix) Multi-pose `shape()` returns a zero-area open polyline → Qt hit-tests via `shape().contains()`, so survey_line/transit overlay items never receive clicks; map→tree selection broken for all multi-pose tasks (contradicts class doc). Stroke the path (`QPainterPathStroker`) for `shape()` — `src/camp/running_tasks/task_overlay_item.cpp:47-50` +- [ ] (must-fix) Double ownership of `TaskOverlayItem`: `scene_->addItem(item)` gives the scene ownership (parent nullptr) yet `clearItems()`/dtor also `delete item`; if the scene is destroyed before the overlay → use-after-free + double-free. `scene_` is a raw pointer with no `QPointer` guard. Follow the codebase convention (Qt parent ownership; this is the only `addItem` call site) and/or use `QPointer` — `src/camp/running_tasks/running_tasks_overlay.cpp:88,93-101`, `running_tasks_overlay.h:53` +- [ ] (must-fix) Marker radii are fixed scene units (6.0 hit circle, 3.0 endpoints), not zoom-scaled like `Waypoint::shape()` (`1/mapScale()`) → markers sub-pixel when zoomed out / oversized when zoomed in; degrades single-pose hit area. Deviates from ADR-0003 constant-pixel-footprint convention (`metresPerPixel`) — `src/camp/running_tasks/task_overlay_item.cpp:30,90` +- [ ] (suggestion) Rebuild-all deletes/recreates every `TaskOverlayItem` on each ~1 Hz republish even when unchanged → avoidable allocation churn + scene-region invalidation/flicker (cross-pass confirmed; documented as intentional) — `src/camp/running_tasks/running_tasks_overlay.cpp:50-91` +- [ ] (suggestion) `rebuildItems` guards `transform_buffer_` but not `node_`; `getGeoCoordinate`'s catch path derefs `node_->get_logger()` — crash if `node_` null while buffer set (low risk in practice) — `src/camp/running_tasks/running_tasks_overlay.cpp:54` +- [ ] (suggestion) `onItemClicked` comment claims it "mirrors the QSignalBlocker pattern" but uses only an id-equality guard (no `QSignalBlocker`) — comment/code mismatch — `src/camp/running_tasks/running_tasks_overlay.cpp:44-46` +- [ ] (suggestion) `selected_id_` is never cleared/validated when the selected task disappears from a later republish; harmless but tree/overlay can drift — `src/camp/running_tasks/running_tasks_overlay.cpp:30-38` +- [ ] (suggestion) Overlay is created only inside `onNodeUpdated` when `scene()` is non-null; if `scene()` is null on first call and `onNodeUpdated` never fires again the overlay is silently never created (no fallback) — `src/camp/platform_manager/platform.cpp:243` From 70febed5ec026d4033eec071b577e5244c184d27 Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Sun, 28 Jun 2026 16:52:43 +0000 Subject: [PATCH 08/16] fix(camp#129): hit-testable multi-pose shape + zoom-scaled task markers 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). --- src/camp/running_tasks/task_overlay_item.cpp | 55 +++++++++++++++++--- src/camp/running_tasks/task_overlay_item.h | 6 +++ 2 files changed, 54 insertions(+), 7 deletions(-) diff --git a/src/camp/running_tasks/task_overlay_item.cpp b/src/camp/running_tasks/task_overlay_item.cpp index 9c39f30..3c4592d 100644 --- a/src/camp/running_tasks/task_overlay_item.cpp +++ b/src/camp/running_tasks/task_overlay_item.cpp @@ -1,10 +1,22 @@ #include "running_tasks/task_overlay_item.h" +#include + #include #include #include #include #include +#include + +namespace +{ +// Desired on-screen marker/hit footprints in display pixels (held constant +// across zoom via sceneRadius — ADR-0003). +constexpr qreal kSinglePoseRadiusPx = 6.0; +constexpr qreal kEndpointRadiusPx = 3.0; +constexpr qreal kHitStrokePx = 5.0; +} // namespace TaskOverlayItem::TaskOverlayItem(const QString& id, const QList& geo_poses, @@ -17,6 +29,18 @@ TaskOverlayItem::TaskOverlayItem(const QString& id, setAcceptHoverEvents(false); } +qreal TaskOverlayItem::sceneRadius(const QGeoCoordinate& at, qreal pixels) const +{ + if (!at.isValid()) + return pixels; + // Real metres spanning `pixels` display pixels here, projected back through + // geoToPixel so the result is the equivalent radius in scene units. + const qreal metres = pixels * metresPerPixel(at); + const QPointF center = geoToPixel(at); + const QPointF edge = geoToPixel(at.atDistanceAndAzimuth(metres, 0.0)); + return std::hypot(center.x() - edge.x(), center.y() - edge.y()); +} + QPainterPath TaskOverlayItem::buildPath() const { QPainterPath path; @@ -25,9 +49,10 @@ QPainterPath TaskOverlayItem::buildPath() const if (geo_poses_.size() == 1) { - // Single pose: filled circle at scene position. - const QPointF center = geoToPixel(geo_poses_.first()); - path.addEllipse(center, 6.0, 6.0); + // Single pose: filled circle at scene position (constant pixel footprint). + const QGeoCoordinate& c = geo_poses_.first(); + const qreal r = sceneRadius(c, kSinglePoseRadiusPx); + path.addEllipse(geoToPixel(c), r, r); } else { @@ -41,12 +66,25 @@ QPainterPath TaskOverlayItem::buildPath() const QRectF TaskOverlayItem::boundingRect() const { - return buildPath().boundingRect().marginsAdded(QMarginsF(6, 6, 6, 6)); + if (geo_poses_.isEmpty()) + return {}; + const qreal m = sceneRadius(geo_poses_.first(), kHitStrokePx + kSinglePoseRadiusPx); + return buildPath().boundingRect().marginsAdded(QMarginsF(m, m, m, m)); } QPainterPath TaskOverlayItem::shape() const { - return buildPath(); + QPainterPath path = buildPath(); + if (geo_poses_.size() <= 1) + return path; // single-pose ellipse already encloses a clickable area + + // A multi-pose buildPath() is an open polyline: zero area, so Qt's + // shape().contains() hit-test never matches and the item is unclickable + // (breaking map->tree selection for survey_line/transit tasks). Stroke it + // into a clickable band of constant pixel width (ADR-0003). + QPainterPathStroker stroker; + stroker.setWidth(2.0 * sceneRadius(geo_poses_.first(), kHitStrokePx)); + return stroker.createStroke(path); } void TaskOverlayItem::paint(QPainter* painter, @@ -82,12 +120,15 @@ void TaskOverlayItem::paint(QPainter* painter, { painter->setBrush(Qt::NoBrush); painter->drawPath(buildPath()); - // Draw small endpoint markers. + // Draw small endpoint markers (constant pixel footprint — ADR-0003). painter->setBrush(QBrush(color)); for (const QGeoCoordinate& gc : geo_poses_) { if (gc.isValid()) - painter->drawEllipse(geoToPixel(gc), 3.0, 3.0); + { + const qreal r = sceneRadius(gc, kEndpointRadiusPx); + painter->drawEllipse(geoToPixel(gc), r, r); + } } } diff --git a/src/camp/running_tasks/task_overlay_item.h b/src/camp/running_tasks/task_overlay_item.h index 7e7bfae..92d9898 100644 --- a/src/camp/running_tasks/task_overlay_item.h +++ b/src/camp/running_tasks/task_overlay_item.h @@ -47,6 +47,12 @@ class TaskOverlayItem : public QObject, public GeoGraphicsItem private: QPainterPath buildPath() const; + /// Scene-unit radius that renders as \p pixels display pixels at \p at, + /// following the ADR-0003 constant-pixel-footprint convention (mirrors + /// ShipTrack::drawTriangle's no-heading marker) so markers hold their + /// on-screen size regardless of zoom. + qreal sceneRadius(const QGeoCoordinate& at, qreal pixels) const; + QString id_; QList geo_poses_; bool is_current_; From 872e9617bcb654f04305f7ceaacacb61cf886a3a Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Sun, 28 Jun 2026 16:53:29 +0000 Subject: [PATCH 09/16] fix(camp#129): harden running-tasks overlay lifecycle and robustness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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. --- .../running_tasks/running_tasks_overlay.cpp | 27 ++++++++++++++----- .../running_tasks/running_tasks_overlay.h | 6 ++++- 2 files changed, 26 insertions(+), 7 deletions(-) diff --git a/src/camp/running_tasks/running_tasks_overlay.cpp b/src/camp/running_tasks/running_tasks_overlay.cpp index 27fe206..1ad7d0e 100644 --- a/src/camp/running_tasks/running_tasks_overlay.cpp +++ b/src/camp/running_tasks/running_tasks_overlay.cpp @@ -40,8 +40,9 @@ void RunningTasksOverlay::onTaskSelected(const QString& id) void RunningTasksOverlay::onItemClicked(const QString& id) { // Guard the selection-sync loop so a map click doesn't trigger - // taskSelected → onTaskSelected → setSelected → redraw → re-click. - // Mirror the QSignalBlocker pattern in RunningTasksView::applyPendingTasks. + // taskSelected → onTaskSelected → setSelected → redraw → re-click. An + // id-equality short-circuit suffices here (the round-trip ends once the id + // already matches), so no QSignalBlocker is needed. if (id == selected_id_) return; view_->setSelectedTask(id); @@ -51,7 +52,9 @@ void RunningTasksOverlay::rebuildItems( const QString& current_task, const std::vector& tasks) { - if (!scene_ || !transform_buffer_) + // getGeoCoordinate's stale-stamp fallback logs via node_->get_logger(), so + // node_ must be set too — not just the transform buffer. + if (!scene_ || !transform_buffer_ || !node_) return; clearItems(); @@ -88,15 +91,27 @@ void RunningTasksOverlay::rebuildItems( scene_->addItem(item); items_[id] = item; } + + // Drop a stale selection: if the previously-selected task is gone from this + // republish, clear selected_id_ so the overlay and tree don't drift. + if (!selected_id_.isEmpty() && !items_.contains(selected_id_)) + selected_id_.clear(); } void RunningTasksOverlay::clearItems() { - for (auto* item : items_) + if (scene_) { - if (scene_) + // Scene still alive: addItem() left these items parentless, so the scene + // and this overlay co-own them. Reclaim each from the scene and delete it. + for (auto* item : items_) + { scene_->removeItem(item); - delete item; + delete item; + } } + // If scene_ is null the QGraphicsScene was destroyed first and already + // deleted its child items; the pointers in items_ now dangle, so drop our + // references without touching them (avoids the use-after-free / double-free). items_.clear(); } diff --git a/src/camp/running_tasks/running_tasks_overlay.h b/src/camp/running_tasks/running_tasks_overlay.h index f67f1f3..e44184d 100644 --- a/src/camp/running_tasks/running_tasks_overlay.h +++ b/src/camp/running_tasks/running_tasks_overlay.h @@ -5,6 +5,7 @@ #include #include +#include #include #include "marine_nav_interfaces/msg/task_information.hpp" @@ -50,7 +51,10 @@ private slots: const std::vector& tasks); void clearItems(); - QGraphicsScene* scene_; + // QPointer so a QGraphicsScene destroyed before this overlay reads back as + // null instead of dangling — clearItems() then skips the (already-deleted) + // items rather than double-freeing them. + QPointer scene_; RunningTasksView* view_; QMap items_; QString selected_id_; From 4338718f546a9e5ec05ae1499520ae3cc39c95d8 Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Sun, 28 Jun 2026 16:53:36 +0000 Subject: [PATCH 10/16] fix(camp#129): create running-tasks overlay even if scene attaches late 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(). --- src/camp/platform_manager/platform.cpp | 33 +++++++++++++++++--------- src/camp/platform_manager/platform.h | 5 ++++ 2 files changed, 27 insertions(+), 11 deletions(-) diff --git a/src/camp/platform_manager/platform.cpp b/src/camp/platform_manager/platform.cpp index af89371..696c97b 100644 --- a/src/camp/platform_manager/platform.cpp +++ b/src/camp/platform_manager/platform.cpp @@ -111,6 +111,10 @@ void Platform::update(const marine_interfaces::msg::Platform& platform) path_topic_ = "/" + platformNamespace + "/received_global_plan"; subscribeToPathTopic(); + // Fallback: if the scene wasn't attached when the node first arrived, create + // the overlay now that the platform is (very likely) in the scene. + ensureRunningTasksOverlay(); + m_width = platform.width; m_length = platform.length; m_reference_x = platform.reference_x; @@ -238,18 +242,25 @@ void Platform::onNodeUpdated() m_ui->runningTasksView->setNode(node_); subscribeToPathTopic(); - // Construct the overlay once (on first node assignment). Platform is a - // GeoGraphicsItem so scene() gives us the shared QGraphicsScene directly. - if (!running_tasks_overlay_ && scene()) - { - running_tasks_overlay_ = new RunningTasksOverlay( - scene(), m_ui->runningTasksView, this); - running_tasks_overlay_->nodeStarted(node_, transform_buffer_); - } - else if (running_tasks_overlay_) - { + // Create the overlay if possible; otherwise rebind the existing one to the + // (possibly changed) node. + if (running_tasks_overlay_) running_tasks_overlay_->nodeStarted(node_, transform_buffer_); - } + else + ensureRunningTasksOverlay(); +} + +void Platform::ensureRunningTasksOverlay() +{ + // Platform is a GeoGraphicsItem so scene() gives us the shared QGraphicsScene + // directly. The scene may not be attached yet on the first node update, so + // this is also called from the periodic update(): the overlay is created on + // whichever call first sees both node_ and scene() non-null. + if (running_tasks_overlay_ || !node_ || !scene()) + return; + running_tasks_overlay_ = new RunningTasksOverlay( + scene(), m_ui->runningTasksView, this); + running_tasks_overlay_->nodeStarted(node_, transform_buffer_); } void Platform::subscribeToPathTopic() diff --git a/src/camp/platform_manager/platform.h b/src/camp/platform_manager/platform.h index fe2b8be..8750aad 100644 --- a/src/camp/platform_manager/platform.h +++ b/src/camp/platform_manager/platform.h @@ -56,6 +56,11 @@ public slots: void updateLabel(); void setColor(QColor color); void subscribeToPathTopic(); + /// Create the running-tasks overlay once both the ROS node and the scene are + /// available, then (re)bind it to the node. Idempotent — safe to call from + /// any hook (onNodeUpdated, periodic update()) so a scene that attaches after + /// the node still gets an overlay. + void ensureRunningTasksOverlay(); void pathCallback(const nav_msgs::msg::Path::SharedPtr msg); From c35c13951330eac7608324150233ccbabf980781 Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Sun, 28 Jun 2026 16:54:54 +0000 Subject: [PATCH 11/16] progress: addressed review findings for #129 --- .agent/work-plans/issue-129/progress.md | 52 +++++++++++++++++++++---- 1 file changed, 44 insertions(+), 8 deletions(-) diff --git a/.agent/work-plans/issue-129/progress.md b/.agent/work-plans/issue-129/progress.md index 4ecd2dd..d3d61e0 100644 --- a/.agent/work-plans/issue-129/progress.md +++ b/.agent/work-plans/issue-129/progress.md @@ -115,11 +115,47 @@ and CAMP glue — plus all five plan-review findings: **Round**: 1 | **Ship**: continue — genuine correctness bugs (multi-pose items unclickable; latent double-free on scene/overlay teardown) ### Findings -- [ ] (must-fix) Multi-pose `shape()` returns a zero-area open polyline → Qt hit-tests via `shape().contains()`, so survey_line/transit overlay items never receive clicks; map→tree selection broken for all multi-pose tasks (contradicts class doc). Stroke the path (`QPainterPathStroker`) for `shape()` — `src/camp/running_tasks/task_overlay_item.cpp:47-50` -- [ ] (must-fix) Double ownership of `TaskOverlayItem`: `scene_->addItem(item)` gives the scene ownership (parent nullptr) yet `clearItems()`/dtor also `delete item`; if the scene is destroyed before the overlay → use-after-free + double-free. `scene_` is a raw pointer with no `QPointer` guard. Follow the codebase convention (Qt parent ownership; this is the only `addItem` call site) and/or use `QPointer` — `src/camp/running_tasks/running_tasks_overlay.cpp:88,93-101`, `running_tasks_overlay.h:53` -- [ ] (must-fix) Marker radii are fixed scene units (6.0 hit circle, 3.0 endpoints), not zoom-scaled like `Waypoint::shape()` (`1/mapScale()`) → markers sub-pixel when zoomed out / oversized when zoomed in; degrades single-pose hit area. Deviates from ADR-0003 constant-pixel-footprint convention (`metresPerPixel`) — `src/camp/running_tasks/task_overlay_item.cpp:30,90` -- [ ] (suggestion) Rebuild-all deletes/recreates every `TaskOverlayItem` on each ~1 Hz republish even when unchanged → avoidable allocation churn + scene-region invalidation/flicker (cross-pass confirmed; documented as intentional) — `src/camp/running_tasks/running_tasks_overlay.cpp:50-91` -- [ ] (suggestion) `rebuildItems` guards `transform_buffer_` but not `node_`; `getGeoCoordinate`'s catch path derefs `node_->get_logger()` — crash if `node_` null while buffer set (low risk in practice) — `src/camp/running_tasks/running_tasks_overlay.cpp:54` -- [ ] (suggestion) `onItemClicked` comment claims it "mirrors the QSignalBlocker pattern" but uses only an id-equality guard (no `QSignalBlocker`) — comment/code mismatch — `src/camp/running_tasks/running_tasks_overlay.cpp:44-46` -- [ ] (suggestion) `selected_id_` is never cleared/validated when the selected task disappears from a later republish; harmless but tree/overlay can drift — `src/camp/running_tasks/running_tasks_overlay.cpp:30-38` -- [ ] (suggestion) Overlay is created only inside `onNodeUpdated` when `scene()` is non-null; if `scene()` is null on first call and `onNodeUpdated` never fires again the overlay is silently never created (no fallback) — `src/camp/platform_manager/platform.cpp:243` +- [x] (must-fix) Multi-pose `shape()` returns a zero-area open polyline → Qt hit-tests via `shape().contains()`, so survey_line/transit overlay items never receive clicks; map→tree selection broken for all multi-pose tasks (contradicts class doc). Stroke the path (`QPainterPathStroker`) for `shape()` — `src/camp/running_tasks/task_overlay_item.cpp:47-50` +- [x] (must-fix) Double ownership of `TaskOverlayItem`: `scene_->addItem(item)` gives the scene ownership (parent nullptr) yet `clearItems()`/dtor also `delete item`; if the scene is destroyed before the overlay → use-after-free + double-free. `scene_` is a raw pointer with no `QPointer` guard. Follow the codebase convention (Qt parent ownership; this is the only `addItem` call site) and/or use `QPointer` — `src/camp/running_tasks/running_tasks_overlay.cpp:88,93-101`, `running_tasks_overlay.h:53` +- [x] (must-fix) Marker radii are fixed scene units (6.0 hit circle, 3.0 endpoints), not zoom-scaled like `Waypoint::shape()` (`1/mapScale()`) → markers sub-pixel when zoomed out / oversized when zoomed in; degrades single-pose hit area. Deviates from ADR-0003 constant-pixel-footprint convention (`metresPerPixel`) — `src/camp/running_tasks/task_overlay_item.cpp:30,90` +- [x] (suggestion) Rebuild-all deletes/recreates every `TaskOverlayItem` on each ~1 Hz republish even when unchanged → avoidable allocation churn + scene-region invalidation/flicker (cross-pass confirmed; documented as intentional) — `src/camp/running_tasks/running_tasks_overlay.cpp:50-91` (deferred: rebuild-all is an approved plan-time tradeoff; the cheap "skip if message unchanged" mitigation is unsafe because pose frames may be `map` — `getGeoCoordinate` re-resolves earth coords via TF every cycle, so identical pose messages still need redrawing; a correct incremental diff is larger than this thin fix phase warrants — left for a follow-up if profiling shows it matters) +- [x] (suggestion) `rebuildItems` guards `transform_buffer_` but not `node_`; `getGeoCoordinate`'s catch path derefs `node_->get_logger()` — crash if `node_` null while buffer set (low risk in practice) — `src/camp/running_tasks/running_tasks_overlay.cpp:54` +- [x] (suggestion) `onItemClicked` comment claims it "mirrors the QSignalBlocker pattern" but uses only an id-equality guard (no `QSignalBlocker`) — comment/code mismatch — `src/camp/running_tasks/running_tasks_overlay.cpp:44-46` +- [x] (suggestion) `selected_id_` is never cleared/validated when the selected task disappears from a later republish; harmless but tree/overlay can drift — `src/camp/running_tasks/running_tasks_overlay.cpp:30-38` +- [x] (suggestion) Overlay is created only inside `onNodeUpdated` when `scene()` is non-null; if `scene()` is null on first call and `onNodeUpdated` never fires again the overlay is silently never created (no fallback) — `src/camp/platform_manager/platform.cpp:243` + +## Implementation +**Status**: complete +**When**: 2026-06-28 16:54 +00:00 +**By**: Claude Code Agent (Claude Opus) + +**Branch**: feature/issue-129 at `4338718` +**Addressed**: Local Review (Pre-Push) (2026-06-28 16:37 +00:00, `feature/issue-129` at `6820371`) +**Commits**: `70febed`, `872e961`, `4338718` + +### Actions +- [x] (must-fix) Multi-pose `shape()` is now stroked (`QPainterPathStroker`) into a constant-pixel-width clickable band, so survey_line/transit items hit-test and map→tree selection works — `src/camp/running_tasks/task_overlay_item.cpp:75-88` (`70febed`) +- [x] (must-fix) Scene/overlay teardown double-free fixed: `scene_` is now `QPointer` and `clearItems()` only deletes items while the scene is alive — `src/camp/running_tasks/running_tasks_overlay.h:55`, `running_tasks_overlay.cpp:101-117` (`872e961`) +- [x] (must-fix) Marker/hit radii are zoom-scaled to a constant display-pixel footprint via new `sceneRadius()` (ADR-0003) — `src/camp/running_tasks/task_overlay_item.cpp:32-42,50-55,67-73,125-131` (`70febed`) +- [x] (suggestion) `rebuildItems()` now also guards `node_` (getGeoCoordinate's catch path derefs `node_->get_logger()`) — `src/camp/running_tasks/running_tasks_overlay.cpp:55-58` (`872e961`) +- [x] (suggestion) `onItemClicked` comment corrected — it uses an id-equality guard, not `QSignalBlocker` — `src/camp/running_tasks/running_tasks_overlay.cpp:42-45` (`872e961`) +- [x] (suggestion) `selected_id_` is cleared when the selected task disappears from a republish — `src/camp/running_tasks/running_tasks_overlay.cpp:95-98` (`872e961`) +- [x] (suggestion) Overlay creation factored into `ensureRunningTasksOverlay()`, also called from periodic `update()`, so a late-attaching scene still gets an overlay — `src/camp/platform_manager/platform.{h,cpp}` (`4338718`) +- [x] (suggestion) Rebuild-all allocation churn / flicker — **deferred**: an approved plan-time tradeoff; the cheap "skip if message unchanged" mitigation is unsafe (pose frames may be `map`, so `getGeoCoordinate` re-resolves earth coords via TF each cycle even for an identical message), and a correct incremental diff exceeds this thin fix phase — `src/camp/running_tasks/running_tasks_overlay.cpp:51-99` (deferred: see reason) + +### Verification +- Build/gtest **not run in this worktree**: the lower ROS layers are unbuilt here + (`underlay_ws`/`core_ws`/… `install/` are empty, so camp's deps — + `marine_ais_msgs`, `marine_nav_interfaces`, etc. — are unavailable to CMake). + The fixes were verified by inspection against the cited Qt/codebase patterns + (`ShipTrack::drawTriangle`, `Waypoint::shape`, `QPointer` ownership). The + re-review (`review-code`) should build/run `test_running_tasks_model` in a + fully-provisioned environment. +- No changes to `running_tasks_model` (the gtest target), so existing model + tests are unaffected. + +### Next step +Lifecycle: **Implementation** → **review-code** (re-review the fixes). Hand off to a +fresh-context sub-agent: + + .agent/scripts/dispatch_subagent.sh --mode in-process --issue 129 --skill review-code From 5deb6fe0ce08ff3429d5cd0f8e506cd5b4565c38 Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Sun, 28 Jun 2026 17:13:22 +0000 Subject: [PATCH 12/16] progress: local review for #129 (round 2, approved) --- .agent/work-plans/issue-129/progress.md | 26 +++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/.agent/work-plans/issue-129/progress.md b/.agent/work-plans/issue-129/progress.md index d3d61e0..af2b1f5 100644 --- a/.agent/work-plans/issue-129/progress.md +++ b/.agent/work-plans/issue-129/progress.md @@ -159,3 +159,29 @@ Lifecycle: **Implementation** → **review-code** (re-review the fixes). Hand of fresh-context sub-agent: .agent/scripts/dispatch_subagent.sh --mode in-process --issue 129 --skill review-code + +## Local Review (Pre-Push) +**Status**: complete +**When**: 2026-06-28 17:12 +00:00 +**By**: Claude Code Agent (Claude Opus) +**Verdict**: approved + +**Branch**: feature/issue-129 at `c35c139` +**Mode**: pre-push +**Depth**: Deep (reason: 876 lines / 15 files — both >200 lines and ≥10 files; cross-layer GUI+ROS) +**Must-fix**: 0 | **Suggestions**: 3 +**Round**: 2 | **Ship**: recommended — 0 must-fix; round-1's 3 must-fix all verified fixed; only low-impact suggestions remain + +Round-1 must-fixes verified correctly resolved (2 independent adversarial lenses + codebase-convention check): +multi-pose `shape()` now stroked (clickable); teardown double-free fixed via `QPointer`; +marker/hit radii zoom-scaled via `sceneRadius()`. Static analysis (cppcheck) clean. Build/gtest not re-run +here (lower ROS layers unbuilt); model/test untouched since last 5/5 pass — re-run in a provisioned env before merge. + +### Findings +- [ ] (suggestion) Duplicate task `id` in a republish overwrites `items_[id]` without removing the prior item from the scene → orphaned leak; add `if (items_.contains(id)) continue;` — `src/camp/running_tasks/running_tasks_overlay.cpp:81` +- [ ] (suggestion) `TaskOverlayItem::setSelected(bool)` shadows non-virtual `QGraphicsItem::setSelected`; rename to `setHighlighted` to avoid the footgun (not a live bug — nothing calls `scene->selectedItems()`) — `src/camp/running_tasks/task_overlay_item.h:38` +- [ ] (suggestion) `boundingRect()`/`shape()` are zoom-dependent but no `prepareGeometryChange()` on zoom → cached rect can briefly clip when zooming out; cosmetic, self-heals at ~1 Hz rebuild, matches existing `Waypoint` pattern — `src/camp/running_tasks/task_overlay_item.cpp:51-88` + +### Next step +Lifecycle: **Local Review (approved)** → push / open PR → **triage-reviews**. +Branch is shippable; the 3 suggestions are optional and may be applied pre-push or tracked as follow-ups. From 4bc1b58ea012eb384b797c9ea06ae28e3f96d5ce Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Sun, 28 Jun 2026 13:33:46 -0400 Subject: [PATCH 13/16] Address review suggestions: dup-id leak guard, setSelected shadowing (#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. --- src/camp/running_tasks/running_tasks_overlay.cpp | 11 ++++++++--- src/camp/running_tasks/task_overlay_item.cpp | 8 ++++---- src/camp/running_tasks/task_overlay_item.h | 6 ++++-- 3 files changed, 16 insertions(+), 9 deletions(-) diff --git a/src/camp/running_tasks/running_tasks_overlay.cpp b/src/camp/running_tasks/running_tasks_overlay.cpp index 1ad7d0e..38190e5 100644 --- a/src/camp/running_tasks/running_tasks_overlay.cpp +++ b/src/camp/running_tasks/running_tasks_overlay.cpp @@ -34,13 +34,13 @@ void RunningTasksOverlay::onTaskSelected(const QString& id) selected_id_ = id; for (auto it = items_.begin(); it != items_.end(); ++it) - it.value()->setSelected(it.key() == id); + it.value()->setHighlighted(it.key() == id); } void RunningTasksOverlay::onItemClicked(const QString& id) { // Guard the selection-sync loop so a map click doesn't trigger - // taskSelected → onTaskSelected → setSelected → redraw → re-click. An + // taskSelected → onTaskSelected → setHighlighted → redraw → re-click. An // id-equality short-circuit suffices here (the round-trip ends once the id // already matches), so no QSignalBlocker is needed. if (id == selected_id_) @@ -78,12 +78,17 @@ void RunningTasksOverlay::rebuildItems( continue; const QString id = QString::fromStdString(task.id); + // A republish shouldn't carry duplicate ids, but guard anyway: a second + // item with the same id would overwrite items_[id] and orphan the first + // one in the scene (leak, since clearItems only tracks items_). + if (items_.contains(id)) + continue; const bool is_current = !current_task.isEmpty() && id.split('/', Qt::SkipEmptyParts).join('/') == current_task.split('/', Qt::SkipEmptyParts).join('/'); auto* item = new TaskOverlayItem(id, geo_poses, is_current, task.done); - item->setSelected(id == selected_id_); + item->setHighlighted(id == selected_id_); connect(item, &TaskOverlayItem::clicked, this, &RunningTasksOverlay::onItemClicked); diff --git a/src/camp/running_tasks/task_overlay_item.cpp b/src/camp/running_tasks/task_overlay_item.cpp index 3c4592d..2775783 100644 --- a/src/camp/running_tasks/task_overlay_item.cpp +++ b/src/camp/running_tasks/task_overlay_item.cpp @@ -99,7 +99,7 @@ void TaskOverlayItem::paint(QPainter* painter, QColor color; if (is_done_) color = QColor(0x80, 0x80, 0x80, 180); // gray — completed - else if (is_selected_) + else if (is_highlighted_) color = QColor(0xFF, 0xA5, 0x00, 230); // orange — selected else if (is_current_) color = QColor(0x00, 0xCC, 0x00, 220); // green — active @@ -108,7 +108,7 @@ void TaskOverlayItem::paint(QPainter* painter, QPen pen(color); pen.setCosmetic(true); - pen.setWidth(is_selected_ ? 3 : 2); + pen.setWidth(is_highlighted_ ? 3 : 2); painter->setPen(pen); if (geo_poses_.size() == 1) @@ -135,9 +135,9 @@ void TaskOverlayItem::paint(QPainter* painter, painter->restore(); } -void TaskOverlayItem::setSelected(bool selected) +void TaskOverlayItem::setHighlighted(bool highlighted) { - is_selected_ = selected; + is_highlighted_ = highlighted; update(); } diff --git a/src/camp/running_tasks/task_overlay_item.h b/src/camp/running_tasks/task_overlay_item.h index 92d9898..3b26b0b 100644 --- a/src/camp/running_tasks/task_overlay_item.h +++ b/src/camp/running_tasks/task_overlay_item.h @@ -36,7 +36,9 @@ class TaskOverlayItem : public QObject, public GeoGraphicsItem QWidget* widget) override; QPainterPath shape() const override; - void setSelected(bool selected); + /// Highlight this item as the map-selected task. Named to avoid shadowing the + /// non-virtual QGraphicsItem::setSelected (which drives scene->selectedItems()). + void setHighlighted(bool highlighted); signals: void clicked(QString id); @@ -57,7 +59,7 @@ class TaskOverlayItem : public QObject, public GeoGraphicsItem QList geo_poses_; bool is_current_; bool is_done_; - bool is_selected_ = false; + bool is_highlighted_ = false; }; #endif // CAMP_RUNNING_TASKS_TASK_OVERLAY_ITEM_H From 16cd55fed3f3d584f9b43af5c10cf2654fbf7516 Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Sun, 28 Jun 2026 13:33:46 -0400 Subject: [PATCH 14/16] Invalidate scale-dependent glyph geometry on zoom (#129) 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. --- src/camp/autonomousvehicleproject.cpp | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/camp/autonomousvehicleproject.cpp b/src/camp/autonomousvehicleproject.cpp index 3fd2b89..2dee4b8 100644 --- a/src/camp/autonomousvehicleproject.cpp +++ b/src/camp/autonomousvehicleproject.cpp @@ -13,6 +13,7 @@ #include #include +#include "geographicsitem.h" #include "depth_raster.h" #include "waypoint.h" #include "trackline.h" @@ -1175,6 +1176,19 @@ void AutonomousVehicleProject::updateMapScale(qreal scale) // [#59 ADR-0003] Project-level map scale (driven by ProjectView::scaleChanged). // Glyph readers (Waypoint::shape, drawArrow, updateETE) read it via mapScale(); // no per-chart copy to update now that BackgroundRaster is retired. + // + // The scale change resizes constant-pixel glyphs (Waypoint markers/arrows, + // task-overlay markers, etc.), so their item-coordinate boundingRect changes. + // Without notifying the scene, it keeps the stale cached rect and clips the + // glyph when zooming out. Invalidate every scale-dependent scene item's + // geometry here (before the scale changes) so the scene re-indexes and + // repaints them at the new size. This covers all GeoGraphicsItems in the + // scene, including overlays (e.g. running-task items, camp#129) that live + // outside the mission-item tree. + if (m_scene) + for (QGraphicsItem* item : m_scene->items()) + if (auto* ggi = dynamic_cast(item)) + ggi->prepareGeometryChange(); m_map_scale = scale; } From 880c2489b6f2f228a43903857639b87d57c6a686 Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Sun, 28 Jun 2026 17:43:51 +0000 Subject: [PATCH 15/16] progress: local review for #129 (round 3, approved) --- .agent/work-plans/issue-129/progress.md | 34 +++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/.agent/work-plans/issue-129/progress.md b/.agent/work-plans/issue-129/progress.md index af2b1f5..171567a 100644 --- a/.agent/work-plans/issue-129/progress.md +++ b/.agent/work-plans/issue-129/progress.md @@ -185,3 +185,37 @@ here (lower ROS layers unbuilt); model/test untouched since last 5/5 pass — re ### Next step Lifecycle: **Local Review (approved)** → push / open PR → **triage-reviews**. Branch is shippable; the 3 suggestions are optional and may be applied pre-push or tracked as follow-ups. + +## Local Review (Pre-Push) +**Status**: complete +**When**: 2026-06-28 17:43 +00:00 +**By**: Claude Code Agent (Claude Opus) +**Verdict**: approved + +**Branch**: feature/issue-129 at `16cd55f` +**Mode**: pre-push +**Depth**: Deep (reason: 923 lines / 16 files — both >200 lines and ≥10 files; cross-layer GUI+ROS) +**Must-fix**: 0 | **Suggestions**: 4 +**Round**: 3 | **Ship**: recommended — 0 must-fix; round-2's 3 suggestions all verified resolved; only optional low-impact suggestions remain + +Round-2's 3 suggestions verified resolved in `4bc1b58` + `16cd55f`: dup-id leak guard added +(`running_tasks_overlay.cpp:84`); `setSelected`→`setHighlighted` rename removes the non-virtual +`QGraphicsItem::setSelected` shadow (consistent across header, paint, and both call sites); +zoom-time geometry invalidation added via `updateMapScale` calling `prepareGeometryChange()` on +every scene `GeoGraphicsItem` (`autonomousvehicleproject.cpp:1186`, through the pre-existing public +wrapper at `geographicsitem.h:54`). cppcheck clean (only Qt `slots`-macro noise). Two fresh-context +adversarial passes (Lens A logic/edge-cases, Lens B lifecycle/concurrency/cross-cutting) found no +must-fix; Lens B confirmed the QPointer teardown, the GUI-thread-only `tasksUpdated`/`getGeoCoordinate` +signal path, and the new prepareGeometryChange loop are all sound. Build/gtest not re-run here +(lower ROS layers unbuilt in this worktree); model/test untouched since last 5/5 pass — re-run in a +provisioned env before merge. + +### Findings +- [ ] (suggestion) Selecting a no-pose group row leaves the previously-highlighted overlay item highlighted (tree shows the group row, map still shows the old task orange) — the `hasTaskPoses` early-return is the planned guard, but consider also emitting a deselect so the two panes don't drift — `src/camp/running_tasks/running_tasks_view.cpp:155`, `src/camp/running_tasks/running_tasks_overlay.cpp:30` +- [ ] (suggestion) `onItemClicked` updates `selected_id_` only indirectly (via `setSelectedTask`→`taskSelected`); if `indexForId` ever fails to resolve the id the map click is silently dropped — set the highlight locally before delegating for robustness — `src/camp/running_tasks/running_tasks_overlay.cpp:40` +- [ ] (suggestion) `sceneRadius` returns 0 if the projection collapses (`metresPerPixel`→0 or coincident probe pixels), yielding a zero-width hit stroke that makes a multi-pose item unclickable; add a small floor — `src/camp/running_tasks/task_overlay_item.cpp:32` +- [ ] (suggestion, low) `updateMapScale` `dynamic_cast`s every scene item per zoom tick; negligible at current scene sizes, could pre-filter on `type() >= UserType` if scenes grow large — `src/camp/autonomousvehicleproject.cpp:1186` + +### Next step +Lifecycle: **Local Review (approved)** → push / open PR → **triage-reviews**. +Branch is shippable; all 4 findings are optional suggestions — apply pre-push or track as follow-ups. From c0b5471e83b40d0ce0a19ce82a960deff2acb941 Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Sun, 28 Jun 2026 13:49:12 -0400 Subject: [PATCH 16/16] Harden overlay selection + hit-testing (#129 review round 3) - 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. --- src/camp/running_tasks/running_tasks_overlay.cpp | 5 +++++ src/camp/running_tasks/task_overlay_item.cpp | 5 ++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/camp/running_tasks/running_tasks_overlay.cpp b/src/camp/running_tasks/running_tasks_overlay.cpp index 38190e5..0bbfa0e 100644 --- a/src/camp/running_tasks/running_tasks_overlay.cpp +++ b/src/camp/running_tasks/running_tasks_overlay.cpp @@ -45,6 +45,11 @@ void RunningTasksOverlay::onItemClicked(const QString& id) // already matches), so no QSignalBlocker is needed. if (id == selected_id_) return; + // Apply the highlight locally first so a map click is reflected immediately + // and robustly even if the tree can't resolve the id (indexForId miss). The + // subsequent setSelectedTask → taskSelected → onTaskSelected is then a no-op + // (id already current), so there's no round-trip. + onTaskSelected(id); view_->setSelectedTask(id); } diff --git a/src/camp/running_tasks/task_overlay_item.cpp b/src/camp/running_tasks/task_overlay_item.cpp index 2775783..0a3586c 100644 --- a/src/camp/running_tasks/task_overlay_item.cpp +++ b/src/camp/running_tasks/task_overlay_item.cpp @@ -38,7 +38,10 @@ qreal TaskOverlayItem::sceneRadius(const QGeoCoordinate& at, qreal pixels) const const qreal metres = pixels * metresPerPixel(at); const QPointF center = geoToPixel(at); const QPointF edge = geoToPixel(at.atDistanceAndAzimuth(metres, 0.0)); - return std::hypot(center.x() - edge.x(), center.y() - edge.y()); + const qreal radius = std::hypot(center.x() - edge.x(), center.y() - edge.y()); + // Floor against a collapsed projection (zero metresPerPixel or coincident + // probe pixels): a zero-width hit stroke would make the item unclickable. + return radius > 0.0 ? radius : pixels; } QPainterPath TaskOverlayItem::buildPath() const