Field import: camp (2026-08-27) - #220
Merged
Merged
Conversation
added 3 commits
August 27, 2026 07:44
The live-coverage catalog subscription was created transient-local to match the boat-side producer. But camp runs on the operator side and never sees that publisher: it receives the catalog as republished by udp_bridge, which emits VOLATILE. A transient-local subscriber is QoS-incompatible with a volatile publisher, so the subscription never matched, handleCatalog() never fired, and the ADR-0006 D4 anti-entropy reconcile never pruned in-memory tiles, their GPU textures, or the on-disk copies. Stale coverage persisted on screen indefinitely after a boat-side store reset. Measured live on pandy during the BizzyBoat deployment, on /bizzy/sensors/m3/cube_bathymetry/coverage_catalog: publisher /operator/udp_bridge RELIABLE/VOLATILE subscriber /operator/camp RELIABLE/TRANSIENT_LOCAL (never matches) subscriber coverage_renderer RELIABLE/VOLATILE (matched fine) Confirmed both directions with `ros2 topic echo --once --qos-durability`: volatile returns a catalog immediately, transient_local returns nothing. Switch the durability to VOLATILE, keeping RELIABLE and depth 1. The tradeoff is explicit and accepted: there is no latched sample, so camp no longer gets the current catalog on join and must wait for the next publication. A never-matching subscription delivers nothing at all, so waiting strictly dominates. The camp#169 buffering in handleCatalog() is unchanged and stays correct — it is now more load-bearing, not less. Its premise was the once-only delivery of a latched sample; under VOLATILE there is no latched sample either, and the catalog is published only on change while the boat's catalog is stable, so a sample dropped while disabled may not recur for the rest of the session. disableLiveCoverage() tears down only the tile stream (unsubscribeTiles); catalog_sub_ is reset only in the destructor, so samples keep arriving and landing in last_catalog_ throughout a disabled window for enableLiveCoverage() to replay. Comments on the buffer, the member, and the class doc corrected to state the real reason. The QoS moves into catalogSubscriptionQos() so a regression test can pin it. Nothing pinned the durability before, which is exactly why the mismatch was invisible; test_sonar_live_catalog_qos now asserts VOLATILE/RELIABLE/ depth 1. Verified: camp builds; 312 tests, 0 failures (1 pre-existing GL skip in test_gggs_render). A camp restart is required for this to take effect.
The ADR-0006 D4 anti-entropy reconcile pruned only the fine tiles. The overview pyramid built by foldIntoParent() was entirely exempt: handleCatalog() erased from `tiles_` and deleted `<cache_dir>/<stem>.tif`, and never touched `overview_tiles_` or `overviews/`. The comment on foldIntoParent() asserted that exemption as intended design — "a local derived product; prune-on-absence must not touch them" — which conflated ADR-0010 D4 (overviews are never entered into the reconciler, true and unchanged) with an exemption from the prune itself. So a retracted region kept its coarse coverage forever, and warm-load reinstated it on every restart. Measured on pandy during the BizzyBoat deployment after a boat-side store reset: with the 532b6df QoS fix in place the fine cache pruned correctly (107M -> 44M, every survivor newer than the restart) while 41M of `overviews/` survived and the operator still saw the pre-reset coverage. No code path could ever reflect a boat-side reset in the pyramid. Compounding it, the fold is purely accumulative — foldChild() only ever folds data IN and has no inverse — so a withdrawn contribution cannot be subtracted from a parent in place. reconcilePyramid() runs at the end of every reconcile, after the fine prune. The catalog is authoritative over the fine tiles and the pyramid is a function of them, so an overview should exist exactly when it is an ancestor of a live fine index. Two failure modes, two repairs: * stale by absence — no live descendant: remove it from `overview_tiles_`, free its GL texture under the renderer's context (the camp#134 discipline the fine prune already follows), and delete its `overviews/` file. The DIRECTORY is swept, not just the resident map: eviction phase 2 frees an overview Entry while deliberately keeping its disk copy (ADR-0010 D2), so a stale overview can be absent from memory and still be warm-loaded back. That is exactly how the pandy pyramid kept returning. * dirty by partial withdrawal — some descendants live, one was withdrawn: rebuild the tile from its surviving children. This reproduces the original construction (D3 folds the WHOLE parent into the grandparent, so every level above the fine tiles is "the fold of my children"), drawing inputs from a child repaired earlier in the same pass, else the resident copy, else the disk copy. A tile with no surviving child is removed instead — a zoomed-out gap is honest, stale coverage is not. Two properties are load-bearing. Proportionality: every fine tile's ancestor chain reaches level 0, so invalidating the chain of each pruned tile would destroy the whole pyramid on any ordinary retraction, and tiles are withdrawn and re-added in normal operation. Nothing is removed while a live descendant remains, and only ancestors of something really withdrawn are rebuilt. Prune-gate parity: a generation_time of 0 disables prune-on-absence in the reconciler (ADR-0008 D4b), so it disables the pyramid sweep too, and the live set is unioned with the ancestors of the fine tiles still held locally so a held tile too NEW to prune keeps its ancestors alive. Budget accounting is preserved: removals shrink overviewResidentBytes(), a rebuild that pulls a non-resident overview back into memory is followed by evictIfOverBudget(), and overviews still never enter the reconciler. Two supporting changes. cancelPendingWrite() drops a deleted tile's queued write-through (a coalesced relaunch would otherwise re-create the .tif the prune just removed); an in-flight write is left to finish with its WriteState intact, because erasing that state would let a later fold launch a second worker racing it on the shared <stem>.tif.tmp path. And the `<level>_<row>_<col>` cache stem, open-coded at three sites, moves into tileStem()/fineTilePath()/overviewTilePath() with indexFromStem() as the inverse the disk sweep needs — a divergence between the writer's name and a deleter's would orphan files silently rather than fail. test_sonar_live_overview_prune pins all of it (GL-free, ROS-free, same offscreen harness as test_sonar_live_eviction): stale overviews leave memory and disk; ancestors with a surviving catalogued descendant are kept (the over-deletion guard); a shared ancestor is rebuilt without the withdrawn descendant's contribution, in memory and on disk; a catalog that retracts nothing changes nothing; an unstamped catalog sweeps nothing. Verified the first two fail against a stubbed reconcilePyramid, and that the over-deletion guard fails against the rejected naive chain-invalidation design. ADR-0010 gains D7 and retires the "overview lifecycle-on-retraction / catalog-prune propagation" deferred follow-up it had carried since 2026-08-20. Per-overview provenance was rejected as an alternative: it cannot survive warm-load, which is precisely the path that resurrects a stale pyramid. Verified: camp builds clean; 317 tests, 0 failures (baseline 312, +4 new gtest cases +1 ctest aggregate entry), 1 pre-existing GL skip in test_gggs_render. A camp restart is required for this to take effect.
… tile The live-coverage draw list appended EVERY resident overview tile, coarse->fine, then the fine tiles on top. Its stated premise (camp#160) was "where a fine tile is present it fully covers its parent" — false: a child covers a QUARTER of its parent. Wherever fine coverage is sparse, which is most of a survey in progress, every coarse ancestor painted through, and the apex is resident by design (kApexProtectLevel, ADR-0010 D1) so the coarsest level of all drew at every zoom. Measured on pandy during the BizzyBoat deployment: every cached tile is 960x960 cells at every level, so ground cell size doubles per level up and a level-0 cell is ~667 x 926 m carrying the MEAN of everything folded beneath it (4 valid cells at level 0; 1264 at level 11). The operator saw a solid ~650 m block over a good portion of the survey area, covering the chart — including water the survey has never touched, since an overview's own extent is 4^n fine tiles wide. Coarse data may now be drawn ONLY as a placeholder for a fine tile that is known to exist and has not loaded, and only over that tile's own footprint. planDraw() resolves exactly two contributions: for each pending index, the finest resident ancestor overview painted over the FINE index's extent through the sub-rect of that ancestor it occupies; and every resident fine tile, whole, on top. Nothing else. "Known to exist" is derived, not guessed: the catalog is authoritative for what exists (catalogued_fine_, assigned from every catalog since a complete catalog is a snapshot), unioned with evicted_fine_indices_ for the warm start with no link, minus what is resident. Scale never enters it. A placeholder exists exactly while a real tile is missing, so zoom-out needs no threshold: zoomed out, nearly every catalogued tile is non-resident and the union of the footprints IS the coverage. The alternative considered first — cell size vs. screen pixel — was rejected: it would have stopped level 0 at survey zoom, but at any zoom where a coarse level did qualify it would still smear that level across its full extent, unsurveyed water included. Clipping needed a renderer change. RasterFieldItem carries one texture and a geographic extent, and the renderer stretches the whole texture across whatever bounds it is given, so narrowing the bounds would SQUASH the overview into the small box rather than clip it. The item gains a normalized texture window [u0,v0]-[u1,v1] (u west->east, v north->south, texture row 0 = north) honoured by RasterGlRenderer's texcoord generation. It defaults to the whole texture, so RasterLayer, GggsTileLayer and this layer's own resident tiles emit exactly the coordinates they did before — verified by reading all three call sites (each default-constructs the item and assigns members, none aggregate-initialises). The window comes from GGGS index arithmetic (rows and columns both double per level, and the +-72/+-80 latitudeScaleFactor bands are bounded by whole grid rows at every level, so an ancestor and its descendants share a band), making it an exact power-of-two fraction; a descendant that does not resolve inside its ancestor draws nothing rather than guessing. The CPU-side alternative — crop each sub-window into its own small texture, no renderer change — was rejected: at zoom-out the pending set is the whole survey, so it pays an upload per placeholder per frame, or keeps a second resident texture per pending index, which is the memory eviction exists to reclaim. camp#103 offscreen culling is intact (the clip test is still before textureFor, now in planDraw), as is the camp#134 GL-context discipline. camp#172 reload is unchanged and consistent by construction: every index it wants to reload is evicted, hence pending, hence currently shown as a placeholder, and a completed reload replaces that placeholder with the real thing. The converse deliberately does not hold — a catalogued tile never received has no disk copy, so it is a request candidate, not a reload one. test_sonar_live_placeholder_draw pins the rule (GL-free, ROS-free, same offscreen harness as test_sonar_live_overview_prune): nothing coarse while the fine tiles are resident (the reported defect); a known-but-unloaded tile IS covered, over its own footprint and through a strict sub-rect of the ancestor, asserted against an independent geographic derivation of the same window; whole-survey coverage survives evicting every fine tile, with no catalog at all; and unsurveyed water inside the apex's extent draws nothing at either zoom. Verified the first, third and fourth fail against the pre-fix draw list and the second and third against an unclipped placeholder. test_raster_gl_renderer gains the renderer half: the default window still samples the whole texture, and each quadrant window samples exactly that quadrant (the north/south pair catches a flipped v). Verified it fails against the pre-change texcoords. ADR-0010 D5 is replaced (draw-order fallback -> clipped placeholder) and D1's guarantee is restated in the shape it now has: the protected apex is not drawn at zoom-out, it is the guaranteed resident SOURCE a zoomed-out view samples through the footprints of the missing tiles. Two consequences recorded: a pyramid whose fine tiles are neither catalogued nor on disk now draws nothing, and zoom-out issues one small quad per missing tile (sharing the same few textures) rather than one per overview. Verified: camp builds; 323 tests, 0 failures (baseline 317, +5 new gtest cases +1 new ctest aggregate entry), 1 pre-existing GL skip in test_gggs_render. A camp restart is required for this to take effect.
This was referenced Aug 31, 2026
Merged
Owner
Author
|
Pre-review findings from a second import pass (which duplicated this PR — see below). The findings Finding 1 — the VOLATILE downgrade is framed as permanent, and the root cause is not what we thought
Investigating that difference turned up something that changes both: So the relay behaved exactly as designed and documented. This is a configuration omission, not a Why that matters for this PR specifically: once the config is set, this subscription should go Finding 2 — ADR edits arrive without separate review
CleanEvery commit ships a test registered in Not verified here: the three test binaries were written and run in the field, not re-built or Note on provenance: I opened a duplicate import (issue #222 / PR #223) for these same three Authored-By: |
…219) The field commit framed the durability downgrade as a permanently accepted tradeoff. The equivalent change in marine_web_view instead marks itself a field workaround to revert once the relay is fixed, and investigating that difference established which framing is right: udp_bridge is not at fault. It already supports per-topic transient-local durability (doc/qos_design.md, "Durability"); the boat's coverage_catalog bridge entry never set it, so the topic took the documented VOLATILE default. The remedy is one key in bizzyboat.yaml, tracked with the coupled reverts as rolker/unh_echoboats_project11#484. No behaviour change -- the comment only. Recorded at the QoS site because nothing will prompt the revert: a transient-local publisher still matches a volatile subscriber, so once the config lands camp keeps working and keeps silently losing late-join.
There was a problem hiding this comment.
🔵 Needs a closer look
It makes substantial, safety-relevant changes to live coverage selection/caching/cleanup behavior and introduces nontrivial reconcile logic that warrants final human review despite strong test coverage.
Pull request overview
Imports the 2026-08-27 field fixes to CAMP’s live-coverage rendering/cache logic (SonarLiveCacheLayer), addressing stale overview persistence on catalog retractions and preventing coarse overview data from “washing over” unsurveyed water by drawing it only as a clipped placeholder for known-missing fine tiles. This also includes a QoS durability adjustment for the operator-side coverage_catalog subscription to match udp_bridge behavior, with accompanying regression tests and ADR updates.
Changes:
- Add overview-pyramid prune-on-absence propagation (including rebuild-on-partial-withdrawal) driven by catalog reconcile.
- Replace “draw all overviews under fine tiles” with a placeholder model: coarse data is drawn only for known-missing fine tiles, clipped to the fine footprint via a texture sub-rect.
- Add and expand headless/GL tests, and extend the raster renderer/item API to support texture windows.
File summaries
| File | Description |
|---|---|
src/camp_map/ros/live_coverage/sonar_live_cache_layer.cpp |
Implements VOLATILE catalog QoS helper, pyramid reconcile/sweep+rebuild, placeholder-based draw planning, and disk/GL cleanup interactions. |
src/camp_map/ros/live_coverage/sonar_live_cache_layer.h |
Exposes catalog QoS helper and adds test seams / draw-plan structures supporting the placeholder model and pyramid reconciliation. |
src/camp_map/raster/raster_field_source.h |
Extends RasterFieldItem with normalized texture window coordinates (u0/v0/u1/v1). |
src/camp_map/raster/raster_gl_renderer.cpp |
Updates texcoord generation to honor the item texture sub-rect window. |
test/test_raster_gl_renderer.cpp |
Adds a GL regression test validating default/full-window behavior and correct quadrant sampling for sub-rect windows. |
test/test_sonar_live_overview_prune.cpp |
New headless tests pinning pyramid prune-on-absence + rebuild semantics and disk persistence behavior. |
test/test_sonar_live_placeholder_draw.cpp |
New headless tests pinning placeholder-only coarse drawing and strict clipping to fine footprints. |
test/test_sonar_live_catalog_qos.cpp |
New test pinning coverage_catalog subscription QoS durability/reliability/depth. |
docs/decisions/0010-bounded-eviction-overview-pyramid.md |
Updates ADR-0010 to replace D5 with placeholder+clip design and add D7 for pyramid prune propagation. |
CMakeLists.txt |
Registers the new gtests and links required dependencies. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1132
to
+1134
| const auto staged_it = staged.find(child); | ||
| if(staged_it != staged.end()) | ||
| return staged_it->second; |
Comment on lines
+44
to
+48
| /// Exposed (rather than inlined at the subscribe site) so the durability can be | ||
| /// pinned by a regression test: the operator-side subscriber receives the catalog | ||
| /// republished by `udp_bridge`, which is volatile, and a transient-local | ||
| /// subscriber never matches it. Nothing pinned that, which is exactly why the | ||
| /// mismatch went unnoticed. See the definition for the full rationale. |
There was a problem hiding this comment.
🔵 Needs a closer look
It changes core live-coverage rendering semantics, catalog QoS compatibility, and pyramid lifecycle (including disk sweeps/rebuilds), which warrants final human review despite strong regression tests.
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 0 new
- Review effort level: Lite
There was a problem hiding this comment.
🔵 Needs a closer look
There are unresolved documentation/discrepancy issues that could mislead future maintenance (notably around the VOLATILE QoS rationale/revert framing and conflicting root-cause narrative).
Review details
Suppressed comments (3)
Previously missed (1) — in code that hasn't changed since the last review.
src/camp_map/ros/live_coverage/sonar_live_cache_layer.cpp:344
- This comment states udp_bridge is “NOT at fault” and the root cause is missing per-topic durability configuration, but the PR description/issue text still frames the problem as udp_bridge downgrading durability. That mismatch makes it hard to know what to track/revert and where the real fix belongs; please align the in-code rationale with the PR description (or vice versa) so future readers don’t get conflicting guidance.
src/camp_map/ros/live_coverage/sonar_live_cache_layer.h:48
- The header docstring for catalogSubscriptionQos() explains why VOLATILE is needed but doesn’t mention that this is a temporary workaround that should be reverted once the bridge preserves TRANSIENT_LOCAL durability. Without that “revert later” framing here (where most callers will look first), it reads like a settled design choice and risks becoming permanent by accident.
/// QoS used for the `<base>/coverage_catalog` subscription: RELIABLE, depth 1,
/// and — [field 2026-08-27] — **VOLATILE**.
///
/// Exposed (rather than inlined at the subscribe site) so the durability can be
/// pinned by a regression test: the operator-side subscriber receives the catalog
/// republished by `udp_bridge`, which is volatile, and a transient-local
/// subscriber never matches it. Nothing pinned that, which is exactly why the
/// mismatch went unnoticed. See the definition for the full rationale.
src/camp_map/ros/live_coverage/sonar_live_cache_layer.cpp:1148
- overviewRebuildChild() returns SonarLiveTile by value (e.g.,
return staged_it->second;/return resident->second.tile;), which copies all band buffers (SonarLiveBand::data is a std::vector). During pyramid rebuild this can cause large, repeated GUI-thread copies of multi-megabyte tiles and avoidable latency.
Consider returning a pointer/reference for staged/resident children and only owning an optional tile for the disk-loaded case (e.g., return const SonarLiveTile* plus an out-param/owned optional, or return std::shared_ptr<const SonarLiveTile>).
if(gone.count(child))
return std::nullopt;
const auto staged_it = staged.find(child);
if(staged_it != staged.end())
return staged_it->second;
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
Three documentation-only fixes from the integrated review. No behaviour change. The header declaration of catalogSubscriptionQos() summarised the VOLATILE durability without any signal that it is temporary, so a reader who stopped at the public API surface would treat it as settled. The .cpp definition already carried the full revert condition; this states it where it is read first. Raised by Copilot on PR #220 and independently in the import pre-review. D7 cited 'ADR-0008 D4b'. uma's ADR-0008 has no such label -- the timestamp gate is correctness condition (b) of D4. The semantics D7 relies on are exactly right; the label was invented. D7's Proportionality property bounds how many tiles a rebuild touches, which is the argument that keeps one retraction from destroying the pyramid. It says nothing about cost per tile, and the sweep runs on the Qt GUI thread doing synchronous GDAL reads, file deletes and multi-MB band copies there. uma ADR-0008 D5 already rules disk work off that thread for the write-through, so this is an existing policy with a gap rather than a new concern. Recorded in the deferred-follow-ups list and tracked as camp#224, which asks for a profile before any threading rework. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014eGztbXhDcsyFFx8Lp1BAr
There was a problem hiding this comment.
🔵 Needs a closer look
The changes materially alter live-coverage pruning and rendering behavior (including GUI-thread rebuild paths) in a safety-relevant operator display component, and warrant final human review despite strong test coverage.
Review details
Suppressed comments (2)
src/camp_map/ros/live_coverage/sonar_live_cache_layer.cpp:1148
- overviewRebuildChild() returns SonarLiveTile by value for staged children ("return staged_it->second"), which copies the full band payload (potentially multiple MB per tile) on the GUI thread during reconcilePyramid(). This can cause noticeable UI stalls when rebuilding many ancestors after a large prune.
Consider changing the rebuild path to avoid copying tile payloads (e.g., return a pointer/reference to staged/resident tiles and only materialize an owned tile for the disk-load case, or store staged tiles as shared_ptr/unique_ptr and pass references into foldChild()).
if(gone.count(child))
return std::nullopt;
const auto staged_it = staged.find(child);
if(staged_it != staged.end())
return staged_it->second;
src/camp_map/ros/live_coverage/sonar_live_cache_layer.cpp:1157
- overviewRebuildChild() also returns resident tiles by value ("return resident->second.tile"), which duplicates the tile payload even though the resident Entry already owns it. If reconcilePyramid() rebuilds multiple ancestors, these copies can multiply quickly and run on the GUI thread.
If you refactor the rebuild path, prefer passing const SonarLiveTile& (or pointer/reference) for staged/resident children and only loading/copying when reading from disk is necessary.
const std::map<gggs::GridIndex, Entry>& pool = child_is_fine ? tiles_ : overview_tiles_;
const auto resident = pool.find(child);
if(resident != pool.end())
return resident->second.tile;
- Files reviewed: 11/11 changed files
- Comments generated: 0 new
- Review effort level: Lite
This was referenced Sep 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Imports the 2026-08-27 field commits from gitcloud. Part of
rolker/camp#219; produced during the wrap-up of
deployment rolker/unh_echoboats_project11#467.
Branched directly from
gitcloud/jazzyso the original field SHAs are preserved. Not diverged; thisis a fast-forward.
What's in it
Three commits reworking the live-coverage cache layer (+666 in
sonar_live_cache_layer.cpp, +158 inits header): catalog prune-on-absence propagated into the overview pyramid, coarse coverage drawn
only as a clipped placeholder where a tile is missing, and the catalog QoS change below.
Reviewer attention:
532b6dfis the mirror of a workaroundIt drops CAMP's
coverage_catalogsubscription to VOLATILE to match udp_bridge's republish. Unlikeits counterpart in
unh_marine_autonomy(1ffc459), this commit does not carry the "fieldworkaround, revert later" framing, so on its own it reads as a settled fix. It is not one.
cube_bathymetrypublishes the catalogtransient_localdeliberately for late joiners(
cube_bathymetry_node.cpp:515); the break is that udp_bridge downgrades durability when itrepublishes, and a TRANSIENT_LOCAL subscriber cannot match a VOLATILE publisher. Where the fix
belongs was put to the operator during the #467 wrap-up and left open: "I don't have an opinion on
1, I'd have to delve deeper into what was happening."
CAMP masked this break for longer than the web renderer did, because it warm-loads a disk cache
while the renderer is memory-only.
Revert together with
rolker/unh_marine_autonomy#363 once the
relay preserves producer durability.
Test plan
Roughly 1,070 lines of new tests accompany the change:
test_sonar_live_overview_prune.cpp(487 lines) — prune-on-absence into the overview pyramidtest_sonar_live_placeholder_draw.cpp(471 lines) — clipped placeholder for a missing tiletest_raster_gl_renderer.cpp(68 lines) — renderer changestest_sonar_live_catalog_qos.cpp(44 lines) — the catalog QoS subscriptionField-verified on pandy during the 2026-08-27 Appledore deployment; the operator confirmed live
coverage rendering after the change.
Authored-By:
Claude Code AgentModel:
Claude Opus 5 (1M context)