fix(s57_layer): avoid unnecessary cache invalidation#16
Merged
Conversation
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) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR reduces unnecessary S57Layer tile-cache invalidation to improve costmap stability and reduce regeneration churn during frequent dataset callbacks and tide-offset updates.
Changes:
- Skip per-tile
complete=falseinvalidation ingetDatasetsCallbackwhen the dataset label set is unchanged. - Preserve cached per-tile
costmappointers across tide changes (still forcing regeneration viachart_count=0/complete=false), and clear stale pointers only on the empty-charts early return ingenerateTile.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+624
to
+625
| for(auto& t: tiles_) | ||
| t.second.complete = false; |
Comment on lines
305
to
313
| // No chart coverage for this area — handle as uncharted. | ||
| 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; |
Comment on lines
+535
to
+547
| 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; } | ||
| } | ||
| } |
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: #15 --- Authored-By: Claude Code Agent Model: Claude Opus 4.7 (1M context) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Owner
Author
|
Addressed all three Copilot comments in
Existing Authored-By: |
There was a problem hiding this comment.
Pull request overview
This PR reduces unnecessary cache invalidation work in S57Layer to avoid repeated full-tile regenerations during small window movements and to keep a usable cached costmap available during tide-driven regeneration cycles.
Changes:
- Skip marking all cached tiles incomplete in
getDatasetsCallbackwhen the returned dataset label set is unchanged. - On tide offset changes, stop clearing
TileInfo::costmappointers; instead force regeneration viacomplete=falseandchart_count=0. - Clear stale tile costmaps in
generateTilewhen a tile becomes uncharted (either because there are no datasets, or because no dataset overlaps that tile).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Summary
Two changes in
s57_layer/src/s57_layer.cppthat remove cache invalidations the layer was doing without need.getDatasetsCallbackno-op detection. Compare the newresult->datasetslabel set tocurrent_charts_. If unchanged, skip the per-tilecomplete=falseloop at the end of the callback. The callback fires every timebounds_need_updatetriggers a service request — every ~5% of window movement, i.e. every ~17.5 m on the bizzyboat 350 m local. In stable survey areas the chart list rarely changes, so most of these invalidations were wasted work that forced a full re-evaluation of every cached tile.Preserve cached costmap on tide change. Drop
t.second.costmap = nullptrfrom the tide-change block inupdateBounds.chart_count = 0still forcesgenerateTileto rebuild from chart grids on the next pass; preserving the cached costmap means the controller has a usable layer to drive against while regen runs (rather than a transient nullptr window every ~30-70 s as the tide moves). Move the explicitnullptrintogenerateTile's empty-charts early-return so the safety still kicks in when a tile genuinely has no chart coverage.Closes #15. Companion to rolker/unh_marine_navigation#19 (controller-side timeout symptom). Related yaml-only PRs to bump local
chart_layer.update_timeoutfrom 0.15 s to 0.2 s inben_project11andseafloor_echoboat_project11will follow separately.Test plan
colcon build --packages-select s57_layer— clean (no new warnings).colcon test --packages-select s57_layer— 21 tests, 0 errors, 0 failures, 5 skipped.TideOffsetTestsuite (3/3 passing) covers tide-change →current_=false→ regen →current_=truecontract.current_=falsecycles around the 1 cm tide threshold or eachbounds_need_update.Notes / out of scope
grids_and is left as future work.tiles_cache (LRU eviction) and chart-bounds-aware partial invalidation ingetDatasetsCallbackare separate, larger improvements; not in this PR.Authored-By:
Claude Code AgentModel:
Claude Opus 4.7 (1M context)