cockpit: /ice real Iceland DEM terrain + geo "terrain" layer chrome#89
Merged
Conversation
Replace the SPARSE Iceland scatter with a REAL DEM heightfield so /ice renders solid terrain under the already-shipped height-profile palette + Kurvenlineal breathing. DEM source: AWS Terrarium terrain-RGB tiles (public, keyless — SRTM stops at 60°N and OpenTopography needs a key, both fail Iceland at 63-67°N). zoom 9 (252 tiles, x220..237 / y126..139), decoded via elev_m = R*256 + G + B/256 - 32768, stitched (4608x3584 px) and 4x block-mean downsampled to a 1152x896 vertex grid (fetched by scripts/fetch_iceland_dem.py). Mesh: 1,032,192 verts · 2,060,290 tris, shared-vertex heightfield (two tris/cell, per-vertex normals from the terrain gradient). Equirectangular projection about the grid centre (matches the OSM bake); normalized to [-1,1] by horizontal extent, elevation TRUE-SCALE (span 0..0.0067, the shader applies uExag=10 + Kurvenlineal — no pre-exaggeration). Sea level (elev <= 0, ~62% of verts) -> y=0, painted as ocean by the shader. Concepts = grid rows (896, contiguous ranges) so the per-concept max-diameter clamp stays a thin full-width strip and drops 0 triangles. Reuses the geo V3 classid (0x0F01_1000) + HHTL tiling the OSM path uses. Format: baked through bso2::encode_mesh_bso2 — a new additive generic-mesh encoder mirroring encode_bso2's exact BSO2 ver-6 layout (F16 pos + Signed360 normals + HXFL trailer). The OSM encode_bso2 path is untouched (all 12 geo tests still pass, incl. the Signed360 round-trip). New baker binary geo/src/bin/iceland_dem.rs (required-features = ["helix"]). The sparse bake cockpit/public/iceland.helix.soa.gz is PRESERVED on disk (unreferenced); only the manifest key iceland_latest is repointed to the new cockpit/public/iceland_dem.helix.soa.gz (9.6 MB gzip). BodyHelix.tsx and main.tsx are untouched. Verified end-to-end: npm run build clean; a Playwright CDP frame-grab of /ice (preview server) shows a SOLID Iceland landmass — recognizable island outline, green coasts -> brown highlands -> white glacier caps, rising out of the flat blue ocean under the sky dome. Header reads "1,032,192 verts · 896 structures". Decode was independently replicated (BodyHelix's decode + clamp) confirming X∈[-1,1], Z∈[-0.78,0.78], unit up-facing normals, and FrontSide winding. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012jEwwaT5JZ5x8qWvcnaMYC
Fixes the reported bug that the Iceland map is categorized as skeleton / body-parts. BodyHelix is shared by /helix (anatomy) and the geo scenes (/geo, /ice), and the layer sidebar + toggle buttons rendered the hardcoded anatomy LAYERS (skin/muscle/organ/skeleton/vessel/nervous/connective/other) regardless of scene — so the terrain concepts (stamped one layer) showed up under "skeleton" with the full anatomy button row. Geo scenes now derive `activeLayers` from the layers the bake actually stamps, relabelled "terrain" (empty anatomy layers dropped). The layer *id* is preserved, so the show/hide toggle still filters the real geometry — only the label + the empty rows change. Anatomy scenes keep `LAYERS` verbatim (byte-identical). Title/subtitle were already scene-gated; this finishes the job for the sidebar + buttons. Verified: `npm run build` (tsc + vite) clean; a pure label swap over the already-render-confirmed DEM scene (geometry/shader untouched). A fresh swiftshader frame of the panel wasn't obtainable — the 1M-vert breathing scene is too heavy for the headless GPU to snapshot — so the label is verified by build + code, not a new screenshot. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012jEwwaT5JZ5x8qWvcnaMYC
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThis PR replaces the sparse Iceland OSM scatter with a DEM-based terrain mesh. It adds a Python tile-fetch script producing a ChangesIceland DEM Terrain Pipeline
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant FetchScript as fetch_iceland_dem.py
participant TerrariumAPI as AWS Terrarium Tiles
participant IcelandDemBin as iceland_dem binary
participant Bso2Encoder as encode_mesh_bso2
participant Cockpit as BodyHelix / manifest
FetchScript->>TerrariumAPI: fetch elevation tiles
TerrariumAPI-->>FetchScript: PNG RGB tiles
FetchScript->>FetchScript: stitch, downsample, write .demgrid
IcelandDemBin->>FetchScript: read .demgrid
IcelandDemBin->>IcelandDemBin: project, normalize, compute mesh + normals + concepts
IcelandDemBin->>Bso2Encoder: encode_mesh_bso2(...)
Bso2Encoder-->>IcelandDemBin: soa + blocks bytes
IcelandDemBin->>Cockpit: write iceland_dem.helix.soa.gz asset
Cockpit->>Cockpit: filter LAYERS by conceptList, render terrain UI
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
geo/src/bso2.rs (1)
429-448: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo bounds validation on
MeshConcept.v_start/v_count— can panic on malformed input.
pos[c.v_start as usize..end.min(nv)](Line 435) clamps the slice end tonv, but not the start. If a caller ever passesc.v_start > nv(orv_start > v_start + v_count), this slice panics with a "start > end" range error instead of a controlled error. The currenticeland_demcaller happens to always produce in-bounds ranges, but this is a generalized public API (doc comment explicitly targets "arbitrary" meshes), so it's worth guarding against misuse by future callers.🛡️ Proposed guard
assert_eq!(nrm.len(), nv, "nrm len must match pos"); assert_eq!(rows.len(), nv, "rows len must match pos"); + for c in concepts { + assert!( + (c.v_start as usize) <= nv && (c.v_start + c.v_count) as usize <= nv, + "concept vertex range out of bounds" + ); + }🤖 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/bso2.rs` around lines 429 - 448, The block that computes the `.blocks` sidecar in `bso2.rs` assumes `MeshConcept.v_start` and `v_count` always define a valid slice, but `pos[c.v_start as usize..end.min(nv)]` can still panic when the start is out of bounds or past the clamped end. Update the loop in this mesh export path to validate or clamp both bounds before slicing, using the existing `concepts`/`pos` data and the `MeshConcept` fields, so malformed or future arbitrary inputs fail safely instead of triggering a range panic.scripts/fetch_iceland_dem.py (1)
85-89: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
int()casts aroundmath.floor().
math.floor()already returns anintin Python 3; wrapping it inint(...)is a no-op (flagged by Ruff RUF046).♻️ Proposed cleanup
- x0 = int(math.floor(lon_to_tilex(LON_W, zoom))) - x1 = int(math.floor(lon_to_tilex(LON_E, zoom))) + x0 = math.floor(lon_to_tilex(LON_W, zoom)) + x1 = math.floor(lon_to_tilex(LON_E, zoom)) # tile-y grows south, so LAT_N (north) -> smaller y. - y0 = int(math.floor(lat_to_tiley(LAT_N, zoom))) - y1 = int(math.floor(lat_to_tiley(LAT_S, zoom))) + y0 = math.floor(lat_to_tiley(LAT_N, zoom)) + y1 = math.floor(lat_to_tiley(LAT_S, zoom))🤖 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 `@scripts/fetch_iceland_dem.py` around lines 85 - 89, The tile index calculations in fetch_iceland_dem.py use redundant int() wrappers around math.floor(), which is a no-op in Python 3. Update the x0, x1, y0, and y1 assignments in the tile-bounds setup to use math.floor() directly, keeping the existing lon_to_tilex() and lat_to_tiley() calls intact and preserving the current southward y-axis comment.Source: Linters/SAST tools
🤖 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 `@geo/src/bin/iceland_dem.rs`:
- Around line 40-51: The header validation in read_demgrid is too short and
allows truncated files to reach the f64_at(24) read path. Update the initial
length check in read_demgrid so it պահանջs enough bytes for the full fixed
header before any u32_at/f64_at access, ensuring files shorter than the needed
header size return the existing error instead of panicking.
In `@scripts/fetch_iceland_dem.py`:
- Around line 96-113: The DEM stitching logic in the tile fetch loop is leaving
failed tiles as zero-valued elevation in `stitched`, which makes missing data
look like valid sea-level terrain. Update the `ThreadPoolExecutor`/`fetch_tile`
processing to track a missing-tile mask or sentinel for each tile, and avoid
writing implicit zeros into `stitched` for absent tiles. Then either fail the
run when gaps remain, or explicitly fill them via interpolation/neighbor-based
recovery before emitting the raster, so downstream `iceland_dem` does not treat
synthetic flat patches as real data.
---
Nitpick comments:
In `@geo/src/bso2.rs`:
- Around line 429-448: The block that computes the `.blocks` sidecar in
`bso2.rs` assumes `MeshConcept.v_start` and `v_count` always define a valid
slice, but `pos[c.v_start as usize..end.min(nv)]` can still panic when the start
is out of bounds or past the clamped end. Update the loop in this mesh export
path to validate or clamp both bounds before slicing, using the existing
`concepts`/`pos` data and the `MeshConcept` fields, so malformed or future
arbitrary inputs fail safely instead of triggering a range panic.
In `@scripts/fetch_iceland_dem.py`:
- Around line 85-89: The tile index calculations in fetch_iceland_dem.py use
redundant int() wrappers around math.floor(), which is a no-op in Python 3.
Update the x0, x1, y0, and y1 assignments in the tile-bounds setup to use
math.floor() directly, keeping the existing lon_to_tilex() and lat_to_tiley()
calls intact and preserving the current southward y-axis comment.
🪄 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: 12a8910c-6675-4e07-8557-6cb6899a0ab0
⛔ Files ignored due to path filters (1)
cockpit/public/iceland_dem.helix.soa.gzis excluded by!**/*.gz
📒 Files selected for processing (6)
cockpit/public/body.manifest.jsoncockpit/src/BodyHelix.tsxgeo/Cargo.tomlgeo/src/bin/iceland_dem.rsgeo/src/bso2.rsscripts/fetch_iceland_dem.py
Comment on lines
+40
to
+51
| fn read_demgrid(path: &str) -> Result<Dem, Box<dyn std::error::Error>> { | ||
| let b = std::fs::read(path)?; | ||
| if b.len() < 24 || &b[0..4] != b"DEMG" { | ||
| return Err("not a DEMG file".into()); | ||
| } | ||
| let u32_at = |o: usize| u32::from_le_bytes(b[o..o + 4].try_into().unwrap()); | ||
| let f64_at = |o: usize| f64::from_le_bytes(b[o..o + 8].try_into().unwrap()); | ||
| let ver = u32_at(4); | ||
| if ver != 1 { | ||
| return Err(format!("unsupported DEMG version {ver}").into()); | ||
| } | ||
| let w = u32_at(8) as usize; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Header length check is insufficient — reads past the validated bound.
b.len() < 24 only guarantees 24 bytes, but the header actually needs 32 bytes before east is read at Line 54 (f64_at(24) reads b[24..32]). A file truncated to between 24 and 32 bytes will panic on an out-of-range slice instead of hitting the intended "not a DEMG file" error path.
🐛 Proposed fix
- if b.len() < 24 || &b[0..4] != b"DEMG" {
+ if b.len() < 32 || &b[0..4] != b"DEMG" {
return Err("not a DEMG file".into());
}📝 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
| fn read_demgrid(path: &str) -> Result<Dem, Box<dyn std::error::Error>> { | |
| let b = std::fs::read(path)?; | |
| if b.len() < 24 || &b[0..4] != b"DEMG" { | |
| return Err("not a DEMG file".into()); | |
| } | |
| let u32_at = |o: usize| u32::from_le_bytes(b[o..o + 4].try_into().unwrap()); | |
| let f64_at = |o: usize| f64::from_le_bytes(b[o..o + 8].try_into().unwrap()); | |
| let ver = u32_at(4); | |
| if ver != 1 { | |
| return Err(format!("unsupported DEMG version {ver}").into()); | |
| } | |
| let w = u32_at(8) as usize; | |
| fn read_demgrid(path: &str) -> Result<Dem, Box<dyn std::error::Error>> { | |
| let b = std::fs::read(path)?; | |
| if b.len() < 32 || &b[0..4] != b"DEMG" { | |
| return Err("not a DEMG file".into()); | |
| } | |
| let u32_at = |o: usize| u32::from_le_bytes(b[o..o + 4].try_into().unwrap()); | |
| let f64_at = |o: usize| f64::from_le_bytes(b[o..o + 8].try_into().unwrap()); | |
| let ver = u32_at(4); | |
| if ver != 1 { | |
| return Err(format!("unsupported DEMG version {ver}").into()); | |
| } | |
| let w = u32_at(8) as usize; |
🤖 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/iceland_dem.rs` around lines 40 - 51, The header validation in
read_demgrid is too short and allows truncated files to reach the f64_at(24)
read path. Update the initial length check in read_demgrid so it պահանջs enough
bytes for the full fixed header before any u32_at/f64_at access, ensuring files
shorter than the needed header size return the existing error instead of
panicking.
Comment on lines
+96
to
+113
| stitched = np.zeros((Htiles * TILE, Wtiles * TILE), dtype=np.float32) | ||
|
|
||
| jobs = [(zoom, x, y) for y in ys for x in xs] | ||
| got, missing = 0, 0 | ||
| with ThreadPoolExecutor(max_workers=16) as ex: | ||
| for x, y, rgb in ex.map(lambda t: fetch_tile(*t), jobs): | ||
| gx = x - x0 | ||
| gy = y - y0 | ||
| if rgb is None: | ||
| missing += 1 | ||
| continue | ||
| elev = rgb[:, :, 0] * 256.0 + rgb[:, :, 1] + rgb[:, :, 2] / 256.0 - 32768.0 | ||
| stitched[gy * TILE:(gy + 1) * TILE, gx * TILE:(gx + 1) * TILE] = elev | ||
| got += 1 | ||
| print(f"fetched {got} tiles, {missing} missing", file=sys.stderr) | ||
| if missing > len(jobs) // 10: | ||
| print("too many missing tiles — aborting", file=sys.stderr) | ||
| sys.exit(1) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Missing tiles silently become flat sea-level, not flagged in output.
stitched is zero-initialized (Line 96), and any tile that fails all retries is simply skipped (Line 104-106), leaving that region at elevation 0. Up to 10% of tiles are tolerated as missing (Line 111-113) with no interpolation, masking, or record of which cells are synthetic. This can silently inject visible flat/sea patches into the baked terrain that the downstream iceland_dem Rust baker and viewer will render as if they were real data — undermining the goal of this PR (replacing a sparse scatter with a "real" DEM).
Consider tracking a missing-tile mask and either failing the run, retrying more aggressively, or interpolating from neighboring valid tiles instead of defaulting to 0.
🤖 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 `@scripts/fetch_iceland_dem.py` around lines 96 - 113, The DEM stitching logic
in the tile fetch loop is leaving failed tiles as zero-valued elevation in
`stitched`, which makes missing data look like valid sea-level terrain. Update
the `ThreadPoolExecutor`/`fetch_tile` processing to track a missing-tile mask or
sentinel for each tile, and avoid writing implicit zeros into `stitched` for
absent tiles. Then either fail the run when gaps remain, or explicitly fill them
via interpolation/neighbor-based recovery before emitting the raster, so
downstream `iceland_dem` does not treat synthetic flat patches as real data.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-on to the merged #88 (
/icepalette + sky + aerial camera + Kurvenlineal breathing): replaces the sparse point-cloud bake with a real Iceland DEM heightfield, and fixes the anatomy-layer chrome so a map doesn't read as skeleton/body-parts.Real DEM terrain (
44a474ff)elev = R·256 + G + B/256 − 32768, stitched to 4608×3584 and downsampled to a 1152×896 vertex grid.bso2::encode_mesh_bso2(mirrors the exact BSO2 ver-6 Signed360 layout the viewer decodes; the OSMencode_bso2path is untouched, all 12 geo tests pass). New bakergeo/src/bin/iceland_dem.rs; DEM fetched byscripts/fetch_iceland_dem.py(keeps the geo crate HTTP-dep-free).iceland_latestrepointed to the newiceland_dem.helix.soa.gz(9.6 MB, ships same-origin like the OSM bake); the sparseiceland.helix.soa.gzis preserved on disk, unreferenced — nothing overwritten.Geo "terrain" chrome (
79f8dce3)BodyHelixis shared by/helix(anatomy) and the geo scenes, and the layer sidebar/buttons rendered the hardcoded anatomyLAYERS(skin/muscle/…/skeleton) regardless of scene — so the terrain showed up categorized under skeleton. Geo scenes now deriveactiveLayersfrom the layers the bake actually stamps, relabelled "terrain" (empty anatomy layers dropped); the layer id is preserved so show/hide still filters the real geometry. Anatomy scenes keepLAYERSbyte-identical.Verify
cd cockpit && npm run buildclean. The DEM scene is screenshot-confirmed (solid terrain); the chrome commit is a type-checked label swap over that same confirmed render (the 1M-vert breathing scene is too heavy for the headless swiftshader GPU to re-snapshot, so the panel label is build+code-verified — noted honestly).BodyHelix.tsxhistory is clean (no shared-tree race: the DEM agent was frozen out of the TSX; the chrome fix is my own commit).🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Chores