diff --git a/.agent/work-plans/issue-112/plan.md b/.agent/work-plans/issue-112/plan.md new file mode 100644 index 0000000..34581da --- /dev/null +++ b/.agent/work-plans/issue-112/plan.md @@ -0,0 +1,114 @@ +# Plan: GGGS tile glob matches `_time`/`_source` companion files + +## Issue + +https://github.com/rolker/camp/issues/112 + +## Context + +The bathy and MBES backscatter stores produce three files per tile in the same +directory: `__.tif` (value), `___time.tif` +(Int64 ns timestamp), and `___source.tif` (uint16 source index). +CAMP's `*.tif`/`*.tiff` globs in `loadDirectory()`, `rescan()`, and `dirHasTifs()` +match all three. CAMP tries to render the `_time` and `_source` companions as +depth/intensity tiles, producing garbage renders. + +**Current scan sites (verified against HEAD, post-camp#104):** + +- `src/camp2/raster/gggs_tile_layer.cpp:122` — `loadDirectory()` `entryList` glob +- `src/camp2/raster/gggs_tile_layer.cpp:170` — `rescan()` `entryList` glob +- `src/camp2/raster/gggs_store_source.cpp:23` — `dirHasTifs()` `entryList` glob + +## Approach + +1. **Add `isValueTile(const QString&)` inline free function** in a new shared + header `src/camp2/raster/gggs_tile_util.h` (namespace `camp::raster`) — + returns `true` iff the **filename** (what `QDir::entryList` returns, never a + full path) matches the **anchored, full-match** positive pattern + `^\d+_\d+_\d+\.tiff?$` via `QRegularExpression::anchoredPattern(...)` with a + function-local `static const QRegularExpression` (compiled once). Exactly three + underscore-separated digit groups then `.tif`/`.tiff`. Positive-match is + preferred over a denylist because the companion-suffix set may grow; the + pattern is the canonical value-tile name shared with the stores' own tile- + naming convention, and a 4th `_time`/`_source` component (or any future + companion suffix) is excluded for free by not matching. A one-line comment in + the header explains this so future companion additions know where to look. + +2. **Apply `isValueTile()` in `loadDirectory()`** — filter the entryList result + before constructing `GggsTile` objects (lines 122–147). + +3. **Apply `isValueTile()` in `rescan()`** — filter the entryList result before + the `known.contains()` check (lines 170–185). + +4. **Share `isValueTile()` via the new `gggs_tile_util.h`** (resolved: shared + header, not a duplicated lambda) — included by both `gggs_tile_layer.cpp` and + `gggs_store_source.cpp` so the value-tile definition lives in exactly one + place, per the issue's "one place" requirement. + +5. **Apply `isValueTile()` in `dirHasTifs()`** in `gggs_store_source.cpp` — filter + after the `entryList` call so the helper sees only value tiles when deciding + whether a directory holds a tile-set. + +6. **Extend the existing tests** (resolved: extend, no new test file). In + `test_catalog_source.cpp` add a case that places `0_0_0.tif`, `0_0_0_time.tif`, + and `0_0_0_source.tif` in a store directory and asserts `discover()` returns a + leaf keyed to the directory, plus a negative case that a directory holding ONLY + companions is not a tile-set (pruned). In `test_gggs_rescan.cpp` add a case + that constructs a `GggsTileLayer` over a directory with base + companions + (companions written at a disjoint eastward extent) and asserts `valid()==true` + with `sceneBounds()` equal to a base-only layer's bounds (companions excluded — + only one tile loaded), plus a case that companions landing after load make + `rescan()` no-op (`false`, extent untouched). + +## Files to Change + +| File | Change | +|------|--------| +| `src/camp2/raster/gggs_tile_util.h` | New header: `isValueTile(const QString&)` inline function | +| `src/camp2/raster/gggs_tile_layer.cpp` | Include `gggs_tile_util.h`; filter `entryList` result in `loadDirectory()` and `rescan()` | +| `src/camp2/raster/gggs_store_source.cpp` | Include `gggs_tile_util.h`; filter `entryList` result in `dirHasTifs()` | +| `test/test_catalog_source.cpp` | Add: tile-set dir with base + `_time` + `_source` companions discovers as a leaf; companions-only dir is not a tile-set | +| `test/test_gggs_rescan.cpp` | Add: `GggsTileLayer` over base + companions loads only the base tile; companions landing after load make `rescan()` no-op | + +## Principles Self-Check + +| Principle | Consideration | +|---|---| +| A change includes its consequences | Companion-filter cases added to the existing `test_catalog_source.cpp` and `test_gggs_rescan.cpp` targets (no new target/CMakeLists change). | +| Test what breaks | Tests explicitly cover the bug path: companions in directory → only base tile enumerated. | +| Only what's needed | Narrow 3-site fix + shared helper + extended tests; no scope beyond issue description. | +| Capture decisions, not just implementations | Positive-pattern vs. denylist choice documented via inline comment in `gggs_tile_util.h`. | +| Improve incrementally | Single PR, no architectural change. | + +## ADR Compliance + +| ADR | Triggered | How addressed | +|---|---|---| +| ADR-0001 (Adopt ADRs) | No | Pattern choice (positive vs. denylist) is a code-comment decision, not ADR-worthy. | +| ADR-0002 (Worktree isolation) | Yes | Worktree `issue-camp-112` already in place; all changes land here. | +| ADR-0005 (Catalog browser + flat selectable display layers) | Context | `dirHasTifs()` in `gggs_store_source.cpp` is the post-#104 successor to `GggsStoreLayer`'s subtree scan; the filter applies identically. | +| ADR-0008 (ROS 2 conventions) | Yes | C++ changes in a ROS 2 ament_cmake package; no convention violations. | +| ADR-0013 (progress.md vocabulary) | Yes | `progress.md` has an `## Issue Review` entry; `## Plan Authored` will be appended. | + +## Consequences + +| If we change... | Also update... | Included in plan? | +|---|---|---| +| `dirHasTifs()` in `gggs_store_source.cpp` | `test_catalog_source.cpp` (companion-in-dir case) | Yes — companion + companions-only cases added | +| `loadDirectory()` / `rescan()` glob filter | `test_gggs_rescan.cpp` | Yes — load + rescan companion cases added | +| Add `gggs_tile_util.h` | `CMakeLists.txt` (install headers if needed) | No install target needed — internal header, not exported | + +## Open Questions (resolved) + +- **Header placement** — RESOLVED: shared header `src/camp2/raster/gggs_tile_util.h` + with an inline `isValueTile()`, included by both `gggs_tile_layer.cpp` and + `gggs_store_source.cpp`. The excluded-suffix logic lives in exactly one place, + per the issue's explicit callout. +- **New test file vs. extending existing tests** — RESOLVED: extend the existing + `test_catalog_source.cpp` and `test_gggs_rescan.cpp` targets (their `touchTif`/ + `writeTile` fixtures already cover the relevant classes); no new test file or + CMakeLists change. + +## Estimated Scope + +Single PR. diff --git a/.agent/work-plans/issue-112/progress.md b/.agent/work-plans/issue-112/progress.md new file mode 100644 index 0000000..cd7a388 --- /dev/null +++ b/.agent/work-plans/issue-112/progress.md @@ -0,0 +1,207 @@ +--- +issue: 112 +--- + +# Issue #112 — GGGS tile glob matches `_time`/`_source` companion files + +## Issue Review +**Status**: complete +**When**: 2026-06-21 00:00 +00:00 +**By**: Claude Code Agent (Claude Sonnet) + +**Issue**: #112 +**Comment**: (best-effort post follows this entry; not recorded inline) +**Scope verdict**: well-scoped + +### Summary + +Companion tiles (`_time.tif`, `_source.tif`) produced by the bathy and MBES backscatter stores (unh_marine_autonomy#178 / #194) land in the same directory as the value tile (`.tif`). The GGGS tile-layer and store-source glob `*.tif`/`*.tiff` with no companion-suffix filter, so CAMP treats the companions as renderable value tiles — producing garbage renders. + +**Current scan sites (verified against HEAD, post–camp#104):** +- `src/camp2/raster/gggs_tile_layer.cpp:122` — `loadDirectory()` entryList glob +- `src/camp2/raster/gggs_tile_layer.cpp:170` — `rescan()` entryList glob +- `src/camp2/raster/gggs_store_source.cpp:23–24` — `dirHasTifs()` entryList glob + +The two stale sites from the original issue body (`gggs_store_layer.cpp:21` / `:27`) no longer exist — that file was retired by camp#104. Only the three sites above remain. + +### Principle Alignment + +| Principle | Status | Notes | +|---|---|---| +| A change includes its consequences | Action needed | The fix requires a companion test (companion files in a store dir; assert only base tiles listed/rendered). Extensive GGGS test coverage already exists (`test_gggs_rescan.cpp`, `test_gggs_flat_layer_spawn.cpp`, `test_gggs_render.cpp`, etc.) — the new test follows the same pattern. | +| Test what breaks | Action needed | No test currently exercises `_time`/`_source` companions; one must be added. | +| Only what's needed | OK | Targeted 3-site fix + shared helper; no scope creep. | +| Improve incrementally | OK | Narrow fix, single PR, no cross-cutting redesign. | +| Capture decisions, not just implementations | Watch | The positive-pattern (`\d+_\d+_\d+\.tif`) vs. denylist (`_time`/`_source` suffix exclusion) choice should be noted inline so future companion additions know which approach was taken and why. | +| Workspace vs. project separation | OK | All changes are in the `camp` project repo; no workspace infra touched. | + +### ADR Applicability + +| ADR | Triggered | Notes | +|---|---|---| +| ADR-0001 (Adopt ADRs) | No | Design choice (positive pattern vs. denylist) is implementor's call; a code comment suffices — no new ADR needed. | +| ADR-0002 (Worktree isolation) | Yes | Worktree already in place for issue-112. | +| ADR-0005 (Stores browser / flat display layers) | Yes (context) | This fix is consistent with ADR-0005: `dirHasTifs()` and `buildNode()` in `GggsStoreSource` are the successors to `GggsStoreLayer`'s subtree scan; the companion filter applies here too. | +| ADR-0008 (ROS 2 conventions) | Yes | C++ change in a ROS 2 package — follow standard ROS 2/ament_cmake C++ conventions. | +| ADR-0013 (progress.md vocabulary) | Yes | This entry. | + +### Consequences + +- A shared `isValueTile(QString)` helper (or positive-pattern lambda) must be placed in a single location and called at all three scan sites — the issue explicitly flags this so future companion bands don't require multi-site edits. +- One new test file (or additions to `test_gggs_flat_layer_spawn.cpp` / `test_gggs_rescan.cpp`) must exercise the companion-filter path. +- No interface changes, no `.msg`/`.srv` changes, no doc updates needed beyond inline comments. + +### Actions +- [ ] Implement `isValueTile()` helper (or positive-pattern filter) at a single definition site. +- [ ] Apply the filter at all three current scan sites: `loadDirectory()`, `rescan()`, `dirHasTifs()`. +- [ ] Add a test: construct a store dir with base + `_time` + `_source` companions; assert only the base tile is listed/loaded/rendered. +- [ ] Document the positive-vs.-denylist choice in an inline comment so future companion bands know which approach to extend. + +## Plan Authored +**Status**: complete +**When**: 2026-06-21 12:00 +00:00 +**By**: Claude Code Agent (Claude Sonnet) + +**Plan**: `.agent/work-plans/issue-112/plan.md` at `1251ab7` +**Branch**: feature/issue-112 at `1251ab7` +**Phases**: single + +### Open questions +- [ ] Header placement: new `gggs_tile_util.h` (shared, one definition site) vs. static lambda duplicated in each `.cpp` — recommendation is shared header per the issue's "one place" requirement. +- [ ] New test file (`test_gggs_companion_filter.cpp`) vs. adding cases to existing `test_gggs_rescan.cpp` and `test_catalog_source.cpp` — recommendation is add to existing tests where fixture already covers the relevant class. + +## Plan Review +**Status**: complete +**When**: 2026-06-21 16:04 +00:00 +**By**: Claude Code Agent (Claude Opus) + +**Plan**: `.agent/work-plans/issue-112/plan.md` at `1251ab7` +**PR**: PR-less (`--issue` mode; `gh` unauthenticated — issue substance cross-checked against the `## Issue Review` entry above) +**Verdict**: approve-with-suggestions + +Independent review (fresh-context Opus dispatch; the `## Plan Authored` entry is a +separate Sonnet dispatch under the shared `Claude Code Agent` name — not author +self-review). Core approach validated against HEAD: three scan sites confirmed +(`gggs_tile_layer.cpp:122`, `:170`; `gggs_store_source.cpp:23`); positive pattern +`\d+_\d+_\d+\.tiff?` matches every legitimate value tile (all tiles/tests use +`__.tif`) and excludes `_time`/`_source` companions without +dropping real tiles. Consequences map and test obligation are complete. All +findings below are suggestions; none block implementation. + +### Findings +- [ ] (suggestion) ADR-0005 mislabeled "Layered enforcement" (workspace title); camp ADR-0005 is "Catalog browser + flat selectable display layers" — fix title, substance is correct — `plan.md:85` +- [ ] (suggestion) Spurious "ADR-0006-D7" citation — camp has no ADR-0006 (only 0001–0005); drop or correct, the "companion set may grow" rationale stands alone — `plan.md:28` +- [ ] (suggestion) Specify anchored/full regex match (`anchoredPattern`/`^…$`) for `isValueTile()` so partial matches can't admit malformed names — `plan.md:27` +- [ ] (suggestion) Resolve test-structure inconsistency: "Files to Change" adds new `test_gggs_companion_filter.cpp` while Approach/Open-Questions recommend extending existing `test_catalog_source.cpp`/`test_gggs_rescan.cpp` (which already have reusable fixtures) — pick one; extending is recommended — `plan.md:56,66,104` + +## Implementation +**Status**: complete +**When**: 2026-06-21 17:30 +00:00 +**By**: Claude Opus + +**Branch**: feature/issue-112 at `4e46fb1` +**Commits**: +- `6f4070d` fix(gggs): filter `_time`/`_source` companion tiles from value-tile scans — helper + 3 sites + plan sync +- `4e46fb1` test(gggs): cover companion-tile filtering — extend existing targets + +### What was built + +**Shared helper** — new `src/camp2/raster/gggs_tile_util.h` with an inline +`camp::raster::isValueTile(const QString& filename)`. It matches the filename +(never a full path) against an anchored, full-match positive pattern +`^\d+_\d+_\d+\.tiff?$` via `QRegularExpression::anchoredPattern(...)` with a +function-local `static const QRegularExpression` (compiled once). Exactly three +underscore-separated digit groups then `.tif`/`.tiff` → the base value tile; +companions (a 4th `_time`/`_source` component) and malformed names fail to +match. A header comment records the positive-pattern decision and that future +companion suffixes are excluded for free. + +**Three patched scan sites** (each now skips non-value tiles), both `.cpp`s +include `gggs_tile_util.h`: +- `gggs_tile_layer.cpp` `loadDirectory()` — `if(!isValueTile(name)) continue;` + before constructing the `GggsTile`. +- `gggs_tile_layer.cpp` `rescan()` — same skip before the `known.contains()` + check, so a rescan never adds a companion as a new tile. +- `gggs_store_source.cpp` `dirHasTifs()` — iterate the `*.tif` entryList and + return true only if at least one entry `isValueTile()`, so a directory of + companions alone is not counted as a tile-set. + +**Tests** (extended existing targets — no new file, no CMakeLists change): +- `test_catalog_source.cpp` → `CompanionTilesDoNotBreakDiscovery`: a tile-set + dir with `0_0_0.tif` + `0_0_0_time.tif` + `0_0_0_source.tif` discovers as a + leaf keyed to the dir; a sibling dir holding ONLY companions is pruned (not a + tile-set). +- `test_gggs_rescan.cpp` → `CompanionTilesAreNotLoaded`: a `GggsTileLayer` over + base + companions (companions written at a disjoint *eastward* extent, so they + would widen `sceneBounds()` if loaded) has `valid()==true` and `sceneBounds()` + equal to a base-only layer's — exactly one tile loaded. +- `test_gggs_rescan.cpp` → `RescanIgnoresCompanionTiles`: companions landing + after the initial load make `rescan()` return `false` with the extent + untouched. +- No tile-count accessor was added; the existing public `sceneBounds()`/`valid()` + surface (with disjoint-extent companions) distinguishes one-tile from + three-tile loads, per the plan. + +### Doc fixes (Plan Review findings, applied to `plan.md`) +- ADR-0005 retitled "Catalog browser + flat selectable display layers". +- Spurious "ADR-0006-D7" citation removed (camp has no ADR-0006). +- `isValueTile()` spec updated to anchored/full-match + filename-only. +- Both Open Questions resolved (shared header; extend existing tests); Files-to- + Change / Consequences tables updated to drop the new-test-file + CMakeLists rows. + +### Build / test +Camp deps were absent (empty `core_ws/install`): built `marine_ais_msgs`, +`marine_interfaces`, `marine_autonomy` via `colcon build --packages-up-to` in +`core_ws` first (warnings only). Then: +- Build: `./ui_ws/build.sh camp` → 1 package finished (pre-existing warnings only). +- Test: `./ui_ws/test.sh camp` → **99 tests, 0 errors, 0 failures, 2 skipped**. +The three new cases were confirmed run and passing (direct binary runs). + +Not pushed. + +## Local Review (Pre-Push) +**Status**: complete +**When**: 2026-06-21 16:29 +00:00 +**By**: Claude Code Agent (Claude Opus) +**Verdict**: approved + +**Branch**: feature/issue-112 at `37981cb` +**Mode**: pre-push +**Depth**: Standard (reason: 5 reviewable code files / ~149 lines; raster-render path) +**Must-fix**: 0 | **Suggestions**: 2 +**Round**: 1 | **Ship**: recommended — no must-fix; only optional case-sensitivity/test-coverage suggestions + +### Findings +- [x] (suggestion) Case mismatch: `*.tif`/`*.tiff` glob may admit `.TIF`/`.TIFF` but regex `\.tiff?` is case-sensitive → uppercase-ext value tile silently dropped; theoretical (producer emits lowercase), optional `CaseInsensitiveOption` — `src/camp2/raster/gggs_tile_util.h:24` +- [x] (suggestion) No test for a 4-group name (`13_0_0_0.tif`); correctly rejected by the anchored regex but unexercised — `src/camp2/raster/gggs_tile_util.h:24` + +### Notes +- Two fresh-context Claude adversarial passes (Lens A logic / Lens B systemic) both verdict the fix correct: regex accepts all legitimate `__.tif` value tiles and rejects all companions; the three patched sites (`loadDirectory`, `rescan`, `dirHasTifs`) are the complete `*.tif` scan set (no missed site); `GggsTile` is constructed only at the two filtered layer sites; the async worker/ctor need no guard (companions never enter `tiles_`). The case-sensitivity suggestion was cross-pass confirmed. +- Static analysis: no actionable findings. `ament_cpplint` copyright/header-guard/C-cast hits dropped — camp does not enforce ament_cpplint (no CI/pre-commit/CMake hook), cast hits are on untouched lines, and the new header matches every sibling's guard style and no-copyright convention. +- Plan adherence: exact — files and approach match `plan.md` as written; no drift/scope creep. +- Doc-citation nit (not code): earlier lifecycle entries cite ADR-0008/ADR-0013, which are workspace ADRs; camp's own ADR set is 0001–0005. + +## Implementation +**Status**: complete +**When**: 2026-06-21 16:52 +00:00 +**By**: Claude Code Agent (Claude Opus) + +**Branch**: feature/issue-112 at `fce9fd8` +**Addressed**: `## Local Review (Pre-Push)` (When 2026-06-21 16:29 +00:00, branch `feature/issue-112` at `37981cb`) — both unchecked suggestions +**Commits**: `c5b907a`, `fce9fd8` + +### Actions +- [x] (suggestion) Make `isValueTile()` extension match case-insensitive so an upper/mixed-case-extension value tile that passes the `*.tif`/`*.tiff` glob isn't silently dropped — added `QRegularExpression::CaseInsensitiveOption` + a comment recording the glob-alignment rationale — `src/camp2/raster/gggs_tile_util.h:21` +- [x] (suggestion) Add a test exercising a 4-group name (`13_0_0_0.tif`) rejection — new `GggsTileUtilTest.IsValueTileNameGrammar` direct unit test in `test_gggs_tile.cpp` pins the predicate: accepts 3-group `.tif`/`.tiff` (incl. upper-case ext), rejects `_time`/`_source` companions, 4-group names, too-few-groups, non-digit groups, wrong/extra extension, and leading/trailing junk — `test/test_gggs_tile.cpp:259` + +### Build / test +- `test_gggs_tile.cpp` already builds with `${CMAKE_CURRENT_SOURCE_DIR}/src/camp2` on its include path and links `Qt5::Core`, so the header-only `isValueTile()` test needed **no CMakeLists change**. +- Built core_ws deps first (`marine_ais_msgs`, `marine_interfaces`, `marine_autonomy` via `colcon build --packages-up-to`), then `./ui_ws/build.sh camp` → 1 package finished (pre-existing warnings only). +- `./ui_ws/test.sh camp` → **100 tests, 0 errors, 0 failures, 2 skipped** (was 99; the new case is the +1). The new case was confirmed run+passing via a direct `--gtest_filter='GggsTileUtilTest.*'` run. + +Not pushed. + +### Next step +Lifecycle: **Implementation** → **review-code** (re-review the fixes). Hand off to a fresh-context sub-agent: + + .agent/scripts/dispatch_subagent.sh --mode in-process --issue 112 --skill review-code diff --git a/src/camp2/raster/gggs_store_source.cpp b/src/camp2/raster/gggs_store_source.cpp index b7c92e4..c3d3a5f 100644 --- a/src/camp2/raster/gggs_store_source.cpp +++ b/src/camp2/raster/gggs_store_source.cpp @@ -1,6 +1,7 @@ #include "gggs_store_source.h" #include "gggs_tile_layer.h" +#include "gggs_tile_util.h" #include "../catalog/catalog_item.h" #include "../map/layer_list.h" @@ -16,12 +17,19 @@ namespace raster namespace { -// A directory holds a tile-set if it contains GeoTIFF tiles directly. Salvaged -// from the retired GggsStoreLayer's anonymous-namespace helper. +// A directory holds a tile-set if it contains a base value GeoTIFF tile +// directly. Salvaged from the retired GggsStoreLayer's anonymous-namespace +// helper. [camp#112] The `*.tif` glob also matches `_time`/`_source` companion +// tiles, so a directory of companions alone is NOT a tile-set — filter to base +// value tiles (isValueTile) before deciding. bool dirHasTifs(const QString& path) { - return !QDir(path).entryList(QStringList() << "*.tif" << "*.tiff", - QDir::Files).isEmpty(); + const QStringList tifs = QDir(path).entryList( + QStringList() << "*.tif" << "*.tiff", QDir::Files); + for(const QString& name : tifs) + if(isValueTile(name)) + return true; + return false; } // Build a CatalogItem subtree for `path`: a leaf if it holds tiles directly, a diff --git a/src/camp2/raster/gggs_tile_layer.cpp b/src/camp2/raster/gggs_tile_layer.cpp index 41bcf78..9a2050d 100644 --- a/src/camp2/raster/gggs_tile_layer.cpp +++ b/src/camp2/raster/gggs_tile_layer.cpp @@ -1,6 +1,7 @@ #include "gggs_tile_layer.h" #include "gggs_tile.h" +#include "gggs_tile_util.h" #include "../map_view/web_mercator.h" #include @@ -124,6 +125,10 @@ void GggsTileLayer::loadDirectory(const QString& directory) bool first_extent = true; // first geometrically-valid tile (scene_bounds_) for(const QString& name : files) { + // [camp#112] Skip companion tiles (`_time`/`_source`): the `*.tif` glob also + // matches them, but only the base value tile is renderable. + if(!isValueTile(name)) + continue; // [camp#102] Extent/metadata only — the GggsTile ctor no longer reads pixels. // boundingRect()/sceneBounds() are valid immediately (fit-to-extent works at // load time); the band reads (and therefore the data range) are deferred to @@ -172,6 +177,10 @@ bool GggsTileLayer::rescan() std::vector> new_tiles; for(const QString& name : files) { + // [camp#112] Skip companion tiles (`_time`/`_source`) before the known check + // so a rescan never adds them as renderable tiles. + if(!isValueTile(name)) + continue; const QString path = dir.filePath(name); if(known.contains(path)) continue; diff --git a/src/camp2/raster/gggs_tile_util.h b/src/camp2/raster/gggs_tile_util.h new file mode 100644 index 0000000..f8687e0 --- /dev/null +++ b/src/camp2/raster/gggs_tile_util.h @@ -0,0 +1,38 @@ +#ifndef RASTER_GGGS_TILE_UTIL_H +#define RASTER_GGGS_TILE_UTIL_H + +#include +#include + +namespace camp +{ +namespace raster +{ + +/// [camp#112] True iff @p filename is a base value tile `__.tif` +/// (or `.tiff`). The positive, anchored full-match pattern is the canonical +/// value-tile name shared with the stores' own tile-naming convention: exactly +/// three underscore-separated digit groups then `.tif`/`.tiff`. Companion tiles +/// (`___time.tif` Int64, `___source.tif` +/// uint16) carry a 4th `_time`/`_source` component and so fail the match — and +/// any future companion suffix is excluded for free by not matching, without a +/// denylist to maintain. Match @p filename only (what QDir::entryList returns), +/// never a full path. +/// +/// [camp#112] Case-insensitive: the scan-site globs (`*.tif`/`*.tiff`) can admit +/// an upper/mixed-case extension (`.TIF`/`.TIFF`), so the pattern matches case- +/// insensitively too — otherwise such a value tile would pass the glob but be +/// silently dropped here. (Producers emit lowercase today; this keeps the filter +/// from diverging from the globs if that ever changes.) +inline bool isValueTile(const QString& filename) +{ + static const QRegularExpression re( + QRegularExpression::anchoredPattern(QStringLiteral("\\d+_\\d+_\\d+\\.tiff?")), + QRegularExpression::CaseInsensitiveOption); + return re.match(filename).hasMatch(); +} + +} // namespace raster +} // namespace camp + +#endif diff --git a/test/test_catalog_source.cpp b/test/test_catalog_source.cpp index 1c07ce6..cb4cd33 100644 --- a/test/test_catalog_source.cpp +++ b/test/test_catalog_source.cpp @@ -106,6 +106,43 @@ TEST(CatalogSourceTest, RootThatIsATileSetIsALeaf) EXPECT_EQ(tree->key(), store.path()); } +// [camp#112] A tile-set directory that also holds `_time`/`_source` companion +// tiles still discovers as a leaf: the companions don't break discovery, and a +// dir whose only `*.tif` entries are companions is NOT a tile-set. +TEST(CatalogSourceTest, CompanionTilesDoNotBreakDiscovery) +{ + QTemporaryDir store; + ASSERT_TRUE(store.isValid()); + const QString root = store.path(); + + // A real tile-set: base value tile plus its Int64 `_time` and uint16 `_source` + // companions, all in the same directory (the bathy/MBES store layout). + const QString tileset = root + "/bathymetry/tileset"; + touchTif(tileset, "0_0_0.tif"); + touchTif(tileset, "0_0_0_time.tif"); + touchTif(tileset, "0_0_0_source.tif"); + + // A directory holding ONLY companions (no base value tile) is not a tile-set. + const QString companions_only = root + "/orphan_companions"; + touchTif(companions_only, "0_0_0_time.tif"); + touchTif(companions_only, "0_0_0_source.tif"); + + GggsStoreSource source; + std::unique_ptr tree = source.discover(root); + ASSERT_NE(tree, nullptr); + + // Only the real tile-set's modality survives; the companions-only branch prunes. + EXPECT_EQ(childNamed(tree.get(), "orphan_companions"), nullptr) + << "a dir of only companion tiles must not count as a tile-set"; + + const CatalogItem* bathy = childNamed(tree.get(), "bathymetry"); + ASSERT_NE(bathy, nullptr); + const CatalogItem* leaf = childNamed(bathy, "tileset"); + ASSERT_NE(leaf, nullptr); + EXPECT_TRUE(leaf->isLeaf()); + EXPECT_EQ(leaf->key(), tileset); +} + // A root with no tiles anywhere (and a non-existent path) yields no catalog. TEST(CatalogSourceTest, EmptyOrMissingRootYieldsNothing) { diff --git a/test/test_gggs_rescan.cpp b/test/test_gggs_rescan.cpp index 7c583a7..64b41b2 100644 --- a/test/test_gggs_rescan.cpp +++ b/test/test_gggs_rescan.cpp @@ -109,6 +109,62 @@ TEST(GggsRescanTest, PicksUpNewlyLandedTile) EXPECT_FALSE(layer->rescan()); // now nothing new } +// [camp#112] loadDirectory() filters companion tiles (`_time`/`_source`): a tile +// directory holding the base value tile plus its companions enumerates EXACTLY +// the base tile. The companions are written at a disjoint (eastward) extent, so +// if they were (wrongly) loaded as tiles the layer's sceneBounds would widen — +// the layer instead matches a base-only layer's extent exactly. +TEST(GggsRescanTest, CompanionTilesAreNotLoaded) +{ + QTemporaryDir base_only; + ASSERT_TRUE(base_only.isValid()); + ASSERT_FALSE(writeTile(base_only, "13_0_0.tif", -71.40, 43.00).isEmpty()); + + camp::map::Map base_map; + auto* base_layer = + new camp::raster::GggsTileLayer(base_map.topLevelLayers(), base_only.path()); + ASSERT_TRUE(base_layer->valid()); + const QRectF base_bounds = base_layer->sceneBounds(); + + QTemporaryDir with_companions; + ASSERT_TRUE(with_companions.isValid()); + ASSERT_FALSE(writeTile(with_companions, "13_0_0.tif", -71.40, 43.00).isEmpty()); + // Companions at a disjoint eastward extent: a real GeoTIFF, excluded by NAME. + ASSERT_FALSE(writeTile(with_companions, "13_0_0_time.tif", -71.39, 43.00).isEmpty()); + ASSERT_FALSE(writeTile(with_companions, "13_0_0_source.tif", -71.38, 43.00).isEmpty()); + + camp::map::Map map; + auto* layer = + new camp::raster::GggsTileLayer(map.topLevelLayers(), with_companions.path()); + ASSERT_TRUE(layer->valid()); + // Only the base tile counted: same extent as the base-only layer (companions' + // eastward extents did not widen it). + EXPECT_EQ(layer->sceneBounds(), base_bounds) + << "companion tiles must not be loaded as renderable tiles"; +} + +// [camp#112] rescan() filters companion tiles too: companions landing after the +// initial load are not treated as new renderable tiles, so rescan() no-ops +// (returns false) and leaves the extent untouched. +TEST(GggsRescanTest, RescanIgnoresCompanionTiles) +{ + QTemporaryDir dir; + ASSERT_TRUE(dir.isValid()); + ASSERT_FALSE(writeTile(dir, "13_0_0.tif", -71.40, 43.00).isEmpty()); + + camp::map::Map map; + auto* layer = new camp::raster::GggsTileLayer(map.topLevelLayers(), dir.path()); + ASSERT_TRUE(layer->valid()); + const QRectF bounds_before = layer->sceneBounds(); + + // Companions for the existing tile land afterward (disjoint eastward extents). + ASSERT_FALSE(writeTile(dir, "13_0_0_time.tif", -71.39, 43.00).isEmpty()); + ASSERT_FALSE(writeTile(dir, "13_0_0_source.tif", -71.38, 43.00).isEmpty()); + + EXPECT_FALSE(layer->rescan()); // companions are not new tiles + EXPECT_EQ(layer->sceneBounds(), bounds_before); // extent untouched +} + int main(int argc, char** argv) { qputenv("QT_QPA_PLATFORM", "offscreen"); diff --git a/test/test_gggs_tile.cpp b/test/test_gggs_tile.cpp index 5942ad0..4378098 100644 --- a/test/test_gggs_tile.cpp +++ b/test/test_gggs_tile.cpp @@ -22,9 +22,11 @@ #include #include "raster/gggs_tile.h" +#include "raster/gggs_tile_util.h" #include "map_view/web_mercator.h" using camp::raster::GggsTile; +using camp::raster::isValueTile; namespace { @@ -256,6 +258,40 @@ TEST(GggsTileTest, GeoToMapMatchesWebMercator) } } +// [camp#112] isValueTile() is the single companion-filter predicate applied at +// every `*.tif` scan site. Pin its name grammar directly (the layer/discovery +// tests cover it behaviorally; this exercises the boundary cases cheaply): +// - exactly three underscore-separated digit groups + .tif/.tiff are accepted; +// - a 4-group name (a `_time`/`_source` companion, or an extra component like +// `13_0_0_0.tif`) is rejected by the anchored full match; +// - the extension match is case-insensitive (mirrors the `*.tif` glob, which +// can admit `.TIF`/`.TIFF`). +TEST(GggsTileUtilTest, IsValueTileNameGrammar) +{ + // Base value tiles: three digit groups, .tif or .tiff. + EXPECT_TRUE(isValueTile("0_0_0.tif")); + EXPECT_TRUE(isValueTile("13_10_20.tif")); + EXPECT_TRUE(isValueTile("13_10_20.tiff")); + + // Companions carry a 4th component -> rejected. + EXPECT_FALSE(isValueTile("13_10_20_time.tif")); + EXPECT_FALSE(isValueTile("13_10_20_source.tif")); + + // A 4-group all-digit name is also rejected by the anchored 3-group match. + EXPECT_FALSE(isValueTile("13_0_0_0.tif")); + + // Malformed / non-value names are rejected (anchored full match). + EXPECT_FALSE(isValueTile("13_0.tif")); // too few groups + EXPECT_FALSE(isValueTile("a_0_0.tif")); // non-digit group + EXPECT_FALSE(isValueTile("13_0_0.png")); // wrong extension + EXPECT_FALSE(isValueTile("x13_0_0.tif")); // leading junk (anchored) + EXPECT_FALSE(isValueTile("13_0_0.tif.bak")); // trailing junk (anchored) + + // Extension match is case-insensitive (the glob admits upper/mixed case). + EXPECT_TRUE(isValueTile("13_0_0.TIF")); + EXPECT_TRUE(isValueTile("13_0_0.Tiff")); +} + int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv);