Repository navigation
Fix USGS 3DEP seams by switching to a fixed global Mercator tile grid - #971
Conversation
|
⏱️ Benchmark run finished in 0m 57s 📈 Compared against baseline: 30s 🟢 Generation time is unchanged. 📅 Last benchmark: 2026-04-23 15:27:36 UTC You can retrigger the benchmark by commenting |
There was a problem hiding this comment.
Pull request overview
This PR reworks the USGS 3DEP elevation provider to eliminate visible seam “ramps” by switching from bbox-relative sub-requests to a fixed global Web Mercator tile grid, improving cache stability and reducing seam artifacts across adjacent runs/users.
Changes:
- Adds a new
Usgs3depprovider implementation (usgs_3dep.rs) that fetches fixed 512×512 Mercator tiles and bilinear-samples per output cell. - Removes the legacy USGS 3DEP
tiled_fetch-based implementation fromregional.rs, keepingtiled_fetchfor IGN providers. - Wires the new provider into the provider module exports and selector.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/elevation/selector.rs | Switches USGS 3DEP selection to the new dedicated provider module. |
| src/elevation/providers/usgs_3dep.rs | New fixed-grid Mercator tiling implementation + unit tests. |
| src/elevation/providers/regional.rs | Removes USGS 3DEP tiled implementation; exposes internal fetch/decode helpers to sibling modules. |
| src/elevation/providers/mod.rs | Exports the new usgs_3dep module. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The `tiled_fetch`-based Usgs3dep implementation split each user's bbox into ad-hoc sub-tiles and stitched the responses with a linear blend. Because USGS ImageServer composites multiple LiDAR flights server-side, adjacent user-bbox-relative sub-requests disagree at their shared edges by 10–500 m of vertical offset. The blend turned that step into a 128-row constant-slope ramp, which rendered as a uniform bright band cutting across the Minecraft world (and a vertical equivalent for grids wide enough to need 2+ columns of tiles). Worked previous fix attempts: - Pre-scale scalar bias correction per tile: shifted whole tiles, changed the grid's global min/max, and leaked correction artefacts into the anchor tile via `scale_to_minecraft`'s rescaler. - Post-scale smoothstep over the blend zone: reshaped the ramp but didn't remove it; the material-selection threshold still fired. Root cause being addressed here is architectural, not numerical: the tile boundaries themselves shouldn't depend on the user's bbox. This commit anchors every USGS request to a fixed global Web Mercator tile grid at one of four resolution levels (M1 = 1 m/px, M3 = 3.44 m/px, M10 = 10.3 m/px, M30 = 30.9 m/px), 512 × 512 px per tile. Two users over the same real-world area now hit the same tile keys, cache the same bytes, and see the same data — the way Tellus's `Usgs3depElevationSource` already works, and the way AWS Terrain Tiles and Japan GSI already work in our sibling providers. Key implementation points: - Each output cell computes its mercator position, looks up the enclosing tile, and bilinear-samples within that tile. Cells on either side of a tile boundary read from different tiles independently — at worst a single-block step at the seam, never a 128-row ramp. - Resolution level is selected from the output cell's physical size in meters, covering the whole GUI scale range (0.30 → 2.50) plus any CLI-only values. - Disk cache layout: `<cache>/usgs_3dep/<level_id>/<ty>/<tx>.tiff`. Stable across runs and users; old per-bbox-hash cache entries under the same cache root will simply be orphaned (safe to delete). - Tile fetches run in parallel through a dedicated 4-thread pool so one flaky tile can't stall the rest, while still being gentle on USGS's rate limits. - `tiled_fetch` is retained for IGN France / IGN Spain since they haven't shown the same artefacts in practice; the constants block documents the rationale and opens the door for future migration. Validation: 9 new unit tests (mercator round-trip, tile key stability, bbox containment per resolution, tile span, resolution selection across the GUI scale range, covering-tile enumeration, bilinear sampling on constant + linear + NaN-contaminated tiles). 74/74 tests pass; clippy `-D warnings` clean; fmt clean.
Both IGN providers used the same `tiled_fetch` ad-hoc bbox-splitting path that caused the USGS 3DEP seam artefacts, with the same root cause: their RGE ALTI / MDT products composite multiple source campaigns server-side and adjacent user-bbox-relative sub-requests disagree by tens to hundreds of metres across campaign boundaries. Switching them to the fixed global Mercator grid (already in use by USGS 3DEP after e2c2f3f) produces stable, cacheable tile URLs that match the Tellus reference implementation. The shared covering-tiles, parallel-fetch, bilinear-sample infrastructure moved into a new `fixed_tile` module. Each provider now only supplies its CACHE_NAME, Resolution enum, and per-tile URL template (WMS 1.3.0 for IGN France, WCS 2.0.1 for IGN Spain). The `tiled_fetch` plumbing and the old IGN provider impls are gone from regional.rs, which now only hosts the Japan GSI XYZ provider and the shared HTTP+TIFF helpers `fetch_or_cache` / `decode_geotiff_f32`.
Lets the user wipe the on-disk elevation (`arnis-tile-cache`) and ESA WorldCover (`arnis-landcover-cache`) roots from the settings panel, so disk pressure from repeat generations is recoverable without hand-editing OS cache directories. Implementation: - New `clear_cache_dir` helper in `elevation::cache` recursively removes entries with a CacheClearStats tally; symlinks are removed but never traversed, and every fs error increments a counter rather than propagating. - `land_cover::clear_land_cover_cache` reuses that helper so the two caches share one entry point. - `gui_clear_tile_caches` Tauri command combines both wipes and returns a human-readable status line (or an Err string if any file couldn't be removed, e.g. still locked by a live run). - Frontend button in the Application section uses the golden- outline style matching existing secondary buttons, stays disabled while the call is in flight, and writes the result to an adjacent aria-live span for screen reader announcement.
Two correctness fixes flagged on PR review: 1. `TileKey::for_mercator` could return an out-of-range tile index at the exact world east/south edges (mx == +MERCATOR_LIMIT for lng 180°, my == -MERCATOR_LIMIT for latitudes clamped below the Mercator south limit). `covering_tiles` uses `.ceil() - 1` so the last valid tile is max_tile; `for_mercator`'s raw floor produced max_tile + 1, so the eastmost/southmost sample column/row would miss the cache and stay NaN. Now clamped to `[0, max_tile]`, matching the range `covering_tiles` populates. Regression test added. 2. Cell-size estimation in `fetch_fixed_tile_grid` divided bbox extent by `grid_width` / `grid_height`, but actual sampling spacing is `extent / (grid_width - 1)` (the sampling loop uses `grid_width - 1` as its denominator). Underestimating cell size could push borderline requests to a finer resolution level than necessary and trigger more tile downloads. Now uses the same `(grid_width - 1).max(1)` divisor as the sampling path, so level selection and sampling agree.
Four improvements to the fixed-tile download path, motivated by a production run where 2/80 USGS 3DEP tiles permanently failed with HTTP 502 (a transient ArcGIS gateway blip) and the original 3-attempt / 2.25 s retry budget wasn't enough to ride through it: - MAX_RETRIES 3 → 5. Exponential backoff now sums to roughly 11 s (0.75 + 1.5 + 3 + 6) across attempts, which comfortably covers a 5–15 s ArcGIS recovery window. - ±50% jitter on the backoff delay. Without jitter, the 4 concurrent tile fetches that all failed on the same 502 wave would retry on the same millisecond and re-synchronise pressure on the same flaky endpoint (thundering herd). `rand::rng().random_range(0.5..1.5)` desynchronises retries at zero cost. - `fetch_fixed_tile_grid` builds one blocking `reqwest::Client` up front and passes it through to every tile, replacing the previous per-call `None` which forced `fetch_or_cache` to build (and drop) a fresh client + TLS stack + connection pool per tile. Addresses Copilot PR review. - AWS Terrarium fallback: for any tile that exhausted the primary provider's retry budget, reproject its mercator bbox to lat/lng and synthesise a replacement raster from AWS's global XYZ service. The new `fetch_aws_fallback_tile` returns the same `Vec<Vec<f64>>` shape as a normal TIFF decode so the downstream sampler is unaware of the source. Fallbacks stay in-memory only (not written to the primary provider's on-disk cache path), so the next run gets a fresh attempt at the primary source rather than a stale AWS tile.
Copilot PR review flagged that `DirEntry::metadata()` returns lstat- equivalent info on most platforms but the behaviour is not unambiguous across all edge cases; `entry.file_type()` is documented to *never* follow symlinks on any platform and is the unambiguous choice for the "don't traverse into a link that points outside the cache" guarantee promised in the module's safety doc comment. Also defer the `len()` lookup until after confirming the entry is a regular file, via `symlink_metadata` (not `metadata`) to rule out any chance of resolving a symlink between the type check and the size read. Behaviour is unchanged for real caches — this just tightens the guarantees for the "stray symlink inside the cache" threat model.
GUI changes: - Clear-cache button now sits in the standard right-aligned `.settings-control` slot without a sibling status span, matching the alignment of the adjacent checkbox/slider rows. - Success/error feedback is a brief (1.5 s) green / red background flash on the button itself instead of a growing text label, so the row stays the same size between idle and confirming states and the overall panel doesn't reflow. Full error text still goes to the browser console for debugging. - Tooltip boxes no longer use `white-space: nowrap` (which made long descriptions overflow horizontally into the `.settings-scrollable` container's `overflow-x: hidden` cut). They now wrap onto multiple lines with a comfortable `max-width: 220px` and stay within the scrollable area. Localization: - Adds `clear_tile_cache` / `clear_tile_cache_button` to every locale file under `src/gui/locales/` (ar, de, es, fi, fr-FR, hu, ja, ko, lt, lv, pl, pt-BR, ru, sl, sv, ua, zh-CN) so non-English users don't fall through to the en.json lazy-load path and see English strings for this feature. Addresses Copilot PR review. - Drops the unused `clear_tile_cache_working` key from en.json (replaced by the class-driven button flash).
The prior `::after` pseudo-element tooltip lived inside the hovered `.tooltip-icon`, which itself lived inside `.settings-scrollable`'s overflow-clipped container — so a long description on an icon near the top or the right edge got visibly cut off even after the multi-line wrap fix. Replaced with a single `<div class="global-tooltip">` appended to `document.body` by `initTooltips()`. On hover it's positioned via `getBoundingClientRect` relative to the icon, auto-flips above → below when there isn't room above (top-of-panel case), and clamps horizontally into the viewport so a tooltip near the right edge doesn't overflow into hidden space. Keyboard-reachable via focus / blur, hides on scroll inside the settings panel, and pointer-events: none so it doesn't steal mouse moves between icons.
Three correctness cleanups plus one optimization flagged in the PR: - Remove the \"usgs_3dep: N/M tiles ready\" progress log that spammed the console during every multi-tile fetch — the final per-run summary line still surfaces tile counts and the per-tile failures, so the mid-run ticker added noise without information. Also drops the now-unused AtomicU32 counter. - Rewrite the doc comment for `select_level_for_cell_size`: the old wording said the 1.5× factor \"allows up to a modest 1.5× upsample\" which was the wrong direction. The actual predicate `mpp * 1.5 >= cell_size` tolerates up to 1.5× *downsampling* from the source (source finer than output) before stepping to a coarser level. Upsampling from coarse to fine output is unbounded by the rule — the new comment spells that out with the maths. - Optimise the per-cell sampling loop. `tile_x` is a function of `gx` only and `tile_y` a function of `gy` only, so we precompute `col_mx` and `col_tile_x` once outside the parallel row loop and derive `tile_y` once per row. Inside the row we carry a `Some(&tile)` reference across consecutive cells that share a tile — in practice most cells do, since tile_x only changes every ~TILE_PIXELS output cells — so the HashMap lookup is hit once per tile boundary instead of once per cell. `for_mercator` stays the single source of the edge-clamp logic and is called from both the precompute and the test suite.
Three fixes to `src/elevation/cache.rs` surfaced by PR review: - `clear_cache_dir` now rejects a symlink *root* via symlink_metadata before walking. Previously, pointing the function at a symlink-to- directory would make `read_dir` follow it and wipe the target directory's contents — outside the cache root the caller named. Non-existent roots still return an empty-stats no-op. - `clear_recursive` iterates `read_dir` entries explicitly instead of `.flatten()`. The doc comment promises unreadable entries contribute to the error count; `.flatten()` silently dropped `Err` items, so the GUI under-reported failures. - `cleanup_old_cached_files` / `cleanup_dir_recursive` (the automatic 7-day aging path that runs on every launch) had the same path.is_dir() / path.is_file() symlink-following bugs the explicit clear path just fixed. Now uses `entry.file_type()` (cross-platform symlink-safe), skips symlink entries entirely, and refuses to walk when the root is a symlink. Also uses symlink_metadata for modified- time reads. Tests added covering the missing-dir no-op, the happy-path wipe (files deleted + bytes tallied + root preserved), and a Unix-only regression for the symlink-root rejection (symlinks on Windows require elevated permissions so that case is gated with `#[cfg(unix)]`).
Tooltip was invisible after the body-level migration because the settings modal has `z-index: 20001` but the global tooltip was only at `z-index: 10000`. Even though the tooltip was positioned correctly in the viewport, the modal overlay painted over it. The tooltip is a body-level sibling of the modal (intentionally, so it escapes the `.settings-scrollable` overflow clip), so its stacking order is determined purely by z-index once both are positioned. Bumps to 30000 with a comment pointing at the modal value so the next edit to modal z-index won't silently re-break this.
`fetch_fixed_tile_grid` was running the per-cell bilinear sampling pass inside `pool.install(...)`, where `pool` is the capped-concurrency pool used for polite tile downloads (default 4 threads). That meant a 4k × 4k output grid's CPU-bound sampling ran on at most 4 threads even on 8–16-core machines, leaving parallelism on the table. The download pass still uses the capped pool (upstream providers don't appreciate bursty 16-way concurrency). The sampling pass now uses Rayon's global pool, which defaults to `num_cpus::get()` and scales with the host. Addresses Copilot PR review.
`<span class="tooltip-icon">` has no `tabindex` in the HTML, so keyboard users could never land focus on an icon and the `focus`/`blur` listeners I added for tooltip visibility were dead code. Flagged in PR review. Rather than edit 14 HTML call sites, `initTooltips` now promotes each icon to `tabindex = 0` at bind time, adds a `role="button"` hint for screen readers, and mirrors `data-tooltip` into an `aria-label` so assistive tech reads something meaningful when the icon is focused. An Escape keydown now also closes the tooltip while it's focused, matching the mouse-leave semantics.
6746ca2 to
4fcadfb
Compare
Logs on a 272-tile USGS 3DEP run (louis-e#971 follow-up) showed the 5-attempt ladder taking ~12 s per tile across three tiles before giving up — feels excessive when the AWS Terrarium fallback is ~2 s away and produces perfectly usable output. Trim to 3 attempts total (initial + 2 retries) so we spend at most ~2.25 s plus jitter on a flaky tile before handing it off. The fallback path already handles persistent upstream failures well: visually-verified recovery on the 1/272 case reported from the field shows no seams or holes in the generated world.
The
tiled_fetch-based Usgs3dep implementation split each user's bbox into ad-hoc sub-tiles and stitched the responses with a linear blend. Because USGS ImageServer composites multiple LiDAR flights server-side, adjacent user-bbox-relative sub-requests disagree at their shared edges by 10–500 m of vertical offset. The blend turned that step into a 128-row constant-slope ramp, which rendered as a uniform bright band cutting across the Minecraft world (and a vertical equivalent for grids wide enough to need 2+ columns of tiles).Worked previous fix attempts:
scale_to_minecraft's rescaler.Root cause being addressed here is architectural, not numerical: the tile boundaries themselves shouldn't depend on the user's bbox. This commit anchors every USGS request to a fixed global Web Mercator tile grid at one of four resolution levels (M1 = 1 m/px, M3 = 3.44 m/px, M10 = 10.3 m/px, M30 = 30.9 m/px), 512 × 512 px per tile. Two users over the same real-world area now hit the same tile keys, cache the same bytes, and see the same data — the way Tellus's
Usgs3depElevationSourcealready works, and the way AWS Terrain Tiles and Japan GSI already work in our sibling providers.Key implementation points:
<cache>/usgs_3dep/<level_id>/<ty>/<tx>.tiff. Stable across runs and users; old per-bbox-hash cache entries under the same cache root will simply be orphaned (safe to delete).tiled_fetchis retained for IGN France / IGN Spain since they haven't shown the same artefacts in practice; the constants block documents the rationale and opens the door for future migration.Validation: 9 new unit tests (mercator round-trip, tile key stability, bbox containment per resolution, tile span, resolution selection across the GUI scale range, covering-tile enumeration, bilinear sampling on constant + linear + NaN-contaminated tiles). 74/74 tests pass; clippy
-D warningsclean; fmt clean.