feat(simulation+ui): the regional map — click-through descent to district-resolution windows (D-226 T-1124) #187
Assignees
jpmschweitzer (Jeroen Schweitzer)
Clear assignees
jpmschweitzer (Jeroen Schweitzer)
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: jpmschweitzer/settled-reach#187
Reference in New Issue
Block a user
Summary
The regional map, end to end (D-226 T-1124 amendment, both halves): click through from the fixed planetary heightmap into a live district-resolution (2 km/cell) windowed view — the jet-plane altitude between orbit and the never-mapped voxel world, and the first screen ever fed by the on-demand
derive_districtinvention path. After this merges:make atlas(or the in-game Atlas), hover the planetary map, click open terrain → descend into the regional composite; pan refetches at window edges; Esc climbs back out.T-1137 — windowed district layer, server half (Dudley)
AtlasLayerRequest.window_center/window_n(serde-default; absent = whole-body, byte-compatible for every existing caller; demux untouched —up_tostays the discriminator).DistrictWindowLayerechoescenter/n+ six parallel arrays (morphology, elev_q, temp_dc i16 with the region sentinel, moisture_q, vegetation incl. Marine=6, glaciation).GenWorkItem::DeriveWindowon the Rayon queue, completion drain caches into a boundedDistrictWindowCache(FIFO 256; no staleness by D-227 purity),serve_district_windowpolls — never an inline derive in the PreInput drain. Per-connection coalescing: a newer still-pending window request for the same(ConnectionId, body)supersedes the older; supersession + key-isolation proven by dedicated tests.run_layer1on the Rayon path (option b) — caching would cost ~1.5MB × 50 LRU slots ≈ 100MB permanent (the exact D-203 bloat T-1044's own text guarded against); re-derivation is ~45ms, paid on the background path. T-1044 verified as within-cascade-only (cascade.rs:358still drops the analysis) — the fork was genuinely open.aliveness_probe::render_window_panels; the T-1123 byte-identical determinism proof promoted from probe-only to a real test. Server clampswindow_nto 1..=64. New fixture exercises all six arrays incl. the airless sentinel and Marine.T-1138 — regional map screen, client half (Stig)
set_viewcapture API survives for the golden harness); hover shows a not-to-scale bracket reticle with the real extent labeled ("~66 × 66 km") — chosen over a true-extent rectangle because n=32 is sub-pixel at planetary scale and the morph transition is explicitly deferred; city-click wins — a click on a settlement marker opens city data unchanged, a click on open terrain descends (one gesture, two contextual reads, no modifier).districtnav screen centered on the click point'sDistrictPos(atlas_descend_geometry.district_pos_at, the verified inverse of the server's mapping). Regional composite: morphology × elev_q lightness base, temp/moisture/veg toggles (temp reuses the region-ramp colorizer exactly — one colorizer across zoom levels), glaciation as an always-on tint usingapply_ice_tint's REAL gate (None|Light no-op) over the amendment's looser prose — documented deviation. Pan-on-held-composite with edge-crossing refetch + border-fade during the background derive; zoom never refetches. Client LRU keyed(body_id, center, n), touch-on-read, evict-only.Cross-verification before either half was committed
Stig spawned Dudley's in-progress server from the shared worktree and drove the real wire path: round-trip on a GJ380c coastal district (6 fields × 1024 cells, all in-range), echo staleness guard, a genuine ~1.4s background-derive wait matching the queue model, a real pan-edge refetch to the adjacent window, and a zero-network cache hit on re-descent. Dudley independently verified the wire shape against Stig's codec — zero deltas from the recorded contract. Both fixture regenerations converged byte-identical (deterministic serialization).
Verification
make atlasrun is the acceptance pass, same as the friction round.Tickets: T-1137, T-1138 (+T-1141 filed for the two flagged follow-ups)
Tyre — architecture review, PR #187 (D-226 T-1124 amendment, both halves)
Verdict: REQUEST_CHANGES (one substantive re-derive/cost finding + three doc/guard fixes; the mechanism is sound and faithful to the amendment throughout).
The design is mine; the implementation is Dudley's (server) and Stig's (client). Judged against the amendment contract with the carried question front and centre. cracks knuckles — the good news first: the serving model, the windowed-family ceiling, the queue coalescing, the client seam set, and the D-010 wire discipline are all built exactly to the record. The queue-served-never-inline contract holds, the echo-key staleness guard is correct, the demux ceiling is untouched, and the glaciation "deviation" is actually more correct than my own prose. Five of the six focus areas are clean. The sixth — the carried question — is real and I'm ruling on it below.
RULING ON THE CARRIED QUESTION — TerrainAnalysis re-derivation granularity
Finding confirmed. The re-derive is NOT "one-time per body" — it is one full
run_layer1per uncached-window request.server/src/atlas/gen_queue.rs:631— theDeriveWindowbranch ofrun_work_itemcallsrun_layer1(&working)unconditionally on every invocation. There is noTerrainAnalysismemoization anywhere:BodyWorldStatedoesn't retain it (confirmed — no field), andDistrictWindowCachecaches finished layers keyed(body, center, n), so it only short-circuits an exact-repeat window. A pan to any new(center, n)→ cache miss → freshrun_layer1.layer1.rs:19documentsrun_layer1as "the ~45 ms drainage pass per body" — the exact cost T-1044 optimised away within the cascade, now re-incurred per window pan.So the real per-request Rayon cost is ~45 ms
run_layer1+ ~7 ms derive (n=32) ≈ 52 ms, not the ~7 ms §4 implies, and not "~45 ms ONE-TIME per body" as the variant doc claims (gen_queue.rs:171). It is tick-safe (off-thread) and under the 0.5 s border-fade grace, so nothing breaks. But two claims in the record are now false, and the worker-occupancy bound theDISTRICT_WINDOW_MAX_N=64cap was sized against (≤29 ms/window) is understated by ~2x (~52–74 ms/window with the re-derive on top).My ruling: require memoization — a small, lazy, per-body
TerrainAnalysisLRU alongside the window cache — AND fix the two cost claims. This is not the "accept-and-document" call I'd make if browsing were a rare path. It isn't: the dominant interaction this whole feature exists for is a player panning around one body's region, which is precisely N cache-missing windows on the samebody_id— every one of them paying a fresh ~45 msrun_layer1for aTerrainAnalysisthat is byte-identical across all of them (same heightmap, same downsample). Memoisation turns the 2nd..Nth window on a body from ~52 ms to ~7 ms — a 7x cut on the common path — for a bounded ~2 MB/browsed-body, lazy (only bodies actually windowed, never the 50-bodyBodyWorldStateCacheen masse), keyed onbody_id, evictable (recompute always valid, D-227). That is the same "cache the expensive invariant, key it right, bound it" instinct the whole cascade already runs on; it's the elegant fit here, not a gold-plate. The cap's starvation-bound reasoning also becomes true again once the per-window worker occupancy drops back to the derive-only cost it was written for.If the team prefers to ship without the LRU, the fallback is acceptable-as-is on the mechanism only with both cost claims corrected to the honest per-request figure — but I'd be leaving a 7x win on the dominant path on the floor, so I'm ruling for the LRU. Either way the doc fix is mandatory.
server/src/atlas/gen_queue.rs:171— "~45 ms ONE-TIME, paid only when a window is actually requested" reads as amortised-once-per-body. It is per-uncached-window. Reword to the real per-request cost (and, if the LRU lands, to "first window per body pays run_layer1; subsequent windows hit the TerrainAnalysis LRU").governance/decisions/architecture.md§4 (~line 1658) — the "≈ 7 ms (n=32) / ≈ 29 ms (n=64)" Rayon-time figures omit the ~45 msrun_layer1that precedes every uncached derive. Amend to state the re-derive is included per request (or removed by the LRU), so the cap's occupancy-bound rationale stays honest.C1 — [server/src/atlas/gen_queue.rs:631] (also §4, gen_queue.rs:171) — re-derive cost: memoize + fix the claim. See RULING above. Primary blocker.
C2 — [client/ui/implant/apps/atlas/atlas_descend_geometry.gd:94] — pixel→DistrictPos inverse is a hand-derived twin of the server forward map, with no round-trip guard = drift risk.
district_pos_atre-implements the inverse ofdistrict_profile.rs::derive_district's forward pixel mapping (serverderive_district:1426-1437). Two independent implementations of the same coordinate transform, each with its ownDISTRICT_M = 2048.0literal (client) vsscale::DISTRICT_M(server), and they already differ subtly: the server forward uses the continuouscircumference_m / DISTRICT_M, the client inverse rounds it to an integerdistrict_cols— a fractional-district skew that grows toward the antimeridian. Correctness-in-the-small is Hoshe's lane; my lane is the duplication contract, and this is a drift risk worth a guard, not an acceptable silent twin. The failure mode is nasty: a wrong inverse descends the player into a different region than they clicked, silently (right-looking map, wrong data). The amendment §5 anticipated a client-sidetrue_district_of_pixel-style inverse — it did not bless duplicating the geodesy with no cross-check.Fix (pick one): (a) a round-trip test that pins
server_forward(client_inverse(px)) ≈ pxwithin one district across the texture (a Rust↔GDScript fixture, or a GDScript test against a committed table of server-forward samples); or (b) hoist the2048.0to a single shared constant surfaced to both sides so at least the scale can't drift independently. (a) is the real guard; (b) is the floor. Absent either, the two sides are free to diverge on any future geodesy change (longitude-origin shift, tilt, wrap change) with no test to catch it.C3 — [server/src/atlas/layer_proxy.rs:1267] — the determinism test proves packing, not the served layer's derivation determinism.
build_district_window_layer_two_passes_are_byte_identicalruns two passes over the sameta— it provesbuild_district_window_layerpacks deterministically, which was never in doubt. It does not prove that two independentrun_layer1re-derivations ofta(the actual served path,gen_queue.rs:631) yield the sameTerrainAnalysis, hence the same window. The lead asked whether the determinism test "genuinely proves the served layer (not just the builder fn)" — it proves the builder fn.derive_window_round_trip_produces_populated_layerexercises the realrun_layer1path but asserts only array lengths, not cross-derive byte-equality. Given D-227 reclassified derivation determinism as save-critical (any drift desyncs the whole save, not just a cosmetic golden), the served path —run_layer1included — needs a two-independent-derive byte-equality assertion, not just the packer. Fix: add a test that derives the same window twice through the fullrun_layer1→build_district_window_layerpath (two separaterun_layer1calls) and asserts byte-identical output. (If C1's LRU lands, this doubles as its correctness guard — the LRU is only sound if two independentrun_layer1s agree.)C4 — [governance/decisions/architecture.md §5, ~line 1665] — glaciation-gate prose is wrong; the code is right. Fix the prose.
Stig gated the ice-tint at
>= Moderate(grade 1 Light → alpha 0.0 no-op), matchingaliveness_probe.rs::apply_ice_tint:604(None | Light => return base) exactly — alphas{2:0.30, 3:0.50, 4:0.70}and the ice-white endpoint all match the probe verbatim. The amendment §5 prose says "gated onglaciation_grade >= Light" — that's my summary, and it's the imprecise one. §5 also explicitly instructs portingapply_ice_tint; Stig chose the load-bearing instruction (port the named fn) over the loose summary and documented the divergence with the probe's own rationale (grade 1 = glacial-erosion signatures, not visible ice; D-239 §5 gates visible glacial forms at ≥ Moderate). This is the correct call. Fix: amend the §5 prose to ">= Moderate(matchingapply_ice_tint; grade 1 Light is erosion-signature, not ice cover)" — documented drift, prose follows code.Clean — no action (stated for the record):
serve_district_window(layer_proxy.rs:772) never derives inline — cache-hit returns a clone, miss submits aDeriveWindowand returnsNone; the completion drain (plugin.rs:409-417) only inserts into the cache, the requester re-polls via the existing D-225 loop. This is the exact whole-body-cache-miss pattern. The clamp lives in exactly one place (layer_proxy.rs:782).(ConnectionId, body)(gen_queue.rs:213,submit_window:396) is right. A pan sequence produces different centres for the same(conn, body)→ only the last-queued survives = the desired supersede; cross-connection is safe (two viewers on the same body from differentConnectionIds do NOT coalesce — proven bysubmit_window_does_not_coalesce_different_keys). Body (not centre) is the correct coalescing granularity. Note for the record: coalescing only collapses items still pending; an already-dispatched derive can't be cancelled (documented, no cancellation channel) — so N deliberate pan-and-stops spaced beyond the derive latency each run a full derive. That's inherent and acceptable; it's the same population C1 is about, and C1's LRU is what makes those N cheap.DeriveWindowandAnalyzeBodyboth submit atImmediate, FIFO-interleaved within the band (gen_queue.rs:371-375). A window burst can sit ahead of a genuine body-analysis submitted just after it. With plural workers + coalescing this is fine and the amendment accepted the cap as the occupancy bound — flagging only that once C1's re-derive is understood, "windows can't starve cascade jobs" rests on the cap being sized for the real (post-re-derive) occupancy, which C1 fixes. No separate change required.district_window,layer_proxy.rs:145); the growth-ceiling doc (layer_proxy.rs:99-119) correctly re-scopes the six-field cap to the dense whole-body family and cites §2's one-windowed-field rule; the inbound demux ceiling (bridge/mod.rs:67-75) still namesBrowseRequestas the fifth-and-last shape — the window rodeAtlasLayerRequestvia#[serde(default)], no sixth shape. Fully compliant.atlas_marker_overlay.gd/atlas_overlay_colors.gdprecedent. Duck-typed reuse of the overlay bar + legend (atlas_window_viewer.gd:59-64, implementingget_overlay_defs()/is_overlay_visible()/set_overlay_visible()) is composition-over-inheritance done right — a drop-in viewer, noAtlasViewersubclass fork. Nav-push (atlas_app.gd:158,DistrictScreenmirroringRegionalScreen) is defensible under Jeroen's click-through revision (the crumb is earned when entry is a click, not a scroll-in). City-click-wins disambiguation (atlas_viewer.gd:759-771) correct.u8×5 +i16temp),centeri32-pair,nu32 — nof32on the wire. Client cache keyed(body_id, center, n)(atlas_window_cache.gd:37), seed implicit viabody_id+session per D-227. Response echo-key staleness guard (atlas_window_request.gd:151-154) discards stale responses correctly.All 18 window-path server tests green; client tests present for descent, cache, colors, viewer, delivery.
Tyre — re-review of the four C1–C4 deltas (
df33e9f2f/4eed3bccb/96f6baa93)Verdict flips to APPROVE. All four findings addressed on the merits — verified against the code and tests at worktree HEAD, not the summary.
C1 (the carried question) — resolved.
TerrainAnalysisCacheis a per-body true-LRU (monotonic access clock,min_by_key(last_used)eviction — the right policy vsDistrictWindowCache's FIFO, since a browsed body carries a recency signal a D-227-pure window doesn't), capacity 8 / ~16 MB worst case, heldArc<Mutex<>>onGenerationQueueand cloned into the Rayon closure — the correct home (the main-threadDistrictWindowCacheis unreachable fromrun_work_item, exactly thein_flight/in_flight_countprecedent).get_or_derivein theDeriveWindowbranch: first window on a body pays the ~45 msrun_layer1, every subsequent window (any(center,n), not just exact repeats) hits the LRU.two_windows_on_same_body_share_one_terrain_analysis_entryproves it end-to-end — two distinct derives from two connections, both complete,cache.len() == 1. The dominant browse-one-body pattern now pays the re-derive once, not per pan. Doc claim corrected to the honest model.C2 — resolved. Denominator fixed (
tex_h - 1.0, pole-clamp guarded the same wayaliveness_probe.rs:510guards the server-side inverse); docstring now explains the deliberate column/row asymmetry (longitude wraps → plain width; latitude clamps →h-1endpoints). The reticle also now hides over a city (_should_draw_descend_reticle), so the affordance matches city-click-wins. And the round-trip drift guard my C2 required is there:test_..._round_trips_off_equator_rows_against_server_forward_maptranscribes the server forward formula (citeddistrict_profile.rs:1436), forward-maps three off-equator rows (both hemispheres + near-pole, real body radius GJ380c) and assertsdistrict_pos_atrecovers each exact row, plus an explicit non-off-by-a-few guard. Residual (non-blocking): the guard transcribes the formula rather than executing the Rust, so a future server-side geodesy change could drift the transcription too — materially stronger than nothing (cited line, exact round-trip), and a cross-language fixture would be disproportionate here. Accepted as the C2 proof.C3 — resolved.
full_path_two_independent_run_layer1_passes_produce_identical_windowrunsrun_layer1twice independently (no sharedta), asserts the twoTerrainAnalysisagree field-by-field (ocean_mask/lake_mask/water_dist/slope_deg/elev_pct), then packs both and asserts byte-identical windows. This exercises the D8 drainage +TerrainAnalysis::analyze— the served path, not just the packer — closing the save-critical D-227 gap. Packer-only test kept as the cheaper different-thing proof.C4 — resolved. Both prose sites corrected: §5 now gates the ice-tint at
>= Moderatewith theapply_ice_tint-matching rationale (None/Light no tint; Light = erosion signatures, not ice); §4 now states the warm-TerrainAnalysisassumption and the full first-window/+LRU/eviction cost model. Prose follows code.One non-blocking observation on C1 for the record:
get_or_deriveholds the terrain-cache mutex across the wholerun_layer1, so concurrent distinct-body first-window misses serialize where the pre-fix code ran them in parallel. Defensible — it guarantees zero wasted same-body derive (the dominant pattern) at the cost of rare cross-body derive parallelism; the derive-outside-lock alternative would reintroduce same-body double-derives. Correct trade for this workload, no change needed.Verified locally: TerrainAnalysisCache 3/3, C3 full-path 1/1,
cargo clippy --lib -D warningsclean, clienttest_atlas_descend_entry32/32,test_atlas_window_viewer28/28. Architecture lane clear. APPROVE.Review: atlas-regional-window → main (type: code + governance) — round 2 (fix round)
Three reviewers: Hoshe (QA), Tyre (architecture — the amendment is his design; implementation only), Araminta (the two §5 UX calls delegated to the implementer). All three: round 1 REQUEST_CHANGES → all findings fixed → all three CONFIRMED, APPROVE.
Hoshe (QA): APPROVE (round 1: 1 blocking finding)
Round-1 finding — a confirmed live bug: the client's pixel→DistrictPos latitude inverse divided by
tex_hwhere the server's forward map usestex_h − 1— every off-equator click descended into the wrong district (live headless repro: forward-mapped row 50, inverse returned 45 = 10.2 km drift; up to ~20 km at high latitude). The suite missed it because its only real-radius latitude test sampledtex_h*0.5— which is not the true equator pixel ((tex_h−1)*0.5is), so the test never verified what it claimed.Fix confirmed (
4eed3bccb) with independent re-verification: fresh second transcription of the server formula (not the test helper), all rows recover exactly incl. the original repro; the corrected equator test's root-cause claim verified algebraically; the added degenerate-texture guard prevents a divide-by-zero the original fix suggestion would have introduced. Everything else in the QA lane verified clean round 1: wire omit-case compat, coalescing survivor-discrimination, end-to-end clamp, mixed-content determinism fixture, real-LRU client cache test, complete pan/zoom removal with the golden-capture API intact.Tyre (Architecture): APPROVE (round 1: C1–C4)
run_layer1ran per uncached window, not "one-time per body" as claimed — 7× the amendment's numbers on the dominant pan-one-body path. Fixed:TerrainAnalysisCache, a per-body true-LRU (recency eviction — right policy vs the window cache's FIFO), capacity 8,Arc<Mutex>on theGenerationQueue(thein_flightprecedent — reachable from Rayon workers),get_or_derivein the DeriveWindow branch; end-to-end test proves two same-body derives from different connections share exactly one entry. Doc claim corrected.run_layer1passes → field-by-field analysis agreement → byte-identical packed windows (the D-227 save-critical proof).>= Moderatematchingapply_ice_tint— code stood, prose was wrong).Araminta (§5 UX calls): APPROVE (round 1: 1 finding)
Round-1 finding — broken promise: the descend reticle drew unconditionally on hover, so hovering a city showed the marker flaring white (city-click signal) and the reticle + extent label (false descend promise) simultaneously. Fixed:
_should_draw_descend_reticle()with the_hovered_city.is_empty()clause — the visible affordance now always matches the click outcome; three state tests (show/hide-over-city/hide-idle) confirmed the right shape. Both delegated calls endorsed: the not-to-scale bracket reticle + real-extent label (honest — a true n=32 rectangle is sub-pixel at planetary scale), and city-click-wins disambiguation. Chrome/legend/palette discipline endorsed as shipped.Verdict: APPROVED
Full suites green via the pre-push gate on every push (server lib 1778, atlas 564; client 3194+). Cross-half live verification pre-commit (real wire round-trip, background-derive wait, pan-edge refetch, cache hit).
pql decisions validateclean on the corrected amendment. Jeroen's hands-onmake atlasdescent is the acceptance pass.Pull request closed