Skip to content

Fix USGS 3DEP seams by switching to a fixed global Mercator tile grid - #971

Merged
louis-e merged 13 commits into
mainfrom
fix/usgs-fixed-tile-grid
Apr 23, 2026
Merged

louis-e merged 13 commits into
mainfrom
fix/usgs-fixed-tile-grid

Conversation

@louis-e

@louis-e louis-e commented Apr 23, 2026

Copy link
Copy Markdown
Owner

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.

Copilot AI review requested due to automatic review settings April 23, 2026 15:25
@github-actions

Copy link
Copy Markdown

⏱️ Benchmark run finished in 0m 57s
🏗️ Generation time: 27s (excl. data fetching)
🧠 Peak memory usage: 1831 MB

📈 Compared against baseline: 30s
🧮 Delta: 27s
🔢 Commit: 2cecd24

🟢 Generation time is unchanged.

📅 Last benchmark: 2026-04-23 15:27:36 UTC

You can retrigger the benchmark by commenting retrigger-benchmark.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Usgs3dep provider 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 from regional.rs, keeping tiled_fetch for 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.

Comment thread src/elevation/providers/usgs_3dep.rs Outdated
Comment thread src/elevation/providers/usgs_3dep.rs Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/elevation/cache.rs Outdated
Comment thread src/elevation/providers/fixed_tile.rs Outdated
Comment thread src/gui/locales/en-US.json Outdated
Comment thread src/gui.rs
Comment thread src/elevation/providers/regional.rs

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/elevation/cache.rs Outdated
Comment thread src/elevation/cache.rs Outdated
Comment thread src/elevation/providers/fixed_tile.rs Outdated
Comment thread src/elevation/providers/fixed_tile.rs Outdated

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread src/gui/js/main.js
Comment thread src/gui/js/main.js
Comment thread src/elevation/providers/fixed_tile.rs Outdated
louis-e added 13 commits April 23, 2026 21:02
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.
@louis-e
louis-e force-pushed the fix/usgs-fixed-tile-grid branch from 6746ca2 to 4fcadfb Compare April 23, 2026 19:03
@louis-e
louis-e merged commit c26ff83 into main Apr 23, 2026
2 checks passed
@louis-e
louis-e deleted the fix/usgs-fixed-tile-grid branch April 23, 2026 19:05
pull Bot pushed a commit to Mu-L/arnis that referenced this pull request Apr 23, 2026
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants