diff --git a/.agent/work-plans/issue-142/plan.md b/.agent/work-plans/issue-142/plan.md new file mode 100644 index 0000000..7b4fb5c --- /dev/null +++ b/.agent/work-plans/issue-142/plan.md @@ -0,0 +1,141 @@ +# Plan: Per-layer colormap range override (PR1 — backend + numeric UI) + +## Issue + +https://github.com/rolker/camp/issues/142 + +## Context + +The `RasterGlRenderer` shader's `u_min`/`u_max` uniforms are fed from each +layer's accumulated `data_min_`/`data_max_` (auto-range over all data). A +single outlier (e.g. backscatter band 2: max=925, mean=0.16) collapses the +useful color range to a single shade. + +**Dependencies merged:** +- camp#134 — unified `RasterGlRenderer` with `u_min`/`u_max` uniforms; three + layer callers: `GggsTileLayer`, `SonarLiveCacheLayer`, `RasterLayer`. +- camp#141 — migrated GPU LUT path to `marine_colormap`; `RasterGlRenderer:: + setColormap()` now takes `std::string`; internal `camp::map::ColorMap` deleted. + `marine_colormap::RangeModel` (in `marine_colormap/transfer.hpp`) is available. + +**This branch** (`feature/issue-142`) diverged from camp at camp#134; it does NOT +yet include camp#141 changes. First implementation step is a rebase. + +**camp#138** (live-cache auto-range refold) is NOT yet merged. Both issues +touch `SonarLiveCacheLayer`'s range; they are orthogonal — #138 changes how +`data_min_`/`data_max_` accumulate; this PR adds an override on top that is +applied at render time, transparent to the accumulation logic. + +## Approach + +**This PR is PR1 of a staged plan.** PR2 (separate run) embeds +`marine_colormap_widgets::ColormapLegendWidget` (drag handles). + +1. **Rebase** `feature/issue-142` onto camp `main` (which includes #141). + Resolve any conflicts (the `ColorMap`-type-enum → string-name API change is + the primary conflict surface). + +2. **Add `marine_colormap::RangeModel range_model_`** to each of the three + layer classes. `marine_colormap` is already a linked dependency (camp#141). + +3. **Wire `range_model_` into `renderImage()`** in each layer: replace the + hardcoded `float(data_min_)` / `float(data_max_)` args to + `renderer_.renderToImage(...)` with the model's resolved `lo()`/`hi()`. + In Auto mode the model tracks `data_min_`/`data_max_` via `update_auto()`. + +4. **Keep auto-range methods unchanged** (`resetAutoRange`/`foldAutoRange` in + `SonarLiveCacheLayer`; `tilesReady` fold in `GggsTileLayer`). After each + fold call `range_model_.update_auto(data_min_, data_max_)` so the Auto + resolved range stays current. + +5. **Add public API** to each layer: + - `setRangeOverride(float lo, float hi)` — calls `range_model_.set_manual()`, + persists, re-renders. + - `resetRangeToAuto()` — calls `range_model_.reset()`, persists, re-renders. + - `rangeMode() const` → `marine_colormap::RangeMode` (for tests + PR2 binding). + +6. **Context-menu additions** in each layer's `contextMenu()`, following the + existing colormap/band submenu pattern. Add a "Colormap range" submenu with: + - "Set range…" → two sequential `QInputDialog::getDouble()` calls (one for lo, + one for hi, pre-filled with current `lo()`/`hi()`); calls `setRangeOverride`. + - "Reset to auto" → calls `resetRangeToAuto()`. + The submenu is only added for scalar layers (gate on the same condition as the + colormap submenu; `RasterLayer` already gates on `is_scalar_`). + +7. **Persist/restore** via the existing `settingsKey()` pattern in each layer's + `readSettings()`/`writeSettings()`: + - `"range_mode"` → `"auto"` | `"manual"` + - `"range_min"` → float + - `"range_max"` → float + All under `settings.beginGroup(settingsKey())`. + +8. **Uncertainty band default — deferred.** Band semantics (#104) are not + resolved; GGGS bands are 1-indexed with no type classification. + `SonarLiveCacheLayer` bands are named, but no "uncertainty" naming convention + is established. PR1 delivers the **mechanism** (operator sets quality palette + + manual range); an auto-default by band name is a follow-on after #104. + +9. **Tests** in a new `test/test_range_persist.cpp`, modelled on + `test_gggs_persistence.cpp`. All tests must be GL-free (the range model is + pure data; `renderImage()` is not called): + - `RangeDefaultIsAuto`: fresh `TestableGggsTileLayer` → `rangeMode()` == Auto. + - `SetOverrideSwitchesToManual`: `setRangeOverride(1, 5)` → mode Manual, + `lo() == 1`, `hi() == 5`. + - `RangeRoundTrips`: write Manual [1,5] → `writeSettings()` → fresh layer → + `readSettings()` → mode Manual, lo/hi correct. + - `AutoRoundTrips`: default auto → `writeSettings()` → fresh layer → + `readSettings()` → mode Auto. + - `ResetToAutoRestoresMode`: set manual, reset → mode Auto. + +## Files to Change + +| File | Change | +|------|--------| +| `src/camp_map/raster/gggs_tile_layer.h` | Add `range_model_` field; add `setRangeOverride`/`resetRangeToAuto`/`rangeMode` | +| `src/camp_map/raster/gggs_tile_layer.cpp` | `tilesReady`: call `range_model_.update_auto`; `renderImage`: use `range_model_` lo/hi; add contextMenu range submenu; add persist keys in `readSettings`/`writeSettings` | +| `src/camp_map/ros/live_coverage/sonar_live_cache_layer.h` | Same additions as GggsTileLayer | +| `src/camp_map/ros/live_coverage/sonar_live_cache_layer.cpp` | `foldAutoRange`: call `range_model_.update_auto`; `renderImage`: use `range_model_` lo/hi; add contextMenu range submenu; add persist keys | +| `src/camp_map/raster/raster_layer.h` | Same additions as GggsTileLayer; scalar-only gate | +| `src/camp_map/raster/raster_layer.cpp` | `imageReady`: call `range_model_.update_auto`; `renderImage`: use `range_model_` lo/hi; add contextMenu range submenu; add persist keys | +| `test/test_range_persist.cpp` | New test file for range persist/restore round-trip (GL-free) | +| `CMakeLists.txt` | Add `test_range_persist` gtest target; link `marine_colormap::marine_colormap` | + +## Principles Self-Check + +| Principle | Consideration | +|---|---| +| Improve incrementally | PR1 is backend + numeric UI only; interactive widget is PR2. Both are independently reviewable. | +| Only what's needed | No new class hierarchy; `RangeModel` is the existing type from `marine_colormap`. `QInputDialog` reuses the already-present Qt dialog facility. | +| A change includes its consequences | Tests for persist/restore ship with the PR; manual verification of context-menu is done at host (camp cannot build in-container). | +| Human control and transparency | Default stays Auto; override is explicit; reset-to-auto is always available; persisted state is visible via the "Set range…" pre-fill. | +| Capture decisions, not just implementations | Uncertainty-band deferral is recorded here; camp#138 sequencing is noted; no ADR is required for this PR (range override is a narrowly-scoped operator control, not a new UI topology). | + +## ADR Compliance + +| ADR | Triggered | How addressed | +|---|---|---| +| camp ADR-0007 (RasterFieldSource unified renderer) | Yes | Override flows through `renderToImage(lo, hi)` — the single GPU-path insertion point; no per-layer shader duplication. | +| camp ADR-0008 (marine_colormap LUT bake) | Yes | Override goes to `u_min`/`u_max` only, not into the LUT. `RangeModel.lo()`/`.hi()` are passed as `renderToImage` args exactly as `data_min_`/`data_max_` were before. | +| camp ADR-0005 (stores browser / flat display layers) | Watch | Context-menu additions follow the existing colormap/band submenu pattern; no new menu topology. | +| workspace ADR-0013 (progress.md vocabulary) | Yes | `## Plan Authored` entry written to progress.md. | + +## Consequences + +| If we change... | Also update... | Included in plan? | +|---|---|---| +| `renderImage()` args in all three layers | Tests must still pass without GL (range model is pure data) | Yes | +| `readSettings`/`writeSettings` in all three layers | The persist round-trip test pins the new keys | Yes | +| camp#138 lands after this PR | `foldAutoRange` may change internals; the `range_model_.update_auto` call site will need review at merge | No — sequencing note only | + +## Open Questions + +- **camp#138 sequencing**: If camp#138 is in-flight at implement time, check that + its `foldAutoRange` changes don't conflict with the `range_model_.update_auto` + call site. The override is transparent to accumulation; no functional conflict + is expected, but a diff review is warranted at merge. + +## Estimated Scope + +Two PRs total (#142): this is **PR1** ("Part of #142"). Single PR, +independently reviewable, does NOT close the issue. PR2 (colorbar widget embed) +closes #142. diff --git a/.agent/work-plans/issue-142/progress.md b/.agent/work-plans/issue-142/progress.md new file mode 100644 index 0000000..b74f3e2 --- /dev/null +++ b/.agent/work-plans/issue-142/progress.md @@ -0,0 +1,249 @@ +--- +issue: 142 +--- + +# Issue #142 — Per-layer colormap range override with interactive colorbar legend + +## Issue Review +**Status**: complete +**When**: 2026-06-29 00:00 +00:00 +**By**: Claude Code Agent (Claude Sonnet) + +**Issue**: #142 +**Comment**: (best-effort post follows this entry; not recorded inline) +**Scope verdict**: well-scoped + +### Actions +- [ ] Plan must stage the work: range plumbing + persist (PR 1) before the colorbar widget (PR 2) — confirm staging in plan-task. +- [ ] Add tests for range persist/restore and auto↔manual flag round-trip, modelled on `test_gggs_persistence.cpp`. +- [ ] Record the colorbar widget placement decision (dock / map overlay / layer panel) in a camp ADR or detailed plan section before implementing the widget. +- [ ] Verify no ordering conflict with camp#138 (live-cache auto-range refold) — both touch SonarLiveCacheLayer's range folding; coordinate or sequence accordingly. +- [ ] Confirm SonarLiveCacheLayer is included in the manual-override plumbing (the issue says all three sources; the live-cache layer is the most likely to be missed). + +### Full review + +#### Scope + +**Well-scoped?** Yes, with staging. Two distinct concerns live in this issue: +1. **Range plumbing** — replace auto `data_min_`/`data_max_` with operator-settable values in each `renderImage()` caller, persist via `settingsKey()` (auto/manual flag + explicit min/max). The render API already threads `data_min`/`data_max` through `renderToImage()`, so the override drops in at the three caller sites. This is a clean, reviewable single PR. +2. **Interactive colorbar legend widget** — a new widget (no existing colorbar/legend widget found anywhere in camp's source tree). Draggable min/max handles are non-trivial UI. The issue's author acknowledges this is the heavier lift and asks the plan to determine staging. It should be a separate PR. + +**Right repo?** Yes — camp project repo, correct worktree (`issue-camp-142`). + +**Dependencies**: +- camp#134 (RasterFieldSource consolidation) — prerequisite, already merged; the consolidated `RasterGlRenderer` with `u_min`/`u_max` uniforms is the baseline. +- camp#138 (live-cache auto-range refold) — also touches `SonarLiveCacheLayer` range. If #138 is in-flight, the range override in this issue must not conflict; sequence or coordinate. + +#### Principle Alignment + +| Principle | Status | Notes | +|---|---|---| +| Improve incrementally | Watch | Two large concerns (plumbing + new widget) in one issue. The plan must split into stageable, independently-reviewable PRs — PR 1: range plumbing; PR 2: colorbar widget. | +| Only what's needed | Watch | Draggable colorbar handles are more complex than a numeric override in the context menu. The plan should justify the full drag UX vs. a simpler first step (e.g., context-menu min/max inputs). Operator-decided UX per the issue is acceptable, but scope should be explicit. | +| A change includes its consequences | Action needed | Tests for range persist/restore and the auto↔manual flag must ship with the plumbing PR. Existing `test_gggs_persistence.cpp` is the model. The colorbar widget PR likewise needs widget tests or at minimum manual verification recorded. | +| Capture decisions, not just implementations | Action needed | Colorbar widget placement (dock / map overlay / layer panel) is a new UI topology decision. It should be recorded in a camp ADR or at minimum as a detailed plan section justifying the choice. | +| Human control and transparency | OK | Override is explicit, per-layer, with a reset-to-auto affordance. Persistence keeps operator settings across sessions. Aligns well. | +| Workspace vs. project separation | OK | Change is entirely within the camp project repo; no workspace infra touched. | + +#### ADR Applicability + +| ADR | Triggered | Notes | +|---|---|---| +| camp ADR-0007 (RasterFieldSource) | Yes | Override must flow through `renderToImage(data_min, data_max)` — each layer's `renderImage()` replaces the auto-folded values with the operator's when in manual mode. This is the correct single insertion point per ADR-0007's "one shader / one render path" mandate. | +| camp ADR-0002 (web-mercator scene + layer model) | Watch | Colorbar widget placement must fit the existing layer/panel model. A map-overlay widget that introduces new rendering outside the scene model needs scrutiny. | +| camp ADR-0005 (stores browser + flat display layers) | Watch | Context-menu additions follow the established GggsTileLayer/RasterLayer pattern (colormap submenu, band submenu). Range override entries should follow the same pattern. | +| workspace ADR-0001 (adopt ADRs) | Yes | Colorbar widget placement is a design decision warranting a camp ADR (or at minimum a well-documented plan entry). | +| workspace ADR-0013 (progress.md vocabulary) | Yes | This entry uses `## Issue Review` per ADR-0013. | + +#### Consequences + +- If a colorbar widget is introduced: its placement is a new UI topology decision — record it (see Action above). +- `SonarLiveCacheLayer` must be included in the manual-override plumbing; it is the third source and the most likely to be accidentally excluded given its different code path under `ros/live_coverage/`. +- Persistence key shape: follow `settingsKey()` pattern already established in `GggsTileLayer::readSettings`/`writeSettings` (colormap + band round-trip). Add `range_mode` (`auto`/`manual`), `range_min`, `range_max` values. +- The existing `test_gggs_persistence.cpp` exercises the persist/restore flow — extend it or add a parallel test for range persistence. + +## Issue Review (updated — dependencies merged) +**Status**: complete +**When**: 2026-06-29 12:00 +00:00 +**By**: Claude Code Agent (Claude Sonnet) + +**Issue**: #142 +**Comment**: (best-effort post follows this entry; not recorded inline) +**Scope verdict**: well-scoped + +### Actions +- [ ] Plan must address "active/selected layer" binding: no existing single-layer-selection API connects MapTreeView selection to an external widget; the plan must propose an approach (e.g. signal from MapTreeView on selection change → mainwindow wires colorbar widget to that layer's range model). +- [ ] Uncertainty band default: plan must specify how a layer identifies an "uncertainty" band (e.g. by band name substring, metadata field from RasterBandMeta, or explicit operator action). camp#108 added band selection but not band-type classification; that mechanism needs to be clarified in the plan. +- [ ] Add camp ADR for colorbar widget placement (dock / treeTabs 4th tab / map overlay). Placement decision is recorded in plan; full ADR lands with the widget PR. +- [ ] Ensure range override goes to `u_min`/`u_max` (the shader uniforms) — NOT into the LUT `TransferParams`. camp ADR-0008 Consequences explicitly flags this: "The colorbar/range UI (camp#142) must respect this split." +- [ ] Add tests for range persist/restore and auto↔manual flag round-trip (modelled on `test_gggs_persistence.cpp`). +- [ ] Confirm SonarLiveCacheLayer is covered by the manual-override plumbing (separate code path under `ros/live_coverage/`; most likely to be missed). +- [ ] Verify no ordering conflict with camp#138 (live-cache auto-range refold) — both touch SonarLiveCacheLayer's range; coordinate or sequence accordingly. + +### Updated scope notes (vs. prior review) + +The prior review noted "no existing colorbar/legend widget found anywhere in camp's source tree." That is now superseded: **`marine_colormap_widgets::ColormapLegendWidget` is available and installed** (marine_colormap#7 MERGED). camp embeds this widget — it does NOT build its own. This resolves the prior "Watch: draggable complexity" concern; the widget design is settled. + +The prior "Only what's needed" watch on drag-handle complexity is now **OK** — the widget is a ready dependency, not scope to be built. + +#### Updated Principle Alignment + +| Principle | Status | Notes | +|---|---|---| +| Improve incrementally | Watch | Still two concerns in one issue (range plumbing + widget embed). Plan should stage: PR 1 range plumbing + persist, PR 2 widget embed + binding. With ColormapLegendWidget pre-built, PR 2 is now "wire + place" rather than "design + build + wire", reducing risk. | +| Only what's needed | OK | Widget is pre-built; embedding it is the correct minimal step. Uncertainty band default is concrete operator pain (backscatter band2 Max=925/Mean=0.16 example). | +| A change includes its consequences | Action needed | Tests for range persist/restore and auto↔manual flag round-trip must ship with plumbing PR. Widget binding tests or manual verification record needed with widget PR. | +| Capture decisions, not just implementations | Action needed | Two decisions still need recording: (1) colorbar widget placement in camp's UI topology, (2) uncertainty-band identification mechanism. Both are non-trivial design choices. | +| Human control and transparency | OK | Explicit per-layer override, visible colorbar, reset-to-auto affordance, persistence across sessions. ColormapLegendWidget's UX (`lo ≤ hi` clamp, drag → Manual, double-click → reset) all align. | +| Workspace vs. project separation | OK | Change entirely within camp project repo; marine_colormap_widgets is a project-level dependency. | + +#### Updated ADR Applicability + +| ADR | Triggered | Notes | +|---|---|---| +| camp ADR-0007 (RasterFieldSource) | Yes | Range override flows through `renderToImage(data_min, data_max)` — each layer's `renderImage()` replaces auto-folded values with the operator's when in Manual mode. The three caller sites (GggsTileLayer, SonarLiveCacheLayer, RasterLayer) each need this substitution. | +| camp ADR-0008 (marine_colormap LUT bake) | Yes | **New trigger vs. prior review.** ADR-0008 Consequences explicitly states: "Any future use of gain, contrast, alpha-ramp or below/no-data must go through the shader (or a deliberate shader rewrite), not by baking them into this LUT — otherwise the range the shader applies is double-counted. The colorbar/range UI (camp#142) must respect this split." Range overrides MUST go to `u_min`/`u_max` only. | +| camp ADR-0002 (web-mercator scene + layer model) | Watch | Colorbar widget placement must fit the existing treeTabs / dock model. A new 4th tab in treeTabs is the least-invasive option; a dock or map overlay should be justified in the ADR. | +| camp ADR-0005 (stores browser + flat display layers) | Watch | Context-menu range entries should follow GggsTileLayer/RasterLayer's colormap-submenu pattern. | +| workspace ADR-0001 (adopt ADRs) | Yes | Widget placement and uncertainty-band identification are design decisions needing recorded rationale. | +| workspace ADR-0013 (progress.md vocabulary) | Yes | This entry uses `## Issue Review` per ADR-0013. | + +#### New dependency now available + +`marine_colormap_widgets::ColormapLegendWidget` (marine_colormap#7, installed): +- Owns a `marine_colormap::RangeModel` (Auto/Manual). +- `setDomain(min, max)` — the data extent. +- `updateAuto(min, max)` — no-op in Manual; use for data-driven auto range. +- `reset()` — returns to Auto. +- `rangeChanged(float lo, float hi)` signal → feed to `renderToImage(lo, hi)` to override `u_min`/`u_max`. +- `setPalette(int index)` — wire to the layer's active colormap palette so the legend shows the correct ramp. +- Layers should use `marine_colormap::RangeModel` directly (or delegate to the widget's owned model) for persist; serialize `mode()` + `lo()`/`hi()`. + +#### Uncertainty band default — open design question + +The issue requires: when the operator views an uncertainty band → default colormap = `quality` ramp + a sensible `[0, threshold]` manual range. The band identification mechanism is unspecified: +- `RasterBandMeta` (returned by `GggsTileLayer::metadata(band)`) carries band name and data-type info. If uncertainty bands are consistently named (e.g. "uncertainty", "quality"), name-matching is sufficient. +- The plan must choose: (a) name-match at band-switch time, (b) explicit metadata field, or (c) operator-only (no auto-default). Option (a) is consistent with the existing band-name-driven context-menu pattern and is the least-invasive path. +- The `threshold` is unspecified in the issue; plan should propose a default (e.g. 1.0, or the band's 95th-percentile from metadata if available) and record it. + +## Plan Authored +**Status**: complete +**When**: 2026-06-29 00:00 +00:00 +**By**: Claude Code Agent (Claude Sonnet) + +**Plan**: `.agent/work-plans/issue-142/plan.md` at `faf2374` +**Branch**: feature/issue-142 at `faf2374` +**Phases**: 2 (PR1 this run — backend + numeric UI; PR2 — colorbar widget embed) + +### Open questions +- [ ] camp#138 sequencing: if live-cache auto-range refold lands before PR1 merges, verify the `range_model_.update_auto` call site in `foldAutoRange` does not conflict with #138's fold-logic changes (expected orthogonal, but warrants diff review at merge time). + +## Plan Review +**Status**: complete +**When**: 2026-06-29 18:10 +00:00 +**By**: Claude Code Agent (Claude Opus) + + +**Plan**: `.agent/work-plans/issue-142/plan.md` at `faf2374` +**PR**: PR-less (`--issue` mode) +**Verdict**: approve-with-suggestions + +Plan is well-targeted, correctly staged (PR1 backend+numeric UI; PR2 widget), and +ADR-compliant. File targeting verified against live code: all three layers call +`renderer_.renderToImage(…, data_min_, data_max_, size)` (exact substitution +points); fold sites `tilesReady()`/`foldAutoRange()`/`imageReady()` and +`contextMenu()`/settings groups all present. `RangeModel` API +(`update_auto`/`set_manual`/`reset`/`lo`/`hi`/`mode`) confirmed in +`marine_colormap/transfer.hpp`. ADR-0008 lands with the #141 rebase (step 1); +pre-#141 `map::ColorMap::allTypes()` in `raster_layer.cpp` confirms the rebase is +genuinely required. gh is unauthenticated in this environment — issue body was +read from the two `## Issue Review` entries above rather than live `gh issue view`. + +### Findings +- [ ] (suggestion) Uncertainty-band auto-default (a #142 requirement, flagged in issue review) is deferred to a post-#104 follow-on, but Estimated Scope says "PR2 closes #142" — reconcile: narrow #142's scope + open a follow-on, or keep #142 open past PR2. — `plan.md:75` +- [ ] (suggestion) Persist round-trip tests use only `TestableGggsTileLayer`; the Consequences table claims the test pins keys for all three layers. Add a per-layer round-trip for SonarLiveCacheLayer + RasterLayer, or record manual verification. — `plan.md:78` +- [ ] (suggestion) Step 7 says persist "via `settingsKey()`" but `RasterLayer::read/writeSettings` group under `itemID()`; implementer should use `itemID()` for RasterLayer. — `plan.md:69` + +## Implementation +**Status**: complete +**When**: 2026-06-29 19:30 +00:00 +**By**: Claude Code Agent (Claude Opus) + +**Commit**: `45c07a6` on `feature/issue-142` +**Scope**: PR1 only (backend + numeric override; the colorbar widget is PR2) + +### What landed +- **RangeModel wiring across all three layers.** `GggsTileLayer`, + `SonarLiveCacheLayer` and `RasterLayer` each own a + `marine_colormap::RangeModel range_model_`. `renderImage()` now feeds + `range_model_.lo()/hi()` into `RasterGlRenderer::renderToImage(...)`'s + `u_min`/`u_max` instead of the raw `data_min_`/`data_max_`. After each + auto-range fold — `tilesReady()` (GGGS), `foldAutoRange()` (sonar), + `imageReady()` (raster) — `range_model_.update_auto(data_min_, data_max_)` + keeps Auto current; it is a no-op in Manual mode, so an incoming tile never + disturbs a pinned override. The `update_auto` call sites carry a **camp#138** + coordination note for diff review at merge (the override is applied at render + time, transparent to the fold). +- **Public API per layer**: `setRangeOverride(float lo, float hi)` + (→ `set_manual`, persist, re-render), `resetRangeToAuto()` (→ `reset` then + re-track current extents, persist, re-render), and + `rangeMode()`/`rangeLo()`/`rangeHi()` accessors (for tests + PR2 binding). +- **Context menu**: a "Colormap range" submenu — "Set range…" (two + `QInputDialog::getDouble()` prompts pre-filled with the current lo/hi) and + "Reset to auto" — added under the same scalar gate as the colormap submenu + (unconditional for the two always-scalar tile layers; behind `is_scalar_` for + RasterLayer). +- **Persistence**: `range_mode` ("auto"/"manual") + `range_min`/`range_max` per + layer. GggsTileLayer + SonarLiveCacheLayer group under `settingsKey()`; + **RasterLayer groups under `itemID()`** — the key its existing read/writeSettings + already use (plan-review suggestion `plan.md:69` addressed). +- **CMake**: `marine_colormap` moved PRIVATE → **PUBLIC** on `camp_map`, because a + marine_colormap type (`RangeModel`) now appears in camp_map's public headers + (it was a std::string-only name under camp#141). The only ament-exported target + (`rqt_helm_manager`) does not link camp_map, so this stays internal. + +### Tests (GL-free — RangeModel is pure data; `renderImage()` never called) +`test/test_range_persist.cpp`, wired into `CMakeLists.txt`. Covers **all three +layers** (plan-review suggestion `plan.md:78` addressed): default is Auto; +`setRangeOverride` → Manual + lo/hi; Manual persist→restore round-trip; Auto +persist→restore; reset→Auto. Each layer is driven through a `TestableXxx` +subclass exposing the protected read/writeSettings (the `test_gggs_persistence.cpp` +pattern); empty tile-set dir / never-opened file / null ROS node keep them +GL/GDAL/ROS-free, so they RUN (not SKIP) in-container. + +### Deferred / noted +- **Uncertainty-band auto-default is DEFERRED to camp#145** (needs band semantics, + camp#104) — plan-review finding `plan.md:75`. PR1 delivers only the mechanism; + #142 stays open past PR2 (or is narrowed) per that finding. Not implemented here. + +### Build status +Container cannot build camp (known) — **host verifies**: +`source setup.bash; ./ui_ws/build.sh camp; ./ui_ws/test.sh camp`. Edits made +cleanly; hooks ran on commit (no `--no-verify`). Not pushed. PR1 is **Part of +#142** (does not close it). + +## Local Review (Pre-Push) +**Status**: complete +**When**: 2026-06-29 19:12 +00:00 +**By**: Claude Code Agent (Claude Opus) +**Verdict**: approved + +**Branch**: feature/issue-142 at `7ca8927` +**Mode**: pre-push +**Depth**: Standard (reason: ~600 LOC C++ across three layer classes in two ROS packages + persistence path) +**Must-fix**: 0 | **Suggestions**: 3 +**Round**: 1 | **Ship**: recommended — no must-fix findings; two independent adversarial passes agree the mechanics are correct and ADR-0008-compliant. + +### Findings +- [ ] (suggestion) `*AutoRoundTrips` tests assert only `rangeMode()==Auto`, never `rangeLo()/rangeHi()` — Auto restore extent is unchecked (false-confidence gap) — `test/test_range_persist.cpp:742,812,878` +- [ ] (suggestion) `range_min`/`range_max` read with `0.0`/`1.0` defaults independent of `range_mode`; partially-corrupt settings could silently pin `[0,1]` Manual (benign — keys always co-written) — `raster_layer.cpp:357`, `gggs_tile_layer.cpp:738`, `sonar_live_cache_layer.cpp:799` +- [ ] (suggestion) `QInputDialog::getDouble(..., decimals=4, ...)` quantizes the pre-filled current range on reopen (minor UX) — `gggs_tile_layer.cpp:648`, `raster_layer.cpp:533`, `sonar_live_cache_layer.cpp:743` + +### Governance / Plan +Full plan adherence (all "Files to Change" present, no scope creep); all three plan-review +suggestions addressed (per-layer tests, RasterLayer `itemID()` keying, camp#145 deferral). +ADR-0007 (single `renderToImage` insertion point) and ADR-0008 (override → `u_min`/`u_max`, +not the LUT) both compliant. CMake PRIVATE→PUBLIC linkage correct; no ament export gap +(only `rqt_helm_manager` is exported and it does not link `camp_map`). Static analysis +inconclusive in-container (cppcheck can't parse Qt macros; cpplint absent) — host ament +build is authoritative. diff --git a/CMakeLists.txt b/CMakeLists.txt index 31862ea..12e158d 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -291,13 +291,17 @@ target_link_libraries(camp_map Qt5::Network Qt5::Xml Qt5::Positioning + # [camp#142] marine_colormap was PRIVATE under camp#141 because the colormap + # was only a std::string name with no marine_colormap type in any camp_map + # header. The per-layer range override (camp#142) adds a + # marine_colormap::RangeModel VALUE member to the raster layer headers + # (gggs_tile_layer.h / raster_layer.h / sonar_live_cache_layer.h), so the + # type — and — now appears in camp_map's + # public headers. Link PUBLIC so the include dir + symbols propagate to every + # consumer of those headers (camp_map_ros, the camp executable, the tests). + marine_colormap::marine_colormap PRIVATE ${GDAL_LIBRARY} - # [camp#141] marine_colormap is consumed only inside camp_map's .cpp files - # (the raster LUT bake / CPU sample); no marine_colormap type appears in any - # installed camp_map header (the colormap is a std::string name), so link it - # PRIVATE — it does not propagate into camp_map's public link/usage interface. - marine_colormap::marine_colormap ) install(TARGETS camp_map @@ -781,6 +785,38 @@ if(BUILD_TESTING) Qt5::Core ${GDAL_LIBRARY} ) + + # [camp#142] Per-layer colormap range override persist/restore round-trip across + # ALL THREE raster layers (GggsTileLayer + RasterLayer in camp_map; + # SonarLiveCacheLayer in camp_map_ros). GL-free: the RangeModel is pure data and + # renderImage() is never called — only the public range API + read/writeSettings + # are exercised, so these RUN (not SKIP) in-container. Links camp_map_ros (which + # pulls camp_map + marine_colormap transitively) plus the ROS deps the sonar layer + # header needs, GDAL (RasterLayer's ctor opens GDAL), and the Qt Widgets/Positioning + # the layers + QSettings/QApplication use. + ament_add_gtest(test_range_persist + test/test_range_persist.cpp + ) + target_include_directories(test_range_persist PRIVATE + ${CMAKE_CURRENT_SOURCE_DIR}/src/camp_map + ) + ament_target_dependencies(test_range_persist + geometry_msgs + marine_autonomy + marine_colormap + marine_interfaces + marine_tiled_raster_store + rclcpp + tf2_ros + ) + target_link_libraries(test_range_persist + camp_map_ros + Qt5::Core + Qt5::Gui + Qt5::Widgets + Qt5::Positioning + ${GDAL_LIBRARY} + ) endif() diff --git a/src/camp_map/raster/gggs_tile_layer.cpp b/src/camp_map/raster/gggs_tile_layer.cpp index d5fc091..05e8307 100644 --- a/src/camp_map/raster/gggs_tile_layer.cpp +++ b/src/camp_map/raster/gggs_tile_layer.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include #include #include @@ -318,6 +319,13 @@ void GggsTileLayer::tilesReady() setStatus("(no data)"); else setStatus(""); + // [camp#142] Keep the Auto resolved range current with the freshly-folded + // extents (a no-op while the operator holds a Manual override, so an incoming + // tile never disturbs a pinned range). camp#138 coordination: this is the + // update_auto() call site to review at merge — the override is applied at + // render time and is transparent to this fold. + if(data_min_ <= data_max_) + range_model_.update_auto(float(data_min_), float(data_max_)); update(boundingRect()); } @@ -412,8 +420,10 @@ QImage GggsTileLayer::renderImage(const QSize& size) if(!renderer_.makeCurrent()) return QImage(); const QList draw = items(); - const QImage image = renderer_.renderToImage(draw, scene_bounds_, float(data_min_), - float(data_max_), size); + // [camp#142] Feed the resolved range (Auto tracks data_min_/data_max_; Manual is + // the operator override) into the shader's u_min/u_max instead of the raw extents. + const QImage image = renderer_.renderToImage(draw, scene_bounds_, range_model_.lo(), + range_model_.hi(), size); renderer_.doneCurrent(); return image; } @@ -471,6 +481,28 @@ void GggsTileLayer::setColormap(const std::string& name) update(boundingRect()); } +void GggsTileLayer::setRangeOverride(float lo, float hi) +{ + // [camp#142] Pin the resolved range to the operator's [lo, hi] (set_manual swaps + // an inverted pair so lo() <= hi()). Re-render with the new range + persist. + range_model_.set_manual(lo, hi); + cached_image_ = QImage(); + writeSettings(); + update(boundingRect()); +} + +void GggsTileLayer::resetRangeToAuto() +{ + // [camp#142] Return to data-driven Auto, then immediately re-track the current + // extents so lo()/hi() reflect the data without waiting for the next fold. + range_model_.reset(); + if(data_min_ <= data_max_) + range_model_.update_auto(float(data_min_), float(data_max_)); + cached_image_ = QImage(); + writeSettings(); + update(boundingRect()); +} + int GggsTileLayer::bandCount() const { // [camp#108] The tiles of a store are uniform, so the first valid tile's band @@ -612,6 +644,30 @@ void GggsTileLayer::contextMenu(QMenu* menu) connect(action, &QAction::triggered, this, [this, name]() { setColormap(name); }); } + // [camp#142] Colormap range override. GGGS tiles are always scalar values, so the + // submenu is offered unconditionally (same gating as the Colormap submenu above). + // "Set range…" prompts for lo then hi (pre-filled with the current resolved range) + // and pins a Manual override; "Reset to auto" returns to the data-driven extents. + QMenu* range_menu = menu->addMenu("Colormap range"); + QAction* set_range = range_menu->addAction("Set range…"); + connect(set_range, &QAction::triggered, this, [this]() + { + bool ok = false; + const double lo = QInputDialog::getDouble( + nullptr, "Colormap range", "Minimum:", range_model_.lo(), + -1.0e9, 1.0e9, 6, &ok); + if(!ok) + return; + const double hi = QInputDialog::getDouble( + nullptr, "Colormap range", "Maximum:", range_model_.hi(), + -1.0e9, 1.0e9, 6, &ok); + if(!ok) + return; + setRangeOverride(float(lo), float(hi)); + }); + QAction* reset_range = range_menu->addAction("Reset to auto"); + connect(reset_range, &QAction::triggered, this, [this]() { resetRangeToAuto(); }); + // [camp#108] Band picker — only for multi-band tile-sets (bathy depth + // uncertainty, backscatter intensity + quality). Single-band stores (the // common sidescan case) get no submenu, so no visual noise. One checkable @@ -675,6 +731,15 @@ void GggsTileLayer::readSettings() // when it differs, to skip the abort+reload on the common no-change path, // mirroring the inline colormap apply directly below. const int band = settings.value("band", 1).toInt(); + // [camp#142] Persisted colormap range. "manual" restores the operator override; + // anything else (default "auto") leaves the data-driven Auto range. Honor Manual + // only when both extents are present too — a partial/corrupt entry falls back to + // Auto rather than snapping to the [0,1] read-defaults. + const QString range_mode = settings.value("range_mode", "auto").toString(); + const bool has_manual_range = range_mode == "manual" && + settings.contains("range_min") && settings.contains("range_max"); + const float range_min = settings.value("range_min", 0.0).toFloat(); + const float range_max = settings.value("range_max", 1.0).toFloat(); settings.endGroup(); settings.endGroup(); if(colormap != renderer_.colormap()) @@ -684,6 +749,12 @@ void GggsTileLayer::readSettings() } if(band != band_) applyBand(band); + // Apply the persisted range AFTER the band switch (applyBand resets data_min_/ + // data_max_; a Manual override is independent of the data extents and survives). + if(has_manual_range) + range_model_.set_manual(range_min, range_max); + else + range_model_.reset(); } void GggsTileLayer::writeSettings() @@ -694,6 +765,13 @@ void GggsTileLayer::writeSettings() settings.beginGroup(settingsKey()); settings.setValue("colormap", QString::fromStdString(renderer_.colormap())); settings.setValue("band", band_); // [camp#108] selected band round-trips + // [camp#142] Persist the colormap range mode + bounds so a Manual override (and + // its [lo, hi]) survives a restart; Auto persists as "auto". + settings.setValue("range_mode", + range_model_.mode() == marine_colormap::RangeMode::Manual ? "manual" + : "auto"); + settings.setValue("range_min", range_model_.lo()); + settings.setValue("range_max", range_model_.hi()); settings.endGroup(); settings.endGroup(); } diff --git a/src/camp_map/raster/gggs_tile_layer.h b/src/camp_map/raster/gggs_tile_layer.h index 9996092..5413fbf 100644 --- a/src/camp_map/raster/gggs_tile_layer.h +++ b/src/camp_map/raster/gggs_tile_layer.h @@ -5,6 +5,8 @@ #include "raster_field_source.h" #include "raster_gl_renderer.h" +#include + #include #include #include @@ -88,6 +90,19 @@ class GggsTileLayer: public map::Layer, public RasterFieldSource /// (the tiles of a store are uniform). 0 if there is no valid tile yet. int bandCount() const; + /// [camp#142] Per-layer colormap range override. In Auto mode the resolved + /// range tracks the data extents (data_min_/data_max_, folded in tilesReady()); + /// in Manual mode it is pinned to an operator-chosen [lo, hi] so an outlier band + /// (e.g. backscatter max 925, mean 0.16) can't collapse the useful colour range. + /// The override is applied at render time (fed to the shader's u_min/u_max via + /// renderToImage) and is transparent to the auto-range accumulation. Each setter + /// persists and re-renders. + void setRangeOverride(float lo, float hi); ///< -> Manual [lo, hi] + void resetRangeToAuto(); ///< -> Auto (tracks the data extents) + marine_colormap::RangeMode rangeMode() const { return range_model_.mode(); } + float rangeLo() const { return range_model_.lo(); } ///< current resolved low bound + float rangeHi() const { return range_model_.hi(); } ///< current resolved high bound + /// [camp#108] Select which 1-indexed band the layer renders, then persist it. /// Delegates the band switch to applyBand() and round-trips the selection to /// QSettings. No-op if @p band is out of range or unchanged. @@ -174,6 +189,11 @@ private slots: double data_min_ = 1.0; // auto-range over all tiles (crossed => no data) double data_max_ = 0.0; + // [camp#142] Resolved colormap range (Auto tracks data_min_/data_max_ via + // update_auto() in tilesReady(); Manual pins an operator override). Fed to the + // renderer's u_min/u_max at render time, replacing the raw data_min_/data_max_. + marine_colormap::RangeModel range_model_; + // [camp#102] Async pixel load mirroring RasterLayer (ADR-0003 §3): the cheap // extent list is built in loadDirectory(); the band reads are deferred to a // single QtConcurrent worker driven by this watcher and kicked lazily from the diff --git a/src/camp_map/raster/raster_layer.cpp b/src/camp_map/raster/raster_layer.cpp index adc504e..498ff50 100644 --- a/src/camp_map/raster/raster_layer.cpp +++ b/src/camp_map/raster/raster_layer.cpp @@ -13,6 +13,7 @@ #include #include #include +#include #include #include @@ -341,6 +342,12 @@ void RasterLayer::imageReady() data_max_ = result.data_max; has_nodata_ = result.has_nodata; nodata_ = result.nodata; + // [camp#142] Keep the Auto resolved range current with the loaded scalar extents + // (a no-op while the operator holds a Manual override). Only scalar charts shade + // through the range; RGB charts bypass the LUT, so their crossed default extents + // are left untracked. + if(is_scalar_ && data_min_ <= data_max_) + range_model_.update_auto(data_min_, data_max_); // [#59 ADR-0003] The placement was already applied synchronously in initExtent() // (identical reprojected geotransform). Re-apply defensively only if initExtent() @@ -414,8 +421,11 @@ QImage RasterLayer::renderImage(const QSize& size) if(!renderer_.makeCurrent()) return QImage(); const QList draw = items(); - const QImage image = renderer_.renderToImage(draw, scene_bounds_, data_min_, - data_max_, size); + // [camp#142] Feed the resolved range (Auto tracks data_min_/data_max_; Manual is + // the operator override) into the shader's u_min/u_max. Scalar charts shade + // through it; RGB charts bypass the LUT, so the range is a don't-care for them. + const QImage image = renderer_.renderToImage(draw, scene_bounds_, range_model_.lo(), + range_model_.hi(), size); renderer_.doneCurrent(); return image; } @@ -474,6 +484,28 @@ void RasterLayer::setColormap(const std::string& name) update(boundingRect()); } +void RasterLayer::setRangeOverride(float lo, float hi) +{ + // [camp#142] Pin the resolved range to the operator's [lo, hi] (set_manual swaps + // an inverted pair so lo() <= hi()). Re-render with the new range + persist. + range_model_.set_manual(lo, hi); + cached_image_ = QImage(); + writeSettings(); + update(boundingRect()); +} + +void RasterLayer::resetRangeToAuto() +{ + // [camp#142] Return to data-driven Auto, then re-track the current scalar extents + // so lo()/hi() reflect the data without waiting for a reload. + range_model_.reset(); + if(is_scalar_ && data_min_ <= data_max_) + range_model_.update_auto(data_min_, data_max_); + cached_image_ = QImage(); + writeSettings(); + update(boundingRect()); +} + void RasterLayer::contextMenu(QMenu* menu) { map::Layer::contextMenu(menu); @@ -488,6 +520,30 @@ void RasterLayer::contextMenu(QMenu* menu) action->setChecked(name == renderer_.colormap()); connect(action, &QAction::triggered, this, [this, name]() { setColormap(name); }); } + + // [camp#142] Colormap range override (scalar only — gated by the is_scalar_ early + // return above, the same condition as the Colormap submenu). "Set range…" prompts + // for lo then hi (pre-filled with the current resolved range) and pins a Manual + // override; "Reset to auto" returns to the data-driven extents. + QMenu* range_menu = menu->addMenu("Colormap range"); + QAction* set_range = range_menu->addAction("Set range…"); + connect(set_range, &QAction::triggered, this, [this]() + { + bool ok = false; + const double lo = QInputDialog::getDouble( + nullptr, "Colormap range", "Minimum:", range_model_.lo(), + -1.0e9, 1.0e9, 6, &ok); + if(!ok) + return; + const double hi = QInputDialog::getDouble( + nullptr, "Colormap range", "Maximum:", range_model_.hi(), + -1.0e9, 1.0e9, 6, &ok); + if(!ok) + return; + setRangeOverride(float(lo), float(hi)); + }); + QAction* reset_range = range_menu->addAction("Reset to auto"); + connect(reset_range, &QAction::triggered, this, [this]() { resetRangeToAuto(); }); } void RasterLayer::readSettings() @@ -502,6 +558,17 @@ void RasterLayer::readSettings() "colormap", QString::fromStdString(renderer_.colormap())).toString().toLower().toStdString(); if(!marine_colormap::palette_index(colormap)) colormap = "grayscale"; + // [camp#142] Persisted colormap range (grouped under itemID(), the key this + // layer's read/writeSettings already use — NOT settingsKey()). "manual" restores + // the operator override; anything else (default "auto") leaves Auto. + // Only honor a Manual override when the mode says so AND both extents are + // present — a partial/corrupt entry falls back to Auto rather than snapping to + // the [0,1] read-defaults. + const QString range_mode = settings.value("range_mode", "auto").toString(); + const bool has_manual_range = range_mode == "manual" && + settings.contains("range_min") && settings.contains("range_max"); + const float range_min = settings.value("range_min", 0.0).toFloat(); + const float range_max = settings.value("range_max", 1.0).toFloat(); settings.endGroup(); settings.endGroup(); // Apply the persisted ramp (re-bake + re-render if it differs); don't re-persist. @@ -511,6 +578,12 @@ void RasterLayer::readSettings() cached_image_ = QImage(); update(boundingRect()); } + // [camp#142] Apply the persisted range. A Manual override is independent of the + // data extents and is restored as-is; "auto" leaves the model tracking the data. + if(has_manual_range) + range_model_.set_manual(range_min, range_max); + else + range_model_.reset(); } void RasterLayer::onRemovedFromMap() @@ -530,6 +603,13 @@ void RasterLayer::writeSettings() settings.beginGroup("MapItem"); settings.beginGroup(itemID()); settings.setValue("colormap", QString::fromStdString(renderer_.colormap())); + // [camp#142] Persist the colormap range mode + bounds so a Manual override (and + // its [lo, hi]) survives a restart; Auto persists as "auto". + settings.setValue("range_mode", + range_model_.mode() == marine_colormap::RangeMode::Manual ? "manual" + : "auto"); + settings.setValue("range_min", range_model_.lo()); + settings.setValue("range_max", range_model_.hi()); settings.endGroup(); settings.endGroup(); } diff --git a/src/camp_map/raster/raster_layer.h b/src/camp_map/raster/raster_layer.h index 4224a5c..4949993 100644 --- a/src/camp_map/raster/raster_layer.h +++ b/src/camp_map/raster/raster_layer.h @@ -5,6 +5,8 @@ #include "raster_field_source.h" #include "raster_gl_renderer.h" +#include + #include #include #include @@ -69,6 +71,16 @@ class RasterLayer: public map::Layer, public RasterFieldSource /// The layer's Web-Mercator extent. Exposed for tests. QRectF sceneBounds() const { return scene_bounds_; } + /// [camp#142] Per-layer colormap range override (scalar charts only; the menu + /// gates on is_scalar_). Auto tracks the data extents (data_min_/data_max_, set + /// in imageReady()); Manual pins an operator [lo, hi]. Applied at render time + /// (shader u_min/u_max via renderToImage), transparent to the auto-range. + void setRangeOverride(float lo, float hi); ///< -> Manual [lo, hi] + void resetRangeToAuto(); ///< -> Auto (tracks the data extents) + marine_colormap::RangeMode rangeMode() const { return range_model_.mode(); } + float rangeLo() const { return range_model_.lo(); } + float rangeHi() const { return range_model_.hi(); } + // [camp#134] RasterFieldSource: a single reprojected-chart item for the renderer. QStringList bands() const override; RasterBandMeta metadata(const QString& band) const override; @@ -136,6 +148,10 @@ class RasterLayer: public map::Layer, public RasterFieldSource RasterFieldItem::Format format_ = RasterFieldItem::Format::Scalar; float data_min_ = 1.0f; // scalar colormap range (crossed => no data) float data_max_ = 0.0f; + // [camp#142] Resolved colormap range (Auto tracks data_min_/data_max_ via + // update_auto() in imageReady(); Manual pins an operator override). Fed to the + // renderer's u_min/u_max at render time, replacing the raw data_min_/data_max_. + marine_colormap::RangeModel range_model_; bool has_nodata_ = false; float nodata_ = 0.0f; diff --git a/src/camp_map/ros/live_coverage/sonar_live_cache_layer.cpp b/src/camp_map/ros/live_coverage/sonar_live_cache_layer.cpp index 2a9dd9b..ffc0abf 100644 --- a/src/camp_map/ros/live_coverage/sonar_live_cache_layer.cpp +++ b/src/camp_map/ros/live_coverage/sonar_live_cache_layer.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -477,6 +478,12 @@ void SonarLiveCacheLayer::foldAutoRange() if(first || band->data_max > data_max_) data_max_ = band->data_max; first = false; } + // [camp#142] Keep the Auto resolved range current with the freshly-folded extents + // (a no-op while the operator holds a Manual override). camp#138 coordination: + // this update_auto() call site is where #138's fold changes meet this PR — the + // override is applied at render time and is transparent to the accumulation. + if(data_min_ <= data_max_) + range_model_.update_auto(float(data_min_), float(data_max_)); } std::string SonarLiveCacheLayer::defaultBand() const @@ -608,8 +615,10 @@ QImage SonarLiveCacheLayer::renderImage(const QSize& size) if(!renderer_.makeCurrent()) return QImage(); const QList draw = items(); - const QImage image = renderer_.renderToImage(draw, scene_bounds_, float(data_min_), - float(data_max_), size); + // [camp#142] Feed the resolved range (Auto tracks data_min_/data_max_; Manual is + // the operator override) into the shader's u_min/u_max instead of the raw extents. + const QImage image = renderer_.renderToImage(draw, scene_bounds_, range_model_.lo(), + range_model_.hi(), size); renderer_.doneCurrent(); return image; } @@ -666,6 +675,28 @@ void SonarLiveCacheLayer::setBandName(const std::string& name) update(boundingRect()); } +void SonarLiveCacheLayer::setRangeOverride(float lo, float hi) +{ + // [camp#142] Pin the resolved range to the operator's [lo, hi] (set_manual swaps + // an inverted pair so lo() <= hi()). Re-render with the new range + persist. + range_model_.set_manual(lo, hi); + cached_image_ = QImage(); + writeSettings(); + update(boundingRect()); +} + +void SonarLiveCacheLayer::resetRangeToAuto() +{ + // [camp#142] Return to data-driven Auto, then re-track the current extents so + // lo()/hi() reflect the data without waiting for the next fold. + range_model_.reset(); + if(data_min_ <= data_max_) + range_model_.update_auto(float(data_min_), float(data_max_)); + cached_image_ = QImage(); + writeSettings(); + update(boundingRect()); +} + // --------------------------------- context menu ------------------------------ void SonarLiveCacheLayer::contextMenu(QMenu* menu) @@ -698,6 +729,30 @@ void SonarLiveCacheLayer::contextMenu(QMenu* menu) connect(action, &QAction::triggered, this, [this, name]() { setColormap(name); }); } + // [camp#142] Colormap range override. Live bands are scalar values (same gating as + // the Colormap submenu above), so the submenu is offered unconditionally. "Set + // range…" prompts for lo then hi (pre-filled with the current resolved range) and + // pins a Manual override; "Reset to auto" returns to the data-driven extents. + QMenu* range_menu = menu->addMenu("Colormap range"); + QAction* set_range = range_menu->addAction("Set range…"); + connect(set_range, &QAction::triggered, this, [this]() + { + bool ok = false; + const double lo = QInputDialog::getDouble( + nullptr, "Colormap range", "Minimum:", range_model_.lo(), + -1.0e9, 1.0e9, 6, &ok); + if(!ok) + return; + const double hi = QInputDialog::getDouble( + nullptr, "Colormap range", "Maximum:", range_model_.hi(), + -1.0e9, 1.0e9, 6, &ok); + if(!ok) + return; + setRangeOverride(float(lo), float(hi)); + }); + QAction* reset_range = range_menu->addAction("Reset to auto"); + connect(reset_range, &QAction::triggered, this, [this]() { resetRangeToAuto(); }); + // Band picker over the union of band names across held tiles. Only shown when // there is more than one band to choose from. std::vector names; @@ -738,6 +793,15 @@ void SonarLiveCacheLayer::readSettings() const std::string band = settings.value("band", QString::fromStdString(band_name_)) .toString().toStdString(); const bool was_enabled = settings.value("live_enabled", false).toBool(); + // [camp#142] Persisted colormap range. "manual" restores the operator override; + // anything else (default "auto") leaves the data-driven Auto range. Honor Manual + // only when both extents are present too — a partial/corrupt entry falls back to + // Auto rather than snapping to the [0,1] read-defaults. + const QString range_mode = settings.value("range_mode", "auto").toString(); + const bool has_manual_range = range_mode == "manual" && + settings.contains("range_min") && settings.contains("range_max"); + const float range_min = settings.value("range_min", 0.0).toFloat(); + const float range_max = settings.value("range_max", 1.0).toFloat(); settings.endGroup(); settings.endGroup(); @@ -748,6 +812,12 @@ void SonarLiveCacheLayer::readSettings() } if(!band.empty()) band_name_ = band; + // [camp#142] Apply the persisted range. A Manual override is independent of the + // data extents and is restored as-is; "auto" leaves the model tracking the data. + if(has_manual_range) + range_model_.set_manual(range_min, range_max); + else + range_model_.reset(); // [ADR-0006 D5] An enabled source re-subscribes on warm restart; a never-enabled // one stays passive. enableLiveCoverage() persists, which is a harmless re-write. if(was_enabled && !enabled_) @@ -763,6 +833,13 @@ void SonarLiveCacheLayer::writeSettings() settings.setValue("colormap", QString::fromStdString(renderer_.colormap())); settings.setValue("band", QString::fromStdString(band_name_)); settings.setValue("live_enabled", enabled_); + // [camp#142] Persist the colormap range mode + bounds so a Manual override (and + // its [lo, hi]) survives a restart; Auto persists as "auto". + settings.setValue("range_mode", + range_model_.mode() == marine_colormap::RangeMode::Manual ? "manual" + : "auto"); + settings.setValue("range_min", range_model_.lo()); + settings.setValue("range_max", range_model_.hi()); settings.endGroup(); settings.endGroup(); } diff --git a/src/camp_map/ros/live_coverage/sonar_live_cache_layer.h b/src/camp_map/ros/live_coverage/sonar_live_cache_layer.h index 4ae9e90..f4eb6c3 100644 --- a/src/camp_map/ros/live_coverage/sonar_live_cache_layer.h +++ b/src/camp_map/ros/live_coverage/sonar_live_cache_layer.h @@ -6,6 +6,8 @@ #include "../../raster/raster_gl_renderer.h" #include "sonar_live_tile.h" +#include + #include "marine_tiled_raster_store/tile_catalog.hpp" #include "marine_interfaces/msg/sonar_visualization_tile.hpp" @@ -85,6 +87,16 @@ class SonarLiveCacheLayer: public Layer, public raster::RasterFieldSource /// The layer's Web-Mercator extent (union of tile extents). Exposed for tests. QRectF sceneBounds() const { return scene_bounds_; } + /// [camp#142] Per-layer colormap range override. Auto tracks the data extents + /// (data_min_/data_max_, folded in foldAutoRange()); Manual pins an operator + /// [lo, hi] so an outlier band can't collapse the useful colour range. Applied at + /// render time (shader u_min/u_max via renderToImage), transparent to the fold. + void setRangeOverride(float lo, float hi); ///< -> Manual [lo, hi] + void resetRangeToAuto(); ///< -> Auto (tracks the data extents) + marine_colormap::RangeMode rangeMode() const { return range_model_.mode(); } + float rangeLo() const { return range_model_.lo(); } + float rangeHi() const { return range_model_.hi(); } + // [camp#134] RasterFieldSource: feed the shared RasterGlRenderer. bands() are the // named live bands; items() returns the held tiles as Scalar items (textures // uploaded lazily under the renderer's current context). @@ -182,6 +194,11 @@ private slots: double data_min_ = 1.0; // auto-range over the selected band (crossed => none) double data_max_ = 0.0; + // [camp#142] Resolved colormap range (Auto tracks data_min_/data_max_ via + // update_auto() in foldAutoRange(); Manual pins an operator override). Fed to the + // renderer's u_min/u_max at render time, replacing the raw data_min_/data_max_. + marine_colormap::RangeModel range_model_; + std::string band_name_; // selected band (persisted) rclcpp::Subscription::SharedPtr tile_sub_; diff --git a/test/test_range_persist.cpp b/test/test_range_persist.cpp new file mode 100644 index 0000000..0146049 --- /dev/null +++ b/test/test_range_persist.cpp @@ -0,0 +1,301 @@ +// [camp#142] Per-layer colormap range override: persist/restore round-trip across +// ALL THREE raster layers (GggsTileLayer, RasterLayer, SonarLiveCacheLayer). +// +// The override lives in a marine_colormap::RangeModel each layer owns. Auto mode +// tracks the data extents; Manual mode pins an operator [lo, hi] that is fed to the +// shader's u_min/u_max at render time (replacing the raw data_min_/data_max_). These +// tests pin the public API + the QSettings round-trip: +// - default is Auto; +// - setRangeOverride(lo, hi) -> Manual with lo()/hi() == the override; +// - resetRangeToAuto() -> Auto; +// - a Manual override (mode + [lo, hi]) survives writeSettings()->readSettings(); +// - an Auto layer persists + restores as Auto. +// +// GL-FREE by construction: the RangeModel is pure data and renderImage() is never +// called, so these RUN (not SKIP) in-container — exactly like the band round-trip in +// test_gggs_persistence.cpp. Each layer is exercised through a tiny Testable subclass +// that exposes the protected read/writeSettings hooks (the same pattern that test +// uses), so the round-trip is asserted synchronously without the deferred +// itemConstructed() timer. +// +// Per-layer settings identity (two instances must share a key to round-trip): +// - GggsTileLayer -> settingsKey() = "dir:" + the tile-set directory; +// - SonarLiveCacheLayer -> settingsKey() = "live:" + the source namespace; +// - RasterLayer -> itemID() (parent path + the file's basename) — NOT +// settingsKey(); RasterLayer's read/writeSettings group under itemID(). +// An empty tile-set dir / a never-opened file / a null ROS node keep all three +// GL/GDAL/ROS-free. + +#include + +#include +#include +#include + +#include + +#include "map/map.h" +#include "map/layer_list.h" +#include "raster/gggs_tile_layer.h" +#include "raster/raster_layer.h" +#include "ros/live_coverage/sonar_live_cache_layer.h" + +using camp::map::Map; +using camp::raster::GggsTileLayer; +using camp::raster::RasterLayer; +using camp::ros::live_coverage::SonarLiveCacheLayer; +using marine_colormap::RangeMode; + +namespace +{ + +// Expose the protected settings hooks so the round-trip is testable synchronously +// (mirrors test_gggs_persistence.cpp's TestableGggsTileLayer). +class TestableGggsTileLayer: public GggsTileLayer +{ +public: + using GggsTileLayer::GggsTileLayer; + using GggsTileLayer::readSettings; + using GggsTileLayer::writeSettings; +}; + +class TestableRasterLayer: public RasterLayer +{ +public: + using RasterLayer::RasterLayer; + using RasterLayer::readSettings; + using RasterLayer::writeSettings; +}; + +class TestableSonarLiveCacheLayer: public SonarLiveCacheLayer +{ +public: + using SonarLiveCacheLayer::SonarLiveCacheLayer; + using SonarLiveCacheLayer::readSettings; + using SonarLiveCacheLayer::writeSettings; +}; + +} // namespace + +// ----------------------------- GggsTileLayer --------------------------------- + +TEST(RangePersist, GggsDefaultOverrideReset) +{ + QSettings().clear(); + Map map; + camp::map::LayerList* layers = map.topLevelLayers(); + ASSERT_NE(layers, nullptr); + QTemporaryDir tileset; + ASSERT_TRUE(tileset.isValid()); + + auto* layer = new TestableGggsTileLayer(layers, tileset.path()); + EXPECT_EQ(layer->rangeMode(), RangeMode::Auto); // default before any override + + layer->setRangeOverride(1.0f, 5.0f); + EXPECT_EQ(layer->rangeMode(), RangeMode::Manual); + EXPECT_FLOAT_EQ(layer->rangeLo(), 1.0f); + EXPECT_FLOAT_EQ(layer->rangeHi(), 5.0f); + + layer->resetRangeToAuto(); + EXPECT_EQ(layer->rangeMode(), RangeMode::Auto); +} + +TEST(RangePersist, GggsManualRoundTrips) +{ + QSettings().clear(); + Map map; + camp::map::LayerList* layers = map.topLevelLayers(); + ASSERT_NE(layers, nullptr); + QTemporaryDir tileset; + ASSERT_TRUE(tileset.isValid()); + + { + auto* layer = new TestableGggsTileLayer(layers, tileset.path()); + layer->setRangeOverride(1.0f, 5.0f); + layer->writeSettings(); + } + { + auto* layer = new TestableGggsTileLayer(layers, tileset.path()); + layer->readSettings(); + EXPECT_EQ(layer->rangeMode(), RangeMode::Manual) + << "restored range mode must be the persisted Manual, not Auto"; + EXPECT_FLOAT_EQ(layer->rangeLo(), 1.0f); + EXPECT_FLOAT_EQ(layer->rangeHi(), 5.0f); + } +} + +TEST(RangePersist, GggsAutoRoundTrips) +{ + QSettings().clear(); + Map map; + camp::map::LayerList* layers = map.topLevelLayers(); + ASSERT_NE(layers, nullptr); + QTemporaryDir tileset; + ASSERT_TRUE(tileset.isValid()); + + { + auto* layer = new TestableGggsTileLayer(layers, tileset.path()); + layer->writeSettings(); // never overridden -> persists Auto + } + { + auto* layer = new TestableGggsTileLayer(layers, tileset.path()); + layer->readSettings(); + EXPECT_EQ(layer->rangeMode(), RangeMode::Auto); + // An Auto round-trip must leave the model tracking the data, not pin a manual + // override; with no tiles loaded it stays at the RangeModel default extents. + EXPECT_FLOAT_EQ(layer->rangeLo(), 0.0f); + EXPECT_FLOAT_EQ(layer->rangeHi(), 1.0f); + } +} + +// ------------------------------ RasterLayer ---------------------------------- + +TEST(RangePersist, RasterDefaultOverrideReset) +{ + QSettings().clear(); + Map map; + camp::map::LayerList* layers = map.topLevelLayers(); + ASSERT_NE(layers, nullptr); + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + const QString file = dir.filePath("chart.tif"); // need not exist for the range API + + auto* layer = new TestableRasterLayer(layers, file); + EXPECT_EQ(layer->rangeMode(), RangeMode::Auto); + + layer->setRangeOverride(2.0f, 8.0f); + EXPECT_EQ(layer->rangeMode(), RangeMode::Manual); + EXPECT_FLOAT_EQ(layer->rangeLo(), 2.0f); + EXPECT_FLOAT_EQ(layer->rangeHi(), 8.0f); + + layer->resetRangeToAuto(); + EXPECT_EQ(layer->rangeMode(), RangeMode::Auto); +} + +TEST(RangePersist, RasterManualRoundTrips) +{ + QSettings().clear(); + Map map; + camp::map::LayerList* layers = map.topLevelLayers(); + ASSERT_NE(layers, nullptr); + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + const QString file = dir.filePath("chart.tif"); + + // RasterLayer groups under itemID() (parent path + the file's basename), so two + // layers over the SAME filename + parent share a settings group. + { + auto* layer = new TestableRasterLayer(layers, file); + layer->setRangeOverride(2.0f, 8.0f); + layer->writeSettings(); + } + { + auto* layer = new TestableRasterLayer(layers, file); + layer->readSettings(); + EXPECT_EQ(layer->rangeMode(), RangeMode::Manual); + EXPECT_FLOAT_EQ(layer->rangeLo(), 2.0f); + EXPECT_FLOAT_EQ(layer->rangeHi(), 8.0f); + } +} + +TEST(RangePersist, RasterAutoRoundTrips) +{ + QSettings().clear(); + Map map; + camp::map::LayerList* layers = map.topLevelLayers(); + ASSERT_NE(layers, nullptr); + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + const QString file = dir.filePath("chart.tif"); + + { + auto* layer = new TestableRasterLayer(layers, file); + layer->writeSettings(); + } + { + auto* layer = new TestableRasterLayer(layers, file); + layer->readSettings(); + EXPECT_EQ(layer->rangeMode(), RangeMode::Auto); + // An Auto round-trip must leave the model tracking the data, not pin a manual + // override; with no raster loaded it stays at the RangeModel default extents. + EXPECT_FLOAT_EQ(layer->rangeLo(), 0.0f); + EXPECT_FLOAT_EQ(layer->rangeHi(), 1.0f); + } +} + +// --------------------------- SonarLiveCacheLayer ----------------------------- + +TEST(RangePersist, SonarDefaultOverrideReset) +{ + QSettings().clear(); + Map map; + camp::map::LayerList* layers = map.topLevelLayers(); + ASSERT_NE(layers, nullptr); + + // A null ROS node keeps this GL/ROS-free: subscribeCatalog() no-ops with no node. + auto* layer = new TestableSonarLiveCacheLayer(layers, nullptr, "/test_source"); + EXPECT_EQ(layer->rangeMode(), RangeMode::Auto); + + layer->setRangeOverride(3.0f, 9.0f); + EXPECT_EQ(layer->rangeMode(), RangeMode::Manual); + EXPECT_FLOAT_EQ(layer->rangeLo(), 3.0f); + EXPECT_FLOAT_EQ(layer->rangeHi(), 9.0f); + + layer->resetRangeToAuto(); + EXPECT_EQ(layer->rangeMode(), RangeMode::Auto); +} + +TEST(RangePersist, SonarManualRoundTrips) +{ + QSettings().clear(); + Map map; + camp::map::LayerList* layers = map.topLevelLayers(); + ASSERT_NE(layers, nullptr); + + // SonarLiveCacheLayer groups under settingsKey() = "live:" + the source namespace, + // so two layers over the same namespace share a settings group. + { + auto* layer = new TestableSonarLiveCacheLayer(layers, nullptr, "/test_source"); + layer->setRangeOverride(3.0f, 9.0f); + layer->writeSettings(); + } + { + auto* layer = new TestableSonarLiveCacheLayer(layers, nullptr, "/test_source"); + layer->readSettings(); + EXPECT_EQ(layer->rangeMode(), RangeMode::Manual); + EXPECT_FLOAT_EQ(layer->rangeLo(), 3.0f); + EXPECT_FLOAT_EQ(layer->rangeHi(), 9.0f); + } +} + +TEST(RangePersist, SonarAutoRoundTrips) +{ + QSettings().clear(); + Map map; + camp::map::LayerList* layers = map.topLevelLayers(); + ASSERT_NE(layers, nullptr); + + { + auto* layer = new TestableSonarLiveCacheLayer(layers, nullptr, "/test_source"); + layer->writeSettings(); + } + { + auto* layer = new TestableSonarLiveCacheLayer(layers, nullptr, "/test_source"); + layer->readSettings(); + EXPECT_EQ(layer->rangeMode(), RangeMode::Auto); + // An Auto round-trip must leave the model tracking the data, not pin a manual + // override; with no source loaded it stays at the RangeModel default extents. + EXPECT_FLOAT_EQ(layer->rangeLo(), 0.0f); + EXPECT_FLOAT_EQ(layer->rangeHi(), 1.0f); + } +} + +int main(int argc, char** argv) +{ + qputenv("QT_QPA_PLATFORM", "offscreen"); + QApplication app(argc, argv); + QCoreApplication::setOrganizationName("camp_test"); + QCoreApplication::setApplicationName("test_range_persist"); + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +}