Skip to content

Terrain: beauty/topo mode split, contour overlay, real grid LOD + mobile auto-LOD, pan restored#98

Open
AdaWorldAPI wants to merge 4 commits into
mainfrom
claude/geo-contour-lines
Open

Terrain: beauty/topo mode split, contour overlay, real grid LOD + mobile auto-LOD, pan restored#98
AdaWorldAPI wants to merge 4 commits into
mainfrom
claude/geo-contour-lines

Conversation

@AdaWorldAPI

@AdaWorldAPI AdaWorldAPI commented Jul 9, 2026

Copy link
Copy Markdown
Owner

Builds on #97. Four commits: the OTM contour overlay, the beauty-vs-topo mode split, and a UX/architecture round (pan, real LOD, mobile, sidebar). All from data we already bake — no new downloads.

1. Contour-line overlay + lake-vs-river fix (daac63d0)

  • Contours are Kind::Line features of GeoKind::Contour, so the overlay reuses the DRP1 pipeline end-to-end: garmin_bake --contour-level N (default 3 ≈ 8k lines) emits a .contour.soa sidecar; new /api/garmin-contours/:location route (mirror of the drape route, + test); client decodes with the same decodeDrape into a thin semi-transparent LineSegments.
  • Lake-vs-river: the type code discriminates (0x4c = flowing river-fill = the Colorado; 0x41/46/47 = still lakes). New tested terrain::river_fill_grid() widens only the river ×2 — lakes stay pinpoints.

2. Beauty vs topo mode split (e7618a07)

"The contours are a map layer, not the skin of the world."

  • Beauty (default): contours OFF. Still-water tanks/ponds retagged in the --arid bake to a 10th palette slot (LAKE_SUBTLE) — a barely-there grey fleck below the shader's blue-dominance threshold, so no vivid water treatment fires (1,081 cells). Blue = flowing water only.
  • Topo (the topo toggle): contour lines + a uTopo shader swap to cartographic paper (beige-white relief, green vegetation tint, carto-blue water, gentle hillshade). The vivid grade never shows under the line web. Iceland untouched.
  • LOD honesty + stats HUD: verified postLod() early-returns on every geo scene (the /api/body/lod cascade culls by the anatomy body's block-bounds) — the LOD toggle was inert on terrain. New HUD (from renderer.info ground truth): tris · lines · calls · verts · LOD status.

3. UX/architecture round (474da00a)

  • Sidebar hidden on terrain scenes — a grid's "structures" are row strips (1,024–3,584 identical unnamed entries); the map gets the full viewport. Anatomy/building scenes keep the list.
  • Pan restored: right-drag + two-finger drag pan; two-finger spread = pinch dolly; context menu suppressed. Root cause of dead two-finger pan on mobile: the canvas never set touch-action: none, so the browser claimed multi-touch gestures before pointer events fired. pointercancel clears gesture state. Single-pointer orbit math unchanged (body scene unaffected).
  • Real terrain LOD: decodeGrid(buf, stride) sub-samples the ver-8 radix grid at decode time — cells are addresses, so stride 2 is exact selection (¼ verts, ¼ tris, ¼ decode work), never resampling. The wire is kept in a ref; the LOD button re-decodes live without refetch.
  • Mobile auto-LOD: coarse-pointer + touch devices start at ½-res — a phone never pays the full-grid decode (the reported ~35 s load on the 16.5M-vert Iceland grid; decode + GPU share drops ~4×).

Verification (headless, pixel-diffed + HUD-scraped)

  • HUD proves the LOD toggle: tris 1.73M → 431k · verts 864k → 216k · "off · full grid" → "on · ½-res grid".
  • Mobile emulation (390×844, coarse + touch): boots straight into LOD on · ½-res grid, zero clicks.
  • Right-drag pan shifts the view (pixel diff 22.4); synthesized two-finger pointer events pan (20.2); canvas.style.touchAction === 'none'.
  • Both modes rendered: beauty default (no contours, subtle lakes, bold Colorado) and topo (beige paper + contour lines; HUD lines 265k → 745k · calls 3 → 4).
  • geo lib 25/25 tests (river-fill split, dilation, arid palette, histogram golden); tsc + vite build clean.
  • Caveat: cockpit-server could not be compiled in the dev container (the rusty_v8 static-lib host is blocked by egress policy and the cache was empty) — the contour route is a line-for-line mirror of the deployed drape route; CI builds it.

Known issue (accepted for now, follow-up filed in the plan doc)

  • LOD toggle resets the camera: the re-decode remounts the scene, resetting the orbit to the default aerial view. Follow-up: persist orbit state across remount.

Next round (post-merge, recorded in claude-notes/plans/2026-07-09-otm-contour-lines.md)

  • Overlay color model: derive feature/contour strokes multiplicatively from local terrain shading (ochre on sunlit sandstone, umber in shadow, near-black in the depths) instead of neutral-grey linework — overlays embedded in the rock, not painted over it. Beauty mode gets the full hierarchy (terrain ≫ Colorado ≫ tributaries ≫ roads ≫ drainage); topo mode keeps classic map lines.

🤖 Generated with Claude Code

https://claude.ai/code/session_012jEwwaT5JZ5x8qWvcnaMYC


Generated by Claude Code

Summary by CodeRabbit

  • New Features

    • Added a TOPO/OTM display mode with cartographic terrain styling and contour-line overlays for supported Garmin terrain scenes.
    • Added automatic contour loading and a terrain level-of-detail toggle.
    • Added rendering statistics for triangles, lines, calls, and active LOD status.
    • Added two-finger panning and pinch-to-zoom gestures for touch devices.
    • Improved water styling to distinguish rivers from lakes.
  • Documentation

    • Added setup and verification guidance for contour overlays and terrain baking.

claude added 4 commits July 9, 2026 21:11
…nyon)

Push the /garmin canyon toward OpenTopoMap quality — from data we already
bake, no new download.

## Contour-line overlay (the big OTM-look step)
Contours are Kind::Line features of GeoKind::Contour, so the overlay reuses the
whole DRP1 pipeline:
- garmin_bake: `--contour-level N` (default 3 ≈ 8k lines; level 4's 50k is too
  heavy for a browser overlay) emits `<stem>.contour.soa` via
  `drape::build_drape(&[Contour])` + `encode_drape` with a darker topo-brown
  palette (CONTOUR_LINE = [128,94,60]).
- cockpit-server: `/api/garmin-contours/:location` + `resolve_garmin_contours`
  + test — a line-for-line mirror of the drape route (independent optional map).
- GeoHelix.tsx: fetch contours (non-blocking) → `decodeDrape` → a thin
  semi-transparent LineSegments riding just under the road/river drape; a live
  `contours` toggle. Reuses the drape decoder + lazy-build-in-tick pattern.
- body.manifest.json: `garmin_contours` entry for grand-canyon.

## Lake-vs-river fix
The ×2 river widening (#97) also fattened the still lakes into chunky blobs.
Probe found the discriminator is the Garmin type code, not shape: 0x4c is the
flowing river-fill (the Colorado); 0x41/0x46/0x47 are still lakes. New
`terrain::river_fill_grid()` (RIVER_FILL_TYPE = 0x4c, tested) stamps only the
river; the bake dilates that mask ×2 and leaves lakes at their 1-cell stamp, so
the Colorado stays bold while lakes shrink to OTM-style pinpoints.

## Verify
- geo-hhtl lib: 25/25 tests (river_fill split + dilation). cockpit: tsc + build
  clean. Headless /garmin/grand-canyon: contour lines etch the relief like a
  topo map; both toggles work (features/contours); lakes are pinpoints.
- cockpit-server could NOT be compiled locally: the rusty_v8 static-lib download
  host is blocked by org egress policy (403) and the cache is empty — CI will
  build it. The route change is a mechanical mirror of the deployed drape route.
- Contours are a garmin_bake feature (the canyon); Iceland's raster DEM has no
  vector contours (deferred).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012jEwwaT5JZ5x8qWvcnaMYC
"The contours are a map layer, not the skin of the world." Separate the two
looks instead of drawing topo lines over the vivid surfel grade:

- BEAUTY (default): contours OFF by default. Still-water tanks/ponds on the
  arid canyon are no longer bright blue lakes — the --arid bake retags them to
  a new 10th palette slot (LAKE_TAG=9, LAKE_SUBTLE=[122,130,134]), a
  barely-there grey fleck deliberately below the shader's blue-dominance `wet`
  threshold so no vivid water treatment fires (1081 cells). Blue is reserved
  for flowing water: the Colorado + perennial 0x4c tributary reaches. The
  ver-8 wire carries its palette count (nK u8) and the client indexes
  palette[kind] from the wire, so the extra slot is decode-safe.
- TOPO (the toggle, renamed `topo`): contour lines + a uTopo shader swap to
  cartographic paper — pale beige-white relief ramp, green vegetation-KIND
  tint, light carto-blue water, gentle hillshade. Classic OTM styling; the
  beauty grade never shows under the line web. uTopo is tied to actual
  contour-layer visibility in the tick, so the palette never swaps without
  lines. Iceland (no contour sidecar) is untouched.

LOD honesty + stats HUD:
- Verified postLod() early-returns for every geo scene: the /api/body/lod
  cascade culls by the ANATOMY body's block-bounds, so on /garmin/* the LOD
  toggle was an inert placeholder. The button is now disabled there
  ("LOD n/a") with an explanatory tooltip; a per-scene .blocks cascade stays
  future work.
- New stats HUD (bottom-right, ≤2 Hz, renderer.info ground truth): rendered
  tris / line segments / draw calls / decoded verts / LOD status. Observed:
  beauty = `tris 1.73M · lines 265k · calls 3`; topo on = `lines 745k ·
  calls 4` — layer toggles provably do real work.

Rebaked canyon.v8grid{,.drape,.contour}.soa.gz (the drape/contour DRP1
palettes gained the 10th slot). geo 25/25 tests; tsc + vite build clean;
headless-verified both modes + HUD + disabled LOD button.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012jEwwaT5JZ5x8qWvcnaMYC
…ile auto-LOD, sidebar

Four user-reported issues on the /garmin terrain scenes:

1. SIDEBAR: hidden on terrain scenes — a grid's "structures" are its row
   strips (1,024–3,584 identical unnamed entries), noise for a map. Anatomy /
   building scenes keep the searchable list.
2. PAN: right-drag (desktop) and two-finger drag (touch) now pan the orbit
   target along the camera basis; two-finger span change = pinch dolly;
   context menu suppressed. ROOT CAUSE of "two-finger did nothing" on mobile:
   the canvas had no `touch-action: none`, so the browser claimed multi-touch
   gestures before pointer events ever fired. pointercancel clears gesture
   state so browser-stolen touches can't wedge the pointer map. Single-pointer
   orbit math is unchanged (body scene unaffected).
3. REAL TERRAIN LOD: decodeGrid(buf, stride) sub-samples the ver-8 radix grid
   at decode time — grid cells are addresses, so stride 2 is exact selection
   (¼ verts/tris AND ¼ decode work), never resampling. The inflated wire is
   kept in a ref, so the LOD button re-decodes live without refetch. Grid
   scenes get a live toggle; mesh geo scenes stay honestly "LOD n/a"; the
   body's server cascade is untouched.
4. MOBILE AUTO-LOD: coarse-pointer + touch devices start with LOD on — a
   phone never pays the full-grid decode (the reported 35 s on the 16.5M-vert
   Iceland grid; ½-res is ~¼ the decode work and ¼ the GPU load).

Headless-verified (log in the plan doc): sidebar gone; HUD proves the LOD
toggle (tris 1.73M → 431k · verts 864k → 216k · "off · full grid" → "on ·
½-res grid"); right-drag pan shifts the view (pixel diff 22.4); synthesized
two-finger pointer events pan (diff 20.2); canvas touch-action none; mobile
emulation (390×844, coarse+touch) auto-starts at ½-res with zero clicks.
Known minor: toggling LOD re-decodes + remounts, which resets the camera.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012jEwwaT5JZ5x8qWvcnaMYC
… color model)

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012jEwwaT5JZ5x8qWvcnaMYC
@cursor

cursor Bot commented Jul 9, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_83fc71be-b8e9-4c28-b403-58875cfc39ff)

@coderabbitai

coderabbitai Bot commented Jul 9, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds Garmin contour baking and serving, river/lake classification, client-side ver-8 grid LOD, TOPO rendering, contour overlays, touch gestures, and renderer statistics for terrain scenes.

Changes

Garmin contour terrain flow

Layer / File(s) Summary
Contour baking and water classification
claude-notes/plans/..., geo/src/garmin/terrain.rs, geo/src/bin/garmin_bake.rs
Garmin baking adds configurable contour levels and contour DRP1 sidecars, while river polygons receive separate handling from still-water cells and palette entries.
Contour asset registration and API
cockpit/public/body.manifest.json, crates/cockpit-server/src/main.rs
The Grand Canyon contour asset is registered and served through an optional validated Garmin contour endpoint, with resolver tests.
Grid LOD and TOPO rendering
cockpit/src/GeoHelix.tsx
Ver-8 grids support stride-based decoding and cached LOD changes; GeoHelix fetches and toggles contour lines, applies TOPO shading, restores touch gestures, and displays renderer statistics.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant TerrainScene
  participant GarminContourAPI
  participant GeoHelix
  participant ThreeRenderer
  TerrainScene->>GarminContourAPI: Request contour overlay
  GarminContourAPI-->>GeoHelix: Return DRP1 gzip data
  GeoHelix->>GeoHelix: Decode overlay and select grid stride
  GeoHelix->>ThreeRenderer: Render terrain, contours, TOPO palette, and HUD
Loading

Possibly related PRs

  • AdaWorldAPI/q2#92: Adds the GeoHelix terrain renderer that this change extends with grid LOD, TOPO mode, and contour overlays.

Poem

I’m a rabbit with contour lines,
Hopping over peaks and pines.
Rivers flow, lakes softly glow,
TOPO paints the paper below.
LOD hops lightly, gestures soar—
New terrain trails to explore! 🐇

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main changes: beauty/topo split, contour overlays, grid LOD, mobile auto-LOD, and restored panning.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (1)
geo/src/bin/garmin_bake.rs (1)

258-298: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Contour sidecar duplicates the drape sidecar's build→encode→log shape.

The new contour block (277-298) repeats the same build_drapeencode_drape → stats eprintln! pattern as the drape block just above (261-269), differing only in level/kinds/palette. Worth extracting a small helper (e.g. fn emit_overlay(name, dec, bbox, pos, w, h, level, kinds, palette) -> Vec<u8>) before a third overlay type repeats the copy-paste.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@geo/src/bin/garmin_bake.rs` around lines 258 - 298, The drape and contour
sidecars duplicate their build, encode, statistics, and logging logic. Extract a
reusable helper near the relevant code, such as emit_overlay, accepting the
overlay name, decoder, bounds, terrain positions, dimensions, level, kinds, and
palette, returning encoded bytes; move the shared build_drape, encode_drape,
point counting, and eprintln behavior into it, then replace both blocks with
helper calls while preserving their existing parameters and log labels.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@cockpit/src/GeoHelix.tsx`:
- Around line 151-165: Stride-based dimensions in decodeGrid do not ensure the
final full-grid row and column are sampled. Update the W/H calculation and
colF/rowF index generation so stride LOD always includes Wf - 1 and Hf - 1,
while retaining regular stride sampling and avoiding duplicate indices.

In `@geo/src/bin/garmin_bake.rs`:
- Around line 231-237: Replace the debug-only palette length check in the
palette construction logic with a runtime assertion that remains enabled in
release builds, preserving the invariant that LAKE_TAG is the next unused
palette slot before appending LAKE_SUBTLE. Keep the assertion tied to the
existing LAKE_TAG and palette symbols so any palette growth fails immediately
during baking instead of silently aliasing a real slot.

---

Nitpick comments:
In `@geo/src/bin/garmin_bake.rs`:
- Around line 258-298: The drape and contour sidecars duplicate their build,
encode, statistics, and logging logic. Extract a reusable helper near the
relevant code, such as emit_overlay, accepting the overlay name, decoder,
bounds, terrain positions, dimensions, level, kinds, and palette, returning
encoded bytes; move the shared build_drape, encode_drape, point counting, and
eprintln behavior into it, then replace both blocks with helper calls while
preserving their existing parameters and log labels.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 5562e997-404e-463e-a444-06ac3d871ddc

📥 Commits

Reviewing files that changed from the base of the PR and between 845e4a5 and b53e2a9.

⛔ Files ignored due to path filters (3)
  • cockpit/public/canyon.v8grid.contour.soa.gz is excluded by !**/*.gz
  • cockpit/public/canyon.v8grid.drape.soa.gz is excluded by !**/*.gz
  • cockpit/public/canyon.v8grid.soa.gz is excluded by !**/*.gz
📒 Files selected for processing (6)
  • claude-notes/plans/2026-07-09-otm-contour-lines.md
  • cockpit/public/body.manifest.json
  • cockpit/src/GeoHelix.tsx
  • crates/cockpit-server/src/main.rs
  • geo/src/bin/garmin_bake.rs
  • geo/src/garmin/terrain.rs

Comment thread cockpit/src/GeoHelix.tsx
Comment on lines +151 to +165
function decodeGrid(buf: ArrayBuffer, stride = 1): Decoded {
const dv = new DataView(buf);
const nC = dv.getUint32(6, true), W = dv.getUint32(10, true), H = dv.getUint32(14, true);
const nV = W * H, nT = (W - 1) * (H - 1) * 2;
const nC = dv.getUint32(6, true), Wf = dv.getUint32(10, true), Hf = dv.getUint32(14, true);
// Terrain LOD: sub-sample the radix grid by `stride` at DECODE time. Grid cells are
// ADDRESSES, so skipping is exact selection (never resampling) — stride 2 keeps every
// other height sample untouched and yields ¼ the verts/tris AND ¼ the decode work
// (the mobile fast path; the LOD toggle re-decodes live). Edge row/col clamp to the
// last full-grid sample so the tile rim survives.
const W = stride > 1 ? Math.ceil(Wf / stride) : Wf;
const H = stride > 1 ? Math.ceil(Hf / stride) : Hf;
const colF = new Uint32Array(W), rowF = new Uint32Array(H);
for (let c = 0; c < W; c++) colF[c] = Math.min(c * stride, Wf - 1);
for (let r = 0; r < H; r++) rowF[r] = Math.min(r * stride, Hf - 1);
const nVf = Wf * Hf; // wire counts (offsets/slices)
const nV = W * H, nT = (W - 1) * (H - 1) * 2; // display counts

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Stride LOD drops the tile's rightmost column/bottommost row (rim not actually preserved).

The comment states the edge clamp keeps the tile rim, but Math.ceil(Wf/stride)/Math.ceil(Hf/stride) don't guarantee (W-1)*stride (or (H-1)*stride) reaches Wf-1/Hf-1. E.g. Wf=1024, stride=2W=512, last sampled column is index 1022, not 1023 — the true rim column is never read. This only triggers when stride > 1 (LOD active), i.e. the new mobile auto-LOD / manual LOD-toggle path this PR introduces.

🐛 Proposed fix — guarantee the last full-grid sample is always reachable
-  const W = stride > 1 ? Math.ceil(Wf / stride) : Wf;
-  const H = stride > 1 ? Math.ceil(Hf / stride) : Hf;
+  const W = stride > 1 ? Math.ceil((Wf - 1) / stride) + 1 : Wf;
+  const H = stride > 1 ? Math.ceil((Hf - 1) / stride) + 1 : Hf;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
function decodeGrid(buf: ArrayBuffer, stride = 1): Decoded {
const dv = new DataView(buf);
const nC = dv.getUint32(6, true), W = dv.getUint32(10, true), H = dv.getUint32(14, true);
const nV = W * H, nT = (W - 1) * (H - 1) * 2;
const nC = dv.getUint32(6, true), Wf = dv.getUint32(10, true), Hf = dv.getUint32(14, true);
// Terrain LOD: sub-sample the radix grid by `stride` at DECODE time. Grid cells are
// ADDRESSES, so skipping is exact selection (never resampling) — stride 2 keeps every
// other height sample untouched and yields ¼ the verts/tris AND ¼ the decode work
// (the mobile fast path; the LOD toggle re-decodes live). Edge row/col clamp to the
// last full-grid sample so the tile rim survives.
const W = stride > 1 ? Math.ceil(Wf / stride) : Wf;
const H = stride > 1 ? Math.ceil(Hf / stride) : Hf;
const colF = new Uint32Array(W), rowF = new Uint32Array(H);
for (let c = 0; c < W; c++) colF[c] = Math.min(c * stride, Wf - 1);
for (let r = 0; r < H; r++) rowF[r] = Math.min(r * stride, Hf - 1);
const nVf = Wf * Hf; // wire counts (offsets/slices)
const nV = W * H, nT = (W - 1) * (H - 1) * 2; // display counts
function decodeGrid(buf: ArrayBuffer, stride = 1): Decoded {
const dv = new DataView(buf);
const nC = dv.getUint32(6, true), Wf = dv.getUint32(10, true), Hf = dv.getUint32(14, true);
// Terrain LOD: sub-sample the radix grid by `stride` at DECODE time. Grid cells are
// ADDRESSES, so skipping is exact selection (never resampling) — stride 2 keeps every
// other height sample untouched and yields ¼ the verts/tris AND ¼ the decode work
// (the mobile fast path; the LOD toggle re-decodes live). Edge row/col clamp to the
// last full-grid sample so the tile rim survives.
const W = stride > 1 ? Math.ceil((Wf - 1) / stride) + 1 : Wf;
const H = stride > 1 ? Math.ceil((Hf - 1) / stride) + 1 : Hf;
const colF = new Uint32Array(W), rowF = new Uint32Array(H);
for (let c = 0; c < W; c++) colF[c] = Math.min(c * stride, Wf - 1);
for (let r = 0; r < H; r++) rowF[r] = Math.min(r * stride, Hf - 1);
const nVf = Wf * Hf; // wire counts (offsets/slices)
const nV = W * H, nT = (W - 1) * (H - 1) * 2; // display counts
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cockpit/src/GeoHelix.tsx` around lines 151 - 165, Stride-based dimensions in
decodeGrid do not ensure the final full-grid row and column are sampled. Update
the W/H calculation and colF/rowF index generation so stride LOD always includes
Wf - 1 and Hf - 1, while retaining regular stride sampling and avoiding
duplicate indices.

Comment on lines +231 to +237
// Slot 9 = LAKE_TAG. The ver-8 wire carries its palette count (nK u8) and the
// client indexes palette[kind] from the wire, so a 10th entry is decode-safe.
debug_assert_eq!(palette.len() as u8, LAKE_TAG);
palette.push(LAKE_SUBTLE);
eprintln!(
"arid palette: Stream drainage → rust-brown, Water river-blue, still lakes → subtle"
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Palette invariant guarded by debug_assert_eq! is stripped in release builds.

If GeoKind::PALETTE ever grows beyond 9 entries, LAKE_TAG (hardcoded 9) would silently alias a real palette slot in a release bake — corrupting the shipped .soa/palette without any runtime signal, since debug_assert_eq! compiles to nothing in release.

🛡️ Proposed fix — use a runtime assertion that survives release builds
-        debug_assert_eq!(palette.len() as u8, LAKE_TAG);
+        assert_eq!(
+            palette.len() as u8,
+            LAKE_TAG,
+            "GeoKind::PALETTE length drifted from LAKE_TAG — update the slot constant"
+        );
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Slot 9 = LAKE_TAG. The ver-8 wire carries its palette count (nK u8) and the
// client indexes palette[kind] from the wire, so a 10th entry is decode-safe.
debug_assert_eq!(palette.len() as u8, LAKE_TAG);
palette.push(LAKE_SUBTLE);
eprintln!(
"arid palette: Stream drainage → rust-brown, Water river-blue, still lakes → subtle"
);
// Slot 9 = LAKE_TAG. The ver-8 wire carries its palette count (nK u8) and the
// client indexes palette[kind] from the wire, so a 10th entry is decode-safe.
assert_eq!(
palette.len() as u8,
LAKE_TAG,
"GeoKind::PALETTE length drifted from LAKE_TAG — update the slot constant"
);
palette.push(LAKE_SUBTLE);
eprintln!(
"arid palette: Stream drainage → rust-brown, Water river-blue, still lakes → subtle"
);
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@geo/src/bin/garmin_bake.rs` around lines 231 - 237, Replace the debug-only
palette length check in the palette construction logic with a runtime assertion
that remains enabled in release builds, preserving the invariant that LAKE_TAG
is the next unused palette slot before appending LAKE_SUBTLE. Keep the assertion
tied to the existing LAKE_TAG and palette symbols so any palette growth fails
immediately during baking instead of silently aliasing a real slot.

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