From 4eed3bccb218f60daa30870875e21a7ffe3c6d42 Mon Sep 17 00:00:00 2001 From: Jeroen Schweitzer Date: Tue, 21 Jul 2026 13:57:28 +0200 Subject: [PATCH] fix(ui): latitude-inverse denominator + reticle city-hover gate (PR #187 findings) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hoshe's blocking find: district_pos_at divided the latitude axis by tex_h where the server forward map uses ta.h.saturating_sub(1) (district_profile.rs:1436; ground-truth inverse aliveness_probe.rs:511) — every off-equator click descended into the wrong district, up to ~20km drift, live-repro'd at 10.2km. Fixed to /(tex_h - 1.0) with the reference implementation's degenerate-texture guard; verified against the repro (row 50: old formula recovered 41, fixed recovers 50 exact). Columns were already correct (longitude wraps — the asymmetry the docstring documented but the code didn't implement). Regression tests transcribe the server forward formula (source cited) and round-trip three off-equator rows — one per hemisphere plus near-pole — asserting EXACT recovery; a dedicated drift guard is framed as Tyre's C2 (a client-side twin of a server mapping owns its own round-trip proof). Root-caused the coverage hole: the old equator test sampled tex_h*0.5 believing it was the equator pixel — the true equator is (tex_h-1)*0.5, so the test never verified what it claimed; corrected with the why documented. Araminta's find: the descend reticle drew unconditionally on hover — over a city the marker flared white (city-click signal) while the reticle+extent label promised descent, though the click correctly resolved to city data. Extracted _should_draw_descend_reticle() with the missing _hovered_city.is_empty() clause; three state tests pin shows-over-map / hides-over-city / hides-when-idle. test_atlas_descend_entry 32/32; adjacent clusters no ripple; gdlint clean (atlas_viewer.gd at exactly the 1000-line cap). Co-Authored-By: Claude Fable 5 --- client/tests/test_atlas_descend_entry.gd | 156 +++++++++++++++++- .../apps/atlas/atlas_descend_geometry.gd | 44 +++-- client/ui/implant/apps/atlas/atlas_viewer.gd | 13 +- 3 files changed, 192 insertions(+), 21 deletions(-) diff --git a/client/tests/test_atlas_descend_entry.gd b/client/tests/test_atlas_descend_entry.gd index dde9e334f..56d85843c 100644 --- a/client/tests/test_atlas_descend_entry.gd +++ b/client/tests/test_atlas_descend_entry.gd @@ -83,16 +83,29 @@ func test_district_pos_at_zero_texture_size_is_safe() -> void: ## The equatorial center of the texture (px = tex_w/2) is district column ## district_cols/2 on a body with a radius (equator wraps at district (0,0) ## per the server's own doc: "district (0,0) sits at lon 0 / the equator"). -## The vertical center (py = tex_h/2) is the equator row (row 0). +## +## The TRUE equator pixel row is `(tex_h - 1) * 0.5`, NOT `tex_h * 0.5` (PR +## #187 review, Hoshe): the server's forward map (district_profile.rs:1436) +## divides by `ta.h.saturating_sub(1)`, so row 0 and row (h-1) are the real +## clamped pole endpoints and the midpoint between them is `(h-1)/2`. This +## test originally sampled `tex_h * 0.5`, which is NOT that midpoint for any +## finite tex_h — it happened to still round to district row 0 under BOTH the +## pre-fix buggy formula and the fix, at this specific radius/tex_h pair, +## which is exactly why this equator-only sample masked the ÷tex_h bug +## instead of catching it (see the off-equator round-trip tests below for the +## real regression coverage). Corrected here to the genuine equator pixel so +## this test asserts something true about the fixed formula, not a +## coincidence of the old one. func test_district_pos_at_center_of_texture_is_near_equator_row_zero() -> void: var radius_km := 6371.0 var tex_w := 1024.0 var tex_h := 512.0 + var true_equator_py: float = (tex_h - 1.0) * 0.5 var pos: Vector2i = AtlasDescendGeometry.district_pos_at( - Vector2(0.0, tex_h * 0.5), tex_w, tex_h, radius_km + Vector2(0.0, true_equator_py), tex_w, tex_h, radius_km ) assert_int(pos.y).override_failure_message( - "the vertical texture center must map to row 0 (the equator)" + "the true equator pixel ((tex_h-1)/2) must map to row 0" ).is_equal(0) @@ -116,6 +129,88 @@ func test_district_pos_at_sweeps_full_column_range_across_texture_width() -> voi assert_int(absi(right.x - left.x)).is_greater(1000) +# ============================================================================= +# Off-equator round-trip (PR #187 review, Hoshe — BLOCKING, live-repro'd bug): +# district_pos_at's row inverse divided by tex_h instead of (tex_h - 1), +# matching the WRONG symmetry with the column inverse (which correctly +# divides by the plain tex_w, since longitude wraps and has no edge case). +# The server's forward map (district_profile.rs:1436) uses +# ta.h.saturating_sub(1) for the SAME reason the ground-truth inverse +# (aliveness_probe.rs:511, `row / (ta_h - 1) - 0.5`) does: latitude CLAMPS at +# the poles, so row 0 / row (h-1) are real endpoints the division must land +# on exactly. This class of bug — a client-side twin of a server coordinate +# mapping silently drifting out of sync — is exactly what Tyre's C2 review +# criterion requires a round-trip drift guard for; that is what every test in +# this section is. The suite previously missed it because the only +# real-radius latitude sample was py = tex_h*0.5 (test above), the ONE point +## where the buggy (÷tex_h) and correct (÷(tex_h-1)) formulas round to the +# same integer district row — never exercising the asymmetry the fix targets. +# ============================================================================= + + +## Forward-maps a district row to its heightmap pixel row using the server's +## OWN formula, transcribed here (district_profile.rs:1436): +## let lat_frac = (dy * DISTRICT_M / meridian_m).clamp(-0.5, 0.5); +## let py = (0.5 + lat_frac) * ta.h.saturating_sub(1); +## `dy` is a DistrictPos.y component (not a pixel) — the caller supplies the +## same `district_rows_half`-scaled value district_pos_at()'s inverse would +## need to recover, so this function and district_pos_at() are meant to be +## exact inverses of one another for any row within the clamped range. +static func _server_forward_map_row(district_row: int, tex_h: float, radius_km: float) -> float: + var meridian_m: float = PI * radius_km * 1000.0 + var district_m: float = AtlasDescendGeometry.DISTRICT_M + var lat_frac: float = clampf((float(district_row) * district_m) / meridian_m, -0.5, 0.5) + return (0.5 + lat_frac) * (tex_h - 1.0) + + +## The actual regression: forward-map three off-equator district rows (one +## per hemisphere, plus a near-pole extreme) to pixel rows via the server's +## transcribed formula, then assert district_pos_at() recovers each EXACT +## row from that pixel. Fails outright under the pre-fix ÷tex_h formula for +## every row here except (coincidentally) very near the equator. +func test_district_pos_at_round_trips_off_equator_rows_against_server_forward_map() -> void: + var radius_km := 6238.4 # GJ380c (server/data/systems.db) — a real body, not a round number + var tex_w := 1024.0 + var tex_h := 512.0 + var col := 400 # arbitrary — this test is about the row axis only + + # One row per hemisphere (moderate latitude) + one extreme near-pole row. + # meridian_m / DISTRICT_M ≈ 9,569 for this radius, so district_rows_half + # ≈ 4,785 — these rows sit well inside that range without saturating the + # clamp (except the "near pole" case, deliberately close to the edge). + var test_rows: Array = [-1200, 900, -4700] + + for district_row: int in test_rows: + var forward_py: float = _server_forward_map_row(district_row, tex_h, radius_km) + var recovered: Vector2i = AtlasDescendGeometry.district_pos_at( + Vector2(float(col), forward_py), tex_w, tex_h, radius_km + ) + assert_int(recovered.y).override_failure_message( + ( + "row round-trip failed: district_row=%d -> forward pixel py=%.4f -> " + + "district_pos_at recovered row=%d (drift=%d)" + ) % [district_row, forward_py, recovered.y, recovered.y - district_row] + ).is_equal(district_row) + + +## Same round-trip, restated as an explicit non-equator drift guard: the +## pre-fix bug produced a WRONG but still-integer row (Hoshe's live repro: +## forward row 50, buggy inverse returned 45 — a 5-district, ~10.2 km drift) +## — a coarse "is it roughly right" tolerance would have passed that. This +## asserts EXACT equality specifically at a moderate off-equator row, not an +## approximate one, so a reintroduced ÷tex_h regression fails loudly again. +func test_district_pos_at_off_equator_row_is_not_off_by_a_few_districts() -> void: + var radius_km := 6238.4 + var tex_w := 1024.0 + var tex_h := 512.0 + var district_row := 2500 + var forward_py: float = _server_forward_map_row(district_row, tex_h, radius_km) + var recovered: Vector2i = AtlasDescendGeometry.district_pos_at( + Vector2(512.0, forward_py), tex_w, tex_h, radius_km + ) + assert_int(recovered.y).is_equal(district_row) + + # ============================================================================= # AtlasViewer — fixed view (no drag-pan/wheel-zoom) + click-through descent # ============================================================================= @@ -160,3 +255,58 @@ func test_regional_screen_forwards_district_descend_requested() -> void: screen._viewer.district_descend_requested.emit(Vector2i(5, 7)) assert_int(received.size()).is_equal(1) assert_that(received[0]).is_equal(Vector2i(5, 7)) + + +# ============================================================================= +# PR #187 review (Araminta — broken promise): the descend reticle must be +# hidden while hovering a city marker — _hovered_city already flares white as +# the "click here for city data" signal, and _try_click_city wins the click +# over descent, so drawing the reticle at the same time promised a descent +# the click would never perform. State-level tests (per the review's own +# "state-level is fine" allowance): assert _should_draw_descend_reticle()'s +# condition directly against _hover_active/_hovered_city/_heightmap_texture, +# rather than a pixel-diff on the actual draw call. +# ============================================================================= + + +static func _one_pixel_texture() -> ImageTexture: + var img := Image.create(1, 1, false, Image.FORMAT_RGB8) + return ImageTexture.create_from_image(img) + + +## Hovering EMPTY map (no city under the cursor) -> reticle SHOWS. +func test_descend_reticle_shows_when_hovering_empty_map() -> void: + var v: AtlasViewer = auto_free(AtlasViewer.new()) + add_child(v) + v._heightmap_texture = _one_pixel_texture() + v._hover_active = true + v._hovered_city = {} + assert_bool(v._should_draw_descend_reticle()).override_failure_message( + "reticle must show when hovering the open map (no city under cursor)" + ).is_true() + + +## Hovering a CITY marker -> reticle HIDES, even though _hover_active is true +## (this is the exact bug: pre-fix, _hovered_city was never consulted here). +func test_descend_reticle_hides_when_hovering_a_city() -> void: + var v: AtlasViewer = auto_free(AtlasViewer.new()) + add_child(v) + v._heightmap_texture = _one_pixel_texture() + v._hover_active = true + v._hovered_city = {"name": "Ridgeback", "pos": [10, 20]} + assert_bool(v._should_draw_descend_reticle()).override_failure_message( + "reticle must hide while hovering a city — the click would open city" + + " data (_try_click_city wins), not descend" + ).is_false() + + +## Not hovering at all (_hover_active false) -> reticle HIDES regardless of +## _hovered_city — the pre-existing base condition, guarded here so the city +## fix can't accidentally invert it. +func test_descend_reticle_hides_when_not_hovering() -> void: + var v: AtlasViewer = auto_free(AtlasViewer.new()) + add_child(v) + v._heightmap_texture = _one_pixel_texture() + v._hover_active = false + v._hovered_city = {} + assert_bool(v._should_draw_descend_reticle()).is_false() diff --git a/client/ui/implant/apps/atlas/atlas_descend_geometry.gd b/client/ui/implant/apps/atlas/atlas_descend_geometry.gd index 4202052f3..469aa5fb2 100644 --- a/client/ui/implant/apps/atlas/atlas_descend_geometry.gd +++ b/client/ui/implant/apps/atlas/atlas_descend_geometry.gd @@ -80,17 +80,30 @@ static func reticle_label(center: Vector2) -> Dictionary: ## function, per the amendment's §5 carry-over wording. The server's forward ## mapping (body has a radius) is: ## px = ((dx * DISTRICT_M) / circumference_m mod 1.0) * tex_w -## py = (0.5 + clamp(dy * DISTRICT_M / meridian_m, -0.5, 0.5)) * tex_h -## which is a linear scaling of the same world-metre fraction the equatorial/ -## meridian district COUNT already IS (district_cols = round(circumference_m -## / DISTRICT_M), the same value build_district_grid()'s `cols` converges to -## for a body tiled edge-to-edge) — so inverting is "pixel fraction * district -## count", not a re-derivation of the server's geodesy. Self-contained: does -## NOT depend on district_grid (the whole-body layer) having arrived yet, so -## descent works immediately on entry even before that async layer resolves. -## The "no radius" branch (tiny test bodies, body_radius_km absent/<=0) -## mirrors the server's own fallback: the district grid IS the heightmap -## grid 1:1. +## py = (0.5 + clamp(dy * DISTRICT_M / meridian_m, -0.5, 0.5)) * ta.h.saturating_sub(1) +## (district_profile.rs:1436 — note `.saturating_sub(1)`, NOT a bare `ta.h`; +## confirmed against the ground-truth inverse aliveness_probe.rs:511 too: +## `row / (ta_h - 1) - 0.5`). Columns and rows are DELIBERATELY asymmetric: +## longitude WRAPS (rem_euclid), so a column has no "last pixel" edge case and +## divides by the plain width; latitude CLAMPS at the poles, so row 0 and row +## (h-1) are real, distinct endpoints (the north/south pole pixels) and the +## division must land exactly on them — dividing by `tex_h` instead of +## `tex_h - 1` introduces a systematic drift that grows with |lat_frac| +## (worst at the poles, invisible only at the exact equator row, `tex_h*0.5`, +## where the two formulas round identically). This is why inverting is "pixel +## fraction * district count" for columns but NOT a symmetric operation for +## rows — the row inverse must undo the SAME -1 the forward map applied. +## +## The (wx/circumference_m, wy/meridian_m) fractions are linear scalings of +## the same world-metre quantity the equatorial/meridian district COUNT +## already IS (district_cols = round(circumference_m / DISTRICT_M), the same +## value build_district_grid()'s `cols` converges to for a body tiled +## edge-to-edge) — so inverting is "pixel fraction * district count", not a +## re-derivation of the server's geodesy. Self-contained: does NOT depend on +## district_grid (the whole-body layer) having arrived yet, so descent works +## immediately on entry even before that async layer resolves. The "no +## radius" branch (tiny test bodies, body_radius_km absent/<=0) mirrors the +## server's own fallback: the district grid IS the heightmap grid 1:1. static func district_pos_at( canvas_pt: Vector2, tex_w: float, tex_h: float, body_radius_km: float ) -> Vector2i: @@ -103,5 +116,12 @@ static func district_pos_at( var district_cols: float = roundf(circumference_m / DISTRICT_M) var district_rows_half: float = roundf((meridian_m / DISTRICT_M) * 0.5) var col: int = roundi((canvas_pt.x / tex_w) * district_cols) - var row: int = roundi(((canvas_pt.y / tex_h) - 0.5) * district_rows_half * 2.0) + # tex_h - 1.0, matching the forward map's ta.h.saturating_sub(1) — NOT a + # bare tex_h (see the docstring above; this was a live bug, T-1138 PR #187 + # review, Hoshe: every off-equator click descended into the wrong district). + # Guarded the same way the server's own inverse (aliveness_probe.rs:510, + # `if ta_h > 1 { ... } else { 0.0 }`) guards the same division — a + # degenerate 1px-tall texture would otherwise divide by zero. + var lat_frac: float = ((canvas_pt.y / (tex_h - 1.0)) - 0.5) if tex_h > 1.0 else 0.0 + var row: int = roundi(lat_frac * district_rows_half * 2.0) return Vector2i(col, row) diff --git a/client/ui/implant/apps/atlas/atlas_viewer.gd b/client/ui/implant/apps/atlas/atlas_viewer.gd index c08e299c3..44e9be4e3 100644 --- a/client/ui/implant/apps/atlas/atlas_viewer.gd +++ b/client/ui/implant/apps/atlas/atlas_viewer.gd @@ -16,8 +16,7 @@ extends Control ## - Empty markers case (server #832/#833 not yet shipped): bare heightmap renders ## fine, no sidebar opens, overlays draw nothing. ## - Overlays (#836) plug into _overlay_visibility dict and _draw_overlays(). -## - T-1138: view is FIXED (set_view() is capture-API-only); descent screen has the reticle math. -## Navigation: Hover reticle · Click descend · Click city data (wins) · R reset(no-op) · Esc back +## Navigation (view FIXED, T-1138): hover reticle (hidden/city) · click descend/city · esc back signal back_pressed signal economics_link_requested(system_id: String) @@ -645,13 +644,15 @@ func screen_to_canvas(screen_point: Vector2) -> Vector2: func _draw() -> void: draw_rect(Rect2(Vector2.ZERO, get_rect().size), COLOR_BG) - if _hover_active and _heightmap_texture != null: + if _should_draw_descend_reticle(): _draw_descend_reticle() -## T-1138 NOT-TO-SCALE reticle (§5's open design point). Draw calls only — -## geometry/rationale in atlas_descend_geometry.gd (draw_line must run on `self`). -func _draw_descend_reticle() -> void: +func _should_draw_descend_reticle() -> bool: # hidden over a city — _try_click_city wins the click + return _hover_active and _heightmap_texture != null and _hovered_city.is_empty() + + +func _draw_descend_reticle() -> void: # draw calls only — geometry in atlas_descend_geometry.gd var center: Vector2 = _hover_screen_pos for seg: Array in AtlasDescendGeometry.reticle_segments(center): draw_line(seg[0], seg[1], AtlasDescendGeometry.COLOR_DESCEND_RETICLE, 1.5)