From e0701b66214c78ae9a4ae93c243621d7c6c8565f Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Mon, 29 Jun 2026 03:59:25 +0000 Subject: [PATCH 01/17] progress: issue review for #134 --- .agent/work-plans/issue-134/progress.md | 111 ++++++++++++++++++++++++ 1 file changed, 111 insertions(+) create mode 100644 .agent/work-plans/issue-134/progress.md diff --git a/.agent/work-plans/issue-134/progress.md b/.agent/work-plans/issue-134/progress.md new file mode 100644 index 0000000..95a7ffe --- /dev/null +++ b/.agent/work-plans/issue-134/progress.md @@ -0,0 +1,111 @@ +--- +issue: 134 +--- + +# Issue #134 — RasterFieldSource: unified source-agnostic render abstraction + +## Issue Review +**Status**: complete +**When**: 2026-06-29 00:00 +00:00 +**By**: Claude Code Agent (Claude Sonnet) + +**Issue**: #134 +**Comment**: (best-effort post follows this entry; not recorded inline) +**Scope verdict**: well-scoped + +### Summary + +Issue #134 proposes extracting a `RasterFieldSource` interface (with `bands()`, +`metadata(band)`, `read(band, extent, lod)` API) once camp#121 Part B (live tile +cache rendering through `GggsTileLayer`) is complete. The two concrete duplications +to unify are confirmed in code: + +- `src/camp_map/raster/gggs_tile_layer.cpp` — fragment shader with + `if(u_has_nodata != 0 && v == u_nodata) discard;` (NaN-unsafe) +- `src/camp_map/ros/live_coverage/sonar_live_cache_layer.cpp` — identical shader, + explicitly marked `// NOTE: shader duplicated from GggsTileLayer; will unify via + RasterFieldSource in camp#134` +- `src/camp_map/raster/raster_layer.cpp` — separate QPainter path (no shader/colormap); + CPU-side path already handles NaN correctly via `std::isnan(v)` checks + +The NaN NoData bug is real: verified data stores use NaN as NoData (chart/backscatter) +and the exact-equality `v == u_nodata` test silently fails for NaN in GLSL, rendering +those cells opaque. + +### Scope Assessment + +**Well-scoped?** Yes — the interface is small (3 methods), the three concrete +implementations are pre-identified, and the NaN fix scopes tightly to the unified +shader. The issue correctly defers until camp#121 Part B lands (the "second +implementation" that justifies extracting the abstraction). + +**Right repo?** Yes — this is a camp UI/rendering concern; the camp project repo is +the correct home. + +**Dependencies**: +- **camp#121 Part B** (live tile cache rendering) — hard prerequisite; implementation + must not begin until Part B is merged. The duplication marker in + `sonar_live_cache_layer.cpp` confirms Part B exists but is not yet integrated. +- `marine_colormap` GPU-shader lib (`unh_marine_autonomy#175` / I4) — referenced for + per-band colormap defaults; should confirm whether the lib is available/merged before + implementation. +- camp#121 (parent) — issue #134 is part of camp#121's scope. + +### Principle Alignment + +| Principle | Status | Notes | +|---|---|---| +| Only what's needed | OK | Explicitly deferred until justified by a second implementation (camp#121 Part B). No speculative abstraction. | +| Improve incrementally | Watch | Issue body acknowledges this is a LARGE refactor and raises staging. Plan should commit to a staging approach: extract shared GL renderer first, then migrate each source, to keep each PR reviewable. | +| A change includes its consequences | Action needed | Behavior parity checklist needed: band select, colormap LUT, NoData discard (including NaN), Nearest sampling. The CPU range path in GggsTileLayer also excludes NaN via `isfinite` — consistency with the unified shader must be verified. | +| Capture decisions, not just implementations | Action needed | `RasterFieldSource` interface design (method signatures, ownership model, threading contract for `read()`) is a significant design choice — should be captured in an ADR or the plan must reference the issue's rationale explicitly. | +| Test what breaks | Watch | The NaN NoData fix is testable (chart + backscatter stores render transparent cells after fix). Plan should include a render verification step. | +| Human control and transparency | OK | Per-band colormap user-overridability is preserved in scope. | +| Workspace vs. project separation | OK | Change is entirely within the camp project repo. | + +### ADR Applicability + +| ADR | Triggered | Notes | +|---|---|---| +| ADR-0001 (Adopt ADRs) | Yes | `RasterFieldSource` interface design is a durable decision; rationale should be recorded (the issue body is a good start, but the plan should reference or create an ADR). | +| ADR-0002 (Worktree isolation) | Yes | Already satisfied — `feature/issue-134` branch exists in the camp repo. | +| ADR-0008 (ROS 2 conventions) | Partial | OpenGL/Qt rendering is not ROS-specific, but if `read(band, extent, lod)` produces ROS message data, conventions apply. | +| ADR-0013 (progress.md entry type) | Yes | This entry satisfies it. | + +### Consequences + +- **Removing duplicate shaders**: when the unified renderer lands, the `kFragmentShader` + literals in both `gggs_tile_layer.cpp` and `sonar_live_cache_layer.cpp` go away — both + files need to delegate to the shared renderer. Tests and layer-level UI (band select, + colormap menus, NoData display) must all pass through the new path. +- **NaN shader fix**: the fix (`if(v != v) discard;` before the finite sentinel check) + must be in the unified shader only — do NOT add it to the two duplicated shaders first, + defeating the whole point of #134. +- **`raster_layer.cpp` migration**: bringing non-tiled rasters into the shader path + changes their rendering semantics (CPU QPainter → GPU colormap). Verify visual parity + with the existing output; existing NaN handling in the CPU path (`std::isnan(v)`) + must be replicated in the shader (which the `v != v` check already covers). +- **`marine_colormap` integration**: per-band colormap defaults (depth→bathy/turbo, + uncertainty→sequential, intensity→grayscale) are new behavior — document defaults + and confirm they match user expectations before merging. + +### Recommendations + +- Confirm camp#121 Part B is merged before starting implementation. +- Confirm `marine_colormap` lib availability (`unh_marine_autonomy#175` / I4) before + coding the colormap-default wiring. +- Stage the refactor: (1) extract `RasterFieldSource` interface + shared GL renderer + with `GggsTileLayer` as the first adapter, (2) migrate `SonarLiveCacheLayer`, (3) + migrate `RasterLayer`. Each stage should be a reviewable PR. +- Add a NaN render test: open a chart store (NoData = NaN), verify cells render + transparent after the fix. +- Consider an ADR for the `RasterFieldSource` interface design (especially the + threading contract for `read()`). + +### Actions +- [ ] Confirm camp#121 Part B is merged before beginning implementation. +- [ ] Confirm `marine_colormap` lib (`unh_marine_autonomy#175`) is available before implementation. +- [ ] Behavior parity checklist: band select, colormap LUT, NaN + finite NoData discard, Nearest sampling — verify all three adapters match current behavior. +- [ ] Stage the refactor into at minimum two PRs: shared renderer + GGGS adapter first, then live cache + raster_layer. +- [ ] Verify CPU range path in GggsTileLayer (`isfinite`) stays consistent with the unified shader's NaN discard. +- [ ] Consider an ADR for `RasterFieldSource` interface design (threading contract for `read()`). From 6fbe701e61a55ea1b0bb69f1a19397a0b94ebf66 Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Mon, 29 Jun 2026 04:11:30 +0000 Subject: [PATCH 02/17] Add work plan for #134 Extract RasterFieldSource interface + shared GL renderer; migrate GggsTileLayer, SonarLiveCacheLayer, RasterLayer; NaN NoData fix in unified shader; single PR. --- .agent/work-plans/issue-134/plan.md | 151 ++++++++++++++++++++++++++++ 1 file changed, 151 insertions(+) create mode 100644 .agent/work-plans/issue-134/plan.md diff --git a/.agent/work-plans/issue-134/plan.md b/.agent/work-plans/issue-134/plan.md new file mode 100644 index 0000000..ce37858 --- /dev/null +++ b/.agent/work-plans/issue-134/plan.md @@ -0,0 +1,151 @@ +# Plan: RasterFieldSource — unified source-agnostic render abstraction + +## Issue + +https://github.com/rolker/camp/issues/134 + +## Context + +camp#121 Part B (live cache rendering through `SonarLiveCacheLayer`) is merged +(PR#139). The duplication this issue targets is confirmed: + +- `raster/gggs_tile_layer.cpp` — GL shader (FBO + fragment shader, colormap LUT, + NoData discard). GDAL-backed tiles, QtConcurrent off-thread load, atomic + `pixelsLoaded()` publish. +- `ros/live_coverage/sonar_live_cache_layer.cpp` — **identical** shader, marked + `// NOTE: shader duplicated; unify via RasterFieldSource camp#134`. In-memory + tiles, all state on GUI thread. +- `raster/raster_layer.cpp` — **separate** QPainter/mipmap path, no GPU shader, + no colormap LUT, no per-cell NoData discard (CPU `std::isnan` only). + +NaN NoData bug: GLSL `v == u_nodata` is false when `v` is NaN. Chart and +backscatter stores use NaN as NoData — those cells render opaque today. The fix +(`if(v != v) discard;`) lands once in the unified shader. + +camp#121 Part B merged; `marine_colormap` GPU-lib migration deferred (separable). +Single PR, full consolidation (operator-decided). + +## Approach + +1. **ADR** — Write `docs/decisions/0007-raster-field-source-interface.md`: + `RasterFieldSource` API (`bands()`, `metadata(band)`, `items(band, extent)`, + `dataRange(band)`), ownership model (textures owned by source, non-owning + pointers valid for the duration of `paint()`), threading contract (`items()` + called on GUI/GL thread; sources ensure texture validity before returning). + Note `marine_colormap` deferral. + +2. **Interface** — Add `src/camp_map/raster/raster_field_source.h`: + ```cpp + struct BandMeta { float nodata; bool has_nodata; QString units; }; + struct RasterFieldItem { + QOpenGLTexture* texture; // R32F, owned by source + QList geo_vertices; // Web-Mercator geo coords + float nodata; bool has_nodata; + }; + class RasterFieldSource { + public: + virtual QStringList bands() const = 0; + virtual BandMeta metadata(const QString& band) const = 0; + virtual QList items(const QString& band, + const QRectF& extent) const = 0; + virtual QPair dataRange(const QString& band) const = 0; + }; + ``` + +3. **Shared GL renderer** — Add `src/camp_map/raster/raster_gl_renderer.{h,cpp}`: + Owns compiled shader program + LUT texture (per-GL context; caller makes + context current before any call). Unified fragment shader: + ```glsl + if(v != v) discard; // NaN NoData (GLSL 1.20-portable; isnan() requires 1.30+) + if(u_has_nodata != 0 && v == u_nodata) discard; // finite sentinel (sidescan/0) + ``` + Methods: `render(items, data_min, data_max, mvp)`, `setColormap(cm)`, + `ensureProgram()`, `ensureLut()`. Vertex tessellation (16 lat subdivisions, + CPU-side geo→Web-Mercator warp, relative to extent origin) moves here from both + layer files. Nearest texture filter set once in `render()` per item. + +4. **Migrate `GggsTileLayer`** — Implement `RasterFieldSource` (or adapt the existing + tile storage to satisfy the interface inline). Delete `kVertexShader`, + `kFragmentShader`, `ensureProgram()`, `ensureLut()`, per-vertex tessellation + from `gggs_tile_layer.cpp`. Replace `renderImage()` body with + `renderer_.render(items(band_, extent), data_min_, data_max_, mvp)`. Keep + QtConcurrent load + atomic `pixelsLoaded()` publish unchanged. + +5. **Migrate `SonarLiveCacheLayer`** — Same pattern: implement `RasterFieldSource`, + delete the duplicated shader block, delegate `renderImage()` to `renderer_`. + All GUI-thread invariants unchanged (ROS callbacks still marshal to GUI thread). + +6. **Migrate `RasterLayer`** — Replace QPainter mipmap path with `RasterGlRenderer`: + Add `QOpenGLContext*` + `QOffscreenSurface*` + `QOpenGLFramebufferObject*` to + class (matching GggsTileLayer pattern). In `loadAndReprojectFile()`, upload + pixels as R32F texture (scalar path) or RGBA8 (palette/RGB path). In `paint()`, + call `renderer_.render()` instead of `painter->drawPixmap()`. Implement + `RasterFieldSource` returning the single-file item. Keep async load + abort flag. + Remove mipmap pyramid (GPU path provides LOD via texture sampling). Verify + visual parity: palette/RGB bands, scalar colormap, NoData cells transparent. + +7. **Verify CPU range path consistency** — `GggsTileLayer` uses `isfinite` to skip + NaN values when folding `data_min_`/`data_max_`. Confirm `SonarLiveCacheLayer`'s + `foldAutoRange()` and `RasterLayer`'s range fold also exclude NaN, so the CPU + range and shader discard are consistent. + +8. **Tests** — Extend `test_gggs_render.cpp` with NaN NoData test (chart store, + verify null cells render transparent). Add `test_raster_gl_renderer.cpp`: + unit tests for renderer (NaN discard, finite-sentinel discard, LUT bake, Nearest + filter). Extend `test_raster_layer_gdal_cleanup.cpp` (or add new test) for + RasterLayer under the GPU path. + +## Files to Change + +| File | Change | +|------|--------| +| `src/camp_map/raster/raster_field_source.h` | **New** — abstract interface | +| `src/camp_map/raster/raster_gl_renderer.h/cpp` | **New** — shared GL renderer, unified shader + NaN fix | +| `docs/decisions/0007-raster-field-source-interface.md` | **New** — ADR | +| `src/camp_map/raster/gggs_tile_layer.h/cpp` | Remove own shaders; implement `RasterFieldSource`; delegate `renderImage()` | +| `src/camp_map/ros/live_coverage/sonar_live_cache_layer.h/cpp` | Delete duplicated shaders; delegate `renderImage()` | +| `src/camp_map/raster/raster_layer.h/cpp` | Replace QPainter/mipmap with GL path; implement `RasterFieldSource`; add GL context | +| `test/test_gggs_render.cpp` | Add NaN NoData render test (chart store) | +| `test/test_raster_gl_renderer.cpp` | **New** — renderer unit tests | +| `CMakeLists.txt` | Add new source files and test | + +## Principles Self-Check + +| Principle | Consideration | +|---|---| +| Only what's needed | Single PR, no speculative features; `marine_colormap` deferred | +| Improve incrementally | Steps 4→5→6 are ordered; each can be reviewed in sequence even within one PR | +| A change includes its consequences | Parity checklist + tests for all three adapters; NaN fix verified on real data stores | +| Capture decisions, not just implementations | ADR step 1 records interface design + threading contract | +| Test what breaks | New tests for NaN discard, finite sentinel, LUT correctness, RasterLayer GPU parity | + +## ADR Compliance + +| ADR | Triggered | How addressed | +|---|---|---| +| ADR-0001 (camp: Adopt ADRs) | Yes | New camp ADR `0007-raster-field-source-interface.md` | +| ADR-0002 (camp: Web-Mercator scene/layer model) | Yes | All three adapters remain Web-Mercator scene objects; geometry warp moves into renderer, not out | +| ADR-0008 (workspace: ROS 2 conventions) | No | Pure Qt/GL; no ROS message data in render path | +| ADR-0013 (workspace: progress.md vocabulary) | Yes | This plan + progress entry satisfy it | + +## Consequences + +| If we change... | Also update... | Included? | +|---|---|---| +| Shader discard logic | CPU range fold (NaN exclusion must match) | Yes — step 7 | +| `RasterLayer` from QPainter to GL | Mipmap pyramid removed; `QPixmap::scaledToWidth()` chain deleted | Yes — step 6 | +| Both GL layers lose own shader strings | `sonar_live_cache_layer.cpp` `// NOTE: shader duplicated` comment | Yes — deleted in step 5 | +| `marine_colormap` migration | Per-band colormap defaults | No — deferred (noted in ADR) | + +## Open Questions + +- `RasterLayer` RGB/palette bands: current QPainter path draws composite RGBA QImage. + The GL renderer is single-band R32F. For multi-band color files, determine whether + to (a) composite on CPU → upload RGBA8 texture (simplest, sidesteps per-band select), + or (b) expose per-band R32F + channel compositor in the renderer. Recommendation: (a) + for this PR; band-per-channel select is a separate future issue. + +## Estimated Scope + +Single PR. ~600–800 lines new (interface + renderer + ADR + tests); +~400 lines deleted (duplicate shaders, mipmap pyramid, QPainter path). From 77186667ee1c5763a1ec42fbbbc847ff2afcdfed Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Mon, 29 Jun 2026 04:11:42 +0000 Subject: [PATCH 03/17] progress: plan authored for #134 --- .agent/work-plans/issue-134/progress.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/.agent/work-plans/issue-134/progress.md b/.agent/work-plans/issue-134/progress.md index 95a7ffe..02d75a1 100644 --- a/.agent/work-plans/issue-134/progress.md +++ b/.agent/work-plans/issue-134/progress.md @@ -109,3 +109,15 @@ the correct home. - [ ] Stage the refactor into at minimum two PRs: shared renderer + GGGS adapter first, then live cache + raster_layer. - [ ] Verify CPU range path in GggsTileLayer (`isfinite`) stays consistent with the unified shader's NaN discard. - [ ] Consider an ADR for `RasterFieldSource` interface design (threading contract for `read()`). + +## Plan Authored +**Status**: complete +**When**: 2026-06-29 04:09 +00:00 +**By**: Claude Code Agent (Claude Sonnet) + +**Plan**: `.agent/work-plans/issue-134/plan.md` at `6fbe701` +**Branch**: feature/issue-134 at `6fbe701` +**Phases**: single + +### Open questions +- [ ] `RasterLayer` RGB/palette bands: composite on CPU → RGBA8 texture (recommended for this PR) vs per-band R32F + channel compositor — decide before implementing step 6. From a33c1c6ed1b7907a2aae56d7df9af583e5c143a4 Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Mon, 29 Jun 2026 04:17:33 +0000 Subject: [PATCH 04/17] progress: plan review for #134 --- .agent/work-plans/issue-134/progress.md | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/.agent/work-plans/issue-134/progress.md b/.agent/work-plans/issue-134/progress.md index 02d75a1..9d589c2 100644 --- a/.agent/work-plans/issue-134/progress.md +++ b/.agent/work-plans/issue-134/progress.md @@ -121,3 +121,21 @@ the correct home. ### Open questions - [ ] `RasterLayer` RGB/palette bands: composite on CPU → RGBA8 texture (recommended for this PR) vs per-band R32F + channel compositor — decide before implementing step 6. + +## Plan Review +**Status**: complete +**When**: 2026-06-29 04:16 +00:00 +**By**: Claude Code Agent (Claude Opus) + +**Plan**: `.agent/work-plans/issue-134/plan.md` at `6fbe701` +**PR**: PR-less (`--issue 134`, layer worktree `issue-camp-134`) +**Verdict**: approve-with-suggestions + +Independent review (plan authored by a Sonnet agent; this review is a fresh +host-dispatched Opus sub-agent — not author self-review). + +### Findings +- [ ] (suggestion) Single PR migrates 3 adapters + 2 new components (~9–11 files), exceeding the >3-component split heuristic and overriding review-issue's 2-PR staging action. Operator-decided and documented — accept, but enforce ordered per-adapter commits (steps 4→5→6) so the PR is reviewable commit-by-commit — `plan.md:26`, `plan.md:117` +- [ ] (suggestion) RasterLayer RGBA/palette path vs single-band R32F renderer: recommended option (a) (CPU composite → RGBA8) needs a colormap-bypass mode in the renderer, but the sketched `render(items, data_min, data_max, mvp)` + `RasterFieldItem{texture, nodata}` only expresses scalar R32F+LUT. Add a format/mode field to `RasterFieldItem` (or a render overload) and resolve the open question before step 6 — `plan.md:46-52`, `plan.md:142-146` +- [ ] (suggestion) ADR-table label wrong: "ADR-0001 (camp: Adopt ADRs)" — camp ADR-0001 is "TopicBridge and the executor contract"; no camp "Adopt ADRs" ADR exists (that's a workspace ADR). Also camp ADR-0006 (live tile cache) governs `SonarLiveCacheLayer` — confirm the render-path migration leaves its persistence/subscription contract untouched and note it in the table — `plan.md:124-129` +- [ ] (suggestion) New `test_raster_gl_renderer.cpp` requires an offscreen GL context; mirror `test_gggs_render`'s harness and confirm CI provides GL — `plan.md:93-96` From 85e66a8c152880e5cb43d638f9b853d1ad7e8cc8 Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Mon, 29 Jun 2026 04:50:23 +0000 Subject: [PATCH 05/17] =?UTF-8?q?camp#134:=20shared=20raster=20render=20pa?= =?UTF-8?q?th=20=E2=80=94=20ADR-0007,=20interface,=20renderer?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract the duplicated GL raster display path into one place (ADR-0007): - docs/decisions/0007-raster-field-source-interface.md — RasterFieldSource API, ownership + threading contract, scalar-vs-RGBA format branch, NaN-fix rationale, marine_colormap deferral, and the corrected ADR references. - raster/raster_field_source.h — the abstract source interface (bands / metadata / items / dataRange) + the RasterFieldItem value type (Scalar R32F + LUT, or RGBA8 bypass; geographic warp vs already-Web-Mercator quad). - raster/raster_gl_renderer.{h,cpp} — the ONE compiled program + colormap LUT (per GL context), the unified fragment shader, the geo->Web-Mercator tessellation, and the data-texture filter. The NaN NoData fix lands here once: if(v != v) discard; // NaN (GLSL 1.20-portable) if(u_has_nodata != 0 && v == u_nodata) discard; // finite sentinel No call sites yet — the three adapters migrate in the following commits. Co-Authored-By: Claude Opus 4.8 --- .agent/work-plans/issue-134/plan.md | 53 ++- CMakeLists.txt | 1 + .../0007-raster-field-source-interface.md | 159 +++++++++ src/camp_map/raster/raster_field_source.h | 98 +++++ src/camp_map/raster/raster_gl_renderer.cpp | 336 ++++++++++++++++++ src/camp_map/raster/raster_gl_renderer.h | 102 ++++++ 6 files changed, 733 insertions(+), 16 deletions(-) create mode 100644 docs/decisions/0007-raster-field-source-interface.md create mode 100644 src/camp_map/raster/raster_field_source.h create mode 100644 src/camp_map/raster/raster_gl_renderer.cpp create mode 100644 src/camp_map/raster/raster_gl_renderer.h diff --git a/.agent/work-plans/issue-134/plan.md b/.agent/work-plans/issue-134/plan.md index ce37858..bce5fcf 100644 --- a/.agent/work-plans/issue-134/plan.md +++ b/.agent/work-plans/issue-134/plan.md @@ -34,23 +34,31 @@ Single PR, full consolidation (operator-decided). called on GUI/GL thread; sources ensure texture validity before returning). Note `marine_colormap` deferral. -2. **Interface** — Add `src/camp_map/raster/raster_field_source.h`: +2. **Interface** — Add `src/camp_map/raster/raster_field_source.h` (as implemented): ```cpp - struct BandMeta { float nodata; bool has_nodata; QString units; }; + struct RasterBandMeta { bool has_nodata; float nodata; QString units; }; struct RasterFieldItem { - QOpenGLTexture* texture; // R32F, owned by source - QList geo_vertices; // Web-Mercator geo coords - float nodata; bool has_nodata; + enum class Format { Scalar, Rgba }; // [review-2] format/mode field + QOpenGLTexture* texture; // R32F (Scalar) or RGBA8 (Rgba), source-owned + Format format; + bool geographic; // true: warp west/east/south/north (deg) + double west, east, south, north; // false: already Web-Mercator metres (a quad) + bool has_nodata; float nodata; // Scalar finite-sentinel discard }; class RasterFieldSource { public: virtual QStringList bands() const = 0; - virtual BandMeta metadata(const QString& band) const = 0; - virtual QList items(const QString& band, - const QRectF& extent) const = 0; - virtual QPair dataRange(const QString& band) const = 0; + virtual RasterBandMeta metadata(const QString& band) const = 0; + virtual QList items() = 0; // current selection; lazy texture upload + virtual QPair dataRange() const = 0; }; ``` + [review-2] The `Format` field resolves the RGB/palette open question: Scalar + items shade through the LUT (NaN/finite discard), Rgba items (RasterLayer's + palette/RGB charts, composited to RGBA8 on the CPU) sample directly, bypassing + the LUT. The `geographic` flag lets one renderer serve both the GGGS/live path + (lat/lon tiles warped in the renderer) and RasterLayer (already GDAL-reprojected + to Web-Mercator → a single linear quad). 3. **Shared GL renderer** — Add `src/camp_map/raster/raster_gl_renderer.{h,cpp}`: Owns compiled shader program + LUT texture (per-GL context; caller makes @@ -121,10 +129,15 @@ Single PR, full consolidation (operator-decided). ## ADR Compliance +[review-3] ADR table corrected: camp ADR-0001 is *`TopicBridge` and the executor +contract* (NOT "Adopt ADRs" — that's a workspace ADR). New camp ADR is `0007`. + | ADR | Triggered | How addressed | |---|---|---| -| ADR-0001 (camp: Adopt ADRs) | Yes | New camp ADR `0007-raster-field-source-interface.md` | -| ADR-0002 (camp: Web-Mercator scene/layer model) | Yes | All three adapters remain Web-Mercator scene objects; geometry warp moves into renderer, not out | +| camp ADR-0001 (`TopicBridge` and the executor contract) | Yes | `SonarLiveCacheLayer`'s ROS-callback → GUI-thread marshalling is untouched by the render migration | +| camp ADR-0002 (Web-Mercator scene/layer model) | Yes | All three adapters remain Web-Mercator scene objects; geometry warp moves into the renderer, not out | +| camp ADR-0006 (live tile cache — persistence + opt-in subscription) | Yes | Governs `SonarLiveCacheLayer`; this migration swaps only `renderImage()`'s GL internals — persistence / subscription / reconcile / prune / write-through are UNTOUCHED | +| New: camp ADR-0007 (RasterFieldSource + unified renderer) | Yes | Captures the interface, ownership + threading contract, and the marine_colormap deferral | | ADR-0008 (workspace: ROS 2 conventions) | No | Pure Qt/GL; no ROS message data in render path | | ADR-0013 (workspace: progress.md vocabulary) | Yes | This plan + progress entry satisfy it | @@ -139,11 +152,19 @@ Single PR, full consolidation (operator-decided). ## Open Questions -- `RasterLayer` RGB/palette bands: current QPainter path draws composite RGBA QImage. - The GL renderer is single-band R32F. For multi-band color files, determine whether - to (a) composite on CPU → upload RGBA8 texture (simplest, sidesteps per-band select), - or (b) expose per-band R32F + channel compositor in the renderer. Recommendation: (a) - for this PR; band-per-channel select is a separate future issue. +- ~~`RasterLayer` RGB/palette bands~~ **RESOLVED [review-2]**: option (a) — composite + on CPU → upload RGBA8 texture, sampled directly via the renderer's `Rgba` format + (LUT bypassed). The `RasterFieldItem::Format` field expresses it; per-band channel + select for colour files stays a future issue. Mipmaps are generated on the RGBA8 + texture so a chart zoomed far out keeps the LOD the old QPainter mipmap gave. + +## Commit ordering [review-1] + +Single PR, but committed step-by-step so it reviews commit-by-commit (overrides +review-issue's 2-PR split; operator-decided, full consolidation in one PR): +(a) ADR-0007 + `raster_field_source.h` + `raster_gl_renderer.{h,cpp}`, +(b) migrate `GggsTileLayer`, (c) migrate `SonarLiveCacheLayer`, +(d) migrate `RasterLayer`, (e) tests (renderer unit + GGGS NaN render). ## Estimated Scope diff --git a/CMakeLists.txt b/CMakeLists.txt index 8e13c67..202fc62 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -260,6 +260,7 @@ set(CAMP_MAP_SOURCES src/camp_map/map_tree_view/map_tree_view.cpp src/camp_map/map_view/map_view.cpp src/camp_map/map_view/web_mercator.cpp + src/camp_map/raster/raster_gl_renderer.cpp src/camp_map/raster/raster_layer.cpp src/camp_map/raster/gggs_tile.cpp src/camp_map/raster/gggs_tile_layer.cpp diff --git a/docs/decisions/0007-raster-field-source-interface.md b/docs/decisions/0007-raster-field-source-interface.md new file mode 100644 index 0000000..64a5dcc --- /dev/null +++ b/docs/decisions/0007-raster-field-source-interface.md @@ -0,0 +1,159 @@ +# ADR-0007: `RasterFieldSource` render abstraction + unified GL raster renderer + +## Status + +Accepted + +Implements CAMP issue [#134](https://github.com/rolker/camp/issues/134) — the +generic raster render abstraction deferred from ADR-0006 (which delivered the +live tile cache, issue #121 Part B, and explicitly left "Part A — the generic +`RasterFieldSource` render abstraction" to this issue). Builds on +[ADR-0002](0002-web-mercator-scene-and-layer-model.md) (the Web-Mercator scene + +layer model — every raster layer stays a Web-Mercator scene object; the geo→mercator +warp moves *into* the shared renderer, not out of the scene model) and +[ADR-0006](0006-live-tile-cache-persistence.md) (the live tile cache, whose +persistence + opt-in-subscription contract this render migration leaves untouched). + +## Context + +Three raster layers each carried their own display path: + +- `raster/gggs_tile_layer.cpp` — GDAL-backed GGGS tiles. Offscreen FBO + a GLSL + 120 fragment shader (single-band R32F value texture → colormap LUT, per-tile + NoData discard), per-vertex geo→Web-Mercator tessellation, Nearest value-texture + filter. +- `ros/live_coverage/sonar_live_cache_layer.cpp` — in-memory live tiles. A + **verbatim copy** of that shader + tessellation, explicitly marked + `// NOTE: shader duplicated from GggsTileLayer; will unify via RasterFieldSource + in camp#134`. Only the data source differs (dequantized Float32 vs. GDAL read). +- `raster/raster_layer.cpp` — a single reprojected chart. A **separate** + QPainter/mipmap path: CPU `ColorMap` shading for scalar bands, a CPU RGBA + composite for palette/RGB, no GPU shader, no per-cell NoData discard on the GPU. + +Two consequences followed from the duplication: + +1. **The NaN NoData bug lived in two shaders.** GLSL `v == u_nodata` is *false* + when `v` is NaN (NaN compares unequal to everything). Chart and backscatter + stores use NaN as their NoData sentinel, so those cells rendered **opaque**. + The CPU range fold already excluded NaN (`std::isfinite`), so the CPU range and + the GPU discard disagreed. Fixing it in two places invites re-drift. + +2. **Every render change had to be made three times** (or twice plus a divergent + CPU path), and the two GL copies could silently fall out of sync. + +## Decision + +Extract a single, source-agnostic GL raster render path: + +- **`raster/raster_field_source.h`** — `RasterFieldSource`, the abstract supplier + of renderable raster items, plus the `RasterFieldItem` value type the renderer + consumes: + + ```cpp + struct RasterFieldItem { + enum class Format { Scalar, Rgba }; + QOpenGLTexture* texture; // R32F (Scalar) or pre-composited RGBA8 (Rgba) + Format format; + bool geographic; // true: warp [west,east]x[south,north] (deg) + double west, east, south, north;// false: already Web-Mercator metres (a quad) + bool has_nodata; float nodata; // Scalar finite-sentinel discard + }; + + class RasterFieldSource { + virtual QStringList bands() const = 0; + virtual RasterBandMeta metadata(const QString& band) const = 0; + virtual QList items() = 0; + virtual QPair dataRange() const = 0; + }; + ``` + +- **`raster/raster_gl_renderer.{h,cpp}`** — `RasterGlRenderer`, which owns the + per-GL-context state (the offscreen context + surface, the FBO, the **one** + compiled program, and the colormap LUT texture) and the **unified fragment + shader**. The geo→Web-Mercator vertex tessellation and the Nearest/Linear + data-texture filter selection live here, once. + +### The unified shader (where the NaN fix lands, once) + +```glsl +if(u_mode == 0) { // Scalar: R32F -> LUT + float v = texture2D(u_tex, v_texcoord).r; + if(v != v) discard; // NaN NoData (GLSL 1.20-portable) + if(u_has_nodata != 0 && v == u_nodata) discard; // finite sentinel (sidescan/0) + ...colormap LUT... +} else { // Rgba: sample directly, bypass LUT + vec4 c = texture2D(u_tex, v_texcoord); + if(c.a == 0.0) discard; // transparent NoData + gl_FragColor = vec4(c.rgb * c.a, c.a); // premultiplied (GL_ONE, 1-SRC_A) +} +``` + +`isnan()` is GLSL 1.30+; `v != v` is the portable 1.20 idiom and matches the +target `#version 120`. + +### Scalar vs. RGBA (the format/mode branch) + +`RasterFieldItem::Format` resolves issue #134's open question. Scalar bands +(GGGS, live, scalar charts) upload as R32F and shade through the colormap LUT. +Palette/RGB charts are **composited to RGBA8 on the CPU** (as RasterLayer does +today) and sampled directly — the LUT is bypassed and the NaN/finite discard does +not apply (transparency comes from the composited alpha). Per-band channel select +for colour files is **out of scope** (a future issue); RasterLayer continues to +composite on CPU. + +### Ownership & threading contract + +- **Textures are owned by the source** (`GggsTile`, the live `Entry`, `RasterLayer`), + not the renderer. The `QOpenGLTexture*` in a `RasterFieldItem` is non-owning and + valid only for the duration of the `renderToImage()` call it is passed to. +- **The renderer owns the GL context.** A source releases its own textures between + a `renderer.makeCurrent()` / `renderer.doneCurrent()` pair (teardown, band + switch). `items()` is called on the GUI/GL thread with that context current, so + a source may lazily upload textures inside it. +- **No GL off the GUI thread.** Async loads (GDAL `RasterIO`, dequantize) produce + CPU buffers off-thread; the texture upload happens on the GUI thread in the + ready/handler slot, exactly as before. + +### CPU range / GPU discard consistency + +All three sources fold their auto-range over **finite, non-NoData** cells +(`std::isfinite` in `GggsTile::loadPixels`, `SonarLiveTile::refoldRange`, and +`RasterLayer::loadAndReprojectFile`). The unified shader now discards NaN (`v != v`) +*and* the finite sentinel, so the CPU range and the GPU discard agree on exactly +which cells are "data". + +## Consequences + +| If we change… | Also update… | Included? | +|---|---|---| +| The raster shader / discard logic | Only `raster_gl_renderer.cpp` — one place | Yes | +| `RasterLayer` from QPainter → GL | Mipmap pyramid + `QPixmap` chain removed; a scalar colormap change now re-bakes the LUT instead of re-warping the file | Yes | +| Both GL layers lose their own shader | The `// NOTE: shader duplicated` marker in `sonar_live_cache_layer.cpp` is removed | Yes | +| `marine_colormap` GPU-lib migration | Per-band colormap defaults | **No — deferred** | + +- **ADR-0006 is untouched.** `SonarLiveCacheLayer`'s persistence (write-through / + warm-load), opt-in tile-stream subscription, and GUI-thread reconciler invariant + are all outside the render path. This migration swaps only `renderImage()`'s GL + internals for the shared renderer; subscription, reconcile, prune, and + write-through code is unchanged. +- **`marine_colormap` deferral.** camp keeps its internal `map::ColorMap` (baked to + the LUT). Migrating to the `marine_colormap` GPU-shader library + (`unh_marine_autonomy#175` / I4) — and the per-band colormap defaults it would + bring — is separable and deferred (a follow-up issue). +- **`bands()` / `metadata()`** are part of the captured interface (they formalize + the per-layer band/NoData access the context-menu band pickers already use); the + renderer itself consumes only `items()` + `dataRange()`. Unifying the band-picker + UI across the three layers behind this interface is left to a follow-up. + +## ADR references (corrected) + +The plan's first draft mislabeled the ADR table. For the record: + +| ADR | Title | Relevance | +|---|---|---| +| camp ADR-0001 | *`TopicBridge` and the executor contract* | ROS-callback → GUI-thread marshalling that `SonarLiveCacheLayer` relies on; unchanged here | +| camp ADR-0002 | *Web-Mercator scene, two-model split, depth-as-layer, shared map library* | All three layers remain Web-Mercator scene objects | +| camp ADR-0006 | *Live tile cache — persistence + opt-in subscription* | Governs `SonarLiveCacheLayer`; its contract is untouched by this render migration | + +(There is no camp "Adopt ADRs" ADR — that is a *workspace* ADR; the earlier table +wrongly attributed it to camp ADR-0001.) diff --git a/src/camp_map/raster/raster_field_source.h b/src/camp_map/raster/raster_field_source.h new file mode 100644 index 0000000..59c27ac --- /dev/null +++ b/src/camp_map/raster/raster_field_source.h @@ -0,0 +1,98 @@ +#ifndef RASTER_RASTER_FIELD_SOURCE_H +#define RASTER_RASTER_FIELD_SOURCE_H + +#include +#include +#include +#include + +class QOpenGLTexture; + +namespace camp +{ +namespace raster +{ + +/// [camp#134] One renderable raster patch handed to RasterGlRenderer. See ADR-0007. +/// +/// The texture is OWNED BY THE SOURCE (a GggsTile, a live-cache Entry, or a +/// RasterLayer); the pointer is non-owning and valid only for the duration of the +/// renderToImage() call it is passed to. +/// +/// A `Scalar` item carries a single-band R32F value texture shaded through the +/// colormap LUT (with NaN + finite-sentinel NoData discard). An `Rgba` item +/// carries a pre-composited RGBA8 texture sampled directly — the LUT is bypassed +/// and transparency comes from the composited alpha (per-band channel select for +/// colour files is a future issue; RasterLayer composites on the CPU as today). +struct RasterFieldItem +{ + enum class Format + { + Scalar, ///< R32F value texture -> colormap LUT + Rgba ///< pre-composited RGBA8 texture, LUT bypassed + }; + + QOpenGLTexture* texture = nullptr; + Format format = Format::Scalar; + + /// Extent of the patch. When `geographic` is true the bounds are degrees and the + /// renderer subdivides in latitude and warps each vertex to Web-Mercator + /// (web_mercator::geoToMap) — the GGGS / live-tile path, where tiles stay in + /// lat/lon and are warped at display time. When false the bounds are already + /// Web-Mercator metres (a single linear quad, no warp) — the RasterLayer path, + /// where GDAL reprojected the chart to EPSG:3857 on load. + bool geographic = true; + double west = 0.0; ///< min lon (deg) or min x (metres) + double east = 0.0; ///< max lon (deg) or max x (metres) + double south = 0.0; ///< min lat (deg) or min y (metres) + double north = 0.0; ///< max lat (deg) or max y (metres) + + /// Scalar only: discard cells exactly equal to `nodata` (the finite sentinel). + /// NaN cells are always discarded by the shader regardless of these. + bool has_nodata = false; + float nodata = 0.0f; +}; + +/// [camp#134] Per-band metadata, surfaced for the (per-layer) band picker. +struct RasterBandMeta +{ + bool has_nodata = false; + float nodata = 0.0f; + QString units; +}; + +/// [camp#134] Source-agnostic supplier of renderable raster items for +/// RasterGlRenderer. GggsTileLayer (GDAL tiles), SonarLiveCacheLayer (in-memory +/// live tiles) and RasterLayer (a single reprojected chart) implement it so the GL +/// shader, the geo→Web-Mercator tessellation, and the colormap LUT live ONCE in +/// the renderer rather than being duplicated per layer. See ADR-0007. +/// +/// **Threading / ownership.** `items()` is called on the GUI/GL thread with the +/// renderer's context current, so a source may lazily upload its textures inside +/// it; the returned texture pointers are owned by the source and stay valid until +/// the enclosing renderToImage() call completes. +class RasterFieldSource +{ +public: + virtual ~RasterFieldSource() = default; + + /// Selectable bands (e.g. {"1","2"} for GggsTile's 1-indexed bands, or the + /// named live-cache bands). Backs the per-layer band picker. + virtual QStringList bands() const = 0; + + /// NoData sentinel + units for @p band (one of bands()). + virtual RasterBandMeta metadata(const QString& band) const = 0; + + /// Items to draw for the current selection. Textures are uploaded lazily here + /// (context current) and owned by the source. + virtual QList items() = 0; + + /// Colormap range [min,max] over the Scalar items; crossed (min > max) when + /// there is no data yet. Ignored by Rgba items. + virtual QPair dataRange() const = 0; +}; + +} // namespace raster +} // namespace camp + +#endif diff --git a/src/camp_map/raster/raster_gl_renderer.cpp b/src/camp_map/raster/raster_gl_renderer.cpp new file mode 100644 index 0000000..c5a7ef0 --- /dev/null +++ b/src/camp_map/raster/raster_gl_renderer.cpp @@ -0,0 +1,336 @@ +#include "raster_gl_renderer.h" + +#include "../map_view/web_mercator.h" + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include + +namespace camp +{ +namespace raster +{ + +namespace +{ + +// Vertex shader: purely linear. The geo->Web-Mercator warp is done on the CPU in +// double precision (web_mercator::geoToMap) per mesh vertex, with positions made +// RELATIVE to the extent origin so values stay small. This is deliberate: +// computing y = R*asinh(tan phi) in the shader used GPU transcendentals (tan/log/ +// sqrt) whose low precision, multiplied by R~6.4e6, produced a ~50 m latitude +// error (longitude was exact because x = R*lambda needs no transcendental). +constexpr char kVertexShader[] = R"( +#version 120 +attribute vec2 a_pos; // local Web-Mercator metres (from extent origin) +attribute vec2 a_texcoord; +uniform mat4 u_mvp; // local metres -> NDC +varying vec2 v_texcoord; +void main() +{ + gl_Position = u_mvp * vec4(a_pos, 0.0, 1.0); + v_texcoord = a_texcoord; +} +)"; + +// [camp#134] The ONE unified fragment shader (consolidated from GggsTileLayer + +// SonarLiveCacheLayer, and now serving RasterLayer too). Two modes: +// +// - Scalar (u_mode == 0): single-band value texture (R32F) auto-ranged and mapped +// through the colormap LUT. The NaN NoData fix lands HERE, once: `v != v` +// discards NaN sentinels (chart/backscatter stores) — the GLSL 1.20-portable +// idiom (isnan() is 1.30+). The finite-sentinel discard (v == u_nodata, e.g. +// sidescan 9999/0) follows. Premultiplied-opaque output (alpha 1). +// +// - Rgba (u_mode == 1): a pre-composited RGBA8 texture sampled directly, BYPASSING +// the LUT (RasterLayer's palette/RGB charts). Transparency comes from the +// composited alpha; fully-transparent cells discard. Output is premultiplied to +// match the GL_ONE / GL_ONE_MINUS_SRC_ALPHA blend. +constexpr char kFragmentShader[] = R"( +#version 120 +uniform sampler2D u_tex; // unit 0: Scalar R32F data, or Rgba RGBA8 +uniform sampler2D u_lut; // unit 1: colormap LUT (256x1 RGBA) +uniform float u_min; +uniform float u_max; +uniform int u_has_nodata; // Scalar: nonzero => discard v == u_nodata +uniform float u_nodata; +uniform int u_mode; // 0 = Scalar (LUT), 1 = Rgba (direct) +varying vec2 v_texcoord; +void main() +{ + if(u_mode == 0) + { + float v = texture2D(u_tex, v_texcoord).r; + if(v != v) + discard; // NaN NoData (1.20-portable) + if(u_has_nodata != 0 && v == u_nodata) + discard; // finite sentinel + float t = clamp((v - u_min) / max(u_max - u_min, 1.0), 0.0, 1.0); + vec4 c = texture2D(u_lut, vec2(t, 0.5)); + gl_FragColor = vec4(c.rgb, 1.0); + } + else + { + vec4 c = texture2D(u_tex, v_texcoord); + if(c.a == 0.0) + discard; + gl_FragColor = vec4(c.rgb * c.a, c.a); + } +} +)"; + +} // namespace + +RasterGlRenderer::RasterGlRenderer() = default; + +RasterGlRenderer::~RasterGlRenderer() +{ + if(gl_context_ && gl_surface_ && gl_context_->makeCurrent(gl_surface_)) + { + releaseGL(); + gl_context_->doneCurrent(); + } + delete gl_context_; gl_context_ = nullptr; + delete gl_surface_; gl_surface_ = nullptr; +} + +bool RasterGlRenderer::ensureGL() +{ + // gl_failed_ first: once GL is declared broken (creation OR a mid-session + // makeCurrent failure), stay failed and don't retry — otherwise every repaint + // re-enters and re-warns. + if(gl_failed_) + return false; + if(gl_context_) + return true; + + gl_surface_ = new QOffscreenSurface(); + gl_surface_->create(); + gl_context_ = new QOpenGLContext(); + if(!gl_surface_->isValid() || !gl_context_->create()) + { + qWarning("RasterGlRenderer: offscreen GL unavailable; raster not rendered"); + gl_failed_ = true; + delete gl_context_; gl_context_ = nullptr; + delete gl_surface_; gl_surface_ = nullptr; + return false; + } + return true; +} + +bool RasterGlRenderer::makeCurrent() +{ + if(!ensureGL()) + return false; + if(!gl_context_->makeCurrent(gl_surface_)) + { + qWarning("RasterGlRenderer: makeCurrent failed; raster not rendered"); + gl_failed_ = true; + return false; + } + return true; +} + +void RasterGlRenderer::doneCurrent() +{ + if(gl_context_) + gl_context_->doneCurrent(); +} + +bool RasterGlRenderer::ensureProgram() +{ + if(program_) + return program_->isLinked(); + program_ = std::make_unique(); + program_->addShaderFromSourceCode(QOpenGLShader::Vertex, kVertexShader); + program_->addShaderFromSourceCode(QOpenGLShader::Fragment, kFragmentShader); + if(!program_->link()) + { + qWarning("RasterGlRenderer: shader link failed: %s", + program_->log().toUtf8().constData()); + return false; + } + return true; +} + +QOpenGLTexture* RasterGlRenderer::ensureLut() +{ + // Bake camp::map::ColorMap into a 256x1 RGBA LUT (re-baked when the ramp + // changes). Sampled by the fragment shader as the colour transfer. + if(lut_texture_ && !lut_dirty_) + return lut_texture_.get(); + std::vector lut(256 * 4); + for(int i = 0; i < 256; ++i) + { + const QColor c = colormap_.colorNormalized(i / 255.0); + lut[i * 4 + 0] = uchar(c.red()); + lut[i * 4 + 1] = uchar(c.green()); + lut[i * 4 + 2] = uchar(c.blue()); + lut[i * 4 + 3] = uchar(c.alpha()); + } + if(!lut_texture_) + { + lut_texture_ = std::make_unique(QOpenGLTexture::Target2D); + lut_texture_->setFormat(QOpenGLTexture::RGBA8_UNorm); + lut_texture_->setSize(256, 1); + lut_texture_->setMipLevels(1); + lut_texture_->allocateStorage(QOpenGLTexture::RGBA, QOpenGLTexture::UInt8); + lut_texture_->setMinMagFilters(QOpenGLTexture::Linear, QOpenGLTexture::Linear); + lut_texture_->setWrapMode(QOpenGLTexture::ClampToEdge); + } + lut_texture_->setData(QOpenGLTexture::RGBA, QOpenGLTexture::UInt8, lut.data()); + lut_dirty_ = false; + return lut_texture_.get(); +} + +void RasterGlRenderer::setColormap(map::ColorMap::Type type) +{ + if(type == colormap_.type()) + return; + colormap_.setType(type); + lut_dirty_ = true; +} + +QImage RasterGlRenderer::renderToImage(const QList& items, + const QRectF& scene_bounds, float data_min, + float data_max, const QSize& size) +{ + if(items.isEmpty() || size.isEmpty() || !gl_context_) + return QImage(); + + QOpenGLFunctions* f = gl_context_->functions(); + if(!fbo_ || fbo_->size() != size) + fbo_ = std::make_unique(size); + + fbo_->bind(); + f->glViewport(0, 0, size.width(), size.height()); + f->glClearColor(0.0f, 0.0f, 0.0f, 0.0f); + f->glClear(GL_COLOR_BUFFER_BIT); + f->glDisable(GL_DEPTH_TEST); + f->glEnable(GL_BLEND); + f->glBlendFunc(GL_ONE, GL_ONE_MINUS_SRC_ALPHA); + + if(ensureProgram()) + { + // Vertices are LOCAL Web-Mercator metres from the extent origin, so map + // [0, width] x [0, height] -> NDC. origin = scene_bounds top-left; the per-item + // warp subtracts it so the float positions stay small (precise at large + // Web-Mercator magnitudes). The layer's fromScale(1,-1) + NW anchor draws the + // result upright (north-up). + const double origin_x = scene_bounds.left(); + const double origin_y = scene_bounds.top(); + const double width_m = scene_bounds.width(); + const double height_m = scene_bounds.height(); + QMatrix4x4 mvp; + mvp.ortho(0.0f, float(width_m), 0.0f, float(height_m), -1.0f, 1.0f); + + program_->bind(); + program_->setUniformValue("u_mvp", mvp); + program_->setUniformValue("u_min", data_min); + program_->setUniformValue("u_max", data_max); + program_->setUniformValue("u_tex", 0); + program_->setUniformValue("u_lut", 1); + QOpenGLTexture* lut = ensureLut(); + if(lut) + lut->bind(1); + + const int pos_loc = program_->attributeLocation("a_pos"); + const int texcoord_loc = program_->attributeLocation("a_texcoord"); + program_->enableAttributeArray(pos_loc); + program_->enableAttributeArray(texcoord_loc); + + std::vector verts; + for(const RasterFieldItem& item : items) + { + if(!item.texture) + continue; + + // Per-item triangle strip. A geographic item subdivides in latitude and warps + // each row to Web-Mercator (the GGGS / live path); a non-geographic item is + // already in Web-Mercator metres, so a single linear quad (2 rows) suffices + // (the RasterLayer path, reprojected by GDAL on load). Interleaved + // [local_x, local_y, u, v] per vertex; texture row 0 = north (v = 0). + const int rows = item.geographic ? (kLatSubdivisions + 1) : 2; + verts.clear(); + verts.reserve(rows * 2 * 4); + for(int r = 0; r < rows; ++r) + { + const double frac = double(r) / (rows - 1); + const float v = float(frac); // tex row 0 = north + QPointF l, rt; + if(item.geographic) + { + const double lat = item.north + (item.south - item.north) * frac; + l = web_mercator::geoToMap(QGeoCoordinate(lat, item.west)); + rt = web_mercator::geoToMap(QGeoCoordinate(lat, item.east)); + } + else + { + // Already Web-Mercator metres; north (max y) at frac 0, linear to south. + const double y = item.north + (item.south - item.north) * frac; + l = QPointF(item.west, y); + rt = QPointF(item.east, y); + } + verts.insert(verts.end(), + {float(l.x() - origin_x), float(l.y() - origin_y), 0.0f, v}); + verts.insert(verts.end(), + {float(rt.x() - origin_x), float(rt.y() - origin_y), 1.0f, v}); + } + + const bool scalar = (item.format == RasterFieldItem::Format::Scalar); + // [camp#134] Data-texture filter set HERE, once. Scalar: Nearest so the + // exact-equality NoData discard never blends across a sentinel boundary (the + // halo bug, camp#122). Rgba: Linear, and LinearMipMapLinear when the source + // supplied mipmaps (RasterLayer's charts) — that restores the LOD the old + // QPainter mipmap pyramid provided, so a chart zoomed far out doesn't shimmer. + if(scalar) + item.texture->setMinMagFilters(QOpenGLTexture::Nearest, + QOpenGLTexture::Nearest); + else + item.texture->setMinMagFilters( + item.texture->mipLevels() > 1 ? QOpenGLTexture::LinearMipMapLinear + : QOpenGLTexture::Linear, + QOpenGLTexture::Linear); + item.texture->bind(0); + program_->setUniformValue("u_mode", scalar ? 0 : 1); + program_->setUniformValue("u_has_nodata", + (scalar && item.has_nodata) ? 1 : 0); + program_->setUniformValue("u_nodata", + (scalar && item.has_nodata) ? item.nodata : 0.0f); + program_->setAttributeArray(pos_loc, GL_FLOAT, verts.data(), 2, + 4 * sizeof(float)); + program_->setAttributeArray(texcoord_loc, GL_FLOAT, verts.data() + 2, 2, + 4 * sizeof(float)); + f->glDrawArrays(GL_TRIANGLE_STRIP, 0, rows * 2); + item.texture->release(0); + } + + if(lut) + lut->release(1); + program_->disableAttributeArray(pos_loc); + program_->disableAttributeArray(texcoord_loc); + program_->release(); + } + + fbo_->release(); + QImage image = fbo_->toImage(); // top-down ARGB32 (premultiplied) + return image; +} + +void RasterGlRenderer::releaseGL() +{ + fbo_.reset(); + program_.reset(); + lut_texture_.reset(); +} + +} // namespace raster +} // namespace camp diff --git a/src/camp_map/raster/raster_gl_renderer.h b/src/camp_map/raster/raster_gl_renderer.h new file mode 100644 index 0000000..a5339d6 --- /dev/null +++ b/src/camp_map/raster/raster_gl_renderer.h @@ -0,0 +1,102 @@ +#ifndef RASTER_RASTER_GL_RENDERER_H +#define RASTER_RASTER_GL_RENDERER_H + +#include "raster_field_source.h" +#include "../map/color_map.h" + +#include +#include +#include +#include +#include + +class QOpenGLContext; +class QOffscreenSurface; +class QOpenGLFramebufferObject; +class QOpenGLShaderProgram; +class QOpenGLTexture; + +namespace camp +{ +namespace raster +{ + +/// [camp#134] The ONE shared GL raster render path (see ADR-0007). Owns the +/// per-GL-context state — an offscreen context + surface, the FBO, the single +/// compiled shader program, and the colormap LUT texture — and the unified +/// fragment shader. The geo→Web-Mercator vertex tessellation and the data-texture +/// filter (Nearest for Scalar, Linear for Rgba) live here so GggsTileLayer, +/// SonarLiveCacheLayer and RasterLayer no longer each carry a copy. +/// +/// Each layer holds one RasterGlRenderer (so each layer keeps its own offscreen +/// context, as before). Usage from a layer's renderImage(): +/// ``` +/// if(!renderer_.makeCurrent()) return QImage(); // GL unavailable -> null +/// QList items = collectItems(); // uploads textures here +/// QImage img = renderer_.renderToImage(items, scene_bounds_, min, max, size); +/// renderer_.doneCurrent(); +/// ``` +/// The textures in @p items are owned by the caller's sources (GggsTile, live +/// Entry, RasterLayer); the renderer never takes ownership. +class RasterGlRenderer +{ +public: + RasterGlRenderer(); + ~RasterGlRenderer(); + + RasterGlRenderer(const RasterGlRenderer&) = delete; + RasterGlRenderer& operator=(const RasterGlRenderer&) = delete; + + /// Lazily create + make current this renderer's offscreen GL context. Returns + /// false (latched, so it never re-warns) if GL is unavailable or makeCurrent + /// fails. A caller releases its own source textures between makeCurrent() and + /// doneCurrent() (teardown / band switch); items() likewise uploads under it. + bool makeCurrent(); + void doneCurrent(); + + /// Whether the offscreen context already exists (created on first makeCurrent()). + /// Lets a caller skip a texture-release pass before the layer has ever rendered + /// (no context, no textures) without forcing the context into existence early. + bool hasContext() const { return gl_context_ != nullptr; } + + /// Warp @p items into an offscreen image of @p size spanning @p scene_bounds + /// (the layer's Web-Mercator extent), with the colormap range [@p data_min, + /// @p data_max] for Scalar items. The context MUST already be current (see + /// makeCurrent()). Returns a null image if the shader fails to compile. + QImage renderToImage(const QList& items, + const QRectF& scene_bounds, float data_min, float data_max, + const QSize& size); + + /// Select the colour ramp baked into the LUT (re-baked on next render). + void setColormap(map::ColorMap::Type type); + map::ColorMap::Type colormap() const { return colormap_.type(); } + + /// Release the FBO / program / LUT. Safe to call with no context; the dtor + /// makes the context current first and then destroys it. + void releaseGL(); + +private: + bool ensureGL(); + bool ensureProgram(); + QOpenGLTexture* ensureLut(); + + // Latitude tessellation per geographic item: longitude is linear in + // Web-Mercator, latitude is the lone nonlinearity. 16 strips is sub-pixel over a + // tile at these zooms (matches the value previously hard-coded in both layers). + static constexpr int kLatSubdivisions = 16; + + QOpenGLContext* gl_context_ = nullptr; + QOffscreenSurface* gl_surface_ = nullptr; + std::unique_ptr fbo_; + std::unique_ptr program_; + std::unique_ptr lut_texture_; // colormap LUT (256x1 RGBA) + + map::ColorMap colormap_{map::ColorMap::Grayscale}; + bool lut_dirty_ = true; + bool gl_failed_ = false; +}; + +} // namespace raster +} // namespace camp + +#endif From 26bedbafb9927d06759dc5c55f036cd080078553 Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Mon, 29 Jun 2026 04:50:32 +0000 Subject: [PATCH 06/17] camp#134: migrate GggsTileLayer onto the shared renderer Delete this layer's kVertex/kFragment shaders, ensureProgram(), ensureLut(), per-vertex tessellation and its own GL context/FBO/program/LUT. The layer now holds a RasterGlRenderer, implements RasterFieldSource (bands/metadata/items/ dataRange), and renderImage() collects the loaded tiles as Scalar items and delegates the warp + draw. Tile textures stay owned by the GggsTiles and are released under the renderer's context in the dtor / band switch. QtConcurrent load + atomic pixelsLoaded() publish are unchanged. The colormap is now held by the renderer (setColormap re-bakes the LUT). Behaviour is identical plus the NaN NoData fix inherited from the unified shader. Co-Authored-By: Claude Opus 4.8 --- src/camp_map/raster/gggs_tile_layer.cpp | 362 ++++++------------------ src/camp_map/raster/gggs_tile_layer.h | 47 ++- 2 files changed, 105 insertions(+), 304 deletions(-) diff --git a/src/camp_map/raster/gggs_tile_layer.cpp b/src/camp_map/raster/gggs_tile_layer.cpp index 80ebbce..412afea 100644 --- a/src/camp_map/raster/gggs_tile_layer.cpp +++ b/src/camp_map/raster/gggs_tile_layer.cpp @@ -5,20 +5,12 @@ #include "../map_view/web_mercator.h" #include -#include -#include #include #include #include #include #include #include -#include -#include -#include -#include -#include -#include #include #include #include @@ -36,54 +28,11 @@ namespace raster namespace { -// Vertex shader: purely linear. The geo->Web-Mercator warp is done on the CPU in -// double precision (web_mercator::geoToMap) per mesh vertex, with positions made -// RELATIVE to the extent origin so values stay small. This is deliberate: -// computing y = R*asinh(tan phi) in the shader used GPU transcendentals (tan/log/ -// sqrt) whose low precision, multiplied by R~6.4e6, produced a ~50 m latitude -// error (longitude was exact because x = R*lambda needs no transcendental). -constexpr char kVertexShader[] = R"( -#version 120 -attribute vec2 a_pos; // local Web-Mercator metres (from extent origin) -attribute vec2 a_texcoord; -uniform mat4 u_mvp; // local metres -> NDC -varying vec2 v_texcoord; -void main() -{ - gl_Position = u_mvp * vec4(a_pos, 0.0, 1.0); - v_texcoord = a_texcoord; -} -)"; - -// Fragment shader: auto-ranged value mapped through a colormap LUT (the shared -// camp::map::ColorMap baked to a 256x1 RGBA texture on the CPU). Each tile's -// per-band NoData (plumbed in as u_has_nodata/u_nodata from GggsTile) is discarded -// so empty cells are transparent; tiles without NoData discard nothing. -// Premultiplied-alpha output (opaque, so straight == premult). -// -// [camp#122] The discard now tests the band's actual NoData sentinel rather than -// the old hardcoded `v <= 0.0` (which assumed the mosaicker's floor-to-1 depth -// convention and wrongly discarded valid 0/negative samples in uncertainty, -// quality, or signed-offset bands after #108 enabled arbitrary band selection). -constexpr char kFragmentShader[] = R"( -#version 120 -uniform sampler2D u_tex; // unit 0: single-band data (R32F) -uniform sampler2D u_lut; // unit 1: colormap LUT (256x1 RGBA) -uniform float u_min; -uniform float u_max; -uniform int u_has_nodata; // 0 = tile has no NoData; nonzero = discard v == u_nodata -uniform float u_nodata; // per-tile NoData sentinel (band-specific) -varying vec2 v_texcoord; -void main() -{ - float v = texture2D(u_tex, v_texcoord).r; - if(u_has_nodata != 0 && v == u_nodata) - discard; - float t = clamp((v - u_min) / max(u_max - u_min, 1.0), 0.0, 1.0); - vec4 c = texture2D(u_lut, vec2(t, 0.5)); - gl_FragColor = vec4(c.rgb, 1.0); -} -)"; +// [camp#134] The kVertex/kFragment shaders, ensureProgram(), ensureLut() and the +// per-vertex geo->Web-Mercator tessellation that used to live here moved into the +// shared raster::RasterGlRenderer (see ADR-0007). The unified shader there also +// fixes the NaN NoData discard (`v != v`). This layer now only collects loaded +// tiles into RasterFieldItems and delegates the draw. // [camp#126] Tree-view display name for a flat store layer: the last two path // components ("parent/leaf", e.g. "sidescan/processed"), falling back to just the @@ -145,7 +94,15 @@ GggsTileLayer::~GggsTileLayer() abort_flag_ = true; abort_flag_mutex_.unlock(); future_watcher_.waitForFinished(); - releaseGL(); + // [camp#134] Release each tile's GL texture under the renderer's context (the + // textures are owned by the GggsTiles, not the renderer). The renderer's own + // program/LUT/FBO/context are freed by its destructor right after this. + if(renderer_.makeCurrent()) + { + for(auto& tile : tiles_) + tile->releaseGL(); + renderer_.doneCurrent(); + } } void GggsTileLayer::loadDirectory(const QString& directory) @@ -383,195 +340,79 @@ QRectF GggsTileLayer::boundingRect() const return QRectF(QPointF(0.0, 0.0), scene_bounds_.size()); } -bool GggsTileLayer::ensureGL() +QStringList GggsTileLayer::bands() const { - // gl_failed_ first: once GL is declared broken (creation OR a mid-session - // makeCurrent failure), stay failed and don't retry — otherwise every repaint - // re-enters renderImage (cached_image_ never populates) and re-warns. - if(gl_failed_) - return false; - if(gl_context_) - return true; - - gl_surface_ = new QOffscreenSurface(); - gl_surface_->create(); - gl_context_ = new QOpenGLContext(); - if(!gl_surface_->isValid() || !gl_context_->create()) - { - qWarning("GggsTileLayer: offscreen GL unavailable; tiles not rendered"); - gl_failed_ = true; - delete gl_context_; gl_context_ = nullptr; - delete gl_surface_; gl_surface_ = nullptr; - return false; - } - return true; + // [camp#134] GggsTile bands are 1-indexed; expose them as "1".."N" for the + // (per-layer) band picker. The renderer consumes only items()/dataRange(). + QStringList result; + const int count = bandCount(); + for(int b = 1; b <= count; ++b) + result << QString::number(b); + return result; } -bool GggsTileLayer::ensureProgram() +RasterBandMeta GggsTileLayer::metadata(const QString&) const { - if(program_) - return program_->isLinked(); - program_ = std::make_unique(); - program_->addShaderFromSourceCode(QOpenGLShader::Vertex, kVertexShader); - program_->addShaderFromSourceCode(QOpenGLShader::Fragment, kFragmentShader); - if(!program_->link()) - { - qWarning("GggsTileLayer: shader link failed: %s", - program_->log().toUtf8().constData()); - setStatus("(shader error)"); - return false; - } - return true; + // [camp#134] NoData is per-tile/per-band and only known after a tile loads, so + // the authoritative sentinel travels per item (see items()); this layer-level + // metadata is a placeholder for a future unified band picker. + return RasterBandMeta{}; } -QOpenGLTexture* GggsTileLayer::ensureLut() +QPair GggsTileLayer::dataRange() const { - // Bake camp::map::ColorMap into a 256x1 RGBA LUT (re-baked when the ramp - // changes). Sampled by the fragment shader as the colour transfer. - if(lut_texture_ && !lut_dirty_) - return lut_texture_.get(); - std::vector lut(256 * 4); - for(int i = 0; i < 256; ++i) - { - const QColor c = colormap_.colorNormalized(i / 255.0); - lut[i * 4 + 0] = uchar(c.red()); - lut[i * 4 + 1] = uchar(c.green()); - lut[i * 4 + 2] = uchar(c.blue()); - lut[i * 4 + 3] = uchar(c.alpha()); - } - if(!lut_texture_) + return {float(data_min_), float(data_max_)}; +} + +QList GggsTileLayer::items() +{ + // [camp#134] Collect the loaded tiles as Scalar items for the shared renderer. + // Called with the renderer's GL context current (renderImage()), so tile-> + // texture() may lazily upload here. The geo->Web-Mercator warp + tessellation + // now lives in the renderer, so this only forwards each tile's lat/lon extent + + // per-tile NoData sentinel. + QList result; + result.reserve(int(tiles_.size())); + for(auto& tile : tiles_) { - lut_texture_ = std::make_unique(QOpenGLTexture::Target2D); - lut_texture_->setFormat(QOpenGLTexture::RGBA8_UNorm); - lut_texture_->setSize(256, 1); - lut_texture_->setMipLevels(1); - lut_texture_->allocateStorage(QOpenGLTexture::RGBA, QOpenGLTexture::UInt8); - lut_texture_->setMinMagFilters(QOpenGLTexture::Linear, QOpenGLTexture::Linear); - lut_texture_->setWrapMode(QOpenGLTexture::ClampToEdge); + // [camp#102] Skip a tile whose pixels haven't loaded yet. pixelsLoaded() is an + // ACQUIRE load pairing with the worker's RELEASE store in loadPixels(), so once + // true the data_/texture() reads see the worker's completed writes — no race. + if(!tile->pixelsLoaded()) + continue; + QOpenGLTexture* texture = tile->texture(); + if(!texture) + continue; + RasterFieldItem item; + item.texture = texture; + item.format = RasterFieldItem::Format::Scalar; + item.geographic = true; + item.west = tile->minLon(); + item.east = tile->maxLon(); + item.south = tile->minLat(); + item.north = tile->maxLat(); + // [camp#122] Per-tile NoData: a non-uniform store can carry different NoData + // per tile/band, so it travels per item rather than as one layer uniform. + item.has_nodata = tile->hasNoData(); + item.nodata = tile->hasNoData() ? float(tile->noData()) : 0.0f; + result.push_back(item); } - lut_texture_->setData(QOpenGLTexture::RGBA, QOpenGLTexture::UInt8, lut.data()); - lut_dirty_ = false; - return lut_texture_.get(); + return result; } QImage GggsTileLayer::renderImage(const QSize& size) { if(tiles_.empty() || data_min_ > data_max_ || size.isEmpty()) return QImage(); - if(!ensureGL()) + // [camp#134] Make the renderer's context current, collect the loaded tiles + // (uploading their textures under it), then delegate the warp + draw. The + // returned image is top-down ARGB32 premultiplied (as before). + if(!renderer_.makeCurrent()) return QImage(); - if(!gl_context_->makeCurrent(gl_surface_)) - { - qWarning("GggsTileLayer: makeCurrent failed; tiles not rendered"); - gl_failed_ = true; - return QImage(); - } - - QOpenGLFunctions* f = gl_context_->functions(); - if(!fbo_ || fbo_->size() != size) - fbo_ = std::make_unique(size); - - fbo_->bind(); - f->glViewport(0, 0, size.width(), size.height()); - f->glClearColor(0.0f, 0.0f, 0.0f, 0.0f); - f->glClear(GL_COLOR_BUFFER_BIT); - f->glDisable(GL_DEPTH_TEST); - f->glEnable(GL_BLEND); - f->glBlendFunc(GL_ONE, GL_ONE_MINUS_SRC_ALPHA); - - if(ensureProgram()) - { - // Vertices are LOCAL Web-Mercator metres from the extent origin (the SW - // corner), so map [0, width] x [0, height] -> NDC. local-y 0 = south, - // height = north, so 'top' = height gives a NORTH-UP image (row 0 = north); - // the item's fromScale(1,-1) + NW anchor draws it upright. - const double origin_x = scene_bounds_.left(); // west - const double origin_y = scene_bounds_.top(); // south (smaller mercator-y) - const double width_m = scene_bounds_.width(); - const double height_m = scene_bounds_.height(); - QMatrix4x4 mvp; - mvp.ortho(0.0f, float(width_m), 0.0f, float(height_m), -1.0f, 1.0f); - - program_->bind(); - program_->setUniformValue("u_mvp", mvp); - program_->setUniformValue("u_min", float(data_min_)); - program_->setUniformValue("u_max", float(data_max_)); - program_->setUniformValue("u_tex", 0); - program_->setUniformValue("u_lut", 1); - QOpenGLTexture* lut = ensureLut(); - if(lut) - lut->bind(1); - - const int pos_loc = program_->attributeLocation("a_pos"); - const int texcoord_loc = program_->attributeLocation("a_texcoord"); - program_->enableAttributeArray(pos_loc); - program_->enableAttributeArray(texcoord_loc); - - // Per-tile triangle strip, subdivided in latitude only. Each vertex's - // Web-Mercator position is computed on the CPU (double precision) via - // web_mercator::geoToMap and made relative to the extent origin. Interleaved - // [local_x, local_y, u, v] per vertex. - const int rows = kLatSubdivisions + 1; - std::vector verts; - verts.reserve(rows * 2 * 4); - for(auto& tile : tiles_) - { - // [camp#102] Skip a tile whose pixels haven't loaded yet (in-flight or not - // yet kicked). pixelsLoaded() is an ACQUIRE load that pairs with the worker's - // RELEASE store in loadPixels() (issued after the data_ move), so once it - // reads true the subsequent data_/texture() reads are guaranteed to see the - // worker's completed writes — no race even while a worker is mid-flight on - // another tile. The layer also repaints on tilesReady() once the load - // completes (to fold the range). - if(!tile->pixelsLoaded()) - continue; - - verts.clear(); - const double min_lon = tile->minLon(); - const double max_lon = tile->maxLon(); - const double min_lat = tile->minLat(); - const double max_lat = tile->maxLat(); - for(int r = 0; r < rows; ++r) - { - const double frac = double(r) / kLatSubdivisions; - const double lat = max_lat + (min_lat - max_lat) * frac; // north -> south - const float v = float(frac); // tex row 0 = north - const QPointF l = web_mercator::geoToMap(QGeoCoordinate(lat, min_lon)); - const QPointF rt = web_mercator::geoToMap(QGeoCoordinate(lat, max_lon)); - verts.insert(verts.end(), - {float(l.x() - origin_x), float(l.y() - origin_y), 0.0f, v}); - verts.insert(verts.end(), - {float(rt.x() - origin_x), float(rt.y() - origin_y), 1.0f, v}); - } - - QOpenGLTexture* texture = tile->texture(); - if(!texture) - continue; - texture->bind(0); - // [camp#122] Per-tile NoData: a non-uniform store can carry different NoData - // values per tile/band, so these are set inside the loop (unlike u_min/u_max, - // which are a layer-global colormap range set once above). - program_->setUniformValue("u_has_nodata", tile->hasNoData() ? 1 : 0); - program_->setUniformValue("u_nodata", - tile->hasNoData() ? float(tile->noData()) : 0.0f); - program_->setAttributeArray(pos_loc, GL_FLOAT, verts.data(), 2, - 4 * sizeof(float)); - program_->setAttributeArray(texcoord_loc, GL_FLOAT, verts.data() + 2, 2, - 4 * sizeof(float)); - f->glDrawArrays(GL_TRIANGLE_STRIP, 0, rows * 2); - texture->release(0); - } - - if(lut) - lut->release(1); - program_->disableAttributeArray(pos_loc); - program_->disableAttributeArray(texcoord_loc); - program_->release(); - } - - fbo_->release(); - QImage image = fbo_->toImage(); // top-down ARGB32 (premultiplied) - gl_context_->doneCurrent(); + const QList draw = items(); + const QImage image = renderer_.renderToImage(draw, scene_bounds_, float(data_min_), + float(data_max_), size); + renderer_.doneCurrent(); return image; } @@ -618,28 +459,12 @@ void GggsTileLayer::paint(QPainter* painter, const QStyleOptionGraphicsItem*, QW painter->restore(); } -void GggsTileLayer::releaseGL() -{ - if(gl_context_ && gl_surface_ && gl_context_->makeCurrent(gl_surface_)) - { - fbo_.reset(); - program_.reset(); - lut_texture_.reset(); - for(auto& tile : tiles_) - tile->releaseGL(); - gl_context_->doneCurrent(); - } - delete gl_context_; gl_context_ = nullptr; - delete gl_surface_; gl_surface_ = nullptr; -} - void GggsTileLayer::setColormap(map::ColorMap::Type type) { - if(type == colormap_.type()) + if(type == renderer_.colormap()) return; - colormap_.setType(type); - lut_dirty_ = true; - cached_image_ = QImage(); // force a re-render with the new ramp + renderer_.setColormap(type); // re-bakes the LUT on next render + cached_image_ = QImage(); // force a re-render with the new ramp writeSettings(); update(boundingRect()); } @@ -719,25 +544,13 @@ void GggsTileLayer::applyBand(int band) // switch so the drop isn't invisible. // A null/not-yet-created context (the layer never painted — applyBand() can fire // from readSettings() before first paint) is expected: no textures exist yet, so - // the switch just clears pixels + reloads with no stale GL state. But if the - // context EXISTS and makeCurrent FAILS, the old-band textures can't be released; - // setBand() below still clears each tile's pixels, and texture() never refreshes - // an EXISTING texture, so a stale old-band texture would render against the new - // band's range once pixels reload. Match renderImage()'s response to a - // makeCurrent failure — mark GL failed so ensureGL() short-circuits and the layer - // stops rendering rather than drawing a stale-band frame. - bool have_context = false; - if(gl_context_ && gl_surface_) - { - if(gl_context_->makeCurrent(gl_surface_)) - have_context = true; - else - { - qWarning("GggsTileLayer: makeCurrent failed during band switch; " - "tiles not rendered"); - gl_failed_ = true; - } - } + // the switch just clears pixels + reloads with no stale GL state — skip the + // release pass entirely (hasContext() == false) rather than forcing the offscreen + // context into existence early. If the context EXISTS but makeCurrent FAILS, the + // renderer latches its GL-failed flag (renderImage() then returns null and the + // layer stops rendering rather than drawing a stale-band frame) and we leave the + // textures alone. + bool have_context = renderer_.hasContext() && renderer_.makeCurrent(); int dropped = 0; for(auto& tile : tiles_) { @@ -751,7 +564,7 @@ void GggsTileLayer::applyBand(int band) tile->setBand(band); } if(have_context) - gl_context_->doneCurrent(); + renderer_.doneCurrent(); if(dropped > 0) qWarning("GggsTileLayer: %d tile(s) lack band %d; left on their prior band", dropped, band); @@ -791,7 +604,7 @@ void GggsTileLayer::contextMenu(QMenu* menu) { QAction* action = colormap_menu->addAction(map::ColorMap::name(type)); action->setCheckable(true); - action->setChecked(type == colormap_.type()); + action->setChecked(type == renderer_.colormap()); connect(action, &QAction::triggered, this, [this, type]() { setColormap(type); }); } @@ -846,7 +659,7 @@ void GggsTileLayer::readSettings() // the first-run default flips. setVisible(settings.value("visible", false).toBool()); const map::ColorMap::Type type = map::ColorMap::typeFromName( - settings.value("colormap", map::ColorMap::name(colormap_.type())).toString()); + settings.value("colormap", map::ColorMap::name(renderer_.colormap())).toString()); // [camp#108] Persisted band (default 1). Applied via applyBand() below — the // non-persisting band switch (texture release + reload + range reset) — so the // read path does NOT write the value straight back out (setBand() would). Only @@ -855,10 +668,9 @@ void GggsTileLayer::readSettings() const int band = settings.value("band", 1).toInt(); settings.endGroup(); settings.endGroup(); - if(type != colormap_.type()) + if(type != renderer_.colormap()) { - colormap_.setType(type); - lut_dirty_ = true; + renderer_.setColormap(type); cached_image_ = QImage(); } if(band != band_) @@ -871,7 +683,7 @@ void GggsTileLayer::writeSettings() QSettings settings; settings.beginGroup("MapItem"); settings.beginGroup(settingsKey()); - settings.setValue("colormap", map::ColorMap::name(colormap_.type())); + settings.setValue("colormap", map::ColorMap::name(renderer_.colormap())); settings.setValue("band", band_); // [camp#108] selected band round-trips 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 0fa78ef..e3af854 100644 --- a/src/camp_map/raster/gggs_tile_layer.h +++ b/src/camp_map/raster/gggs_tile_layer.h @@ -3,6 +3,8 @@ #include "../map/layer.h" #include "../map/color_map.h" +#include "raster_field_source.h" +#include "raster_gl_renderer.h" #include #include @@ -11,12 +13,6 @@ #include #include -class QOpenGLContext; -class QOffscreenSurface; -class QOpenGLFramebufferObject; -class QOpenGLShaderProgram; -class QOpenGLTexture; - namespace camp { namespace raster @@ -42,7 +38,7 @@ class GggsTile; /// tessellation; whole-extent render cached by on-screen size. Multi-band /// GeoTIFFs expose a per-layer band picker (camp#108); visible-region-only /// render is Slice 2. -class GggsTileLayer: public map::Layer +class GggsTileLayer: public map::Layer, public RasterFieldSource { Q_OBJECT Q_INTERFACES(QGraphicsItem) @@ -119,6 +115,14 @@ class GggsTileLayer: public map::Layer /// (ADR-0005); a per-layer watcher is a follow-up. bool rescan(); + // [camp#134] RasterFieldSource: feed the shared RasterGlRenderer. bands() lists + // the 1-indexed tile bands; items() returns the loaded tiles as Scalar items + // (textures uploaded lazily under the renderer's current context). + QStringList bands() const override; + RasterBandMeta metadata(const QString& band) const override; + QList items() override; + QPair dataRange() const override; + protected: void contextMenu(QMenu* menu) override; void readSettings() override; @@ -150,16 +154,8 @@ private slots: /// [camp#102] Worker body (runs off-thread): loadPixels() each not-yet-loaded /// tile, honoring abort_flag_ between tiles. GDAL only — never touches GL. void loadTilesWorker(); - bool ensureGL(); - bool ensureProgram(); - QOpenGLTexture* ensureLut(); - void releaseGL(); - - // Latitude tessellation per tile. The geo->Web-Mercator warp is separable: - // longitude is linear, latitude is the lone nonlinearity. 16 strips is - // sub-pixel over a tile at these zooms (Slice 2 tunes this to a < 0.5 px - // budget and renders only the visible region for large surveys). - static constexpr int kLatSubdivisions = 16; + + // [camp#134] Latitude tessellation moved into RasterGlRenderer (the shared warp). static constexpr int kMaxImageEdge = 4096; // clamp the offscreen target QString directory_; @@ -186,18 +182,11 @@ private slots: QMutex abort_flag_mutex_; bool load_started_ = false; // first paint() kicks the load exactly once - // The layer's own offscreen GL context — created lazily, used only for the - // FBO render; never touches the GUI's context. - QOpenGLContext* gl_context_ = nullptr; - QOffscreenSurface* gl_surface_ = nullptr; - std::unique_ptr fbo_; - std::unique_ptr program_; - std::unique_ptr lut_texture_; // colormap LUT (256x1 RGBA) - bool gl_failed_ = false; - - // Default grayscale preserves the original look; selectable via context menu. - map::ColorMap colormap_{map::ColorMap::Grayscale}; - bool lut_dirty_ = true; // re-bake the LUT on next render after a change + // [camp#134] The shared GL raster renderer (its own offscreen context + the + // unified shader + colormap LUT). Replaces this layer's former duplicated + // shader/program/LUT/FBO. Default ramp is Grayscale (the renderer's default), + // preserving the original look; selectable via the context menu. + RasterGlRenderer renderer_; QImage cached_image_; // last render, reused on pan (re-rendered on zoom) QSize cached_size_; From f517d2748c8cdbde62ff1502440b7ec68bb1f7b7 Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Mon, 29 Jun 2026 04:50:41 +0000 Subject: [PATCH 07/17] camp#134: migrate SonarLiveCacheLayer onto the shared renderer Delete the duplicated shader block (and its `// NOTE: shader duplicated; unify via RasterFieldSource camp#134` marker), ensureProgram/ensureLut, the per-vertex tessellation, and the layer's own GL context/FBO/program/LUT. The layer now holds a RasterGlRenderer, implements RasterFieldSource, and renderImage() collects the held tiles' selected band as Scalar items and delegates to the renderer. Entry textures stay owned by the Entries (released under the renderer's context). ADR-0006 is untouched: persistence (write-through / warm-load), the opt-in tile-stream subscription, the catalog reconcile/prune, and the GUI-thread marshalling invariant are all outside the render path and unchanged here. Co-Authored-By: Claude Opus 4.8 --- .../live_coverage/sonar_live_cache_layer.cpp | 329 ++++++------------ .../live_coverage/sonar_live_cache_layer.h | 38 +- 2 files changed, 116 insertions(+), 251 deletions(-) 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 87a509d..bfc9291 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 @@ -4,18 +4,11 @@ #include "../../map_view/web_mercator.h" #include -#include #include #include #include -#include #include #include -#include -#include -#include -#include -#include #include #include #include @@ -39,42 +32,12 @@ namespace live_coverage namespace { -// NOTE: shader duplicated from GggsTileLayer; will unify via RasterFieldSource in -// camp#134. The pipeline is identical (CPU-side geo->Web-Mercator warp per vertex, -// single-band R32F value texture, colormap LUT, per-tile NoData discard); only the -// data source differs (in-memory dequantized Float32 vs. GggsTile's GDAL read). -constexpr char kVertexShader[] = R"( -#version 120 -attribute vec2 a_pos; -attribute vec2 a_texcoord; -uniform mat4 u_mvp; -varying vec2 v_texcoord; -void main() -{ - gl_Position = u_mvp * vec4(a_pos, 0.0, 1.0); - v_texcoord = a_texcoord; -} -)"; - -constexpr char kFragmentShader[] = R"( -#version 120 -uniform sampler2D u_tex; -uniform sampler2D u_lut; -uniform float u_min; -uniform float u_max; -uniform int u_has_nodata; -uniform float u_nodata; -varying vec2 v_texcoord; -void main() -{ - float v = texture2D(u_tex, v_texcoord).r; - if(u_has_nodata != 0 && v == u_nodata) - discard; - float t = clamp((v - u_min) / max(u_max - u_min, 1.0), 0.0, 1.0); - vec4 c = texture2D(u_lut, vec2(t, 0.5)); - gl_FragColor = vec4(c.rgb, 1.0); -} -)"; +// [camp#134] The duplicated kVertex/kFragment shaders, ensureProgram(), ensureLut() +// and the per-vertex geo->Web-Mercator tessellation moved into the shared +// raster::RasterGlRenderer (see ADR-0007), which also fixes the NaN NoData discard. +// This layer now collects its held tiles into RasterFieldItems and delegates the +// draw; only the data source differs (in-memory dequantized Float32 vs. GDAL read). +// ADR-0006's persistence + opt-in-subscription contract is untouched by this change. // Flat, filesystem-safe token for a source namespace (percent-encode every '/'). QString sanitize(const std::string& ns) @@ -142,7 +105,15 @@ SonarLiveCacheLayer::~SonarLiveCacheLayer() shutting_down_ = true; for(QFutureWatcher* watcher : write_watchers_) watcher->waitForFinished(); - releaseGL(); + // [camp#134] Release each tile's GL texture under the renderer's context (the + // textures are owned by the Entries, not the renderer). The renderer frees its + // own program/LUT/FBO/context in its destructor right after this. + if(renderer_.makeCurrent()) + { + for(auto& item : tiles_) + item.second.texture.reset(); + renderer_.doneCurrent(); + } } QString SonarLiveCacheLayer::settingsKey() const @@ -510,71 +481,6 @@ QRectF SonarLiveCacheLayer::boundingRect() const return QRectF(QPointF(0.0, 0.0), scene_bounds_.size()); } -bool SonarLiveCacheLayer::ensureGL() -{ - if(gl_failed_) - return false; - if(gl_context_) - return true; - gl_surface_ = new QOffscreenSurface(); - gl_surface_->create(); - gl_context_ = new QOpenGLContext(); - if(!gl_surface_->isValid() || !gl_context_->create()) - { - qWarning("SonarLiveCacheLayer: offscreen GL unavailable; tiles not rendered"); - gl_failed_ = true; - delete gl_context_; gl_context_ = nullptr; - delete gl_surface_; gl_surface_ = nullptr; - return false; - } - return true; -} - -bool SonarLiveCacheLayer::ensureProgram() -{ - if(program_) - return program_->isLinked(); - program_ = std::make_unique(); - program_->addShaderFromSourceCode(QOpenGLShader::Vertex, kVertexShader); - program_->addShaderFromSourceCode(QOpenGLShader::Fragment, kFragmentShader); - if(!program_->link()) - { - qWarning("SonarLiveCacheLayer: shader link failed: %s", - program_->log().toUtf8().constData()); - setStatus("(shader error)"); - return false; - } - return true; -} - -QOpenGLTexture* SonarLiveCacheLayer::ensureLut() -{ - if(lut_texture_ && !lut_dirty_) - return lut_texture_.get(); - std::vector lut(256 * 4); - for(int i = 0; i < 256; ++i) - { - const QColor c = colormap_.colorNormalized(i / 255.0); - lut[i * 4 + 0] = uchar(c.red()); - lut[i * 4 + 1] = uchar(c.green()); - lut[i * 4 + 2] = uchar(c.blue()); - lut[i * 4 + 3] = uchar(c.alpha()); - } - if(!lut_texture_) - { - lut_texture_ = std::make_unique(QOpenGLTexture::Target2D); - lut_texture_->setFormat(QOpenGLTexture::RGBA8_UNorm); - lut_texture_->setSize(256, 1); - lut_texture_->setMipLevels(1); - lut_texture_->allocateStorage(QOpenGLTexture::RGBA, QOpenGLTexture::UInt8); - lut_texture_->setMinMagFilters(QOpenGLTexture::Linear, QOpenGLTexture::Linear); - lut_texture_->setWrapMode(QOpenGLTexture::ClampToEdge); - } - lut_texture_->setData(QOpenGLTexture::RGBA, QOpenGLTexture::UInt8, lut.data()); - lut_dirty_ = false; - return lut_texture_.get(); -} - QOpenGLTexture* SonarLiveCacheLayer::textureFor(Entry& entry) { const SonarLiveBand* band = entry.tile.band(band_name_); @@ -582,121 +488,101 @@ QOpenGLTexture* SonarLiveCacheLayer::textureFor(Entry& entry) return nullptr; if(entry.texture && !entry.texture_dirty) return entry.texture.get(); - // (Re)upload the selected band as an R32F value texture, Nearest-filtered so the - // shader's exact-equality NoData discard never blends across a sentinel boundary - // (same rationale as GggsTile::texture()). + // (Re)upload the selected band as an R32F value texture. The Nearest filter (so + // the exact-equality NoData discard never blends across a sentinel boundary) is + // now applied by the renderer at draw time (camp#134); we just upload the data + // and the clamp wrap. entry.texture = std::make_unique(QOpenGLTexture::Target2D); entry.texture->setFormat(QOpenGLTexture::R32F); entry.texture->setSize(entry.tile.width(), entry.tile.height()); entry.texture->setMipLevels(1); entry.texture->allocateStorage(QOpenGLTexture::Red, QOpenGLTexture::Float32); entry.texture->setData(QOpenGLTexture::Red, QOpenGLTexture::Float32, band->data.data()); - entry.texture->setMinMagFilters(QOpenGLTexture::Nearest, QOpenGLTexture::Nearest); entry.texture->setWrapMode(QOpenGLTexture::ClampToEdge); entry.texture_dirty = false; return entry.texture.get(); } -QImage SonarLiveCacheLayer::renderImage(const QSize& size) +QStringList SonarLiveCacheLayer::bands() const { - if(tiles_.empty() || data_min_ > data_max_ || size.isEmpty()) - return QImage(); - if(!ensureGL()) - return QImage(); - if(!gl_context_->makeCurrent(gl_surface_)) - { - qWarning("SonarLiveCacheLayer: makeCurrent failed; tiles not rendered"); - gl_failed_ = true; - return QImage(); - } - - QOpenGLFunctions* f = gl_context_->functions(); - if(!fbo_ || fbo_->size() != size) - fbo_ = std::make_unique(size); - - fbo_->bind(); - f->glViewport(0, 0, size.width(), size.height()); - f->glClearColor(0.0f, 0.0f, 0.0f, 0.0f); - f->glClear(GL_COLOR_BUFFER_BIT); - f->glDisable(GL_DEPTH_TEST); - f->glEnable(GL_BLEND); - f->glBlendFunc(GL_ONE, GL_ONE_MINUS_SRC_ALPHA); - - if(ensureProgram()) - { - const double origin_x = scene_bounds_.left(); - const double origin_y = scene_bounds_.top(); - const double width_m = scene_bounds_.width(); - const double height_m = scene_bounds_.height(); - QMatrix4x4 mvp; - mvp.ortho(0.0f, float(width_m), 0.0f, float(height_m), -1.0f, 1.0f); - - program_->bind(); - program_->setUniformValue("u_mvp", mvp); - program_->setUniformValue("u_min", float(data_min_)); - program_->setUniformValue("u_max", float(data_max_)); - program_->setUniformValue("u_tex", 0); - program_->setUniformValue("u_lut", 1); - QOpenGLTexture* lut = ensureLut(); - if(lut) - lut->bind(1); - - const int pos_loc = program_->attributeLocation("a_pos"); - const int texcoord_loc = program_->attributeLocation("a_texcoord"); - program_->enableAttributeArray(pos_loc); - program_->enableAttributeArray(texcoord_loc); - - const int rows = kLatSubdivisions + 1; - std::vector verts; - verts.reserve(rows * 2 * 4); - for(auto& item : tiles_) + // [camp#134] Named live bands (union across held tiles), for the band picker. + QStringList result; + for(const auto& item : tiles_) + for(const auto& name : item.second.tile.bandNames()) { - Entry& entry = item.second; - const SonarLiveBand* band = entry.tile.band(band_name_); - if(!band) - continue; - QOpenGLTexture* texture = textureFor(entry); - if(!texture) - continue; - - verts.clear(); - const double min_lon = entry.tile.minLon(); - const double max_lon = entry.tile.maxLon(); - const double min_lat = entry.tile.minLat(); - const double max_lat = entry.tile.maxLat(); - for(int r = 0; r < rows; ++r) - { - const double frac = double(r) / kLatSubdivisions; - const double lat = max_lat + (min_lat - max_lat) * frac; // north -> south - const float v = float(frac); - const QPointF l = web_mercator::geoToMap(QGeoCoordinate(lat, min_lon)); - const QPointF rt = web_mercator::geoToMap(QGeoCoordinate(lat, max_lon)); - verts.insert(verts.end(), - {float(l.x() - origin_x), float(l.y() - origin_y), 0.0f, v}); - verts.insert(verts.end(), - {float(rt.x() - origin_x), float(rt.y() - origin_y), 1.0f, v}); - } + const QString q = QString::fromStdString(name); + if(!result.contains(q)) + result << q; + } + return result; +} - texture->bind(0); - program_->setUniformValue("u_has_nodata", band->has_nodata ? 1 : 0); - program_->setUniformValue("u_nodata", band->has_nodata ? band->nodata : 0.0f); - program_->setAttributeArray(pos_loc, GL_FLOAT, verts.data(), 2, 4 * sizeof(float)); - program_->setAttributeArray(texcoord_loc, GL_FLOAT, verts.data() + 2, 2, - 4 * sizeof(float)); - f->glDrawArrays(GL_TRIANGLE_STRIP, 0, rows * 2); - texture->release(0); +raster::RasterBandMeta SonarLiveCacheLayer::metadata(const QString& band) const +{ + // [camp#134] Surface the selected band's dequantized NoData sentinel from the + // first tile that carries it (the renderer reads NoData per item; this is for a + // future unified band picker). + raster::RasterBandMeta meta; + const std::string name = band.toStdString(); + for(const auto& item : tiles_) + if(const SonarLiveBand* b = item.second.tile.band(name)) + { + meta.has_nodata = b->has_nodata; + meta.nodata = b->nodata; + break; } + return meta; +} + +QPair SonarLiveCacheLayer::dataRange() const +{ + return {float(data_min_), float(data_max_)}; +} - if(lut) - lut->release(1); - program_->disableAttributeArray(pos_loc); - program_->disableAttributeArray(texcoord_loc); - program_->release(); +QList SonarLiveCacheLayer::items() +{ + // [camp#134] Collect the held tiles' selected band as Scalar items for the shared + // renderer. Called with the renderer's GL context current (renderImage()), so + // textureFor() may lazily (re)upload here. The geo->Web-Mercator warp lives in + // the renderer; this only forwards each tile's lat/lon extent + NoData sentinel. + QList result; + result.reserve(int(tiles_.size())); + for(auto& item : tiles_) + { + Entry& entry = item.second; + const SonarLiveBand* band = entry.tile.band(band_name_); + if(!band) + continue; + QOpenGLTexture* texture = textureFor(entry); + if(!texture) + continue; + raster::RasterFieldItem fi; + fi.texture = texture; + fi.format = raster::RasterFieldItem::Format::Scalar; + fi.geographic = true; + fi.west = entry.tile.minLon(); + fi.east = entry.tile.maxLon(); + fi.south = entry.tile.minLat(); + fi.north = entry.tile.maxLat(); + fi.has_nodata = band->has_nodata; + fi.nodata = band->has_nodata ? band->nodata : 0.0f; + result.push_back(fi); } + return result; +} - fbo_->release(); - QImage image = fbo_->toImage(); - gl_context_->doneCurrent(); +QImage SonarLiveCacheLayer::renderImage(const QSize& size) +{ + if(tiles_.empty() || data_min_ > data_max_ || size.isEmpty()) + return QImage(); + // [camp#134] Make the renderer's context current, collect the held tiles + // (uploading textures under it), then delegate the warp + draw. + if(!renderer_.makeCurrent()) + return QImage(); + const QList draw = items(); + const QImage image = renderer_.renderToImage(draw, scene_bounds_, float(data_min_), + float(data_max_), size); + renderer_.doneCurrent(); return image; } @@ -724,29 +610,13 @@ void SonarLiveCacheLayer::paint(QPainter* painter, const QStyleOptionGraphicsIte painter->restore(); } -void SonarLiveCacheLayer::releaseGL() -{ - if(gl_context_ && gl_surface_ && gl_context_->makeCurrent(gl_surface_)) - { - fbo_.reset(); - program_.reset(); - lut_texture_.reset(); - for(auto& item : tiles_) - item.second.texture.reset(); - gl_context_->doneCurrent(); - } - delete gl_context_; gl_context_ = nullptr; - delete gl_surface_; gl_surface_ = nullptr; -} - // ------------------------------- band / colormap ----------------------------- void SonarLiveCacheLayer::setColormap(map::ColorMap::Type type) { - if(type == colormap_.type()) + if(type == renderer_.colormap()) return; - colormap_.setType(type); - lut_dirty_ = true; + renderer_.setColormap(type); // re-bakes the LUT on next render cached_image_ = QImage(); writeSettings(); update(boundingRect()); @@ -795,7 +665,7 @@ void SonarLiveCacheLayer::contextMenu(QMenu* menu) { QAction* action = colormap_menu->addAction(map::ColorMap::name(type)); action->setCheckable(true); - action->setChecked(type == colormap_.type()); + action->setChecked(type == renderer_.colormap()); connect(action, &QAction::triggered, this, [this, type]() { setColormap(type); }); } @@ -831,17 +701,16 @@ void SonarLiveCacheLayer::readSettings() // turns coverage on explicitly. setVisible(settings.value("visible", false).toBool()); const map::ColorMap::Type type = map::ColorMap::typeFromName( - settings.value("colormap", map::ColorMap::name(colormap_.type())).toString()); + settings.value("colormap", map::ColorMap::name(renderer_.colormap())).toString()); const std::string band = settings.value("band", QString::fromStdString(band_name_)) .toString().toStdString(); const bool was_enabled = settings.value("live_enabled", false).toBool(); settings.endGroup(); settings.endGroup(); - if(type != colormap_.type()) + if(type != renderer_.colormap()) { - colormap_.setType(type); - lut_dirty_ = true; + renderer_.setColormap(type); cached_image_ = QImage(); } if(!band.empty()) @@ -858,7 +727,7 @@ void SonarLiveCacheLayer::writeSettings() QSettings settings; settings.beginGroup("MapItem"); settings.beginGroup(settingsKey()); - settings.setValue("colormap", map::ColorMap::name(colormap_.type())); + settings.setValue("colormap", map::ColorMap::name(renderer_.colormap())); settings.setValue("band", QString::fromStdString(band_name_)); settings.setValue("live_enabled", enabled_); 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 dc67775..fa6a40f 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 @@ -3,6 +3,8 @@ #include "../layer.h" #include "../../map/color_map.h" +#include "../../raster/raster_field_source.h" +#include "../../raster/raster_gl_renderer.h" #include "sonar_live_tile.h" #include "marine_tiled_raster_store/tile_catalog.hpp" @@ -21,10 +23,6 @@ #include #include -class QOpenGLContext; -class QOffscreenSurface; -class QOpenGLFramebufferObject; -class QOpenGLShaderProgram; class QOpenGLTexture; namespace camp @@ -57,7 +55,7 @@ namespace live_coverage /// reconcile / markHave / drop / applyPatch / write-through scheduling. Warm-load /// runs on the GUI thread before the subscriptions are created, so it cannot race a /// callback. -class SonarLiveCacheLayer: public Layer +class SonarLiveCacheLayer: public Layer, public raster::RasterFieldSource { Q_OBJECT Q_INTERFACES(QGraphicsItem) @@ -88,6 +86,14 @@ class SonarLiveCacheLayer: public Layer /// The layer's Web-Mercator extent (union of tile extents). Exposed for tests. QRectF sceneBounds() const { return scene_bounds_; } + // [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). + QStringList bands() const override; + raster::RasterBandMeta metadata(const QString& band) const override; + QList items() override; + QPair dataRange() const override; + public slots: /// [camp#121] Subscribe to the tile stream, warm-load the disk cache, start /// request/prune, and persist enabled=true. No-op if already enabled. @@ -147,14 +153,10 @@ private slots: void setColormap(map::ColorMap::Type type); void setBandName(const std::string& name); - // GL helpers (duplicated from GggsTileLayer; see camp#134). - bool ensureGL(); - bool ensureProgram(); - QOpenGLTexture* ensureLut(); + // [camp#134] (Re)upload the selected band of @p entry as an R32F value texture + // (owned by the Entry). The shader + LUT + tessellation now live in renderer_. QOpenGLTexture* textureFor(Entry& entry); - void releaseGL(); - static constexpr int kLatSubdivisions = 16; static constexpr int kMaxImageEdge = 4096; std::string base_namespace_; // e.g. "/cube_bathymetry" @@ -173,8 +175,6 @@ private slots: double data_max_ = 0.0; std::string band_name_; // selected band (persisted) - map::ColorMap colormap_{map::ColorMap::Grayscale}; - bool lut_dirty_ = true; rclcpp::Subscription::SharedPtr tile_sub_; rclcpp::Subscription::SharedPtr catalog_sub_; @@ -199,14 +199,10 @@ private slots: std::vector*> write_watchers_; bool shutting_down_ = false; - // Offscreen GL (the layer owns it; never touches the GUI context). Duplicated - // from GggsTileLayer — unify via RasterFieldSource camp#134. - QOpenGLContext* gl_context_ = nullptr; - QOffscreenSurface* gl_surface_ = nullptr; - std::unique_ptr fbo_; - std::unique_ptr program_; - std::unique_ptr lut_texture_; - bool gl_failed_ = false; + // [camp#134] The shared GL raster renderer (its own offscreen context + the + // unified shader + colormap LUT). Replaces the shader/program/LUT/FBO this layer + // used to duplicate from GggsTileLayer. Default ramp Grayscale (renderer default). + raster::RasterGlRenderer renderer_; QImage cached_image_; QSize cached_size_; From 3f2e8552832b6964db2a4dd6784a1371e13f90c0 Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Mon, 29 Jun 2026 04:50:53 +0000 Subject: [PATCH 08/17] camp#134: migrate RasterLayer onto the shared renderer Replace the QPainter/mipmap path with the shared GL renderer. GDAL still warps the chart to Web-Mercator on load; the reprojected pixels are uploaded as a GL texture (scalar -> R32F shaded by the LUT; palette/RGB -> CPU-composited RGBA8 sampled directly, LUT bypassed, with mipmaps for zoomed-out LOD) and drawn through RasterGlRenderer as a single non-geographic quad. Adopts the GggsTileLayer placement convention (NW anchor + fromScale(1,-1)). - Remove the mipmap pyramid + QPixmap chain + the CPU ColorMap shading loop. - A scalar colormap change is now an LUT re-bake (no re-warp). - Keep the async GDAL load + abort flag and the RAII handle-close. - Implement RasterFieldSource; the texture is owned here and released under the renderer's context in the dtor. CPU auto-range still excludes NaN + the finite sentinel, so it agrees with the shader's discard. Co-Authored-By: Claude Opus 4.8 --- src/camp_map/raster/raster_layer.cpp | 401 ++++++++++++++++++--------- src/camp_map/raster/raster_layer.h | 101 +++++-- 2 files changed, 349 insertions(+), 153 deletions(-) diff --git a/src/camp_map/raster/raster_layer.cpp b/src/camp_map/raster/raster_layer.cpp index 2a73205..0af4a4e 100644 --- a/src/camp_map/raster/raster_layer.cpp +++ b/src/camp_map/raster/raster_layer.cpp @@ -3,8 +3,10 @@ #include #include "../map_view/web_mercator.h" #include -#include +#include +#include #include +#include #include #include #include @@ -27,6 +29,8 @@ RasterLayer::RasterLayer(map::MapItem* parentItem, const QString& filename): { if(GDALGetDriverCount() == 0) GDALAllRegister(); + // [camp#63] Default scalar ramp is Viridis (the renderer defaults to Grayscale). + renderer_.setColormap(map::ColorMap::Viridis); connect(&future_watcher_, &QFutureWatcher::finished, this, &RasterLayer::imageReady); // [#59 ADR-0003] Establish the scene extent + world transform synchronously, // before kicking off the async pixel load, so the layer knows where it is @@ -42,46 +46,65 @@ RasterLayer::~RasterLayer() abort_flag_ = true; abort_flag_mutex_.unlock(); future_watcher_.waitForFinished(); + // [camp#134] Release the chart texture under the renderer's context (the texture + // is owned here, not by the renderer). The renderer frees its own + // program/LUT/FBO/context in its destructor right after this. + if(renderer_.makeCurrent()) + { + texture_.reset(); + renderer_.doneCurrent(); + } } QRectF RasterLayer::boundingRect() const { - // [#59 ADR-0003] Prefer the synchronously-known reprojected dimensions so the - // extent is valid before pixels load. The mipmap rect (same dimensions) is the - // fallback for the failed-initExtent / not-yet-set case. - if(reprojected_width_ > 0 && reprojected_height_ > 0) - return QRectF(0, 0, reprojected_width_, reprojected_height_); - if(!mipmaps_.empty()) - return mipmaps_.begin()->second.rect(); + // [camp#134] Local space spans (0,0)..(width_m,height_m) in Web-Mercator metres; + // the item is setPos()'d at the NW corner with a fromScale(1,-1) transform (the + // camp_map raster convention shared with GggsTileLayer). + if(!scene_bounds_.isNull()) + return QRectF(QPointF(0.0, 0.0), scene_bounds_.size()); return QRectF(); } void RasterLayer::paint(QPainter *painter, const QStyleOptionGraphicsItem *option, QWidget *widget) { - if(!mipmaps_.empty()) + if(scene_bounds_.isNull()) + return; + if(is_scalar_ && data_min_ > data_max_) // scalar: nothing valid to colour yet + return; + + // Target the offscreen render at the extent's on-screen size, so the image is + // crisp at the current zoom. Re-render only when that size changes (zoom); pan + // reuses the cached image (drawImage repositions it via the world transform). + const QRectF dev = painter->worldTransform().mapRect(boundingRect()); + const int w = std::min(kMaxImageEdge, + std::max(1, int(std::ceil(std::abs(dev.width()))))); + const int h = std::min(kMaxImageEdge, + std::max(1, int(std::ceil(std::abs(dev.height()))))); + const QSize size(w, h); + + if(cached_image_.isNull() || cached_size_ != size) { - auto lod = QStyleOptionGraphicsItem::levelOfDetailFromTransform(painter->worldTransform()); - auto level = mipmaps_.lower_bound( std::min(255, int(1.0/lod)) ); - if(level == mipmaps_.end()) - level--; - painter->save(); - painter->setRenderHint(QPainter::SmoothPixmapTransform); - painter->scale(level->first, level->first); - painter->drawPixmap(0,0, level->second); - painter->restore(); + cached_image_ = renderImage(size); + cached_size_ = size; } + if(cached_image_.isNull()) + return; + painter->save(); + painter->setRenderHint(QPainter::SmoothPixmapTransform); + painter->drawImage(boundingRect(), cached_image_); + painter->restore(); } void RasterLayer::initExtent(const QString& filename) { // [#59 ADR-0003] Read only the reprojected geotransform + dimensions (no - // pixels) and apply the world transform/position synchronously, so the layer - // has a valid extent and scene position before the async pixel load. Mirrors - // the placement loadAndReprojectFile/imageReady would set, just earlier and - // pixel-free. GDALAutoCreateWarpedVRT is metadata-only and effectively instant. + // pixels) and apply the Web-Mercator scene placement synchronously, so the layer + // has a valid extent and scene position before the async pixel load. + // GDALAutoCreateWarpedVRT is metadata-only and effectively instant. auto dataset = GDALDataset::FromHandle(GDALOpen(filename.toLatin1(), GA_ReadOnly)); if(!dataset) return; @@ -93,9 +116,24 @@ void RasterLayer::initExtent(const QString& filename) reprojected->GetGeoTransform(geo_transform); reprojected_width_ = reprojected->GetRasterXSize(); reprojected_height_ = reprojected->GetRasterYSize(); + // [camp#134] Web-Mercator extent from the reprojected geotransform (geo[2] == + // geo[4] == 0, geo[1] > 0, geo[5] < 0 — north-up): the NW corner is (geo[0], + // geo[3]); the SE corner subtracts width/height. north = max y, south = min y. + const double x0 = geo_transform[0]; + const double y0 = geo_transform[3]; + const double x1 = x0 + reprojected_width_ * geo_transform[1]; + const double y1 = y0 + reprojected_height_ * geo_transform[5]; + merc_x_min_ = std::min(x0, x1); + merc_x_max_ = std::max(x0, x1); + merc_y_min_ = std::min(y0, y1); + merc_y_max_ = std::max(y0, y1); prepareGeometryChange(); - setTransform(QTransform::fromScale(geo_transform[1], geo_transform[5]), true); - setPos(geo_transform[0], geo_transform[3]); + scene_bounds_ = QRectF(QPointF(merc_x_min_, merc_y_min_), + QPointF(merc_x_max_, merc_y_max_)).normalized(); + // North-up image anchored at the NW corner with a negative-Y transform (matches + // GggsTileLayer); the MapView's own scale(s,-s) composes to a net upright draw. + setTransform(QTransform::fromScale(1.0, -1.0)); + setPos(QPointF(scene_bounds_.left(), scene_bounds_.bottom())); // NW corner GDALClose(reprojected); } GDALClose(dataset); @@ -129,13 +167,10 @@ RasterLayer::LoadResult RasterLayer::loadAndReprojectFile(const QString& filenam // [#96] RAII-close both GDAL handles so every return path — normal, the // !reprojected_dataset early return, and the abort-flag `return {}` paths in - // the scan loops below — frees them; previously they leaked on every call - // (ctor / setColormap / readSettings). Declaration order matters: local + // the scan loops below — frees them. Declaration order matters: local // unique_ptrs destruct in reverse declaration order, so `dataset` MUST stay // declared before `reprojected_dataset` — the warped VRT references the source - // dataset, so it has to close first, then the source. This mirrors - // initExtent()'s deliberate GDALClose(reprojected) then GDALClose(dataset). - // Do not reorder. + // dataset, so it has to close first, then the source. Do not reorder. const auto gdal_closer = [](GDALDataset* d){ if(d) GDALClose(d); }; std::unique_ptr dataset( GDALDataset::FromHandle(GDALOpen(filename.toLatin1(), GA_ReadOnly)), gdal_closer); @@ -159,32 +194,38 @@ RasterLayer::LoadResult RasterLayer::loadAndReprojectFile(const QString& filenam double reprojected_geo_transform[6] = {0.0}; reprojected_dataset->GetGeoTransform(reprojected_geo_transform); - result.world_x = reprojected_geo_transform[0]; - result.world_y = reprojected_geo_transform[3]; - - result.scale_x = reprojected_geo_transform[1]; - result.scale_y = reprojected_geo_transform[5]; - - auto width = reprojected_dataset->GetRasterXSize(); - auto height = reprojected_dataset->GetRasterYSize(); - - QImage image(width, height, QImage::Format_ARGB32); - image.fill(Qt::black); + const int width = reprojected_dataset->GetRasterXSize(); + const int height = reprojected_dataset->GetRasterYSize(); + result.width = width; + result.height = height; + result.reprojected_width = width; + result.reprojected_height = height; + + // [camp#134] Web-Mercator extent (same derivation as initExtent), so imageReady() + // can re-apply the placement defensively if initExtent() did not run. + const double x0 = reprojected_geo_transform[0]; + const double y0 = reprojected_geo_transform[3]; + const double x1 = x0 + width * reprojected_geo_transform[1]; + const double y1 = y0 + height * reprojected_geo_transform[5]; + result.merc_x_min = std::min(x0, x1); + result.merc_x_max = std::max(x0, x1); + result.merc_y_min = std::min(y0, y1); + result.merc_y_max = std::max(y0, y1); auto first_band = reprojected_dataset->GetRasterBand(1); if(reprojected_dataset->GetRasterCount() == 1 && first_band->GetColorTable() == nullptr) { - result.is_scalar = true; - // [camp#63/#59 PR3c / camp#90] Single-band scalar field (depth raster, GGGS - // backscatter/bathy tiles, etc.): shade through the ColorMap over the data - // range — read as Float32 so any numeric dtype (Float32/UInt16/...) works — - // instead of the UInt32 RGB path (which renders non-8-bit values as near- - // black AND opaque, ignoring NoData). NoData / NaN -> transparent. Paletted + // [camp#63/#59 PR3c / camp#90 / camp#134] Single-band scalar field (depth + // raster, GGGS backscatter/bathy tiles, etc.): read as Float32 and upload as an + // R32F value texture coloured by the shared shader's LUT (NoData / NaN -> + // transparent on the GPU). Read as Float32 so any numeric dtype works. Paletted // single-band rasters still use the colour-table path below. - image.fill(Qt::transparent); + result.is_scalar = true; int has_nodata = 0; const double nodata = first_band->GetNoDataValue(&has_nodata); + result.has_nodata = has_nodata != 0; + result.nodata = float(nodata); std::vector values(static_cast(width) * height); if(first_band->RasterIO(GF_Read, 0, 0, width, height, values.data(), width, height, GDT_Float32, 0, 0) == CE_None) { @@ -192,130 +233,227 @@ RasterLayer::LoadResult RasterLayer::loadAndReprojectFile(const QString& filenam double max_value = std::numeric_limits::lowest(); for(float v : values) { - if(std::isnan(v) || (has_nodata && v == nodata)) + // Exclude NaN + the finite NoData sentinel from the auto-range, so the CPU + // range and the shader's discard (`v != v` and v == u_nodata) agree. + if(std::isnan(v) || (result.has_nodata && v == result.nodata)) continue; min_value = std::min(min_value, double(v)); max_value = std::max(max_value, double(v)); } - // A constant-valued raster (all valid samples equal) would otherwise hit - // ColorMap::color()'s degenerate max<=min guard and render fully - // transparent — the raster would vanish. Widen the range so the single - // value maps to the top of the ramp (matches grid_map's handling). If - // there were no valid samples at all, min/max stay crossed and pixels - // correctly stay transparent. + // A constant-valued raster (all valid samples equal) would otherwise hit the + // degenerate max<=min guard and render transparent. Widen the range so the + // single value maps to the top of the ramp. If there were no valid samples at + // all, min/max stay crossed and the layer correctly shows nothing. if(min_value == max_value) min_value -= 1.0; - const map::ColorMap cm = colormap_; - for(int j = 0; j < height; ++j) + result.data_min = float(min_value); + result.data_max = float(max_value); + // Periodic abort check (cheap, whole-buffer granularity is fine post-read). { - uchar* scanline = image.scanLine(j); - for(int i = 0; i < width; ++i) - { - const float v = values[static_cast(j) * width + i]; - const QColor c = (std::isnan(v) || (has_nodata && v == nodata)) - ? QColor(0, 0, 0, 0) : cm.color(v, min_value, max_value); - scanline[i*4+0] = c.blue(); - scanline[i*4+1] = c.green(); - scanline[i*4+2] = c.red(); - scanline[i*4+3] = c.alpha(); - } QMutexLocker lock(&abort_flag_mutex_); if(abort_flag_) return {}; } + result.values = std::move(values); } } else - for(auto&& band: reprojected_dataset->GetBands()) { - auto color_table = band->GetColorTable(); - std::vector buffer(width); - for(int j = 0; jGetBands()) { - if(band->RasterIO(GF_Read, 0, j, width, 1, &buffer.front(), width, 1, GDT_UInt32, 0, 0)== CE_None) + auto color_table = band->GetColorTable(); + std::vector buffer(width); + for(int j = 0; jRasterIO(GF_Read, 0, j, width, 1, &buffer.front(), width, 1, GDT_UInt32, 0, 0)== CE_None) { - if(color_table) + uchar *scanline = image.scanLine(j); + for(int i = 0; i < width; ++i) { - GDALColorEntry const *ce = color_table->GetColorEntry(buffer[i]); - scanline[i*4] = ce->c3; - scanline[i*4+1] = ce->c2; - scanline[i*4+2] = ce->c1; - scanline[i*4+3] = ce->c4; - } - else - { - if(band->GetColorInterpretation() == GCI_GrayIndex) + if(color_table) + { + GDALColorEntry const *ce = color_table->GetColorEntry(buffer[i]); + scanline[i*4] = ce->c3; + scanline[i*4+1] = ce->c2; + scanline[i*4+2] = ce->c1; + scanline[i*4+3] = ce->c4; + } + else { - scanline[i*4+0] = buffer[i]; - scanline[i*4+1] = buffer[i]; - scanline[i*4+2] = buffer[i]; + if(band->GetColorInterpretation() == GCI_GrayIndex) + { + scanline[i*4+0] = buffer[i]; + scanline[i*4+1] = buffer[i]; + scanline[i*4+2] = buffer[i]; + } + if(band->GetColorInterpretation() == GCI_RedBand) + scanline[i*4+2] = buffer[i]; + if(band->GetColorInterpretation() == GCI_GreenBand) + scanline[i*4+1] = buffer[i]; + if(band->GetColorInterpretation() == GCI_BlueBand) + scanline[i*4+0] = buffer[i]; + if(band->GetColorInterpretation() == GCI_AlphaBand) + scanline[i*4+3] = buffer[i]; } - if(band->GetColorInterpretation() == GCI_RedBand) - scanline[i*4+2] = buffer[i]; - if(band->GetColorInterpretation() == GCI_GreenBand) - scanline[i*4+1] = buffer[i]; - if(band->GetColorInterpretation() == GCI_BlueBand) - scanline[i*4+0] = buffer[i]; - if(band->GetColorInterpretation() == GCI_AlphaBand) - scanline[i*4+3] = buffer[i]; } } + QMutexLocker lock(&abort_flag_mutex_); + if(abort_flag_) + return {}; } - QMutexLocker lock(&abort_flag_mutex_); - if(abort_flag_) - return {}; } + result.rgba = std::move(image); } - result.mipmaps[1] = QPixmap::fromImage(image); - for(int i = 2; i < 128; i*=2) - { - result.mipmaps[i] = QPixmap::fromImage(image.scaledToWidth(width/float(i),Qt::SmoothTransformation)); - } + result.ok = true; return result; } void RasterLayer::imageReady() { auto result = future_watcher_.result(); - if(result.mipmaps.empty()) // failed load (null/unreprojectable dataset); - { // don't apply the zero-filled transform/pos + if(!result.ok) // failed/aborted load (null/unreprojectable dataset) + { setStatus("(load failed)"); return; } is_scalar_ = result.is_scalar; - mipmaps_ = result.mipmaps; - - // [#59 ADR-0003] The world transform + position were already applied - // synchronously in initExtent() (the warped geotransform is identical here), - // and boundingRect() comes from the reprojected dimensions, so the geometry - // does not change when the pixels arrive — only the painted content does. - // Defensive fallback: if initExtent() did not establish the extent (e.g. a - // transient open failure) but the async load nonetheless succeeded, apply the - // placement here so the layer is still positioned correctly. - if(reprojected_width_ <= 0 || reprojected_height_ <= 0) + format_ = is_scalar_ ? RasterFieldItem::Format::Scalar + : RasterFieldItem::Format::Rgba; + data_min_ = result.data_min; + data_max_ = result.data_max; + has_nodata_ = result.has_nodata; + nodata_ = result.nodata; + + // [#59 ADR-0003] The placement was already applied synchronously in initExtent() + // (identical reprojected geotransform). Re-apply defensively only if initExtent() + // did not establish the extent (e.g. a transient open failure) but the async load + // nonetheless succeeded. + if(scene_bounds_.isNull()) { prepareGeometryChange(); - reprojected_width_ = result.mipmaps.begin()->second.width(); - reprojected_height_ = result.mipmaps.begin()->second.height(); - setTransform(QTransform::fromScale(result.scale_x, result.scale_y), true); - setPos(result.world_x, result.world_y); + reprojected_width_ = result.reprojected_width; + reprojected_height_ = result.reprojected_height; + merc_x_min_ = result.merc_x_min; + merc_x_max_ = result.merc_x_max; + merc_y_min_ = result.merc_y_min; + merc_y_max_ = result.merc_y_max; + scene_bounds_ = QRectF(QPointF(merc_x_min_, merc_y_min_), + QPointF(merc_x_max_, merc_y_max_)).normalized(); + setTransform(QTransform::fromScale(1.0, -1.0)); + setPos(QPointF(scene_bounds_.left(), scene_bounds_.bottom())); } + + // [camp#134] Upload the reprojected pixels to a GL texture under the renderer's + // context (the texture is owned here). Scalar -> R32F (Nearest-filtered by the + // renderer); colour -> RGBA8 with mipmaps (so a chart zoomed far out keeps the + // LOD the old QPainter mipmap pyramid provided). + if(renderer_.makeCurrent()) + { + texture_.reset(); + if(is_scalar_ && !result.values.empty()) + { + texture_ = std::make_unique(QOpenGLTexture::Target2D); + texture_->setFormat(QOpenGLTexture::R32F); + texture_->setSize(result.width, result.height); + texture_->setMipLevels(1); + texture_->allocateStorage(QOpenGLTexture::Red, QOpenGLTexture::Float32); + texture_->setData(QOpenGLTexture::Red, QOpenGLTexture::Float32, + result.values.data()); + texture_->setWrapMode(QOpenGLTexture::ClampToEdge); + } + else if(!is_scalar_ && !result.rgba.isNull()) + { + const QImage rgba8 = result.rgba.convertToFormat(QImage::Format_RGBA8888); + texture_ = std::make_unique(QOpenGLTexture::Target2D); + texture_->setFormat(QOpenGLTexture::RGBA8_UNorm); + texture_->setSize(rgba8.width(), rgba8.height()); + texture_->setMipLevels(texture_->maximumMipLevels()); + texture_->allocateStorage(QOpenGLTexture::RGBA, QOpenGLTexture::UInt8); + texture_->setData(QOpenGLTexture::RGBA, QOpenGLTexture::UInt8, rgba8.constBits()); + texture_->generateMipMaps(); + texture_->setWrapMode(QOpenGLTexture::ClampToEdge); + } + renderer_.doneCurrent(); + } + + cached_image_ = QImage(); update(boundingRect()); setStatus(""); } +QImage RasterLayer::renderImage(const QSize& size) +{ + if(scene_bounds_.isNull() || size.isEmpty()) + return QImage(); + if(is_scalar_ && data_min_ > data_max_) + return QImage(); + if(!renderer_.makeCurrent()) + return QImage(); + const QList draw = items(); + const QImage image = renderer_.renderToImage(draw, scene_bounds_, data_min_, + data_max_, size); + renderer_.doneCurrent(); + return image; +} + +QStringList RasterLayer::bands() const +{ + // A reprojected chart is a single composited field; one nominal band. + return QStringList() << "1"; +} + +RasterBandMeta RasterLayer::metadata(const QString&) const +{ + RasterBandMeta meta; + meta.has_nodata = has_nodata_; + meta.nodata = nodata_; + return meta; +} + +QPair RasterLayer::dataRange() const +{ + return {data_min_, data_max_}; +} + +QList RasterLayer::items() +{ + // [camp#134] One non-geographic item: GDAL already reprojected the chart to + // Web-Mercator, so the renderer draws it as a single linear quad over the + // mercator extent (no per-vertex geo warp). + QList result; + if(!texture_) + return result; + RasterFieldItem item; + item.texture = texture_.get(); + item.format = format_; + item.geographic = false; + item.west = merc_x_min_; + item.east = merc_x_max_; + item.south = merc_y_min_; + item.north = merc_y_max_; + item.has_nodata = has_nodata_; + item.nodata = nodata_; + result.push_back(item); + return result; +} + void RasterLayer::setColormap(map::ColorMap::Type type) { - if(type == colormap_.type()) + if(type == renderer_.colormap()) return; - colormap_.setType(type); + // [camp#134] A colormap change is now just an LUT re-bake — no re-warp. The + // scalar value texture already holds the raw data, so the shader recolours it on + // the next render. + renderer_.setColormap(type); writeSettings(); - if(!filename_.isEmpty()) // re-warp + re-shade with the new ramp - loadFile(filename_); + cached_image_ = QImage(); + update(boundingRect()); } void RasterLayer::contextMenu(QMenu* menu) @@ -328,7 +466,7 @@ void RasterLayer::contextMenu(QMenu* menu) { QAction* action = colormap_menu->addAction(map::ColorMap::name(type)); action->setCheckable(true); - action->setChecked(type == colormap_.type()); + action->setChecked(type == renderer_.colormap()); connect(action, &QAction::triggered, this, [this, type]() { setColormap(type); }); } } @@ -340,16 +478,15 @@ void RasterLayer::readSettings() settings.beginGroup("MapItem"); settings.beginGroup(itemID()); const map::ColorMap::Type type = map::ColorMap::typeFromName( - settings.value("colormap", map::ColorMap::name(colormap_.type())).toString()); + settings.value("colormap", map::ColorMap::name(renderer_.colormap())).toString()); settings.endGroup(); settings.endGroup(); - // Apply the persisted ramp (re-render if it differs from the default used by - // the initial load); don't re-persist here. - if(type != colormap_.type()) + // Apply the persisted ramp (re-bake + re-render if it differs); don't re-persist. + if(type != renderer_.colormap()) { - colormap_.setType(type); - if(!filename_.isEmpty()) - loadFile(filename_); + renderer_.setColormap(type); + cached_image_ = QImage(); + update(boundingRect()); } } @@ -369,7 +506,7 @@ void RasterLayer::writeSettings() QSettings settings; settings.beginGroup("MapItem"); settings.beginGroup(itemID()); - settings.setValue("colormap", map::ColorMap::name(colormap_.type())); + settings.setValue("colormap", map::ColorMap::name(renderer_.colormap())); settings.endGroup(); settings.endGroup(); } diff --git a/src/camp_map/raster/raster_layer.h b/src/camp_map/raster/raster_layer.h index 878386a..5685ac6 100644 --- a/src/camp_map/raster/raster_layer.h +++ b/src/camp_map/raster/raster_layer.h @@ -3,14 +3,32 @@ #include "../map/layer.h" #include "../map/color_map.h" +#include "raster_field_source.h" +#include "raster_gl_renderer.h" + #include +#include +#include +#include +#include +#include + +class QOpenGLTexture; namespace camp { namespace raster { -class RasterLayer: public map::Layer +/// [#59 ADR-0003 / camp#134] Map layer for a single georeferenced raster chart. +/// GDAL warps the file to Web-Mercator (EPSG:3857) on load; the reprojected pixels +/// are uploaded as a GL texture and drawn through the shared raster::RasterGlRenderer +/// (the same offscreen-FBO path GggsTileLayer and SonarLiveCacheLayer use). Scalar +/// (single-band Float32) charts shade through the colormap LUT with per-cell NoData +/// discard; palette/RGB charts are composited to RGBA8 on the CPU and sampled +/// directly (the LUT is bypassed). Replaces the former QPainter/mipmap path. See +/// ADR-0007. +class RasterLayer: public map::Layer, public RasterFieldSource { Q_OBJECT Q_INTERFACES(QGraphicsItem) @@ -29,7 +47,8 @@ class RasterLayer: public map::Layer void paint(QPainter *painter, const QStyleOptionGraphicsItem *option, QWidget *widget) override; /// [camp#63] Select the colour ramp for a scalar (single-band Float32) raster; - /// persists and re-renders. No effect on RGB rasters. + /// persists and re-renders (just re-bakes the LUT — no re-warp). No effect on + /// RGB rasters. void setColormap(map::ColorMap::Type type); /// [#59 ADR-0003] Source file this layer renders (kept for re-render, removal, @@ -42,6 +61,20 @@ class RasterLayer: public map::Layer /// missing or non-georeferenced file. bool valid() const { return reprojected_width_ > 0; } + /// [camp#134] Render the reprojected chart into an offscreen image of @p size + /// spanning the layer's Web-Mercator extent (boundingRect). Null image if there + /// is nothing to draw or GL is unavailable. Exposed for headless tests. + QImage renderImage(const QSize& size); + + /// The layer's Web-Mercator extent. Exposed for tests. + QRectF sceneBounds() const { return scene_bounds_; } + + // [camp#134] RasterFieldSource: a single reprojected-chart item for the renderer. + QStringList bands() const override; + RasterBandMeta metadata(const QString& band) const override; + QList items() override; + QPair dataRange() const override; + protected: void contextMenu(QMenu* menu) override; void readSettings() override; @@ -50,18 +83,26 @@ class RasterLayer: public map::Layer void onRemovedFromMap() override; private: - // Store lower resolutions in an image pyramid - using Mipmaps = std::map; - Mipmaps mipmaps_; - + // [camp#134] Off-thread load product: the reprojected pixels (scalar R32F values + // or a composited RGBA QImage) + extent, uploaded to a GL texture on the GUI + // thread in imageReady(). struct LoadResult { - Mipmaps mipmaps; - double world_x = 0.0; - double world_y = 0.0; - double scale_x = 0.0; - double scale_y = 0.0; - bool is_scalar = false; // single-band Float32 (depth) -> ColorMap-shaded + bool ok = false; + bool is_scalar = false; + int width = 0; + int height = 0; + std::vector values; // scalar path: row-major R32F + float data_min = 1.0f; // scalar auto-range (crossed => no data) + float data_max = 0.0f; + bool has_nodata = false; + float nodata = 0.0f; + QImage rgba; // colour path: pre-composited RGBA + // Web-Mercator extent from the reprojected geotransform (metres). + double merc_x_min = 0.0, merc_x_max = 0.0; + double merc_y_min = 0.0, merc_y_max = 0.0; + int reprojected_width = 0; + int reprojected_height = 0; }; QFutureWatcher future_watcher_; @@ -70,22 +111,40 @@ class RasterLayer: public map::Layer bool abort_flag_ = false; QMutex abort_flag_mutex_; - // [camp#63/#59 PR3c] Colour ramp for single-band scalar (e.g. depth) rasters, - // which would otherwise render as near-black via the UInt32 RGB path. - map::ColorMap colormap_{map::ColorMap::Viridis}; QString filename_; // kept so a colormap change can re-render bool is_scalar_ = false; // set once loaded; gates the colormap menu // [#59 ADR-0003] Reprojected pixel dimensions, set synchronously in the - // constructor (cheap GDAL metadata) so boundingRect() — and therefore - // sceneBoundingRect() / fit-to-extent — is valid immediately, before the - // async pixel/mipmap build finishes. 0 until the extent is known (or on a - // failed/unreprojectable file). + // constructor (cheap GDAL metadata) so boundingRect() / fit-to-extent is valid + // immediately, before the async pixel load finishes. 0 until the extent is known + // (or on a failed/unreprojectable file). int reprojected_width_ = 0; int reprojected_height_ = 0; - // Compute the reprojected extent + world transform/pos from file metadata - // (no pixel reads) and apply them. Synchronous; safe before pixels load. + // [camp#134] Web-Mercator scene extent (set in initExtent) + the placement + // convention shared with GggsTileLayer (north-up image anchored at the NW corner + // with a fromScale(1,-1) item transform). The renderer draws the chart as a + // single non-geographic quad spanning these metres. + QRectF scene_bounds_; + double merc_x_min_ = 0.0, merc_x_max_ = 0.0; + double merc_y_min_ = 0.0, merc_y_max_ = 0.0; + + // [camp#134] The shared GL renderer (own offscreen context + unified shader + + // colormap LUT). Default ramp Viridis (scalar charts); set in the ctor. + RasterGlRenderer renderer_; + std::unique_ptr texture_; // reprojected chart (R32F or RGBA8) + RasterFieldItem::Format format_ = RasterFieldItem::Format::Scalar; + float data_min_ = 1.0f; // scalar colormap range (crossed => no data) + float data_max_ = 0.0f; + bool has_nodata_ = false; + float nodata_ = 0.0f; + + QImage cached_image_; // last render, reused on pan (re-rendered on zoom) + QSize cached_size_; + static constexpr int kMaxImageEdge = 4096; // clamp the offscreen target + + // Compute the reprojected extent + Web-Mercator scene placement from file + // metadata (no pixel reads) and apply them. Synchronous; safe before pixels load. void initExtent(const QString& filename); LoadResult loadAndReprojectFile(const QString& filename); From 10f1159f91170b037a8cacc757fe7f7a857ddce3 Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Mon, 29 Jun 2026 04:51:07 +0000 Subject: [PATCH 09/17] =?UTF-8?q?camp#134:=20tests=20=E2=80=94=20RasterGlR?= =?UTF-8?q?enderer=20unit=20tests=20+=20GGGS=20NaN=20render?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - test/test_raster_gl_renderer.cpp (new): drives the shared renderer directly with hand-built textures — scalar NaN + finite-sentinel discard, colormap LUT bake, Nearest sampling (no bleed across a NoData boundary), and the RGBA bypass path (sampled directly, a==0 discards). Mirrors test_gggs_render's offscreen-GL harness + the offscreenGLAvailable() SKIP guard (SKIPs in-container, runs on a GL host). - test/test_gggs_render.cpp: add NanNoDataDiscardsTransparent — a Float32 tile with a NaN NoData sentinel and a NaN interior block must render the block transparent (was opaque before the fix); same dense-column discriminator as the finite case. Co-Authored-By: Claude Opus 4.8 --- CMakeLists.txt | 18 +++ test/test_gggs_render.cpp | 78 ++++++++++++ test/test_raster_gl_renderer.cpp | 211 +++++++++++++++++++++++++++++++ 3 files changed, 307 insertions(+) create mode 100644 test/test_raster_gl_renderer.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 202fc62..5df1dea 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -574,6 +574,24 @@ if(BUILD_TESTING) ${GDAL_LIBRARY} ) + # [camp#134] Shared RasterGlRenderer unit tests (ADR-0007): the unified shader's + # NaN + finite-sentinel discard, LUT bake, Nearest sampling, and the RGBA bypass + # path, driven directly with hand-built textures. Self-skips with no offscreen GL. + ament_add_gtest(test_raster_gl_renderer + test/test_raster_gl_renderer.cpp + ) + target_include_directories(test_raster_gl_renderer PRIVATE + ${CMAKE_CURRENT_SOURCE_DIR}/src/camp_map + ) + target_link_libraries(test_raster_gl_renderer + camp_map + Qt5::Core + Qt5::Gui + Qt5::Widgets + Qt5::Positioning + ${GDAL_LIBRARY} + ) + # GGGS layer-level band selection (camp#108): a 2-band tile-set renders band 1 # by default; setBand(2) reloads and shifts the auto-range so the render differs. # Self-skips when no offscreen GL context is available. diff --git a/test/test_gggs_render.cpp b/test/test_gggs_render.cpp index a8df4eb..c12ac15 100644 --- a/test/test_gggs_render.cpp +++ b/test/test_gggs_render.cpp @@ -12,6 +12,7 @@ #include #include +#include #include #include @@ -242,6 +243,83 @@ TEST(GggsRenderTest, NoDataDiscardHonorsUniform) img.save("/tmp/gggs_nodata_render.png"); } +// [camp#134] NaN NoData discard. Chart + backscatter stores use NaN as their +// NoData sentinel; the OLD shader tested only `v == u_nodata`, which is FALSE for +// NaN (NaN compares unequal to everything), so those cells rendered OPAQUE. The +// unified renderer's `if(v != v) discard;` fixes it. This writes a Float32 tile +// whose NoData sentinel is NaN: a valid 0.0 background (two positive stripes seed a +// real range) with a NaN block punched into the interior. After the fix the NaN +// block is a transparent hole enclosed by the opaque valid background. +// +// DISCRIMINATION mirrors NoDataDiscardHonorsUniform: the dense-column assertion +// (max_fill > 0.8) holds only because the valid 0.0 background renders; if NaN +// cells rendered opaque (the bug) the "hole" would fill in and enclosed_transparent +// would be 0. (Runs only where offscreen GL exists; SKIPs in-container by design.) +TEST(GggsRenderTest, NanNoDataDiscardsTransparent) +{ + if(!offscreenGLAvailable()) + GTEST_SKIP() << "no offscreen GL context available"; + + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + + const int w = 100, h = 100; + const double geo[6] = {-71.40, 0.0001, 0.0, 43.00, 0.0, -0.0001}; + std::vector samples(w * h, 0.0f); // valid zero background + for(int c = 0; c < w; ++c) + { + samples[10 * w + c] = 5.0f; // positive stripes -> real range + samples[20 * w + c] = 10.0f; + } + const float nan = std::numeric_limits::quiet_NaN(); + for(int r = 40; r < 60; ++r) + for(int c = 40; c < 60; ++c) + samples[r * w + c] = nan; // interior NaN block + const QString path = writeFloatTile(dir, w, h, geo, nan, samples); // NoData = NaN + ASSERT_FALSE(path.isEmpty()); + + camp::map::Map map; + camp::map::LayerList* layers = map.topLevelLayers(); + ASSERT_NE(layers, nullptr); + auto* layer = new camp::raster::GggsTileLayer(layers, dir.path()); + ASSERT_TRUE(layer->valid()); + layer->waitForLoad(); + + const QImage img = layer->renderImage(QSize(200, 200)); + ASSERT_FALSE(img.isNull()); + + int opaque = 0, enclosed_transparent = 0; + double max_fill = 0.0; + for(int x = 0; x < img.width(); ++x) + { + int first_opaque = -1, last_opaque = -1, col_opaque = 0; + for(int y = 0; y < img.height(); ++y) + { + if(img.pixelColor(x, y).alpha() > 0) + { + ++opaque; + ++col_opaque; + if(first_opaque < 0) + first_opaque = y; + last_opaque = y; + } + } + if(first_opaque >= 0) + { + const int span = last_opaque - first_opaque + 1; + max_fill = std::max(max_fill, double(col_opaque) / span); + for(int y = first_opaque + 1; y < last_opaque; ++y) + if(img.pixelColor(x, y).alpha() == 0) + ++enclosed_transparent; + } + } + EXPECT_GT(opaque, 0); // valid 0.0 background renders opaque + EXPECT_GT(max_fill, 0.8); // a column of valid background is dense + EXPECT_GT(enclosed_transparent, 0); // NaN block discarded -> enclosed hole + + img.save("/tmp/gggs_nan_render.png"); +} + // Optional real-data smoke render: set GGGS_TEST_STORE to a tile directory to // warp it headlessly to /tmp/gggs_real.png (QA / preview; skipped otherwise). TEST(GggsRenderTest, RealStoreRendersWhenProvided) diff --git a/test/test_raster_gl_renderer.cpp b/test/test_raster_gl_renderer.cpp new file mode 100644 index 0000000..1fe9e8a --- /dev/null +++ b/test/test_raster_gl_renderer.cpp @@ -0,0 +1,211 @@ +// [camp#134] Unit tests for the shared raster::RasterGlRenderer (ADR-0007): the +// one GL raster path GggsTileLayer / SonarLiveCacheLayer / RasterLayer delegate to. +// Exercises the unified shader directly with hand-built textures: +// - Scalar NaN NoData discard (the bug #134 fixes: `v != v` discards NaN cells), +// - Scalar finite-sentinel discard (v == u_nodata), +// - colormap LUT bake (a valid scalar shades to the ramp colour), +// - Nearest sampling on the value texture (no blend across a NoData boundary), +// - the Rgba bypass path (RGBA8 sampled directly, LUT bypassed; a==0 discards). +// +// Uses the renderer's own offscreen context (makeCurrent), a non-geographic item +// (1:1 unit-per-pixel quad) so a texture cell maps straight to an output pixel. +// Skips automatically where no offscreen GL context can be created (in-container). + +#include + +#include +#include +#include +#include + +#include +#include +#include +#include +#include + +#include "raster/raster_field_source.h" +#include "raster/raster_gl_renderer.h" + +using camp::raster::RasterFieldItem; +using camp::raster::RasterGlRenderer; + +namespace +{ + +bool offscreenGLAvailable() +{ + QOffscreenSurface surface; + surface.create(); + QOpenGLContext ctx; + return surface.isValid() && ctx.create(); +} + +// A non-geographic item over the unit-per-pixel quad [0,n]x[0,n], so an n x n +// render maps texture cell (col,row) straight to output pixel (col,row) — texture +// row 0 (v=0) is north = the top of the image. +RasterFieldItem scalarItem(QOpenGLTexture* tex, int n, bool has_nodata, float nodata) +{ + RasterFieldItem item; + item.texture = tex; + item.format = RasterFieldItem::Format::Scalar; + item.geographic = false; + item.west = 0.0; item.east = double(n); + item.south = 0.0; item.north = double(n); + item.has_nodata = has_nodata; + item.nodata = nodata; + return item; +} + +std::unique_ptr makeScalarTexture(int w, int h, const std::vector& v) +{ + auto tex = std::make_unique(QOpenGLTexture::Target2D); + tex->setFormat(QOpenGLTexture::R32F); + tex->setSize(w, h); + tex->setMipLevels(1); + tex->allocateStorage(QOpenGLTexture::Red, QOpenGLTexture::Float32); + tex->setData(QOpenGLTexture::Red, QOpenGLTexture::Float32, v.data()); + tex->setWrapMode(QOpenGLTexture::ClampToEdge); + return tex; +} + +} // namespace + +// NaN + finite-sentinel discard, plus colormap shading on the valid cells. A 4x4 +// R32F tile: cell (row0,col0) = NaN, (row0,col1) = 9999 (finite sentinel), every +// other cell = 10 (the range max). Grayscale ramp, range [0,10]: the valid cells +// shade to white; the two NoData cells discard (transparent). +TEST(RasterGlRendererTest, ScalarNanAndSentinelDiscard) +{ + if(!offscreenGLAvailable()) + GTEST_SKIP() << "no offscreen GL context available"; + + RasterGlRenderer renderer; + ASSERT_TRUE(renderer.makeCurrent()); + + const int n = 4; + std::vector data(n * n, 10.0f); + data[0 * n + 0] = std::numeric_limits::quiet_NaN(); // north-west cell + data[0 * n + 1] = 9999.0f; // finite sentinel + auto tex = makeScalarTexture(n, n, data); + + const RasterFieldItem item = scalarItem(tex.get(), n, /*has_nodata=*/true, 9999.0f); + const QImage img = renderer.renderToImage({item}, QRectF(0, 0, n, n), 0.0f, 10.0f, + QSize(n, n)); + tex.reset(); + renderer.doneCurrent(); + + ASSERT_FALSE(img.isNull()); + EXPECT_EQ(img.width(), n); + EXPECT_EQ(img.height(), n); + + // Row 0 (north) col 0 = NaN, col 1 = sentinel -> transparent; col 2,3 valid. + EXPECT_EQ(img.pixelColor(0, 0).alpha(), 0) << "NaN cell must discard"; + EXPECT_EQ(img.pixelColor(1, 0).alpha(), 0) << "finite sentinel must discard"; + EXPECT_GT(img.pixelColor(2, 0).alpha(), 0) << "valid cell must render"; + + // Valid cells shade to the top of the Grayscale ramp (white). + const QColor valid = img.pixelColor(2, 2); + EXPECT_GT(valid.alpha(), 0); + EXPECT_GT(valid.red(), 200); + EXPECT_GT(valid.green(), 200); + EXPECT_GT(valid.blue(), 200); + + // Nearest sampling: the discard does not bleed onto the neighbouring valid cell. + EXPECT_GT(img.pixelColor(1, 1).alpha(), 0); +} + +// The Rgba bypass path: an RGBA8 texture is sampled directly (LUT bypassed). A +// uniform red texture renders red everywhere (NOT a colormap of red.r), proving the +// LUT is skipped; a fully-transparent texture discards everywhere. +TEST(RasterGlRendererTest, RgbaBypassesLut) +{ + if(!offscreenGLAvailable()) + GTEST_SKIP() << "no offscreen GL context available"; + + RasterGlRenderer renderer; + ASSERT_TRUE(renderer.makeCurrent()); + + const int n = 4; + // Uniform opaque red (RGBA bytes). + std::vector red(n * n * 4); + for(int i = 0; i < n * n; ++i) + { + red[i * 4 + 0] = 255; red[i * 4 + 1] = 0; + red[i * 4 + 2] = 0; red[i * 4 + 3] = 255; + } + auto tex = std::make_unique(QOpenGLTexture::Target2D); + tex->setFormat(QOpenGLTexture::RGBA8_UNorm); + tex->setSize(n, n); + tex->setMipLevels(1); + tex->allocateStorage(QOpenGLTexture::RGBA, QOpenGLTexture::UInt8); + tex->setData(QOpenGLTexture::RGBA, QOpenGLTexture::UInt8, red.data()); + tex->setWrapMode(QOpenGLTexture::ClampToEdge); + + RasterFieldItem item = scalarItem(tex.get(), n, false, 0.0f); + item.format = RasterFieldItem::Format::Rgba; + const QImage img = renderer.renderToImage({item}, QRectF(0, 0, n, n), 0.0f, 10.0f, + QSize(n, n)); + + ASSERT_FALSE(img.isNull()); + const QColor c = img.pixelColor(2, 2); + EXPECT_GT(c.alpha(), 0); + EXPECT_GT(c.red(), 200); + EXPECT_LT(c.green(), 60); + EXPECT_LT(c.blue(), 60); + + // Fully-transparent texture -> every cell discards. + std::vector clear(n * n * 4, 0); + tex->setData(QOpenGLTexture::RGBA, QOpenGLTexture::UInt8, clear.data()); + RasterFieldItem clear_item = item; + const QImage clear_img = renderer.renderToImage({clear_item}, QRectF(0, 0, n, n), + 0.0f, 10.0f, QSize(n, n)); + tex.reset(); + renderer.doneCurrent(); + + ASSERT_FALSE(clear_img.isNull()); + int opaque = 0; + for(int y = 0; y < clear_img.height(); ++y) + for(int x = 0; x < clear_img.width(); ++x) + if(clear_img.pixelColor(x, y).alpha() > 0) + ++opaque; + EXPECT_EQ(opaque, 0) << "a fully-transparent RGBA texture must discard everywhere"; +} + +// A colormap change re-bakes the LUT: the same valid scalar shades differently +// under Grayscale vs Viridis (the max maps to white vs Viridis' yellow). +TEST(RasterGlRendererTest, ColormapRebakesLut) +{ + if(!offscreenGLAvailable()) + GTEST_SKIP() << "no offscreen GL context available"; + + RasterGlRenderer renderer; + ASSERT_TRUE(renderer.makeCurrent()); + + const int n = 2; + std::vector data(n * n, 10.0f); + auto tex = makeScalarTexture(n, n, data); + const RasterFieldItem item = scalarItem(tex.get(), n, false, 0.0f); + + renderer.setColormap(camp::map::ColorMap::Grayscale); + const QColor gray = renderer.renderToImage({item}, QRectF(0, 0, n, n), 0.0f, 10.0f, + QSize(n, n)).pixelColor(0, 0); + renderer.setColormap(camp::map::ColorMap::Viridis); + const QColor viridis = renderer.renderToImage({item}, QRectF(0, 0, n, n), 0.0f, 10.0f, + QSize(n, n)).pixelColor(0, 0); + tex.reset(); + renderer.doneCurrent(); + + // Grayscale max -> white-ish (high, near-equal channels); Viridis max -> yellow + // (high red+green, low blue). They must differ. + EXPECT_NE(gray.rgb(), viridis.rgb()); + EXPECT_LT(viridis.blue(), viridis.green()); +} + +int main(int argc, char** argv) +{ + qputenv("QT_QPA_PLATFORM", "offscreen"); + QApplication app(argc, argv); + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +} From f94bd6dc71d3a8dda69a14a724f8e05405b7230e Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Mon, 29 Jun 2026 04:52:28 +0000 Subject: [PATCH 10/17] progress: implementation complete for #134 Co-Authored-By: Claude Opus 4.8 --- .agent/work-plans/issue-134/progress.md | 112 ++++++++++++++++++++++++ 1 file changed, 112 insertions(+) diff --git a/.agent/work-plans/issue-134/progress.md b/.agent/work-plans/issue-134/progress.md index 9d589c2..97c6ed8 100644 --- a/.agent/work-plans/issue-134/progress.md +++ b/.agent/work-plans/issue-134/progress.md @@ -139,3 +139,115 @@ host-dispatched Opus sub-agent — not author self-review). - [ ] (suggestion) RasterLayer RGBA/palette path vs single-band R32F renderer: recommended option (a) (CPU composite → RGBA8) needs a colormap-bypass mode in the renderer, but the sketched `render(items, data_min, data_max, mvp)` + `RasterFieldItem{texture, nodata}` only expresses scalar R32F+LUT. Add a format/mode field to `RasterFieldItem` (or a render overload) and resolve the open question before step 6 — `plan.md:46-52`, `plan.md:142-146` - [ ] (suggestion) ADR-table label wrong: "ADR-0001 (camp: Adopt ADRs)" — camp ADR-0001 is "TopicBridge and the executor contract"; no camp "Adopt ADRs" ADR exists (that's a workspace ADR). Also camp ADR-0006 (live tile cache) governs `SonarLiveCacheLayer` — confirm the render-path migration leaves its persistence/subscription contract untouched and note it in the table — `plan.md:124-129` - [ ] (suggestion) New `test_raster_gl_renderer.cpp` requires an offscreen GL context; mirror `test_gggs_render`'s harness and confirm CI provides GL — `plan.md:93-96` + +## Implementation +**Status**: complete +**When**: 2026-06-29 04:51 +00:00 +**By**: Claude Opus + +**Branch**: feature/issue-134 — commits `85e66a8`..`10f1159` (5, ordered) +**Plan**: `.agent/work-plans/issue-134/plan.md` at `85e66a8` (synced) + +### What landed + +Full consolidation in one PR, committed step-by-step (review suggestion 1): + +- **(a) `85e66a8`** — ADR-0007 + `raster/raster_field_source.h` (interface: + `bands()`/`metadata()`/`items()`/`dataRange()` + `RasterFieldItem`) + + `raster/raster_gl_renderer.{h,cpp}` (the shared renderer). One compiled program + + colormap LUT per GL context, the unified fragment shader, the geo→Web-Mercator + tessellation, and the data-texture filter all live here now. The renderer owns + its own offscreen context/surface/FBO; sources release their textures between + `makeCurrent()`/`doneCurrent()`. +- **(b) `26bedba`** — `GggsTileLayer`: deleted its kVertex/kFragment shaders, + `ensureProgram`/`ensureLut`, per-vertex tessellation and own GL context/FBO; + implements `RasterFieldSource`; `renderImage()` delegates. QtConcurrent load + + atomic `pixelsLoaded()` unchanged. +- **(c) `f517d27`** — `SonarLiveCacheLayer`: deleted the duplicated shader block + + its `// NOTE: shader duplicated; unify via camp#134` marker; delegates. ADR-0006 + invariants (persistence, opt-in subscription, reconcile/prune, GUI-thread + marshalling) untouched — only `renderImage()`'s GL internals changed. +- **(d) `3f2e855`** — `RasterLayer`: replaced the QPainter/mipmap path with the GL + renderer (own offscreen context via the shared renderer). Adopts the GggsTileLayer + placement convention (NW anchor + `fromScale(1,-1)`). Async GDAL load + abort + + RAII handle-close kept; a scalar colormap change is now an LUT re-bake (no + re-warp). +- **(e) `10f1159`** — tests. + +### NaN fix location + +Lands ONCE, in the unified fragment shader in `raster_gl_renderer.cpp`: +`if(v != v) discard;` (NaN, GLSL 1.20-portable) before +`if(u_has_nodata != 0 && v == u_nodata) discard;` (finite sentinel). All three +adapters inherit it; the two old per-layer copies are gone. + +### RGBA format handling (review suggestion 2) + +`RasterFieldItem::Format{Scalar,Rgba}` resolves the open question. Scalar bands → +R32F + colormap LUT (NaN/finite discard); RasterLayer's palette/RGB charts → +CPU-composited RGBA8 sampled directly (LUT bypassed; transparency from the +composited alpha), with mipmaps for zoomed-out LOD. A `geographic` flag lets the +one renderer serve both lat/lon tiles (warped in-shader) and RasterLayer (already +GDAL-reprojected → a single linear quad). Per-band channel select for colour files +is left a future issue (composited on CPU as today). + +### Parity verification + +- Multi-band select, colormap LUT + per-band/auto-range, Nearest sampling: preserved + per adapter (the renderer reproduces the prior vertex math + mvp + Nearest filter + exactly; the geographic warp is byte-for-byte the old tessellation). +- NoData discard NaN **and** finite: now both, in the one shader. +- CPU range fold excludes NaN consistently in all three (`std::isfinite` in + `GggsTile::loadPixels`, `SonarLiveTile::refoldRange`, and RasterLayer's range loop), + so CPU range and shader discard agree. +- RasterLayer GPU change: scalar colormap, palette/RGB (RGBA bypass), and NoData + transparency covered; north-up placement mirrors GggsTileLayer's verified convention. + +### Review suggestions addressed + +1. Ordered per-adapter commits — done (a→e). 2. Format/mode field — done +(`RasterFieldItem::Format` + `geographic`). 3. ADR table fix — ADR-0007 + plan +reference camp ADR-0001 correctly (*TopicBridge…*, not "Adopt ADRs") and state +ADR-0006 governs `SonarLiveCacheLayer` and is untouched. 4. Test harness — +`test_raster_gl_renderer.cpp` mirrors `test_gggs_render`'s offscreen-GL harness + +`offscreenGLAvailable()` SKIP guard. + +### Build / test — HONEST STATUS (env cannot do a full build) + +`source setup.bash → ./ui_ws/build.sh camp` **fails to configure**: every lower +layer is unbuilt in this container (`underlay/core/platforms/site/sensors/simulation` +install dirs are empty; `setup.bash` warns "exists but is not built"), so camp's +ROS message deps are absent: + +``` +CMake Error at CMakeLists.txt:14 (find_package): + Could not find a package configuration file provided by "marine_ais_msgs" +``` + +Building all lower layers from source here is impractical, so `./ui_ws/test.sh camp` +was **not run** — no verbatim ctest counts to report. I did NOT fake a build. + +To validate the change I instead ran standalone `g++ -std=c++17 -fsyntax-only` +against system Qt5 + GDAL on every new/migrated ROS-free unit — all clean (only the +pre-existing `QOpenGLTexture::setData(void*)` deprecation warning the codebase +already uses): + +- `raster_gl_renderer.cpp` — clean +- `gggs_tile_layer.cpp` — clean +- `raster_layer.cpp` — clean +- `test_gggs_render.cpp` — clean +- `test_raster_gl_renderer.cpp` — clean + +`sonar_live_cache_layer.cpp` pulls in ROS headers so can't be syntax-checked in +isolation; its diff mirrors the (compiling) GggsTileLayer migration exactly. + +The offscreen-GL render tests (`test_gggs_render`, `test_raster_gl_renderer`) are +designed to **SKIP in-container** and **RUN on a GL host** — expected. A full +`build.sh`/`test.sh` on a host with the lower layers built (and verbatim counts) is +the remaining verification step. + +### Next step + +Run `./ui_ws/build.sh camp && ./ui_ws/test.sh camp` on a host with the lower layers +built; confirm a clean build and that the offscreen-GL render tests RUN (not SKIP) +and pass. Then code review. From e3cec24287738dc448e25293ad52eb8f3737acc5 Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Mon, 29 Jun 2026 05:08:56 +0000 Subject: [PATCH 11/17] progress: local review for #134 --- .agent/work-plans/issue-134/progress.md | 35 +++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/.agent/work-plans/issue-134/progress.md b/.agent/work-plans/issue-134/progress.md index 97c6ed8..d6bd288 100644 --- a/.agent/work-plans/issue-134/progress.md +++ b/.agent/work-plans/issue-134/progress.md @@ -251,3 +251,38 @@ the remaining verification step. Run `./ui_ws/build.sh camp && ./ui_ws/test.sh camp` on a host with the lower layers built; confirm a clean build and that the offscreen-GL render tests RUN (not SKIP) and pass. Then code review. + +## Local Review (Pre-Push) +**Status**: complete +**When**: 2026-06-29 05:07 +00:00 +**By**: Claude Code Agent (Claude Opus) +**Verdict**: changes-requested + +**Branch**: feature/issue-134 at `f94bd6d` +**Mode**: pre-push +**Depth**: Deep (reason: large cross-cutting GL refactor, new ADR-0007, concurrency/lifecycle) +**Must-fix**: 2 | **Suggestions**: 5 +**Round**: 1 | **Ship**: continue — two verified must-fixes (a runtime GPU-texture leak on prune; a silent scalar-colormap parity regression), both with precise low-risk fixes; warrants one more round. + +Deep review: 2 disjoint-lens Claude Adversarial passes (Lens A logic, Lens B +lifecycle/concurrency). Both must-fixes were verified by the lead against the code +and against the pre-refactor (`origin/jazzy`) baseline. Static analysis limited (env +cannot build lower layers; cppcheck cannot resolve Qt headers); implementer's +`g++ -fsyntax-only` pass was clean. Plan adherence strong; ADR-0006/0002/0001 +contracts verified untouched; ADR-0007 added cleanly. + +### Findings +- [ ] (must-fix) Scalar-colormap parity regression: shared shader floors denominator at `max(span,1.0)`; old RasterLayer used true span via `ColorMap::color` — sub-unit-range scalar charts lose contrast — `src/camp_map/raster/raster_gl_renderer.cpp:76` +- [ ] (must-fix) Prune destroys `Entry::texture` (QOpenGLTexture) without a current GL context → runtime GPU leak as tiles churn; every other teardown path guards with makeCurrent() — `src/camp_map/ros/live_coverage/sonar_live_cache_layer.cpp:333` +- [ ] (suggestion) Scalar RasterIO read failure sets `result.ok=true` + `setStatus("")` (reports success, shows nothing) — `src/camp_map/raster/raster_layer.cpp:224` +- [ ] (suggestion) Pre-existing null-deref: `GetColorEntry(buffer[i])` may return nullptr before `ce->c3` — `src/camp_map/raster/raster_layer.cpp:280` +- [ ] (suggestion) `renderToImage` doc says null-on-shader-failure but returns a non-null transparent image (then cached) — `src/camp_map/raster/raster_gl_renderer.h:64` +- [ ] (suggestion) RGBA mipmaps generated from straight (non-premultiplied) alpha → possible dark fringes zoomed out — `src/camp_map/raster/raster_layer.cpp:379` +- [ ] (suggestion) Colormap-range floor (must-fix 1) untested; add a sub-unit-range renderer case — `test/test_raster_gl_renderer.cpp` + +### Next step + +Verdict is **changes-requested** → host (`/run-issue`) should dispatch +**`address-findings`** to work the two must-fixes (and the suggestions as warranted), +then re-dispatch `review-code` for a round-2 pre-push read. The diff is not pushed +until a pre-push review returns **approved**. From 736922b7a192558074d99cf4675882bf6dff0f2f Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Mon, 29 Jun 2026 05:21:13 +0000 Subject: [PATCH 12/17] fix(camp#134): true-span colormap; guard prune texture destroy; raster_layer error/alpha fixes --- src/camp_map/raster/raster_gl_renderer.cpp | 29 ++++++++---- src/camp_map/raster/raster_layer.cpp | 19 +++++++- .../live_coverage/sonar_live_cache_layer.cpp | 18 ++++++++ test/test_raster_gl_renderer.cpp | 44 +++++++++++++++++++ 4 files changed, 101 insertions(+), 9 deletions(-) diff --git a/src/camp_map/raster/raster_gl_renderer.cpp b/src/camp_map/raster/raster_gl_renderer.cpp index c5a7ef0..7466805 100644 --- a/src/camp_map/raster/raster_gl_renderer.cpp +++ b/src/camp_map/raster/raster_gl_renderer.cpp @@ -50,10 +50,11 @@ void main() // idiom (isnan() is 1.30+). The finite-sentinel discard (v == u_nodata, e.g. // sidescan 9999/0) follows. Premultiplied-opaque output (alpha 1). // -// - Rgba (u_mode == 1): a pre-composited RGBA8 texture sampled directly, BYPASSING -// the LUT (RasterLayer's palette/RGB charts). Transparency comes from the -// composited alpha; fully-transparent cells discard. Output is premultiplied to -// match the GL_ONE / GL_ONE_MINUS_SRC_ALPHA blend. +// - Rgba (u_mode == 1): a pre-composited, PREMULTIPLIED RGBA8 texture sampled +// directly, BYPASSING the LUT (RasterLayer's palette/RGB charts). The uploader +// premultiplies (so mipmap/linear filtering doesn't fringe), so the texel is +// emitted as-is — already matching the GL_ONE / GL_ONE_MINUS_SRC_ALPHA blend. +// Transparency comes from the composited alpha; fully-transparent cells discard. constexpr char kFragmentShader[] = R"( #version 120 uniform sampler2D u_tex; // unit 0: Scalar R32F data, or Rgba RGBA8 @@ -73,16 +74,23 @@ void main() discard; // NaN NoData (1.20-portable) if(u_has_nodata != 0 && v == u_nodata) discard; // finite sentinel - float t = clamp((v - u_min) / max(u_max - u_min, 1.0), 0.0, 1.0); + // Normalize over the TRUE data span (matches ColorMap::color), so sub-unit + // ranges still stretch across the colormap. The 1e-6 floor is ONLY a + // divide-by-zero guard for a genuinely degenerate (zero-width) range — a true + // span of 0 collapses to t=0 (a flat LUT value). The old 1.0 floor silently + // crushed contrast for any span < 1.0 (harmless for large-range GGGS/sidescan + // depth, but wrong once small-range RasterLayer charts share this shader). + // Negative-valued ranges are fine: span = u_max - u_min stays positive. + float t = clamp((v - u_min) / max(u_max - u_min, 1e-6), 0.0, 1.0); vec4 c = texture2D(u_lut, vec2(t, 0.5)); gl_FragColor = vec4(c.rgb, 1.0); } else { - vec4 c = texture2D(u_tex, v_texcoord); + vec4 c = texture2D(u_tex, v_texcoord); // already premultiplied if(c.a == 0.0) discard; - gl_FragColor = vec4(c.rgb * c.a, c.a); + gl_FragColor = c; } } )"; @@ -206,6 +214,12 @@ QImage RasterGlRenderer::renderToImage(const QList& items, if(items.isEmpty() || size.isEmpty() || !gl_context_) return QImage(); + // [camp#134] Fail BEFORE allocating/binding the FBO so a shader-compile failure + // returns a null image (per the header contract) — not a transparent one the + // caller would cache as a valid empty render. + if(!ensureProgram()) + return QImage(); + QOpenGLFunctions* f = gl_context_->functions(); if(!fbo_ || fbo_->size() != size) fbo_ = std::make_unique(size); @@ -218,7 +232,6 @@ QImage RasterGlRenderer::renderToImage(const QList& items, f->glEnable(GL_BLEND); f->glBlendFunc(GL_ONE, GL_ONE_MINUS_SRC_ALPHA); - if(ensureProgram()) { // Vertices are LOCAL Web-Mercator metres from the extent origin, so map // [0, width] x [0, height] -> NDC. origin = scene_bounds top-left; the per-item diff --git a/src/camp_map/raster/raster_layer.cpp b/src/camp_map/raster/raster_layer.cpp index 0af4a4e..5dd97ad 100644 --- a/src/camp_map/raster/raster_layer.cpp +++ b/src/camp_map/raster/raster_layer.cpp @@ -256,6 +256,15 @@ RasterLayer::LoadResult RasterLayer::loadAndReprojectFile(const QString& filenam } result.values = std::move(values); } + else + { + // [camp#134] A failed scalar read must NOT fall through to result.ok = true: + // that reported success while uploading nothing (an empty values buffer -> + // no texture -> a blank layer). Leave ok = false so imageReady() surfaces the + // failure ("(load failed)"), matching the RGBA path's load-failure handling. + qDebug("RasterLayer::loadFile scalar band RasterIO read failed"); + return result; // result.ok stays false (default) + } } else { @@ -278,6 +287,8 @@ RasterLayer::LoadResult RasterLayer::loadAndReprojectFile(const QString& filenam if(color_table) { GDALColorEntry const *ce = color_table->GetColorEntry(buffer[i]); + if(!ce) + continue; // index outside the palette -> leave the fill (skip) scanline[i*4] = ce->c3; scanline[i*4+1] = ce->c2; scanline[i*4+2] = ce->c1; @@ -369,7 +380,13 @@ void RasterLayer::imageReady() } else if(!is_scalar_ && !result.rgba.isNull()) { - const QImage rgba8 = result.rgba.convertToFormat(QImage::Format_RGBA8888); + // [camp#134] Upload PREMULTIPLIED RGBA so mipmap generation (and linear + // minification) box-filter premultiplied texels — straight-alpha averaging + // bleeds the transparent cells' black RGB into the edges as dark fringes when + // the chart is zoomed out. The shared shader's Rgba branch samples this texel + // as already-premultiplied (no second c.rgb*c.a), matching the premultiplied + // GL_ONE / GL_ONE_MINUS_SRC_ALPHA blend. + const QImage rgba8 = result.rgba.convertToFormat(QImage::Format_RGBA8888_Premultiplied); texture_ = std::make_unique(QOpenGLTexture::Target2D); texture_->setFormat(QOpenGLTexture::RGBA8_UNorm); texture_->setSize(rgba8.width(), rgba8.height()); 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 bfc9291..b22d37a 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 @@ -243,6 +243,12 @@ void SonarLiveCacheLayer::warmLoad() if(!level_) return; // nothing cached yet + // [camp#134] insert_or_assign below replaces any pre-existing Entry for an index + // (reachable on a disable→re-enable: disable leaves tiles_ — and their GL textures + // — intact). A displaced Entry's QOpenGLTexture must be freed under a current GL + // context, so make it current for the seed loop when tiles already hold textures. + // On the first (empty-map) warm load there is nothing to displace, so skip it. + const bool gl_current = !tiles_.empty() && renderer_.makeCurrent(); const gggs::Level level(*level_); for(auto& tile : SonarLiveTile::loadCacheDir(cache_dir_, level)) { @@ -255,6 +261,8 @@ void SonarLiveCacheLayer::warmLoad() reconciler_.markHave(index, 0); tiles_.insert_or_assign(index, Entry{std::move(tile), nullptr, true}); } + if(gl_current) + renderer_.doneCurrent(); if(band_name_.empty()) band_name_ = defaultBand(); recomputeBounds(); @@ -324,12 +332,20 @@ void SonarLiveCacheLayer::handleCatalog(const marine_interfaces::msg::TileCatalo publishRequest(result.to_request); + // [camp#134] A pruned Entry owns a QOpenGLTexture whose dtor frees GPU resources + // ONLY under a current GL context — erasing it here without one leaks the texture + // as tiles churn (the boat keeps producing; the catalog keeps pruning). Make the + // renderer's context current for the erase loop, exactly like the dtor's teardown. + // If makeCurrent() fails (offscreen GL unavailable) no live texture was ever + // uploaded, so a plain erase is harmless — mirrors the dtor's null-context guard. bool pruned = false; + const bool gl_current = !result.to_prune.empty() && renderer_.makeCurrent(); for(const auto& index : result.to_prune) { auto it = tiles_.find(index); if(it != tiles_.end()) { + it->second.texture.reset(); // free the GPU texture under the current context tiles_.erase(it); pruned = true; } @@ -342,6 +358,8 @@ void SonarLiveCacheLayer::handleCatalog(const marine_interfaces::msg::TileCatalo fs::remove(fs::path(cache_dir_) / stem, ec); reconciler_.drop(index); } + if(gl_current) + renderer_.doneCurrent(); if(pruned) { // Reset before re-folding so a pruned tile's extreme min/max can't linger in diff --git a/test/test_raster_gl_renderer.cpp b/test/test_raster_gl_renderer.cpp index 1fe9e8a..bd21c3d 100644 --- a/test/test_raster_gl_renderer.cpp +++ b/test/test_raster_gl_renderer.cpp @@ -115,6 +115,50 @@ TEST(RasterGlRendererTest, ScalarNanAndSentinelDiscard) EXPECT_GT(img.pixelColor(1, 1).alpha(), 0); } +// [camp#134 must-fix] Sub-unit value range: the colormap must stretch across a data +// range narrower than 1.0. A 2x1 R32F tile: col0 = data_min (0.0), col1 = data_max +// (0.3), Grayscale ramp, range [0.0, 0.3]. With the TRUE-span denominator the max +// sample reaches t = 1.0 (white, ~255). Under the old max(u_max - u_min, 1.0) +// denominator floor it only reached t = 0.3 -> grey ~76, collapsing the contrast — +// so the EXPECT_GT(hi.red(), 200) below FAILS under the floor and PASSES with the fix. +TEST(RasterGlRendererTest, ScalarSubUnitRangeSpansColormap) +{ + if(!offscreenGLAvailable()) + GTEST_SKIP() << "no offscreen GL context available"; + + RasterGlRenderer renderer; + ASSERT_TRUE(renderer.makeCurrent()); + renderer.setColormap(camp::map::ColorMap::Grayscale); + + const int w = 2, h = 1; + std::vector data = {0.0f, 0.3f}; // col0 = data_min, col1 = data_max + auto tex = makeScalarTexture(w, h, data); + + RasterFieldItem item; + item.texture = tex.get(); + item.format = RasterFieldItem::Format::Scalar; + item.geographic = false; + item.west = 0.0; item.east = double(w); + item.south = 0.0; item.north = double(h); + item.has_nodata = false; + item.nodata = 0.0f; + + const QImage img = renderer.renderToImage({item}, QRectF(0, 0, w, h), 0.0f, 0.3f, + QSize(w, h)); + tex.reset(); + renderer.doneCurrent(); + + ASSERT_FALSE(img.isNull()); + const QColor lo = img.pixelColor(0, 0); // data_min -> bottom of ramp (black) + const QColor hi = img.pixelColor(1, 0); // data_max -> top of ramp (white) + + EXPECT_GT(hi.red(), 200) << "sub-unit max must span to the top of the colormap " + "(fails under the max(span, 1.0) floor)"; + EXPECT_LT(lo.red(), 40) << "data_min stays at the bottom of the colormap"; + // Distinct LUT outputs at min vs max: the ramp spans the range. + EXPECT_GT(hi.red() - lo.red(), 150) << "sub-unit range must preserve contrast"; +} + // The Rgba bypass path: an RGBA8 texture is sampled directly (LUT bypassed). A // uniform red texture renders red everywhere (NOT a colormap of red.r), proving the // LUT is skipped; a fully-transparent texture discards everywhere. From 396bb744617760da98815f6faf0b90322b4eef49 Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Mon, 29 Jun 2026 05:21:16 +0000 Subject: [PATCH 13/17] progress: address-findings round 1 for #134 --- .agent/work-plans/issue-134/progress.md | 88 +++++++++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/.agent/work-plans/issue-134/progress.md b/.agent/work-plans/issue-134/progress.md index d6bd288..10eb289 100644 --- a/.agent/work-plans/issue-134/progress.md +++ b/.agent/work-plans/issue-134/progress.md @@ -286,3 +286,91 @@ Verdict is **changes-requested** → host (`/run-issue`) should dispatch **`address-findings`** to work the two must-fixes (and the suggestions as warranted), then re-dispatch `review-code` for a round-2 pre-push read. The diff is not pushed until a pre-push review returns **approved**. + +## Implementation +**Status**: complete +**When**: 2026-06-29 05:20 +00:00 +**By**: Claude Code Agent (Claude Opus) +**Phase**: address-findings (Round 1 — pre-push review changes-requested) +**Branch**: feature/issue-134 + +Worked the 2 must-fixes and all 4 folded suggestions from the Round-1 Local Review, +plus the requested sub-unit-range test. + +### Must-fixes +1. **Scalar-colormap true-span parity** — `raster_gl_renderer.cpp` fragment shader. + Changed the Scalar denominator from `max(u_max - u_min, 1.0)` to + `max(u_max - u_min, 1e-6)`. The `1.0` floor crushed contrast for any data span + < 1.0 (silent regression once small-range RasterLayer charts joined the shared + shader); the `1e-6` floor keeps the divide-by-zero guard for a genuinely + degenerate (zero-width) range while normalizing over the TRUE span like the old + `ColorMap::color`. Verified against baseline (`85e66a8~1`): GGGS and Sonar used + the same `1.0` floor but their depth/backscatter spans are always ≫1, so they + render identically; only sub-unit RasterLayer/GGGS data now stretches correctly. + Negative-VALUED ranges are unaffected (span stays positive). +2. **Prune GPU-texture leak** — `sonar_live_cache_layer.cpp` `handleCatalog()`. + The prune loop erased `Entry`s (destroying each `QOpenGLTexture`) without a current + GL context → GPU resources leaked as tiles churn. Wrapped the erase loop in + `renderer_.makeCurrent()` / `doneCurrent()` (guarded on `!to_prune.empty()` and a + successful makeCurrent, mirroring the dtor's null-context handling) and reset each + pruned texture under the current context before erase. + **Other eviction sites verified**: the only other `tiles_` mutation that can + destroy a live texture is `warmLoad()`'s `insert_or_assign` on a disable→re-enable + (disable leaves textured entries intact). Guarded that loop with the same + makeCurrent pattern (skipped on the first, empty-map warm load). + +### Suggestions (folded in) +- **Silent scalar RasterIO failure** (`raster_layer.cpp`): a failed scalar read no + longer falls through to `result.ok = true`; it returns with `ok = false` so + `imageReady()` reports `"(load failed)"` instead of silent success + blank layer. +- **`renderToImage` doc/behavior** (`raster_gl_renderer.cpp`): on shader-compile + failure it now returns a null `QImage()` (matching the header contract) BEFORE + allocating/binding the FBO, instead of returning a non-null transparent image the + caller would cache as a valid empty render. +- **RGBA mipmap alpha fringing** (`raster_layer.cpp` + shader): the RGBA upload now + converts to `Format_RGBA8888_Premultiplied` so mipmap/linear filtering box-filters + premultiplied texels (no dark fringes from transparent cells' black RGB). The + shared shader's Rgba branch correspondingly emits the texel as-is (`gl_FragColor = + c`) instead of premultiplying again (`c.rgb * c.a`). Only RasterLayer produces Rgba + items, so the change is contained; the opaque-red unit test is unaffected. +- **Pre-existing null-deref** (`raster_layer.cpp`): guarded + `GDALColorEntry* ce = GetColorEntry(...)` with `if(!ce) continue;` before reading + `ce->c3` (palette index out of range now skips the pixel rather than crashing). + +### Test (must-fix 1) +Added `RasterGlRendererTest.ScalarSubUnitRangeSpansColormap` to +`test/test_raster_gl_renderer.cpp`: a 2×1 R32F tile (col0 = data_min 0.0, +col1 = data_max 0.3), Grayscale, range [0.0, 0.3]. Asserts the max sample reaches the +top of the ramp (`hi.red() > 200`) — under the old `max(span, 1.0)` floor it would +only reach t = 0.3 → grey ~76, so the test FAILS under the floor and PASSES with the +true-span fix. Also asserts preserved contrast (`hi.red() - lo.red() > 150`). + +### Build / test — HONEST STATUS (not built in-container) +`source setup.bash → ./ui_ws/build.sh camp` **fails to configure** here exactly as the +prior entries reported: the lower layers are unbuilt in this container, so camp's ROS +message deps are absent: + +``` +CMake Error at CMakeLists.txt:14 (find_package): + Could not find a package configuration file provided by "marine_ais_msgs" +``` + +So `./ui_ws/test.sh camp` was **not run** — no verbatim ctest counts. I did NOT fake a +build. Instead I `g++ -std=c++17 -fsyntax-only`'d the changed ROS-free units against +system Qt5 (Core/Gui/Widgets/OpenGL/Positioning/Concurrent) + GDAL — all clean (only +the codebase's pre-existing `QOpenGLTexture::setData(void*)` deprecation warning): + +- `raster_gl_renderer.cpp` — clean +- `raster_layer.cpp` — clean +- `test_raster_gl_renderer.cpp` — clean + +`sonar_live_cache_layer.cpp` pulls in ROS headers so can't be syntax-checked in +isolation; its diff is a localized makeCurrent/doneCurrent guard mirroring the dtor. + +The offscreen-GL render tests SKIP in-container and RUN on a GL host (by design). A +full `build.sh`/`test.sh` on a host with the lower layers built — including the new +`ScalarSubUnitRangeSpansColormap` case — is the remaining verification step. + +### Next step +Host should re-dispatch `review-code` for a Round-2 pre-push read; on a GL host, +confirm `test_raster_gl_renderer` RUNs (not SKIP) and the new sub-unit case passes. From ca01178eee6e35bf191ee1fe06c04418fb568f3e Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Mon, 29 Jun 2026 05:33:56 +0000 Subject: [PATCH 14/17] progress: local review (round 2, approved) for #134 --- .agent/work-plans/issue-134/progress.md | 39 +++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/.agent/work-plans/issue-134/progress.md b/.agent/work-plans/issue-134/progress.md index 10eb289..865478a 100644 --- a/.agent/work-plans/issue-134/progress.md +++ b/.agent/work-plans/issue-134/progress.md @@ -374,3 +374,42 @@ full `build.sh`/`test.sh` on a host with the lower layers built — including th ### Next step Host should re-dispatch `review-code` for a Round-2 pre-push read; on a GL host, confirm `test_raster_gl_renderer` RUNs (not SKIP) and the new sub-unit case passes. + +## Local Review (Pre-Push) +**Status**: complete +**When**: 2026-06-29 05:32 +00:00 +**By**: Claude Code Agent (Claude Opus) +**Verdict**: approved + +**Branch**: feature/issue-134 at `396bb74` +**Mode**: pre-push +**Depth**: Deep (reason: large cross-cutting GL refactor, new ADR-0007, concurrency/lifecycle) +**Must-fix**: 0 | **Suggestions**: 3 +**Round**: 2 | **Ship**: recommended — both Round-1 must-fixes verified correct and complete; two independent Deep-tier adversarial passes (Lens A logic, Lens B lifecycle) plus the lead found no new must-fix; only low-priority consistency nits remain. + +Round-2 read of the address-findings fix commit `736922b`. Verified against the code +and the `origin/jazzy` baseline: +- **Must-fix 1 (scalar-colormap true-span)** — `raster_gl_renderer.cpp:84` `max(span, 1e-6)` + matches `ColorMap::color`'s true-span normalization (`color_map.cpp:95-100`); new test + `ScalarSubUnitRangeSpansColormap` discriminates the fix from the old `1.0` floor. Correct. +- **Must-fix 2 (prune GPU-texture leak)** — `sonar_live_cache_layer.cpp:342-362` guards the + prune-erase loop with makeCurrent/doneCurrent; warmLoad's `insert_or_assign` displacement + (`:251,:264`) guarded the same way. All texture destroy/create sites audited (Lens B): + every one runs under a current GL context; member-decl order safe via dtor-body release. + +Static analysis limited (env cannot build lower layers; cppcheck clean but Qt/ROS headers +unresolved). Governance: ADR-0007 added cleanly; ADR-0006 persistence/subscription and +ADR-0001 GUI-thread marshalling verified untouched. Plan adherence strong (ordered +per-adapter commits a→e; `RasterFieldItem::Format` resolved the plan's open RGBA question). + +### Findings +- [ ] (suggestion) Gate sonar prune/warm-load `makeCurrent()` on `hasContext()` like `GggsTileLayer::applyBand` — avoids forcing the offscreen context into existence just to reset null textures — `src/camp_map/ros/live_coverage/sonar_live_cache_layer.cpp:342,251` +- [ ] (suggestion) Declare `renderer_` before `tiles_` in the two ROS-layer headers so safe teardown order is intrinsic, not dtor-body-dependent (current code is safe) — `src/camp_map/raster/gggs_tile_layer.h`, `src/camp_map/ros/live_coverage/sonar_live_cache_layer.h` +- [ ] (suggestion) Drop the now-redundant `Nearest` filter set at upload (renderer sets the scalar filter at draw time; `textureFor` already dropped it) — `src/camp_map/raster/gggs_tile.cpp:180` (pre-existing, unchanged file) + +### Next step +Verdict is **approved** (Round 2, 0 must-fix). Lifecycle: **Local Review** → push / open PR +→ **triage-reviews**. The 3 suggestions are optional and non-blocking; they can be applied +before push or tracked as follow-ups. Remaining verification (unchanged): on a GL host with +the lower layers built, run `./ui_ws/build.sh camp && ./ui_ws/test.sh camp` and confirm +`test_raster_gl_renderer` RUNs (not SKIP) — including the new sub-unit case — and passes. From 7a80af957e91a7676165acf6bef3e1c41a3c3287 Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Mon, 29 Jun 2026 05:40:59 +0000 Subject: [PATCH 15/17] refactor(camp#134): hasContext-gate makeCurrent; member order; drop redundant filter set --- src/camp_map/raster/gggs_tile.cpp | 13 ++++--------- src/camp_map/raster/gggs_tile_layer.h | 15 +++++++++------ .../ros/live_coverage/sonar_live_cache_layer.cpp | 16 ++++++++++++---- .../ros/live_coverage/sonar_live_cache_layer.h | 14 +++++++++----- 4 files changed, 34 insertions(+), 24 deletions(-) diff --git a/src/camp_map/raster/gggs_tile.cpp b/src/camp_map/raster/gggs_tile.cpp index 588feca..4265bd6 100644 --- a/src/camp_map/raster/gggs_tile.cpp +++ b/src/camp_map/raster/gggs_tile.cpp @@ -169,15 +169,10 @@ QOpenGLTexture* GggsTile::texture() texture_->setMipLevels(1); texture_->allocateStorage(QOpenGLTexture::Red, QOpenGLTexture::Float32); texture_->setData(QOpenGLTexture::Red, QOpenGLTexture::Float32, data_.data()); - // [camp#122] Nearest (not Linear) on the value texture. The shader discards by - // exact equality (v == u_nodata); Linear filtering interpolates boundary texels - // between real data and the 9999 sentinel, so they neither equal the sentinel - // (no discard) nor a real value (clamp to u_max), producing a bright one-texel - // halo around NoData regions. Nearest never blends across the NoData boundary, - // removing the halo. Roland accepts the blocky (cell-accurate) raster tradeoff. - // NOTE: only the value texture is Nearest; the colormap LUT (in gggs_tile_layer) - // stays Linear for a smooth ramp. - texture_->setMinMagFilters(QOpenGLTexture::Nearest, QOpenGLTexture::Nearest); + // [camp#134] The Nearest min/mag filter on this scalar value texture is set at + // draw time by RasterGlRenderer (per-item, once per frame) — see camp#122 for the + // rationale (Nearest avoids the NoData-sentinel halo the shader's exact-equality + // discard would otherwise blend across). No upload-time setMinMagFilters() here. texture_->setWrapMode(QOpenGLTexture::ClampToEdge); // Free the CPU copy once it's on the GPU — the texture persists for the // tile's lifetime, so we never re-upload (releaseGL = teardown). Halves diff --git a/src/camp_map/raster/gggs_tile_layer.h b/src/camp_map/raster/gggs_tile_layer.h index e3af854..c88fb83 100644 --- a/src/camp_map/raster/gggs_tile_layer.h +++ b/src/camp_map/raster/gggs_tile_layer.h @@ -158,6 +158,15 @@ private slots: // [camp#134] Latitude tessellation moved into RasterGlRenderer (the shared warp). static constexpr int kMaxImageEdge = 4096; // clamp the offscreen target + // [camp#134] The shared GL raster renderer (its own offscreen context + the + // unified shader + colormap LUT). Replaces this layer's former duplicated + // shader/program/LUT/FBO. Default ramp is Grayscale (the renderer's default), + // preserving the original look; selectable via the context menu. Declared BEFORE + // the texture-holding tiles_ so reverse-declaration destruction tears the tiles + // (and their GL textures) down before the renderer's context — the invariant the + // dtor body already enforces explicitly, now also structural. + RasterGlRenderer renderer_; + QString directory_; int band_ = 1; // [camp#108] selected 1-indexed band (persisted) std::vector> tiles_; @@ -182,12 +191,6 @@ private slots: QMutex abort_flag_mutex_; bool load_started_ = false; // first paint() kicks the load exactly once - // [camp#134] The shared GL raster renderer (its own offscreen context + the - // unified shader + colormap LUT). Replaces this layer's former duplicated - // shader/program/LUT/FBO. Default ramp is Grayscale (the renderer's default), - // preserving the original look; selectable via the context menu. - RasterGlRenderer renderer_; - QImage cached_image_; // last render, reused on pan (re-rendered on zoom) QSize cached_size_; }; 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 b22d37a..d62ffbd 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 @@ -248,7 +248,11 @@ void SonarLiveCacheLayer::warmLoad() // — intact). A displaced Entry's QOpenGLTexture must be freed under a current GL // context, so make it current for the seed loop when tiles already hold textures. // On the first (empty-map) warm load there is nothing to displace, so skip it. - const bool gl_current = !tiles_.empty() && renderer_.makeCurrent(); + // Gate on hasContext() (mirrors GggsTileLayer::applyBand): when no context exists + // yet no texture was ever uploaded, so skip makeCurrent() rather than forcing the + // offscreen context into existence to reset null textures. + const bool gl_current = + !tiles_.empty() && renderer_.hasContext() && renderer_.makeCurrent(); const gggs::Level level(*level_); for(auto& tile : SonarLiveTile::loadCacheDir(cache_dir_, level)) { @@ -336,10 +340,14 @@ void SonarLiveCacheLayer::handleCatalog(const marine_interfaces::msg::TileCatalo // ONLY under a current GL context — erasing it here without one leaks the texture // as tiles churn (the boat keeps producing; the catalog keeps pruning). Make the // renderer's context current for the erase loop, exactly like the dtor's teardown. - // If makeCurrent() fails (offscreen GL unavailable) no live texture was ever - // uploaded, so a plain erase is harmless — mirrors the dtor's null-context guard. + // Gate on hasContext() (mirrors GggsTileLayer::applyBand): with no context yet no + // texture was ever uploaded, so a plain erase is harmless and we skip makeCurrent() + // rather than forcing the offscreen context into existence. If the context EXISTS + // but makeCurrent() fails, the textures stay alive but a plain erase still mirrors + // the dtor's null-context guard. bool pruned = false; - const bool gl_current = !result.to_prune.empty() && renderer_.makeCurrent(); + const bool gl_current = + !result.to_prune.empty() && renderer_.hasContext() && renderer_.makeCurrent(); for(const auto& index : result.to_prune) { auto it = tiles_.find(index); 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 fa6a40f..2684461 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 @@ -167,6 +167,15 @@ private slots: // Needed to recover a GridIndex from a cached GeoTIFF on warm-load. std::optional level_; + // [camp#134] The shared GL raster renderer (its own offscreen context + the + // unified shader + colormap LUT). Replaces the shader/program/LUT/FBO this layer + // used to duplicate from GggsTileLayer. Default ramp Grayscale (renderer default). + // Declared BEFORE the texture-holding tiles_ so reverse-declaration destruction + // tears the tiles (and their Entry GL textures) down before the renderer's + // context — the invariant the dtor body already enforces explicitly, now also + // structural. + raster::RasterGlRenderer renderer_; + marine_tiled_raster_store::TileCatalogReconciler reconciler_; std::map tiles_; @@ -199,11 +208,6 @@ private slots: std::vector*> write_watchers_; bool shutting_down_ = false; - // [camp#134] The shared GL raster renderer (its own offscreen context + the - // unified shader + colormap LUT). Replaces the shader/program/LUT/FBO this layer - // used to duplicate from GggsTileLayer. Default ramp Grayscale (renderer default). - raster::RasterGlRenderer renderer_; - QImage cached_image_; QSize cached_size_; }; From 8bbd868dac45150187c78958ba47220d12e117c9 Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Mon, 29 Jun 2026 05:41:20 +0000 Subject: [PATCH 16/17] progress: address-findings round 2 for #134 --- .agent/work-plans/issue-134/progress.md | 43 +++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/.agent/work-plans/issue-134/progress.md b/.agent/work-plans/issue-134/progress.md index 865478a..826ffdc 100644 --- a/.agent/work-plans/issue-134/progress.md +++ b/.agent/work-plans/issue-134/progress.md @@ -413,3 +413,46 @@ Verdict is **approved** (Round 2, 0 must-fix). Lifecycle: **Local Review** → p before push or tracked as follow-ups. Remaining verification (unchanged): on a GL host with the lower layers built, run `./ui_ws/build.sh camp && ./ui_ws/test.sh camp` and confirm `test_raster_gl_renderer` RUNs (not SKIP) — including the new sub-unit case — and passes. + +--- + +## Implementation — Round-2 consistency nits (address-findings) + +**By**: Claude Code Agent (Claude Opus) +**Status**: complete +**Commit**: `7a80af9` +**Branch**: feature/issue-134 + +Applied all 3 low-priority consistency suggestions from the Round-2 approved review. +No behavior change. + +1. **hasContext()-gate `makeCurrent()`** — `sonar_live_cache_layer.cpp` warm-load + (`:251`) and prune (`:342`) paths now read + `!… .empty() && renderer_.hasContext() && renderer_.makeCurrent()`, mirroring + `GggsTileLayer::applyBand` (`gggs_tile_layer.cpp:553`). When no GL context exists + yet, no texture was ever uploaded, so we skip `makeCurrent()` rather than forcing + the offscreen context into existence to reset null textures. Textures that DO + exist are still destroyed under a current context (correctness preserved). +2. **Member declaration order** — `renderer_` now declared BEFORE `tiles_` (the + texture-holding member) in both `gggs_tile_layer.h` and `sonar_live_cache_layer.h`, + so reverse-declaration destruction tears tiles/textures down before the + renderer/GL context intrinsically. The dtor bodies already enforced this + explicitly; the change makes the invariant structural. No initializer-list + reorder warning introduced (each ctor init-list names only one early member — + `directory_` / `base_namespace_`). +3. **Drop redundant Nearest filter set** — removed + `setMinMagFilters(Nearest, Nearest)` at upload in `gggs_tile.cpp:180`. The + renderer sets the scalar data-texture Nearest filter per-item at draw time + (`raster_gl_renderer.cpp:307-309`), which fully covers this path; sampling stays + Nearest. `setWrapMode(ClampToEdge)` retained (renderer does not set wrap mode). + +**Build status**: not built in-container — `./ui_ws/build.sh camp` fails at the +configure step (`find_package` cannot locate `marine_ais_msgs`; lower layers +unbuilt in this container). Edits made cleanly; host to verify with the lower +layers built. Tests not run for the same reason. + +### Next step +Lifecycle unchanged: **Local Review** (approved) → push / open PR → **triage-reviews**. +All 3 optional suggestions now applied, so nothing remains pending from Round 2. On a +GL host with lower layers built, run `./ui_ws/build.sh camp && ./ui_ws/test.sh camp` +and confirm `test_raster_gl_renderer` RUNs (not SKIP) and passes. From 6e8b6e2a5066420beb25bd38689a5e217ea3a25a Mon Sep 17 00:00:00 2001 From: Claude Code Agent Date: Mon, 29 Jun 2026 05:53:41 +0000 Subject: [PATCH 17/17] progress: local review (round 3, approved) for #134 --- .agent/work-plans/issue-134/progress.md | 48 +++++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/.agent/work-plans/issue-134/progress.md b/.agent/work-plans/issue-134/progress.md index 826ffdc..30d30fa 100644 --- a/.agent/work-plans/issue-134/progress.md +++ b/.agent/work-plans/issue-134/progress.md @@ -456,3 +456,51 @@ Lifecycle unchanged: **Local Review** (approved) → push / open PR → **triage All 3 optional suggestions now applied, so nothing remains pending from Round 2. On a GL host with lower layers built, run `./ui_ws/build.sh camp && ./ui_ws/test.sh camp` and confirm `test_raster_gl_renderer` RUNs (not SKIP) and passes. + +## Local Review (Pre-Push) +**Status**: complete +**When**: 2026-06-29 05:52 +00:00 +**By**: Claude Code Agent (Claude Opus) +**Verdict**: approved + +**Branch**: feature/issue-134 at `8bbd868` (code under review: `7a80af9`) +**Mode**: pre-push +**Depth**: Deep (reason: large cross-cutting GL refactor, new ADR-0007, concurrency/lifecycle) +**Must-fix**: 0 | **Suggestions**: 0 +**Round**: 3 | **Ship**: recommended — Round-3 read of the consistency-nits commit; 0 must-fix, 0 surviving suggestions; delta since Round-2 approval is a no-behavior-change refactor and all 3 Round-2 suggestions are applied. + +Round-3 read of the address-findings consistency commit `7a80af9` (the 3 Round-2 +suggestions: hasContext-gated makeCurrent, member-decl reorder, redundant Nearest +filter drop). Deep tier: 2 disjoint-lens Claude Adversarial passes (Lens A logic, +Lens B lifecycle/concurrency). Lens A clean. Lens B raised 4 findings — the lead +adversarially verified ALL 4 as false positives / non-actionable against the code: +- warmLoad `insert_or_assign` "leak": FP — `makeCurrent()` is hoisted ABOVE the + insert loop (`sonar_live_cache_layer.cpp:32-33` before `:44`), so displaced + textures destruct under a current context. +- stale-texture race between `items()` and `renderToImage()` (raised twice): FP — + `renderImage()` calls both back-to-back synchronously (`:7-12`); the Qt event loop + never spins between them, so the queued `handleCatalog` slot cannot interleave; + textures are touched only on the GUI thread. +- dtor leak if `makeCurrent()` fails: not actionable — if makeCurrent fails, uploads + (also gated on makeCurrent) never succeeded, so there are no GPU textures to leak. + +Lead verification of the delta: member reorder is correct (no `-Wreorder`; each ctor +init-list names one early member; dtor bodies already release textures under a current +context, so the reorder is a sound defensive backstop). hasContext() gating sound +(no context ⟹ no texture uploaded). Redundant Nearest-filter drop safe (renderer sets +the scalar filter per-item at draw, `raster_gl_renderer.cpp:307-309`). Prior Round-1 +must-fixes verified still in place (true-span `max(span,1e-6)` `:84`; null-image on +shader-compile-fail before FBO `:220`; Rgba emitted as-is `:93`). Governance: ADR-0007 +added cleanly; ADR-0006/0001 contracts untouched. Plan adherence strong. Static +analysis limited (env cannot build lower layers; cppcheck misparses Qt namespaces); +implementer `g++ -fsyntax-only` clean. + +### Findings +- [ ] No issues found. LGTM. + +### Next step +Verdict is **approved** (Round 3, 0 must-fix). Lifecycle: **Local Review** → push / +open PR → **triage-reviews**. Nothing remains pending. Remaining verification +(unchanged, environmental only): on a GL host with the lower layers built, run +`./ui_ws/build.sh camp && ./ui_ws/test.sh camp` and confirm `test_raster_gl_renderer` +RUNs (not SKIP) — including the sub-unit-range case — and passes.