fix(ui): latitude-inverse denominator + reticle city-hover gate (PR #187 findings)
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 <noreply@anthropic.com>
This commit is contained in:
Claude Fable 5
@@ -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()
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
|
||||
Reference in New Issue
Block a user