From 0159a63cc2d6be6a5778f4e991eea4bf60a11a6b Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Wed, 22 Jul 2026 00:47:53 +0200 Subject: [PATCH] =?UTF-8?q?fix(simulation):=20PR=20#191=20review=20round?= =?UTF-8?q?=20=E2=80=94=20n-clamp=20mirror,=20min=5Fwl=20band=20quantizati?= =?UTF-8?q?on,=20coalescing=20coverage,=20fixture=20consumer?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All seven Hoshe/Tyre findings addressed, none retracted: - n-clamp/echo/staleness triangle (Tyre C1): client _clamp_window_n_mirror (bit-for-bit twin of the server clamp, canonicalize_district_center precedent) applied before _n is stored/sent; server test pins the quarter n=32 -> echo 16 contract. - min_wl band quantization (Hoshe 1/Tyre C3): quantize_min_wl_m snaps to MIN_WL_BANDS_M {0, 32768, 16384, 8192, 4096} before cache key and echo (design doc §5's unbounded-key fix), reusing the one true OCTAVE_WAVELENGTHS_M array; docstrings now state the server-quantizes/ client-sends-raw split; same-band cache-sharing test. - coalescing granularity axis (Hoshe 2): two tests pin different- granularity requests as separate in-flight slots and same-granularity coalescing unchanged. - orphaned fixture (Hoshe 3): test_protocol.gd consumer decodes atlas_response_ready_with_window.msgpack through the real IPC path and asserts the new fields. - atlas_window_request coverage (Hoshe 4): new test file — stale-drop on granularity mismatch, old-server-shape defaults accepted, clamp mirror formula + wiring. First draft's quarter-via-request_now test would have passed for the wrong reason (request_now resets granularity by design until T-1153) — split into formula pin + reachable-path wiring proof. - granularity type seam (Tyre C2): field + resolver docstrings state finer-only integer multiples with resolve_window_granularity as the single widening point; matching contract note added to the D-226 T-1143-rulings amendment. cargo --lib 1807/1807; goldens bit-identical; gdlint clean. --- client/scripts/protocol/atlas_map_protocol.gd | 10 + client/tests/test_atlas_window_request.gd | 177 ++++++++++ client/tests/test_protocol.gd | 21 ++ .../apps/atlas/atlas_window_request.gd | 49 ++- governance/decisions/architecture.md | 2 +- server/src/atlas/detail_scatter.rs | 10 +- server/src/atlas/gen_queue.rs | 98 +++++- server/src/atlas/layer_proxy.rs | 317 +++++++++++++++++- 8 files changed, 670 insertions(+), 14 deletions(-) create mode 100644 client/tests/test_atlas_window_request.gd diff --git a/client/scripts/protocol/atlas_map_protocol.gd b/client/scripts/protocol/atlas_map_protocol.gd index c38818582..8935f8b49 100644 --- a/client/scripts/protocol/atlas_map_protocol.gd +++ b/client/scripts/protocol/atlas_map_protocol.gd @@ -39,6 +39,16 @@ class_name AtlasMapProtocol ## requests quarter granularity yet (that's T-1153); this function just makes ## it possible to ask, byte-compatible with every existing caller that ## doesn't pass them. +## +## **Quantization split (PR #191 review, Hoshe 1 / Tyre C3):** `window_min_wl_m` +## is sent HERE as a raw, unquantized value — this codec does NOT snap it to +## the design doc §5 fixed band set. The SERVER is the one place quantization +## happens (`serve_district_window` → `quantize_min_wl_m`, `layer_proxy.rs`): +## it snaps every request's value to the nearest band before touching the +## cache key or the echo, so a caller here is free to send a +## viewport-continuous estimate (e.g. `E/C` from the rung-selection rule) — +## don't pre-quantize client-side, it would just duplicate logic the server +## already owns and could drift out of sync with it. static func encode_atlas_layer_request( mp, body_id: String, diff --git a/client/tests/test_atlas_window_request.gd b/client/tests/test_atlas_window_request.gd new file mode 100644 index 000000000..942d243dd --- /dev/null +++ b/client/tests/test_atlas_window_request.gd @@ -0,0 +1,177 @@ +## T-1150 (PR #191 review, Hoshe 4): atlas_window_request.gd had NO test file +## at all before this — direct coverage of the granularity/min_wl_m staleness +## guard, the n-clamp mirror (Tyre C1), and the old-server-shape default +## disposition. Follows test_atlas_window_viewer.gd's own +## "AtlasWindowRequest — cache reuse" section conventions (same +## instantiation pattern: `AtlasWindowRequest.new(owner_stub)`, `add_child()` +## for the debounce Timer, hand-built response dicts) rather than +## re-inventing a shape. +class_name TestAtlasWindowRequest +extends GdUnitTestSuite + +# atlas_window_request.gd has no class_name (review #8 precedent throughout +# this cluster) — preloaded once here, not re-load()ed per test (gdlint +# duplicated-load). +const AtlasWindowRequest := preload("res://ui/implant/apps/atlas/atlas_window_request.gd") + + +## Build a hand-authored DistrictWindowLayer dict, granularity-aware +## (T-1150) — mirrors test_atlas_window_viewer.gd's own _mock_window(), with +## granularity/min_wl_m added as optional params so callers can build both +## rungs' echo shapes with one helper. +static func _mock_window( + center: Vector2i, n: int = 2, granularity: int = 1, min_wl_m: int = 0 +) -> Dictionary: + return { + "center": [center.x, center.y], + "n": n, + "granularity": granularity, + "min_wl_m": min_wl_m, + "morphology": PackedByteArray([8, 14, 0, 1]), + "elev_q": PackedByteArray([40, 90, 5, 60]), + "temp_dc": [120, 95, -32768, 60], + "moisture_q": PackedByteArray([50, 30, 90, 20]), + "vegetation": PackedByteArray([2, 1, 6, 3]), + "glaciation": PackedByteArray([0, 0, 1, 2]), + } + + +static func _mock_response(body_id: String, window: Variant) -> Dictionary: + return {"body_id": body_id, "status": "Ready", "district_window": window} + + +func _make_request() -> Variant: + var owner_stub := RefCounted.new() + var req = auto_free(AtlasWindowRequest.new(owner_stub)) + add_child(req) + return req + + +# ============================================================================= +# (a) granularity mismatch on the echo -> dropped as stale +# ============================================================================= + + +## The mandatory item-(a) case: request_now() asks at the default district +## granularity (1); a response echoing granularity=4 (quarter) for the SAME +## center/n must be dropped as stale, not accepted — a different rung's +## derive answering a request for a different rung is exactly as stale as a +## mismatched center (T-1150 extends §2's guard to this axis). +func test_on_response_with_mismatched_granularity_is_dropped_as_stale() -> void: + var req = _make_request() + req.request_now("GJ380c", Vector2i(2, 2), 2) + assert_bool(req.is_pending()).is_true() + + var quarter_window: Dictionary = _mock_window(Vector2i(2, 2), 2, 4, 0) + req.on_response(_mock_response("GJ380c", quarter_window)) + + assert_bool(req.is_pending()).override_failure_message( + "a granularity-mismatched response must be dropped as stale, leaving the district request still pending" + ).is_true() + + +# ============================================================================= +# (b) old-server-shape response (no granularity/min_wl_m keys) -> defaults +# ============================================================================= + + +## A response from a hypothetical pre-T-1150 server (or any response whose +## district_window dict simply omits the new keys) must decode granularity +## as district (1) and min_wl_m as 0 via the same defaulting on_response() +## already applies — and since request_now()'s own defaults are identical, +## the response is ACCEPTED, not treated as stale just because two keys are +## missing. +func test_on_response_missing_granularity_and_min_wl_defaults_and_is_accepted() -> void: + var req = _make_request() + req.request_now("GJ380c", Vector2i(3, 3), 2) + assert_bool(req.is_pending()).is_true() + + # Old-shape window: no "granularity"/"min_wl_m" keys at all. + var old_shape_window := { + "center": [3, 3], + "n": 2, + "morphology": PackedByteArray([8, 14, 0, 1]), + "elev_q": PackedByteArray([40, 90, 5, 60]), + "temp_dc": [120, 95, -32768, 60], + "moisture_q": PackedByteArray([50, 30, 90, 20]), + "vegetation": PackedByteArray([2, 1, 6, 3]), + "glaciation": PackedByteArray([0, 0, 1, 2]), + } + req.on_response(_mock_response("GJ380c", old_shape_window)) + + assert_bool(req.is_pending()).override_failure_message( + ( + "an old-server-shape response (missing granularity/min_wl_m) must " + + "default to district/0 and be ACCEPTED, not dropped as stale" + ) + ).is_false() + + var received: Array = [] + req.window_ready.connect(func(w: Dictionary) -> void: received.append(w)) + # Re-request the same (body, center, n) — must now be a cache hit, proving + # on_response() actually stored the old-shape window under the + # district/0 key, not silently discarding it. + req.request_now("GJ380c", Vector2i(3, 3), 2) + assert_int(received.size()).is_equal(1) + assert_bool(req.is_pending()).is_false() + + +# ============================================================================= +# (c) n-clamp mirror (Tyre C1) — quarter n=32 stores clamped n=16 +# ============================================================================= + + +## **Item (c) as literally scoped by the ticket** ("the clamp-mirror from +## item 1"): `_clamp_window_n_mirror()` reproduces the server's +## `clamp_window_n(raw_n, granularity)` bit-for-bit, INCLUDING the quarter +## n=32 -> 16 case — pinned directly against the static helper, independent +## of the request/response plumbing (`request_now()` has no public +## "request quarter" entry point today; T-1150 is struct/key plumbing only, +## requesting quarter is T-1153's job — see the class-level docstring on +## `_clamp_window_n_mirror()` for why calling `request_now()` at district +## granularity can never itself exercise the quarter branch: it unconditionally +## resets `_granularity` to district BEFORE clamping, by design, since no +## caller can ask for quarter yet). +func test_clamp_window_n_mirror_matches_server_formula_at_quarter_n32() -> void: + assert_int(AtlasWindowRequest._clamp_window_n_mirror(32, 4)).is_equal(16) + # District granularity: the per-axis cap (64) governs, matching the + # server's clamp_window_n_district_granularity_uses_per_axis_cap test. + assert_int(AtlasWindowRequest._clamp_window_n_mirror(640, 1)).is_equal(64) + # Small n well under budget at quarter granularity stays unclamped, + # matching clamp_window_n_quarter_granularity_leaves_small_n_unclamped. + assert_int(AtlasWindowRequest._clamp_window_n_mirror(8, 4)).is_equal(8) + + +## **Item (c), the request/response half:** `request_now()` actually WIRES +## the mirror in (not just defines it) — a request for a district-legal but +## per-axis-oversized `n` (e.g. 640, mirroring the server's own +## `DISTRICT_WINDOW_MAX_N*10` oversized-request test) stores the CLAMPED +## `_n=64`, so a server response echoing the server's OWN clamped n=64 is +## ACCEPTED, not rejected as stale for "not matching" the raw 640 that was +## asked for. This is the exact n-clamp/echo/staleness triangle Tyre C1 +## flagged, exercised through the reachable (district) path today; the +## quarter-specific n=32->16 number is pinned by the formula test above since +## no public API can drive quarter through `request_now()` yet. +func test_oversized_n_request_stores_clamped_n_and_accepts_matching_echo() -> void: + var req = _make_request() + req.request_now("GJ380c", Vector2i(4, 4), 640) + + assert_int(req._n).override_failure_message( + ( + "request_now() must mirror the server's clamp_window_n(640, granularity=1) " + + "== 64 BEFORE storing _n, not store the raw requested 640" + ) + ).is_equal(64) + assert_bool(req.is_pending()).is_true() + + # The server's real response for this request echoes n=64 (its own + # clamp_window_n() result) — must be ACCEPTED, not stale. + var clamped_echo: Dictionary = _mock_window(Vector2i(4, 4), 64, 1, 0) + req.on_response(_mock_response("GJ380c", clamped_echo)) + + assert_bool(req.is_pending()).override_failure_message( + ( + "a response echoing the CLAMPED n=64 must be accepted, since _n was " + + "already clamped to 64 before the request fired" + ) + ).is_false() diff --git a/client/tests/test_protocol.gd b/client/tests/test_protocol.gd index 5c3428437..c8d3e8d59 100644 --- a/client/tests/test_protocol.gd +++ b/client/tests/test_protocol.gd @@ -511,6 +511,27 @@ func test_decode_atlas_response_not_found() -> void: assert_that(resp.status).is_equal("NotFound") +## PR #191 review, Hoshe 3: `atlas_response_ready_with_window.msgpack` had NO +## consumer anywhere in client/tests — regenerated by the T-1150 `granularity`/ +## `min_wl_m` field additions but nothing decoded it through the real IPC path. +## This is that consumer, matching the sibling `test_decode_atlas_response_*` +## tests' style/fixture-dir convention above: full decode_atlas_layer_response() +## round trip (not a hand-built Dictionary like test_atlas_data_delivery.gd's +## passthrough tests), confirming `district_window.granularity`/`.min_wl_m` +## (T-1150's two new echo fields) survive the real client decode path. +func test_decode_atlas_response_ready_with_window() -> void: + var bytes := _load_fixture("atlas_response_ready_with_window") + var resp = Protocol.decode_atlas_layer_response(bytes) + assert_that(resp).is_not_null() + assert_that(resp.status).is_equal("Ready") + assert_that(resp.district_window).is_not_null() + var window: Dictionary = resp.district_window + assert_that(window.get("center")).is_equal([10, -5]) + assert_that(int(window.get("n"))).is_equal(2) + assert_that(int(window.get("granularity"))).is_equal(1) + assert_that(int(window.get("min_wl_m"))).is_equal(0) + + func test_snapshot_is_not_decoded_as_atlas_response() -> void: # Disambiguation: an ObserverSnapshot has no "status" key, so the atlas # decoder rejects it. receive_bytes relies on this to route correctly. diff --git a/client/ui/implant/apps/atlas/atlas_window_request.gd b/client/ui/implant/apps/atlas/atlas_window_request.gd index 28ea6ed88..7f1c7b97d 100644 --- a/client/ui/implant/apps/atlas/atlas_window_request.gd +++ b/client/ui/implant/apps/atlas/atlas_window_request.gd @@ -48,6 +48,15 @@ const MAX_RETRIES: int = 20 # ~10s ceiling, matches atlas_generation_proxy.gd's const DEFAULT_GRANULARITY: int = AtlasWindowCache.DISTRICT_GRANULARITY const DEFAULT_MIN_WL_M: int = 0 +## Mirrors server/src/atlas/layer_proxy.rs's DISTRICT_WINDOW_MAX_N / +## WIRE_CAP_CELLS exactly (PR #191 review, Tyre C1). `_clamp_window_n_mirror()` +## below reproduces `clamp_window_n()` bit-for-bit — the load-bearing-mirror +## pattern `AtlasDescendGeometry.canonicalize_district_center()` already uses +## for the server's `normalize_window_center()`. Keep both numbers in sync +## with the server constants of the same name if either ever changes. +const SERVER_DISTRICT_WINDOW_MAX_N: int = 64 +const SERVER_WIRE_CAP_CELLS: int = 4_096 + var _owner = null # AtlasWindowViewer (untyped to avoid cyclic ref) var _cache = null # AtlasWindowCache var _body_id: String = "" @@ -86,6 +95,38 @@ func reset() -> void: _debounce_timer.stop() +## Mirrors server/src/atlas/layer_proxy.rs's `clamp_window_n(raw_n, +## granularity)` EXACTLY (PR #191 review, Tyre C1 — "the sharpest" finding): +## `serve_district_window` echoes the CLAMPED `n` back in +## `DistrictWindowLayer.n`, but `on_response()`'s staleness guard compares the +## echo against `_n`. Without this mirror, `_n` would hold the RAW requested +## value while the server echoes the CLAMPED one — the moment a caller +## requests quarter (granularity=4) at n=32, the server clamps to n=16 and +## echoes THAT, `on_response()` sees `echoed_n=16 != _n=32`, decides the +## response is stale, and the window silently never loads (no error, no log +## on this side — just an eternally-pending request). +## +## Clamping HERE, before `_n` is ever stored or sent, means `_n` already +## equals what the server will echo — no drift between the two sides, the +## SAME load-bearing-mirror pattern `AtlasDescendGeometry. +## canonicalize_district_center()` uses for the server's +## `normalize_window_center()` (see that function's docstring for the general +## rationale: canonicalizing before the request is sent means the client's +## held state already equals what the server will echo back). +## +## Formula, bit-for-bit: `n = raw_n.clamp(1, SERVER_DISTRICT_WINDOW_MAX_N)`, +## then `n = min(n, floor(sqrt(SERVER_WIRE_CAP_CELLS) / max(granularity, 1)))` +## — applied in that order (per-axis cap first, then the granularity-aware +## wire-size ceiling), matching `clamp_window_n`'s own comment ("Applied AFTER +## the per-axis clamp so a request that already satisfies +## DISTRICT_WINDOW_MAX_N still shrinks further at granularity 4"). +static func _clamp_window_n_mirror(raw_n: int, granularity: int) -> int: + var n: int = clampi(raw_n, 1, SERVER_DISTRICT_WINDOW_MAX_N) + var g: int = maxi(granularity, 1) + var cap_n: int = int(floor(sqrt(float(SERVER_WIRE_CAP_CELLS)) / float(g))) + return mini(n, maxi(cap_n, 1)) + + ## Entry point + pan re-request: request the window centered on `center` ## (a DistrictPos-equivalent Vector2i) for `body_id`. Cache hit -> immediate ## synchronous window_ready emit, no network traffic at all. Cache miss -> @@ -97,12 +138,12 @@ func reset() -> void: func request_now(body_id: String, center: Vector2i, n: int = DISTRICT_WINDOW_DEFAULT_N) -> void: _body_id = body_id _center = center - _n = n _granularity = DEFAULT_GRANULARITY _min_wl_m = DEFAULT_MIN_WL_M + _n = _clamp_window_n_mirror(n, _granularity) # Tyre C1 — mirror BEFORE storing/requesting _debounce_timer.stop() # a direct request supersedes any pending debounced one - var cached: Variant = _cache.get_window(body_id, center, n, _granularity, _min_wl_m) + var cached: Variant = _cache.get_window(body_id, center, _n, _granularity, _min_wl_m) if cached != null: _pending = false _retries = 0 @@ -111,7 +152,7 @@ func request_now(body_id: String, center: Vector2i, n: int = DISTRICT_WINDOW_DEF _pending = true _retries = 0 - SimBridge.request_atlas_layers(body_id, "Topography", center, n, _granularity, _min_wl_m) + SimBridge.request_atlas_layers(body_id, "Topography", center, _n, _granularity, _min_wl_m) ## Pan-triggered re-request (§4/§5: "150ms after the last drag-release, not @@ -122,9 +163,9 @@ func request_now(body_id: String, center: Vector2i, n: int = DISTRICT_WINDOW_DEF func request_debounced(body_id: String, center: Vector2i, n: int = DISTRICT_WINDOW_DEFAULT_N) -> void: _body_id = body_id _center = center - _n = n _granularity = DEFAULT_GRANULARITY _min_wl_m = DEFAULT_MIN_WL_M + _n = _clamp_window_n_mirror(n, _granularity) # Tyre C1 — mirror BEFORE storing/requesting _debounce_timer.start() diff --git a/governance/decisions/architecture.md b/governance/decisions/architecture.md index 53fd6d2a5..902a10ad9 100644 --- a/governance/decisions/architecture.md +++ b/governance/decisions/architecture.md @@ -1669,7 +1669,7 @@ Technical foundation decisions that constrain implementation: engine, client-ser **Amended 2026-07-21 (T-1145 — Jeroen, second companion hands-on, KALLAST window):** three regional-window presentation fixes, all client-only. **Cover-fit supersedes contain:** `fit_window_view()`'s zoom now derives from the LARGER viewport dimension with no margin factor (`max(viewport.x, viewport.y) / composite_native`, not the old `0.9 * min(...)`), so the square district-window composite fills a wide/tall viewport edge to edge instead of leaving side margins, with the shorter axis' data extending into pan-space (the same "cover" concept as CSS `object-fit: cover`) — the existing §4 pan-edge refetch is unaffected (it keys off the screen-center-to-DistrictPos mapping, which any fit already centers on `_held_center` by construction, so no refetch churn at rest). **WASD + edge-scroll supersedes drag-pan:** LMB-drag panning is removed entirely (Jeroen's ruling — drag conflicts with click semantics for the map objects, e.g. settlements, this window will host later); panning is now held WASD/arrow keys (continuous, frame-rate-independent, `_process`-polled, physical-keycode reads to stay independent of the project's existing `move_north`/etc. gameplay-movement InputMap actions bound to the same keys) plus edge-scrolling (cursor within ~24px of a viewport edge, suppressed over UI and while the OS window lacks focus); wheel zoom is unchanged; pole-wall (§5 amendment above) and east-west wrap (T-1142) semantics are preserved unchanged under the new input source. **Smoothing is an interim presentation, pending T-1143:** the composite renders as an `n`×`n` `Image`/`ImageTexture` (one pixel per district, the identical existing per-cell color pipeline) drawn scaled with linear filtering — the same treatment the planetary heightmap already gets — instead of `n`×`n` flat rects, so GPU bilinear sampling reads as a terrain gradient rather than hard blocks; the original crisp per-cell path survives behind a compile-time const specifically so T-1143's design pass can compare both directly, and this smoothing is **not** T-1143's answer to district-tier legibility, only a stopgap ahead of it. - **Amended 2026-07-21 (T-1143 design-pass rulings — Jeroen, after the zoom-ladder design pass, `docs/architecture/atlas-zoom-ladder-t1143.md`):** three rulings on the pass's reserved decisions. **(1) The item-(d) ceiling is opened for the Atlas ladder** — Jeroen: *"we set a new BHAG so old restrictions are up for debate."* The D-166 2026-07-21 zoom-ladder condition ("down to tile scale") is read **literally**: the Atlas windowed viewport may descend below quarter (512 m) toward block/tile granularity. This is an explicit ruling, not erosion — exactly the deliberate revisit the T-1112 §2 anti-erosion clause was hardened to force into the open. Item (d)'s substance survives in narrowed form: chunk/tile/voxel output still never appears as a **whole-body planetary map layer**, and the below-quarter rungs are implementation-gated on their own **measurement pass** (costs/wire for block and tile rungs are unmeasured — design pass §2/§7); the harness-verification path for L5 fill remains primary until that pass lands. **(2) Planetary rung wire carrier: progressive capped-density tiling** riding the generalized `district_window` carrier (granularity parameter, §3 of the design pass) — no new dense-raster wire shape, no forced tagged-envelope migration. **(3) The §5 entry click-through cut is superseded by continuous cursor-anchored zoom**: wheel-zoom carries the view from the orbital frame down through regional granularities continuously, anchored at the cursor, **with the condition that a full zoom-out resets to the original canonical planetary frame and location** (the fixed orbital framing is the ladder's top rest state, not a drifted pan state). The click-through descent and rectangle reticle are retired as the *sole* entry (T-1138's shipped mechanic stands until the continuous ladder replaces it in the same change — close inspection is never stranded, same discipline as the §5 fixed-view transition). D-013's "the zoom gesture owns spatial descent" reading is **restored** for this seam. + **Amended 2026-07-21 (T-1143 design-pass rulings — Jeroen, after the zoom-ladder design pass, `docs/architecture/atlas-zoom-ladder-t1143.md`):** three rulings on the pass's reserved decisions. **(1) The item-(d) ceiling is opened for the Atlas ladder** — Jeroen: *"we set a new BHAG so old restrictions are up for debate."* The D-166 2026-07-21 zoom-ladder condition ("down to tile scale") is read **literally**: the Atlas windowed viewport may descend below quarter (512 m) toward block/tile granularity. This is an explicit ruling, not erosion — exactly the deliberate revisit the T-1112 §2 anti-erosion clause was hardened to force into the open. Item (d)'s substance survives in narrowed form: chunk/tile/voxel output still never appears as a **whole-body planetary map layer**, and the below-quarter rungs are implementation-gated on their own **measurement pass** (costs/wire for block and tile rungs are unmeasured — design pass §2/§7); the harness-verification path for L5 fill remains primary until that pass lands. **(2) Planetary rung wire carrier: progressive capped-density tiling** riding the generalized `district_window` carrier (granularity parameter, §3 of the design pass) — no new dense-raster wire shape, no forced tagged-envelope migration. **(3) The §5 entry click-through cut is superseded by continuous cursor-anchored zoom**: wheel-zoom carries the view from the orbital frame down through regional granularities continuously, anchored at the cursor, **with the condition that a full zoom-out resets to the original canonical planetary frame and location** (the fixed orbital framing is the ladder's top rest state, not a drifted pan state). The click-through descent and rectangle reticle are retired as the *sole* entry (T-1138's shipped mechanic stands until the continuous ladder replaces it in the same change — close inspection is never stranded, same discipline as the §5 fixed-view transition). D-013's "the zoom gesture owns spatial descent" reading is **restored** for this seam. **Wire-contract note (T-1150, PR #191 review — Tyre):** the `window_granularity` field ruling (2) rides on expresses **finer-than-district integer multiples only** (1 = district, 4 = quarter today; each new rung is a deliberate widening of `resolve_window_granularity`'s whitelist — the single widening point; unknown values fall back to district, never trusted from the wire). Coarser-than-district reuse (the region/orbital rungs of ruling (2)'s progressive tiling) requires the design pass's R5 signed/log-scale-or-enum redesign of the field — a new magic value is not the path. Recorded here so the type's limit is contract, not only a design-doc risk row. - **Rationale:** Reusing the real UI — rather than a parallel offline renderer or dumped files — means the debug/review surface never diverges from what ships, and a dropped artifact can't go stale. Agent-navigability converts qualitative "does the synthesis look natural?" review from a manual eyeball pass into an automatable sweep that flags the few outliers for a human. The harness rides seams that already exist (`TickRate::Paused`, the paused-allowlist, `gameplay_occluded`, the bridge framing, the `run-visual` capture primitive) — a naming-and-contract exercise, not a new subsystem. - **New surface:** server pause-gating (run-conditions on the world phases keyed to a pause command); client `AtlasAgentInterface` (`observe`/`act`, Control-tree walker) + its local transport; the generation overlay rendering + selector + legend; interactive capture wired to `run-visual`. - **Implementation:** Phase 4 (epic T-750), built bottom-up — auto-pause substrate, T-969 proxy (D-225), T-960 viewer, agent channel, agent capture. Geography is the first consumer. diff --git a/server/src/atlas/detail_scatter.rs b/server/src/atlas/detail_scatter.rs index 7f2e7d29f..86a3b19b2 100644 --- a/server/src/atlas/detail_scatter.rs +++ b/server/src/atlas/detail_scatter.rs @@ -22,7 +22,15 @@ use crate::seed::splitmix64; /// Mid-scale octave wavelengths in metres — the 2–40 km band. Coarsest first. /// Below the finest (~4 km) the district→voxel layers own the detail; above the /// coarsest (~33 km) the heightmap itself carries the shape. -const OCTAVE_WAVELENGTHS_M: [f64; 4] = [32_768.0, 16_384.0, 8_192.0, 4_096.0]; +/// +/// `pub(crate)`: also the quantization band set for `layer_proxy`'s +/// `window_min_wl_m` (T-1150, zoom ladder design doc §5 — "quantize +/// `min_wl_m` to a small fixed set of bands per rung, **matching the rung's +/// own octave bands**"). District/quarter windows both derive via +/// `terrain_detail`, so this IS "the rung's own octave bands" for both rungs +/// today — one array, no duplicated magic numbers that could drift out of +/// sync with the actual cutoff behavior. +pub(crate) const OCTAVE_WAVELENGTHS_M: [f64; 4] = [32_768.0, 16_384.0, 8_192.0, 4_096.0]; /// Voxel-tier octave wavelengths in metres — the ≈0.13–1 km **sub-district** band /// (all finer than the 2 km district planning unit) that the district-tier diff --git a/server/src/atlas/gen_queue.rs b/server/src/atlas/gen_queue.rs index ea26c32f8..780ae17d7 100644 --- a/server/src/atlas/gen_queue.rs +++ b/server/src/atlas/gen_queue.rs @@ -1144,8 +1144,26 @@ mod tests { // ------------------------------------------------------------------- /// Build a `DeriveWindow` work item pointing at a tiny test heightmap, - /// mirroring `analyze()`'s fixture shape. + /// mirroring `analyze()`'s fixture shape. `granularity` defaults to + /// district (matching every pre-T-1150 call site) via + /// `derive_window_at()` below — extended (PR #191 review, Hoshe 2) so + /// coalescing tests can exercise the granularity axis of + /// `window_supersede_key()` without a second near-duplicate helper. fn derive_window(body_id: &str, conn_id: ConnectionId, center: DistrictPos) -> GenWorkItem { + derive_window_at( + body_id, + conn_id, + center, + crate::atlas::layer_proxy::WINDOW_GRANULARITY_DISTRICT, + ) + } + + fn derive_window_at( + body_id: &str, + conn_id: ConnectionId, + center: DistrictPos, + granularity: u32, + ) -> GenWorkItem { GenWorkItem::DeriveWindow { body_id: body_id.to_string(), conn_id, @@ -1161,7 +1179,7 @@ mod tests { }), center, n: 4, - granularity: crate::atlas::layer_proxy::WINDOW_GRANULARITY_DISTRICT, + granularity, min_wl_m: 0, } } @@ -1282,6 +1300,82 @@ mod tests { ); } + /// **PR #191 review, Hoshe 2 — zero coverage before this test.** + /// `window_supersede_key()`'s doc claims district and quarter requests + /// for the SAME `(connection, body)` are separate in-flight slots (the + /// key is `(conn_id, body_id, granularity)`, not `(conn_id, body_id)`). + /// Two submissions for the same connection+body but DIFFERENT + /// granularity must NOT coalesce — both survive as independent pending + /// items. + #[test] + fn submit_window_does_not_coalesce_different_granularity() { + let q = GenerationQueue::with_threads(1); + // See `submit_window_coalesces_same_connection_and_body`'s comment on + // why the occupier must be `analyze()`, not `FillChunk`. + q.submit(analyze("Occupier3"), GenPriority::Low); + + let conn = ConnectionId(9); + q.submit_window( + derive_window_at( + "GranBody", + conn, + (0, 0), + crate::atlas::layer_proxy::WINDOW_GRANULARITY_DISTRICT, + ), + GenPriority::Immediate, + ); + q.submit_window( + derive_window_at( + "GranBody", + conn, + (0, 0), + crate::atlas::layer_proxy::WINDOW_GRANULARITY_QUARTER, + ), + GenPriority::Immediate, + ); + assert_eq!( + q.pending_count(), + 2, + "same (connection, body) but DIFFERENT granularity must NOT coalesce — \ + district and quarter are separate in-flight slots" + ); + } + + /// The coalescing-DOES-happen counterpart to the test above: two + /// submissions for the SAME `(connection, body, granularity)` still + /// collapse to one pending item — confirms the granularity axis didn't + /// accidentally loosen the existing same-key coalescing behavior. + #[test] + fn submit_window_coalesces_same_connection_body_and_granularity() { + let q = GenerationQueue::with_threads(1); + q.submit(analyze("Occupier4"), GenPriority::Low); + + let conn = ConnectionId(11); + q.submit_window( + derive_window_at( + "SameGranBody", + conn, + (0, 0), + crate::atlas::layer_proxy::WINDOW_GRANULARITY_QUARTER, + ), + GenPriority::Immediate, + ); + q.submit_window( + derive_window_at( + "SameGranBody", + conn, + (5, 5), + crate::atlas::layer_proxy::WINDOW_GRANULARITY_QUARTER, + ), + GenPriority::Immediate, + ); + assert_eq!( + q.pending_count(), + 1, + "same (connection, body, granularity) must still coalesce to one pending item" + ); + } + // ------------------------------------------------------------------- // TerrainAnalysisCache (T-1137, PR #187 review — Tyre C1) // ------------------------------------------------------------------- diff --git a/server/src/atlas/layer_proxy.rs b/server/src/atlas/layer_proxy.rs index 5281ee990..54c6d364d 100644 --- a/server/src/atlas/layer_proxy.rs +++ b/server/src/atlas/layer_proxy.rs @@ -66,6 +66,16 @@ pub const WIRE_CAP_CELLS: u32 = 4_096; /// Resolve a wire-supplied `window_granularity` value to one of the two legal /// granularities, clamping anything else down to district spacing — **never /// trust the wire** (same posture as `window_n`/`normalize_window_center`). +/// +/// **This is THE single widening point (Tyre C2, PR #191 review).** The +/// field only ever expresses finer-than-district integer multiples (see +/// [`AtlasLayerRequest::window_granularity`]'s doc for the full type-seam +/// contract); adding a future finer rung means adding its legal value here +/// and nowhere else. Do NOT add a value < 1 or attempt to encode +/// coarser-than-district rungs (region/orbital) through this function — +/// design doc §5/§9 R5 requires a signed/log-scale or enum redesign for that +/// direction, which this `u32` cannot express regardless of what this +/// function returns. fn resolve_window_granularity(raw: u32) -> u32 { if raw == WINDOW_GRANULARITY_QUARTER { WINDOW_GRANULARITY_QUARTER @@ -79,6 +89,16 @@ fn resolve_window_granularity(raw: u32) -> u32 { /// ([`WIRE_CAP_CELLS`]) — `window_n² × granularity² ≤ WIRE_CAP_CELLS` (T-1150 /// design doc §3). Applied AFTER the per-axis clamp so a request that already /// satisfies `DISTRICT_WINDOW_MAX_N` still shrinks further at granularity 4. +/// +/// **This clamp is echoed, not silently applied** — `serve_district_window` +/// puts the CLAMPED `n` into `DistrictWindowLayer.n`, so a client that +/// requests an oversized `n` gets back a smaller one. Any client-side +/// staleness guard comparing its own requested `n` against the echo MUST +/// mirror this exact function first (PR #191 review, Tyre C1) — see +/// `atlas_window_request.gd`'s `_clamp_window_n_mirror()`, which matches this +/// function bit-for-bit, the same load-bearing-mirror pattern +/// `canonicalize_district_center()` (`atlas_descend_geometry.gd`) already +/// uses for `normalize_window_center`. fn clamp_window_n(raw_n: u32, granularity: u32) -> u32 { let n = raw_n.clamp(1, DISTRICT_WINDOW_MAX_N); let g = granularity.max(1); @@ -86,6 +106,49 @@ fn clamp_window_n(raw_n: u32, granularity: u32) -> u32 { n.min(cap_n.floor().max(1.0) as u32) } +/// Quantized `window_min_wl_m` bands (T-1150, zoom ladder design doc §5): +/// `0` (no cutoff) plus every entry of +/// [`crate::atlas::detail_scatter::OCTAVE_WAVELENGTHS_M`] — the SAME array +/// `terrain_detail`'s octave sum truncates against (both district and +/// quarter rungs derive via `terrain_detail`, so this is genuinely "the +/// rung's own octave bands", not a second independently-chosen scale). +/// Descending order except the leading `0.0` sentinel, matched by +/// `quantize_min_wl_m`'s scan below. +const MIN_WL_BANDS_M: [f64; 5] = [ + 0.0, + crate::atlas::detail_scatter::OCTAVE_WAVELENGTHS_M[0], + crate::atlas::detail_scatter::OCTAVE_WAVELENGTHS_M[1], + crate::atlas::detail_scatter::OCTAVE_WAVELENGTHS_M[2], + crate::atlas::detail_scatter::OCTAVE_WAVELENGTHS_M[3], +]; + +/// Snap a wire-supplied `window_min_wl_m` to the nearest fixed band in +/// [`MIN_WL_BANDS_M`] (T-1150, design doc §5's gap-fix): "as specified, +/// `window_min_wl_m` is viewport-continuous while the cache key/echo tuple is +/// `(body, center, n, granularity)` — same key, different `min_wl`, would +/// silently collide. Fix: quantize `min_wl_m` to a small fixed set of bands +/// ... and add the quantized band to both the echo and the cache key." This +/// is that quantization, applied unconditionally to every request before it +/// touches either the cache key or the `DeriveWindow` work item — **never +/// the raw wire value past this point**, same discipline as `window_n`'s +/// clamp and `window_center`'s normalization. Nearest-band snap (ties round +/// to the coarser/lower band, i.e. `<=` on the running best distance) keeps +/// the mapping total and deterministic for any `u32` input, including values +/// far outside the octave range (e.g. `u32::MAX` snaps to the coarsest band). +fn quantize_min_wl_m(raw: u32) -> u32 { + let raw_f = raw as f64; + let mut best = MIN_WL_BANDS_M[0]; + let mut best_dist = (raw_f - best).abs(); + for &band in &MIN_WL_BANDS_M[1..] { + let dist = (raw_f - band).abs(); + if dist < best_dist { + best = band; + best_dist = dist; + } + } + best as u32 +} + /// A client request for a body's generation layers (D-225), extended with an /// optional district-resolution window query (D-226 T-1124 amendment §1, T-1137). /// @@ -115,15 +178,38 @@ pub struct AtlasLayerRequest { /// [`WINDOW_GRANULARITY_QUARTER`]. Resolved via /// [`resolve_window_granularity`] — **never trusted from the wire**, /// unrecognized values fall back to district. + /// + /// **Type seam (Tyre C2, PR #191 review):** this field expresses ONLY + /// finer-than-district integer multiples of the district spacing — `1` + /// and `4` are legal today, and each new finer rung (e.g. a future + /// block/tile value) is a deliberate widening of + /// [`resolve_window_granularity`]'s whitelist, the single point where + /// that widening happens. It CANNOT express coarser-than-district rungs + /// (region/orbital, granularity < 1) — reusing this field for those is + /// explicitly out of scope; design doc §5/§9 R5 requires a + /// signed/log-scale value or an explicit rung enum instead. Do not smuggle + /// a "granularity 0 means region" convention into this `u32` — that is + /// the redesign R5 already flags, not a value to add here. #[serde(default)] pub window_granularity: u32, /// Octave cutoff for the invented-terrain scatter (T-1149's /// `min_wavelength_m`), in whole metres. `0` (absent) = no cutoff = the - /// pre-T-1150 behavior. Threaded straight to `derive_at_metres` as - /// `min_wl as f64` — no client-side quantization band is enforced here - /// (the design doc's §5 quantized-band gap-fix is a client-request-shaping - /// concern; the server takes whatever whole-metre value it's given and - /// keys the cache on it verbatim, same posture as `window_n`). + /// pre-T-1150 behavior. + /// + /// **Quantization contract (Hoshe 1 / Tyre C3, PR #191 review; design doc + /// §5):** the wire value here is an UNQUANTIZED, unclamped raw passthrough + /// — client codecs may send any `u32`. The SERVER is the one place + /// quantization happens: `serve_district_window` snaps every request's + /// value to the nearest fixed band in + /// [`MIN_WL_BANDS_M`] via [`quantize_min_wl_m`] BEFORE it ever touches + /// the cache key or the `DeriveWindow` work item, and the QUANTIZED value + /// (not this raw field) is what gets echoed back on + /// `DistrictWindowLayer.min_wl_m` and used as the cache key component. + /// This closes the §5 gap: without quantization, two requests differing + /// only in a continuous-valued `min_wl_m` would silently miss each + /// other's cache entries (the unbounded-key-space problem §5 exists to + /// close) — the client is free to send a viewport-continuous estimate; + /// the server's quantization is what makes the key space bounded again. #[serde(default)] pub window_min_wl_m: u32, } @@ -1182,7 +1268,11 @@ fn serve_district_window( let raw_center = req.window_center?; let granularity = resolve_window_granularity(req.window_granularity); let n = clamp_window_n(req.window_n, granularity); - let min_wl_m = req.window_min_wl_m; + // T-1150 design doc §5: quantize BEFORE either the cache key or the + // DeriveWindow work item sees it — the raw wire value never reaches + // either (same discipline as window_n's clamp above and + // normalize_window_center's wrap/clamp below). + let min_wl_m = quantize_min_wl_m(req.window_min_wl_m); // body_params is needed to normalize the centre BEFORE either cache key // exists (T-1142) — read it first, unconditionally (not gated on a cache @@ -1971,6 +2061,67 @@ mod tests { ); } + /// **Contract-pinning test (PR #191 review, Tyre C1):** a quarter + /// (granularity=4) request for `n=32` echoes the WIRE-CAP-CLAMPED `n=16`, + /// not the requested 32 — `32² × 4² = 16,384` cells, 4x over + /// `WIRE_CAP_CELLS`. This is the exact scenario the review flagged as + /// silently breaking the client the moment T-1153 requests quarter at + /// n=32: the server echoes a DIFFERENT `n` than what was asked for, and + /// any client staleness guard comparing raw `_n` against the echo must + /// already know this will happen (see + /// `atlas_window_request.gd::_clamp_window_n_mirror()`, the client-side + /// fix landed alongside this test). + #[test] + fn quarter_n32_request_echoes_clamped_n16() { + let mut cache = BodyWorldStateCache::new(CACHE_CAPACITY); + let mut window_cache = DistrictWindowCache::new(DISTRICT_WINDOW_CACHE_CAPACITY); + let (_db, resolver, params_reader, _root) = resolver_and_params_reader("GJ1c"); + let queue = GenerationQueue::with_threads(1); + + let quarter_n32_req = AtlasLayerRequest { + body_id: "GJ1c".to_string(), + up_to: CascadeLayer::Topography, + window_center: Some((0, 0)), + window_n: 32, + window_granularity: WINDOW_GRANULARITY_QUARTER, + window_min_wl_m: 0, + }; + + let resp = handle_atlas_request( + &quarter_n32_req, + &mut cache, + &mut window_cache, + &queue, + &resolver, + None, + Some(¶ms_reader), + 42, + 1, + test_conn_id(), + ); + assert!(resp.district_window.is_none(), "first request — cache miss"); + + std::thread::sleep(Duration::from_millis(300)); + let completions = queue.drain_completions(); + let window_completion = completions.into_iter().find_map(|c| { + if let GenCompletion::WindowDerived { body_id, layer } = c { + if body_id == "GJ1c" { + return Some(layer); + } + } + None + }); + let layer = window_completion.expect("DeriveWindow must complete for GJ1c"); + assert_eq!( + layer.granularity, WINDOW_GRANULARITY_QUARTER, + "granularity must echo back as requested (4 is within budget on its own)" + ); + assert_eq!( + layer.n, 16, + "a quarter n=32 request must echo the wire-cap-clamped n=16, not the requested 32" + ); + } + // ------------------------------------------------------------------- // resolve_window_granularity / clamp_window_n (T-1150) // ------------------------------------------------------------------- @@ -2043,6 +2194,160 @@ mod tests { assert_eq!(clamp_window_n(8, WINDOW_GRANULARITY_QUARTER), 8); } + // ------------------------------------------------------------------- + // quantize_min_wl_m (T-1150, PR #191 review — Hoshe 1 / Tyre C3, design doc §5) + // ------------------------------------------------------------------- + + #[test] + fn quantize_min_wl_m_exact_band_values_are_stable() { + for &band in &MIN_WL_BANDS_M { + assert_eq!(quantize_min_wl_m(band as u32), band as u32); + } + } + + #[test] + fn quantize_min_wl_m_zero_stays_zero() { + assert_eq!(quantize_min_wl_m(0), 0); + } + + /// A value nearer to 0 than to the finest real octave band (4,096) snaps + /// to 0 (no cutoff) — the band set includes 0 as a real, selectable band, + /// not just a special-cased default. + #[test] + fn quantize_min_wl_m_small_value_snaps_to_zero_band() { + assert_eq!(quantize_min_wl_m(500), 0); + } + + /// A value between two real octave bands snaps to the NEAREST one, not + /// always up or always down. + #[test] + fn quantize_min_wl_m_mid_value_snaps_to_nearest_band() { + // Between 4,096 and 8,192: 5,000 is nearer 4,096 (dist 904 vs 3,192). + assert_eq!(quantize_min_wl_m(5_000), 4_096); + // 7,500 is nearer 8,192 (dist 692 vs 3,404). + assert_eq!(quantize_min_wl_m(7_500), 8_192); + } + + /// A value far above the coarsest band snaps to the coarsest band, never + /// panics or overflows — quantization must be a TOTAL function over all + /// u32 input (never trust the wire). + #[test] + fn quantize_min_wl_m_huge_value_snaps_to_coarsest_band() { + assert_eq!(quantize_min_wl_m(u32::MAX), 32_768); + assert_eq!(quantize_min_wl_m(1_000_000), 32_768); + } + + /// The mandatory §5 aliasing-closing test: two requests differing only in + /// an UNQUANTIZED `min_wl_m` that both fall in the SAME band must share + /// ONE cache entry, not two — this is the exact gap §5 flags ("same key, + /// different min_wl, would silently collide" becomes "same key, same + /// quantized min_wl, correctly coalesce"). + #[test] + fn two_requests_in_same_min_wl_band_share_one_cache_entry() { + let mut cache = BodyWorldStateCache::new(CACHE_CAPACITY); + let mut window_cache = DistrictWindowCache::new(DISTRICT_WINDOW_CACHE_CAPACITY); + let (_db, resolver, params_reader, _root) = + resolver_and_params_reader_with_radius("BandBody", 6371.0); + let queue = GenerationQueue::with_threads(2); + + // Both values are nearer 4,096 than any other band (4,000 and 4,300 + // both round to 4,096 — see the mid-value test above for the + // distance math), so they must land in the SAME quantized band. + let req_a = AtlasLayerRequest { + body_id: "BandBody".to_string(), + up_to: CascadeLayer::Topography, + window_center: Some((10, -5)), + window_n: 4, + window_granularity: WINDOW_GRANULARITY_DISTRICT, + window_min_wl_m: 4_000, + }; + let req_b = AtlasLayerRequest { + body_id: "BandBody".to_string(), + up_to: CascadeLayer::Topography, + window_center: Some((10, -5)), + window_n: 4, + window_granularity: WINDOW_GRANULARITY_DISTRICT, + window_min_wl_m: 4_300, + }; + + handle_atlas_request( + &req_a, + &mut cache, + &mut window_cache, + &queue, + &resolver, + None, + Some(¶ms_reader), + 42, + 1, + test_conn_id(), + ); + handle_atlas_request( + &req_b, + &mut cache, + &mut window_cache, + &queue, + &resolver, + None, + Some(¶ms_reader), + 42, + 1, + test_conn_id(), + ); + + std::thread::sleep(Duration::from_millis(300)); + let completions = queue.drain_completions(); + for c in completions { + if let GenCompletion::WindowDerived { body_id, layer } = c { + if body_id == "BandBody" { + window_cache.insert( + (body_id, layer.center, layer.n, layer.granularity, layer.min_wl_m), + *layer, + ); + } + } + } + + assert_eq!( + window_cache.len(), + 1, + "two requests in the SAME quantized min_wl_m band at identical \ + (body, center, n, granularity) must share ONE cache entry, not two" + ); + + // Re-request both — each must hit the SAME cached entry and echo the + // QUANTIZED band (4,096), not either raw wire value. + let resp_a = handle_atlas_request( + &req_a, + &mut cache, + &mut window_cache, + &queue, + &resolver, + None, + Some(¶ms_reader), + 42, + 2, + test_conn_id(), + ); + let resp_b = handle_atlas_request( + &req_b, + &mut cache, + &mut window_cache, + &queue, + &resolver, + None, + Some(¶ms_reader), + 42, + 2, + test_conn_id(), + ); + let layer_a = resp_a.district_window.expect("req_a must hit the cache"); + let layer_b = resp_b.district_window.expect("req_b must hit the cache"); + assert_eq!(layer_a.min_wl_m, 4_096, "echo must be the QUANTIZED band"); + assert_eq!(layer_b.min_wl_m, 4_096, "echo must be the QUANTIZED band"); + assert_eq!(layer_a, layer_b, "both requests must resolve to the identical cached layer"); + } + // ------------------------------------------------------------------- // normalize_window_center (T-1142 — letterbox-click out-of-range bug) // -------------------------------------------------------------------