Grid layers: stop crashing on removal, stop rendering hidden layers, and keep empty live-coverage cells transparent by rolker · Pull Request #210 · rolker/camp

rolker · 2026-08-24T17:46:49Z

Summary

Four defects found while investigating the CAMP out-of-memory kill (#208). Three are memory or display; one is a crash. All were surfaced by operator use rather than by inspection — removing a layer, and a giant purple tile over the chart.

1. SIGSEGV when removing a costmap layer (#209)

OccupancyGrid dispatches its render to a worker bound to this via QtConcurrent::run, and had no destructor. Remove the layer mid-render and the worker writes into a destroyed object.

Both siblings already handle this — ~GridMap() is documented as "joins the in-flight render worker before teardown" and ~RasterLayer() calls future_watcher_.waitForFinished(). OccupancyGrid was simply missed. The window is wide, not narrow: a 5000×5000 costmap is 25 M cells per render.

2. Work done for layers the operator switched off (#208)

There was no visibility gate anywhere in either grid layer. CAMP auto-creates a layer for every OccupancyGrid/GridMap topic with a publisher (GridManager::updateTopics), which on the sim is eleven layers: one costmap plus ten S-57 chart datasets. Each rendered on arrival regardless of whether it was checked, and each holds a grid-sized image plus its pixmap for as long as the layer exists.

On the dev host an unchecked costmap layer was among the largest consumers in the process.

GridMap needed more than the costmap's gate: those datasets are latched — an S-57 chart layer publishes once and never again, so skipping the render while hidden would leave it permanently blank when re-enabled. A costmap self-heals on its next publish; this cannot. itemChange(ItemVisibleHasChanged) therefore re-renders from the retained message on the hidden→shown transition.

3. The costmap image was 4× larger than the data it draws (#208)

Format_ARGB32 at grid resolution — 100 MB for a 5000×5000 grid whose source is one signed byte per cell. Now Format_Indexed8 using the 256-entry table built from the palette this file already had; identical output, a quarter of the memory.

The fill also stopped being per-pixel: setPixelColor() cost a virtual call plus a QColor conversion for every one of those 25 million cells, on every update. Rows now go through scanLine(), where the palette index is the pixel value.

4. Live coverage painted empty cells opaque (#208) — the purple tile

A mostly-empty tile painted solid over the chart: an 8°×8° apex tile covering all of New England, drawn opaque because its empty cells read back as real depths of −327.68 m.

Bands each carry their own NoData sentinel on the wire (quantization needs an in-range integer), and the cache writer called SetNoDataValue() once per band. GeoTIFF's TIFFTAG_GDAL_NODATA holds one value per dataset, so only the last band's sentinel survived and was applied to all three. CAMP logged this itself and it was mistaken for GDAL noise:

Warning 1: band 2: Setting nodata to -327.67999267578125 on band 2, but band 1
has nodata at 2.0039370059967041. The TIFFTAG_GDAL_NODATA only support one
value per dataset.

gdalinfo on the real tile shows the consequence exactly:

band	stored NoData	content	valid
backscatter	12.75	constant 2.004	100%
depth	12.75	constant −327.68	100%
uncertainty	12.75	real data	0.004%

Only uncertainty — whose sentinel won the race — knows it is nearly empty.

Normalising every band to NaN removes the failure mode rather than working around it: one slot is enough when all bands agree, and NaN needs no tag at all to be recognised. This also matches what the world store already does (marine_bathymetry_store s102/convert.cpp writes SetNoDataValue(nan)), so the live cache stops being the one place with its own convention.

No consumer changes were needed — every reader already guards with !std::isfinite(v) || (has_nodata && v == nodata), and the !isfinite clause catches NaN.

Test plan

293 tests, 0 failures, 1 skipped (pre-existing).

New test_sonar_live_tile_nodata builds a three-band tile with distinct wire sentinels, writes it, reloads it, and asserts only genuinely covered cells are drawable. Negative-controlled: with the fix reverted it fails on exactly backscatter and depth while uncertainty passes — reproducing the field asymmetry above rather than an approximation of it.

Two existing tests asserted the old contract and were updated, not bypassed:

PatchApplyDequantize expected the empty cell to equal -327.68; it now asserts NaN, and that the band's nodata is NaN.
FoldChildPropagatesNoData counted holes with v == nodata, which can never match NaN; it now uses the !isfinite guard — the same one every consumer already used, which is precisely why no production code needed changing.

Not verified

No GUI eyeball since these landed. The riskiest path is the S-57 visibility transition: uncheck an S-57 layer and re-check it — it must come back, not stay blank. That is latched data with no republish, so a headless test cannot fully cover it.

Deliberately out of scope

CAMP still auto-subscribes to every grid topic. Making that opt-in is the largest remaining memory lever and the operator's stated preference, but it changes what appears in the Layers tab and is not something to land the night before a survey. To be filed separately.

Closes #209
Part of #208

… memory Three defects in OccupancyGrid, found while investigating the CAMP OOM. 1. SIGSEGV on layer removal (#209). processOccupancyGrid runs on a worker bound to `this` via QtConcurrent::run, and OccupancyGrid had NO destructor, so removing the layer mid-render let the worker write into a destroyed object. Both siblings already handle this — ~GridMap() "joins the in-flight render worker before teardown" and ~RasterLayer() calls future_watcher_.waitForFinished(); OccupancyGrid was the one that was missed. The window is wide, not narrow: a 5000x5000 costmap is 25 million cells per render. 2. Work done for a layer the operator switched off (#208). There was no visibility gate anywhere in the file, so an UNCHECKED costmap layer still rendered every update and kept a grid-sized image and pixmap resident. On the dev host that made a hidden layer one of the largest consumers in the process. Costmaps republish on their own timer, so the display refreshes on the next publish after the layer is re-enabled. 3. The image was four times larger than the data it draws (#208). Format_ARGB32 at grid resolution is 100 MB for a 5000x5000 grid whose source is one signed byte per cell. Now Format_Indexed8 with the 256-entry table built from the palette this file already had — identical output, a quarter of the memory. The fill also stopped being per-pixel: setPixelColor() cost a virtual call plus a QColor conversion for every one of those 25 million cells on every update. Rows are now written through scanLine(), where the palette index IS the pixel value, so the inner loop is a byte store. Adds a null check on the image allocation, which a grid-sized QImage can genuinely fail. Closes #209 Part of #208

…parent A mostly-empty live-coverage tile painted solid over the chart — an 8x8 degree apex tile covering all of New England, drawn opaque because its "empty" cells read back as real depths of -327.68 m. Cause: bands each carry their own NoData sentinel on the wire (quantization needs an in-range integer), and the cache writer called SetNoDataValue() once per band. GeoTIFF's TIFFTAG_GDAL_NODATA holds ONE value per dataset, so only the last band's sentinel survived and was applied to all three. camp logged this itself and it was mistaken for GDAL noise: Warning 1: band 2: Setting nodata to -327.67999267578125 on band 2, but band 1 has nodata at 2.0039370059967041. The TIFFTAG_GDAL_NODATA only support one value per dataset. On reload the other bands had no recognised NoData, so gdalinfo reported STATISTICS_VALID_PERCENT=100 for depth and backscatter (constant -327.68 and 2.004) while uncertainty — the band whose sentinel happened to win — correctly reported 0.004%. NaN removes the failure mode rather than working around it: every band's sentinel is now the same value, so one slot is enough, and NaN needs no tag to be recognised at all. This also matches what the world store already does (marine_bathymetry_store s102/convert.cpp writes SetNoDataValue(nan)), so the live cache stops being the one place with its own convention. No consumer changes needed: every reader already guards with !std::isfinite(v) || (has_nodata && v == nodata), and the !isfinite clause catches NaN — an equality test alone never would. NOTE: tiles already on disk keep their lost sentinels and will still render opaque. The cache needs clearing once for this to take effect. Part of #208

camp auto-creates a layer for every OccupancyGrid/GridMap topic with a publisher (GridManager::updateTopics), which on the sim is ELEVEN layers: one costmap plus ten S-57 chart datasets. Each rendered on arrival regardless of whether it was checked in the Layers tab, and each holds a grid-sized ARGB image plus its pixmap for as long as the layer exists. Rendering is now gated on isVisible(). The retained last_msg_ stays — a colormap change or a re-show needs it (camp#63) — but the expensive half, the rasterisation and the two full-size buffers it produces, no longer happens for layers nobody is looking at. A GridMap needs more than the costmap's gate: these datasets are LATCHED. An S-57 chart layer publishes once and never again, so skipping the render while hidden would leave the layer permanently blank when re-enabled — a costmap self-heals on its next publish, this cannot. itemChange(ItemVisibleHasChanged) therefore re-renders from last_msg_ on the hidden->shown transition. Part of #208

New test_sonar_live_tile_nodata reproduces the failure that painted a New-England-sized apex tile solid over the chart: a tile with three bands, each with its own wire sentinel, written and reloaded. It asserts that only the genuinely covered cells are drawable afterwards. Negative-controlled — with the fix reverted it fails on exactly the two bands whose sentinel lost the single GeoTIFF nodata slot (backscatter and depth) while uncertainty, whose value won, passes. That is the same asymmetry gdalinfo showed on the real tile: STATISTICS_VALID_PERCENT=100 for depth and backscatter against 0.004% for uncertainty. Two existing tests asserted the OLD contract and are updated, not bypassed: - PatchApplyDequantize expected the empty cell to equal the dequantized sentinel (-327.68). It now asserts NaN, and additionally that the band's nodata IS NaN. - FoldChildPropagatesNoData counted holes with `v == nodata`, which can never match NaN. It now counts them the way every consumer does, with the !isfinite guard — which is the point: the guard already handled NaN, which is why no production code needed changing. Full camp suite: 293 tests, 0 failures. Part of #208

Copilot

🟡 Changes recommended

The new visibility gating calls isVisible() from ROS executor callback threads in both grid layers, which is not thread-safe for Qt graphics items and can introduce new race/crash modes.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Fixes several stability, performance, and rendering correctness issues in CAMP’s grid and live-coverage layers, addressing a reported crash on layer removal, wasted rendering while layers are hidden, excessive costmap memory use, and incorrect NoData handling that could paint empty sonar tiles opaque.

Changes:

Added safe teardown for OccupancyGrid rendering work and improved costmap rendering memory/performance (Indexed8 + scanLine path), plus visibility gating.
Added visibility-aware rendering behavior for GridMap, including re-render on hidden→shown for latched datasets.
Normalized sonar live-tile NoData handling to NaN across bands, updated existing tests, and added a regression test to ensure empty cells remain transparent across GeoTIFF write/reload.

File summaries

File	Description
test/test_sonar_live_tile_nodata.cpp	New regression tests for multi-band NoData/transparent empty cells across GeoTIFF round trips.
test/test_sonar_live_cache.cpp	Updates existing assertions to the new NaN NoData contract and correct hole-count logic.
src/camp_map/ros/live_coverage/sonar_live_tile.cpp	Normalizes band NoData to NaN and writes NaN as dataset NoData to avoid per-band sentinel lossiness.
src/camp_map/ros/grids/occupancy_grid.h	Declares an `~OccupancyGrid()` to join in-flight render work.
src/camp_map/ros/grids/occupancy_grid.cpp	Joins in-flight renders; adds visibility gate; reduces memory and speeds rendering via Indexed8 palette + scanLine writes.
src/camp_map/ros/grids/grid_map.h	Adds `itemChange()` override to re-render on hidden→shown transitions for latched grid datasets.
src/camp_map/ros/grids/grid_map.cpp	Skips rasterization while hidden and triggers re-render on show.
CMakeLists.txt	Registers the new `test_sonar_live_tile_nodata` gtest target.

Review details

Files reviewed: 8/8 changed files
Comments generated: 3
Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+  process_future_.waitForFinished();
+}


+  if(!isVisible())
+    return;
+
  if(!process_future_.isRunning())
  {


+  if(isVisible())
+    requestRenderLocked();


…fore teardown Copilot's review of PR #210 caught a hazard the visibility gating introduced, and it was right: gridMapCallback and occupancyGridCallback run on the ROS executor thread — grid_map.h says so in as many words ("touched from the ROS callback thread (gridMapCallback), the UI thread ..., and the QtConcurrent worker") — while QGraphicsItem::isVisible() is GUI-thread-owned state. Reading it from the callback was a data race that tests would not catch. Both layers now mirror visibility into a std::atomic<bool> from itemChange(ItemVisibleHasChanged), which runs on the GUI thread, and the ROS callbacks read the atomic. Also from that review: reset the subscription BEFORE waiting on the render future in ~OccupancyGrid, so an executor callback cannot enqueue a fresh render into an object being destroyed. That last point applies to GridMap too, and exposed a pre-existing hole there: ~GridMap set shutdown_ under the mutex and waited on the future it had already captured, but nothing stopped a late callback taking the mutex and reaching requestRenderLocked() — which starts a NEW worker bound to `this` that the captured future does not cover. GridMap now drops its subscription first, and requestRenderLocked() refuses to start work once shutdown_ is set. 293 tests, 0 failures. Part of #208

Copilot

🟡 Changes recommended

OccupancyGrid teardown still has a likely destructor/callback race (potential UAF) and the new gtest target wiring is inconsistent with existing sonar-live tests (likely build/link/include failure).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files reviewed: 8/8 changed files
Comments generated: 4
Review effort level: Lite

+  /// [camp#208] Visibility mirrored from the GUI thread (itemChange) so
+  /// gridMapCallback can test it without touching GUI-owned QGraphicsItem state.
+  std::atomic<bool> visible_{false};
+


+  /// QGraphicsItem::isVisible() is GUI-thread-owned state, and
+  /// occupancyGridCallback runs on the ROS executor thread — reading it there is
+  /// a data race. itemChange() (GUI thread) mirrors it into this atomic, and the
+  /// callback reads the atomic instead.
+  std::atomic<bool> visible_{false};


+  # [camp#208] Multi-band NoData must survive the GeoTIFF round trip, or empty
+  # cells come back drawable and a near-empty tile paints solid.
+  ament_add_gtest(test_sonar_live_tile_nodata
+    test/test_sonar_live_tile_nodata.cpp
+  )
+  target_link_libraries(test_sonar_live_tile_nodata camp_map_ros)
+


+  // can still fire while we wait and enqueue a fresh render into the object being
+  // destroyed.
+  subscription_.reset();
+  process_future_.waitForFinished();
+}


…208, #209) Four must-fixes from the PR #210 review. 1. THE VISIBILITY MIRROR NEVER OPENED — a startup regression introduced by the gating commit. A QGraphicsItem is VISIBLE from construction, and setVisibleHelper() returns before itemChange() when the state is unchanged, so map::Layer::readSettings()'s setVisible(true) fires no event. The mirror stayed false for the whole life of any layer the operator never toggled: every costmap and every S-57 grid layer came up blank, recovering only on an uncheck/recheck. Both constructors now seed visible_ from isVisible(), where parenting is complete and we are on the GUI thread. Worth recording how this nearly shipped: the on-screen check was "toggle an S-57 layer off and on — it repaints", which is consistent WITH the bug rather than evidence against it, since toggling is exactly what sets the mirror. Confirmed by two adversarial lenses, a second Copilot review and a compiled Qt probe. 2. LEGACY CACHED TILES ARE NOW REJECTED ON LOAD. The NaN fix did not heal files already on disk: those were written with per-band sentinels, GeoTIFF kept only one, and the bands that lost the slot have FINITE empty cells that pass every !isfinite || == nodata guard. They would fold into the range and the pyramid as real data, and the next write would stamp a NaN tag over the garbage — a file that looks repaired with pixels still wrong. Which cells were empty is unrecoverable, so a tile whose nodata tag is finite is not loaded; the reconciler re-requests it from the producer. 3. ~OccupancyGrid HAD NO SHUTDOWN GATE. subscription_.reset() alone does not close the window: rclcpp's executor holds its own strong reference across dispatch, so reset() neither cancels nor joins an in-flight callback, which could still touch process_future_ while the destructor waits on it. It now has the shutdown_ handshake GridMap already had. 4. The SonarLiveBand header still documented NoData as the dequantized sentinel — the contract this branch inverted, in the place a future author reads before writing an `== nodata` test that can never match. Corrected, including why the !isfinite clause is the one that matters. 293 tests, 0 failures. Part of #208

Copilot

🟡 Changes recommended

The OccupancyGrid render future lifecycle is still unsafely synchronized between the ROS callback and destructor (data race / potential UAF), which must be fixed before approval.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

src/camp_map/ros/grids/occupancy_grid.cpp:130

process_future_ is accessed concurrently from the ROS executor callback and the destructor with no synchronization. Even with shutdown_ set, a callback already in flight can still race this block and assign a new QtConcurrent::run(...) future while the destructor is waiting, which can allow work to outlive the object (use-after-free) or trigger a data race on process_future_. Consider mirroring GridMap's pattern: guard process_future_ and the launch decision with a mutex, set shutdown_ under that lock in the destructor, copy the pending future, then wait on the copy outside the lock (and keep the subscription_.reset() first).

  if(!process_future_.isRunning())
  {
    process_future_ = QtConcurrent::run(this, &OccupancyGrid::processOccupancyGrid, grid);
  }

CMakeLists.txt:1052

The new gtest target is missing the explicit include dirs / ament deps / Qt/GDAL link libraries that every neighboring SonarLive* test target uses. Relying on transitive propagation here is brittle and makes this test behave differently from the rest of the suite.

  ament_add_gtest(test_sonar_live_tile_nodata
    test/test_sonar_live_tile_nodata.cpp
  )
  target_link_libraries(test_sonar_live_tile_nodata camp_map_ros)

Files reviewed: 10/10 changed files
Comments generated: 1
Review effort level: Lite

+    const std::size_t row_start = std::size_t(row) * grid.info.width;
+    // Source row 0 is the grid's south edge; the image is north-up.
+    uchar* dest = data.grid_image.scanLine(int(grid.info.height - 1 - row));
    for(uint32_t col = 0; col < grid.info.width; col++)
-    {
-      auto value = grid.data[row_start+col];
-      data.grid_image.setPixelColor(QPoint(col, grid.info.height-1-row),
-                                    palette[static_cast<uint8_t>(value + 128)]);
-    }
+      dest[col] = static_cast<uchar>(grid.data[row_start + col] + 128);


Claude Code Agent added 4 commits August 24, 2026 09:50

Copilot AI lite review requested due to automatic review settings August 24, 2026 17:46

Copilot started reviewing on behalf of rolker August 24, 2026 17:47 View session

Copilot AI reviewed Aug 24, 2026

View reviewed changes

rolker mentioned this pull request Aug 24, 2026

Grid layers should be opt-in: discovery auto-subscribes to every topic, and removal does not persist #211

Open

Copilot AI review requested due to automatic review settings August 24, 2026 18:13

Copilot started reviewing on behalf of rolker August 24, 2026 18:14 View session

Copilot AI reviewed Aug 24, 2026

View reviewed changes

Claude Code Agent added 2 commits August 24, 2026 14:29

progress: local review for #209

9c350db

Copilot AI review requested due to automatic review settings August 24, 2026 18:34

Copilot started reviewing on behalf of rolker August 24, 2026 18:34 View session

Copilot AI reviewed Aug 24, 2026

View reviewed changes

rolker merged commit c97153c into jazzy Aug 24, 2026
2 checks passed

rolker deleted the feature/issue-209 branch August 24, 2026 18:49

Navigation Menu

Grid layers: stop crashing on removal, stop rendering hidden layers, and keep empty live-coverage cells transparent - #210

Grid layers: stop crashing on removal, stop rendering hidden layers, and keep empty live-coverage cells transparent#210
rolker merged 7 commits into
jazzyfrom
feature/issue-209

rolker commented Aug 24, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

rolker commented Aug 24, 2026

Summary

1. SIGSEGV when removing a costmap layer (#209)

2. Work done for layers the operator switched off (#208)

3. The costmap image was 4× larger than the data it draws (#208)

4. Live coverage painted empty cells opaque (#208) — the purple tile

Test plan

Not verified

Deliberately out of scope

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (2)

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants