From 8cd6feeb83f002253e522a7104eee24a635b020f Mon Sep 17 00:00:00 2001 From: Roland Arsenault Date: Tue, 21 Apr 2026 21:07:11 -0400 Subject: [PATCH 1/2] fix(s57_layer): avoid unnecessary cache invalidation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two paths invalidated cached tile state more aggressively than needed and contributed to costmap stalls during BizzyBoat field operations (deployment-log analysis: line 3 of survey #2 aborted because the local costmap could not become current within 1.0s after a 97m inter-line transit). - getDatasetsCallback fires every time the costmap moves >5% of its window (every ~17.5m on a 350m local). It used to mark every cached tile incomplete on every callback, even when the returned dataset list was identical to the cached one. Compare label sets and skip the per-tile invalidation when unchanged. - The tide-change block in updateBounds nulled tiles[*].costmap when the offset moved >1cm (every ~30-70s on a real tide). With the pointer cleared, the controller had no costmap to drive against while regen ran. Keep the cached costmap as a fallback (chart_count=0 still forces generateTile to rebuild from grids) and move the safety nullptr into generateTile's empty-charts early-return so it only fires when a tile actually has no chart coverage. Existing tide tests cover the behavioural contract and still pass: TideChangeInvalidatesTiles, NoChartDatumFrameSkipsTideLookup, SmallTideChangeIgnored — 3/3 passing. Mid-cache "preserve costmap pointer" coverage requires test scaffolding for grids_ and is left as future work. Closes #15 Related: rolker/unh_marine_navigation#19 --- Authored-By: Claude Code Agent Model: Claude Opus 4.7 (1M context) Co-Authored-By: Claude Opus 4.7 (1M context) --- s57_layer/src/s57_layer.cpp | 42 ++++++++++++++++++++++++++++++------- 1 file changed, 35 insertions(+), 7 deletions(-) diff --git a/s57_layer/src/s57_layer.cpp b/s57_layer/src/s57_layer.cpp index 5a51f89..451f208 100644 --- a/s57_layer/src/s57_layer.cpp +++ b/s57_layer/src/s57_layer.cpp @@ -139,11 +139,14 @@ void S57Layer::updateBounds(double, double, double, double* min_x, double* min_y { tide_offset_ = new_offset; RCLCPP_INFO_STREAM(logger_, "Tide offset updated: " << tide_offset_ << " m (water above chart datum)"); + // Mark tiles for regeneration but keep the cached costmap as a + // fallback so the controller has a usable costmap during regen. + // generateTile rebuilds the costmap from grids when chart_count=0, + // and the empty-charts early-return clears the pointer for safety. for(auto& t: tiles_) { t.second.complete = false; t.second.chart_count = 0; - t.second.costmap = nullptr; t.second.needs_update = true; } current_ = false; @@ -303,9 +306,10 @@ void S57Layer::generateTile(TileID id) if(current_charts_.empty()) { tiles_[id].complete = true; - // No tile costmap is created. In updateCosts, a nullptr tile - // with complete=true is recognized as uncharted territory and - // handled according to allow_uncharted_. + // Clear any stale costmap from a prior era when this tile had + // chart coverage. updateCosts treats nullptr+complete as uncharted + // and handles it according to allow_uncharted_. + tiles_[id].costmap = nullptr; return; } @@ -523,6 +527,25 @@ void S57Layer::getDatasetsCallback(GetDatasetsClient::SharedFuture future) RCLCPP_DEBUG_STREAM(logger_, "Received " << result->datasets.size() << " datasets from service"); } + // Compare new dataset labels against current_charts_ before overwriting. + // bounds_need_update fires every ~5% of window movement, so this callback + // runs frequently while the boat moves; in steady-state survey areas the + // label set rarely changes. If unchanged, skip the per-tile invalidation + // at the end of this function so the cache survives. + bool chart_list_unchanged = result->datasets.size() == current_charts_.size(); + if(chart_list_unchanged) + { + for(const auto& d: result->datasets) + { + bool found = false; + for(const auto& c: current_charts_) + { + if(c.label == d.label) { found = true; break; } + } + if(!found) { chart_list_unchanged = false; break; } + } + } + current_charts_ = result->datasets; chart_bounds_.clear(); @@ -593,9 +616,14 @@ void S57Layer::getDatasetsCallback(GetDatasetsClient::SharedFuture future) grids_.erase(grid); } - // mark all tiles as needing regeneration - for(auto& t: tiles_) - t.second.complete = false; + // Mark tiles as needing regeneration only when the chart set actually + // changed — unchanged callbacks are common while the boat moves and + // would otherwise force a full re-evaluation of every cached tile. + if(!chart_list_unchanged) + { + for(auto& t: tiles_) + t.second.complete = false; + } } From 358c6ac52a5bafb99acddf7ce7e37c88665f3dab Mon Sep 17 00:00:00 2001 From: Roland Arsenault Date: Tue, 21 Apr 2026 21:42:35 -0400 Subject: [PATCH 2/2] fix(s57_layer): address Copilot review on PR #16 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three follow-ups from Copilot review on the cache-invalidation PR: 1. When the chart set changes in getDatasetsCallback, also reset chart_count=0 (and needs_update=true) on every tile. Without this, a same-count chart swap (one removed + one added) leaves the stale costmap in place because generateTile only rebuilds when grids.size() > tiles_[id].chart_count. 2. In generateTile, after iterating current_charts_, if no chart bounds overlap this tile (grids.empty() && all_charts_available), treat it as uncharted: clear the cached costmap and return — same handling as the empty-current_charts_ early return. Prevents stale chart costs from a prior era being copied into the master grid for a tile that is now uncharted. 3. Replace the O(n²) nested-loop label compare with an unordered_set comparison. unordered_set is already pulled in via s57_layer.h. Existing TideOffsetTest suite (3/3) and full s57_layer test suite (21 tests, 0 failures) still pass. Refs: rolker/s57_tools#15 --- Authored-By: Claude Code Agent Model: Claude Opus 4.7 (1M context) Co-Authored-By: Claude Opus 4.7 (1M context) --- s57_layer/src/s57_layer.cpp | 39 ++++++++++++++++++++++++------------- 1 file changed, 26 insertions(+), 13 deletions(-) diff --git a/s57_layer/src/s57_layer.cpp b/s57_layer/src/s57_layer.cpp index 451f208..fe08693 100644 --- a/s57_layer/src/s57_layer.cpp +++ b/s57_layer/src/s57_layer.cpp @@ -340,6 +340,19 @@ void S57Layer::generateTile(TileID id) else all_charts_available = false; } + + // No chart from current_charts_ overlaps this tile. Treat as uncharted — + // same handling as the empty-current_charts_ early return — and clear + // any costmap left over from a prior era when this tile had coverage. + if(grids.empty() && all_charts_available) + { + tiles_[id].complete = true; + tiles_[id].costmap = nullptr; + tiles_[id].chart_count = 0; + tiles_[id].needs_update = true; + return; + } + tiles_[id].complete = all_charts_available; if(grids.size() > tiles_[id].chart_count) { @@ -532,19 +545,11 @@ void S57Layer::getDatasetsCallback(GetDatasetsClient::SharedFuture future) // runs frequently while the boat moves; in steady-state survey areas the // label set rarely changes. If unchanged, skip the per-tile invalidation // at the end of this function so the cache survives. - bool chart_list_unchanged = result->datasets.size() == current_charts_.size(); - if(chart_list_unchanged) - { - for(const auto& d: result->datasets) - { - bool found = false; - for(const auto& c: current_charts_) - { - if(c.label == d.label) { found = true; break; } - } - if(!found) { chart_list_unchanged = false; break; } - } - } + std::unordered_set new_labels; + for(const auto& d: result->datasets) new_labels.insert(d.label); + std::unordered_set old_labels; + for(const auto& c: current_charts_) old_labels.insert(c.label); + bool chart_list_unchanged = (new_labels == old_labels); current_charts_ = result->datasets; chart_bounds_.clear(); @@ -619,10 +624,18 @@ void S57Layer::getDatasetsCallback(GetDatasetsClient::SharedFuture future) // Mark tiles as needing regeneration only when the chart set actually // changed — unchanged callbacks are common while the boat moves and // would otherwise force a full re-evaluation of every cached tile. + // Also reset chart_count so the rebuild branch in generateTile fires + // unconditionally; otherwise a same-count chart swap (one chart removed, + // one added) would leave stale tile costmaps because grids.size() would + // equal the previous chart_count. if(!chart_list_unchanged) { for(auto& t: tiles_) + { t.second.complete = false; + t.second.chart_count = 0; + t.second.needs_update = true; + } } }