diff --git a/client/README.md b/client/README.md index d097614..cb799e3 100644 --- a/client/README.md +++ b/client/README.md @@ -95,7 +95,7 @@ The main scene includes a **prototype terminal** at the map center (same world * `scripts/target_selection_client.gd` wires the server targeting API from [NEO-23](../docs/plans/NEO-23-implementation-plan.md) to a tab-cycle input and a HUD overlay. -- **Bindings** (in `project.godot`): **`target_tab`** → **Tab** cycles targets in the order defined by **`scripts/prototype_target_constants.gd`** (`prototype_target_alpha`, then `prototype_target_beta`; matches `PrototypeTargetRegistry.cs` ascending id), wrapping from last → first. **`target_clear`** → **Esc** POSTs with `targetId` omitted to clear the server lock. +- **Bindings** (in `project.godot`): **`target_tab`** → **Tab** cycles targets in the order defined by **`scripts/prototype_target_constants.gd`** (`prototype_target_alpha`, then `prototype_target_beta`; matches `PrototypeTargetRegistry.cs` ascending id), wrapping from last → first. When the player reference is wired (via `set_freshness_kick`, which `main.gd` does), Tab **skips client-computed out-of-range anchors** and selects the first in-range id in cycle order so pressing Tab while visibly standing next to an anchor actually locks it instead of trying the next-in-order id that happens to be across the map. Falls back to the plain cycle when nothing is in range so the server still picks the denial reason. **`target_clear`** → **Esc** POSTs with `targetId` omitted to clear the server lock. - **HUD:** `UICanvas/TargetLockLabel` shows the last **server-acknowledged** state: `lockedTargetId` (or **`—`** when no lock), `validity` (`none` / `ok` / `out_of_range` / `invalid_target`), `sequence`, one **display-only** line per known target in the form `: m / (in|out)` (computed client-side from the live capsule position against `prototype_target_constants.gd`), and (on denials) `Denied: ` on the last line. **UI never shows an optimistic lock the server denied** — it always paints from the authoritative `targetState` echoed by each `200` response. Distance lines are a lag diagnostic: if the HUD shows `Validity: ok` but `alpha: 10.3 m / 8.0 (out)`, the server's position snapshot is lagging the visible capsule, and the next `authoritative_ack` refresh will reconcile. `UICanvas/PlayerPositionLabel` also shows the last **server-acknowledged** world position + horizontal delta from the visible capsule — when that `Δ` is large, targeting denials are being evaluated against a stale server snapshot. - **Freshness kick:** `main.gd` calls `TargetSelectionClient.set_freshness_kick(authority, player)`, so each `POST /target/select` is preceded by a `submit_stream_targets([player.global_position])` on the `PositionAuthorityClient`. That nudges the server's stored position as close as possible to the visible capsule before the range check runs, collapsing the common "Tab right after stopping" false-denial race without waiting for the `move-stream` response. Tests can omit the wiring to run the client with an isolated mock transport. - **Visible anchors:** `scripts/prototype_target_markers.gd` (under `World/PrototypeTargetMarkers`) spawns a small colored mast + a flat translucent ring at every `ANCHORS` entry so the lock radius is visible in-world. The script and `prototype_target_constants.gd` mirror the server's `PrototypeTargetRegistry.cs` anchors/radii; change them together in the same commit so the markers never drift from server authority. diff --git a/client/scripts/prototype_target_constants.gd b/client/scripts/prototype_target_constants.gd index 803be22..d91f2b2 100644 --- a/client/scripts/prototype_target_constants.gd +++ b/client/scripts/prototype_target_constants.gd @@ -65,6 +65,37 @@ static func ordered_ids() -> Array[String]: return ORDERED_IDS.duplicate() +## Returns the next id in [constant ORDERED_IDS] that is **in range** of [param world] +## (client-side horizontal distance ≤ anchor radius), starting after [param current] and +## wrapping last → first. Returns [code]""[/code] when no anchor is in range so callers can +## fall back to [method next_id_after] and let the server pick the denial reason. +## +## Client math here is a UX convenience (same purpose as the HUD distance lines): the +## server remains authoritative for the actual lock decision in +## `TargetingApi.MapPost(".../target/select")`. If the client says "out" but the server +## says "in" at ~1 radius (float noise / position drift), the request still flows through +## the fallback on the next tick — no state gets stuck. +static func next_in_range_id_after(current: Variant, world: Vector3) -> String: + if ORDERED_IDS.is_empty(): + return "" + var start_idx: int = 0 + if current is String: + var current_str: String = (current as String).strip_edges() + if not current_str.is_empty(): + var found: int = ORDERED_IDS.find(current_str) + if found >= 0: + start_idx = (found + 1) % ORDERED_IDS.size() + for i in range(ORDERED_IDS.size()): + var probe_idx: int = (start_idx + i) % ORDERED_IDS.size() + var probe_id: String = ORDERED_IDS[probe_idx] + var radius: float = anchor_radius(probe_id) + if radius <= 0.0: + continue + if horizontal_distance_to(probe_id, world) <= radius: + return probe_id + return "" + + ## Returns the next id in [constant ORDERED_IDS] after [param current]. Returns the **first** id ## when [param current] is [code]null[/code], empty, or not found (start of cycle). Wraps from ## last → first. diff --git a/client/scripts/target_selection_client.gd b/client/scripts/target_selection_client.gd index 3e408fd..0168128 100644 --- a/client/scripts/target_selection_client.gd +++ b/client/scripts/target_selection_client.gd @@ -93,7 +93,17 @@ func request_sync_from_server() -> void: func request_tab_next() -> void: var current: Variant = _state.get("lockedTargetId", null) - var next_id: String = PrototypeTargetConstants.next_id_after(current) + var next_id: String = "" + # Prefer a client-side in-range pick when the player ref is wired so Tab locks the + # anchor the user is visibly standing next to instead of cycling to whatever happens + # to come first in `ORDERED_IDS`. Fall back to the plain cycle order (and let the + # server pick the denial reason) when nothing is in range or no player is wired. + if _freshness_player != null and is_instance_valid(_freshness_player): + next_id = PrototypeTargetConstants.next_in_range_id_after( + current, _freshness_player.global_position + ) + if next_id.is_empty(): + next_id = PrototypeTargetConstants.next_id_after(current) if next_id.is_empty(): return request_select_target_id(next_id) diff --git a/client/test/target_selection_client_test.gd b/client/test/target_selection_client_test.gd index ecfc193..afdac45 100644 --- a/client/test/target_selection_client_test.gd +++ b/client/test/target_selection_client_test.gd @@ -262,3 +262,53 @@ func test_select_post_without_freshness_wiring_is_a_noop() -> void: c.request_select_target_id(ALPHA_ID) assert_that(transport.last_url).contains("/target/select") assert_that(c.cached_state().get("lockedTargetId")).is_equal(ALPHA_ID) + + +func test_tab_from_no_lock_skips_out_of_range_and_picks_beta() -> void: + # Player stands next to beta (well outside alpha's 8 m ring). Without the range-aware + # tab cycle the server denies with `out_of_range` on alpha — see NEO-24 follow-up #3. + var transport := MockHttpTransport.new() + transport.enqueue( + HTTPRequest.RESULT_SUCCESS, 200, _select_response_json(true, BETA_ID, "ok", 1) + ) + var c := _make_client(transport) + var authority := FreshnessAuthorityStub.new() + auto_free(authority) + add_child(authority) + # Near beta anchor (8, 8), ~2.4 m horizontal — inside 4 m. Far from alpha at (-5, -5). + var player := _make_player(Vector3(5.9, 0.5, 9.2)) + c.set_freshness_kick(authority, player) + c.request_tab_next() + assert_that(transport.last_body).contains('"targetId":"%s"' % BETA_ID) + assert_that(c.cached_state().get("lockedTargetId")).is_equal(BETA_ID) + + +func test_tab_from_no_lock_with_nothing_in_range_falls_back_to_first_id() -> void: + # No anchors are in range → cycle-order fallback. Server owns the denial reason. + var transport := MockHttpTransport.new() + transport.enqueue( + HTTPRequest.RESULT_SUCCESS, + 200, + _select_response_json(false, null, "none", 0, "out_of_range") + ) + var c := _make_client(transport) + var authority := FreshnessAuthorityStub.new() + auto_free(authority) + add_child(authority) + var player := _make_player(Vector3(20.0, 0.5, 20.0)) + c.set_freshness_kick(authority, player) + c.request_tab_next() + assert_that(transport.last_body).contains('"targetId":"%s"' % ALPHA_ID) + assert_that(c.cached_state().get("reasonCode", "")).is_equal("out_of_range") + + +func test_tab_without_player_ref_still_uses_plain_cycle_order() -> void: + # Regression: headless unit tests and any client that skips `set_freshness_kick(...)` + # must keep the pre-existing cycle-order semantics. Alpha is requested from no-lock. + var transport := MockHttpTransport.new() + transport.enqueue( + HTTPRequest.RESULT_SUCCESS, 200, _select_response_json(true, ALPHA_ID, "ok", 1) + ) + var c := _make_client(transport) + c.request_tab_next() + assert_that(transport.last_body).contains('"targetId":"%s"' % ALPHA_ID) diff --git a/docs/manual-qa/NEO-24.md b/docs/manual-qa/NEO-24.md index 5112492..1a156d4 100644 --- a/docs/manual-qa/NEO-24.md +++ b/docs/manual-qa/NEO-24.md @@ -63,6 +63,7 @@ Pair the client (Godot **F5**) with the server (`cd server/NeonSprawl.Server && - `Validity: ok` - `Seq` increments by 1 - [ ] Press **Tab** once more. Server denies (alpha is far away) — HUD stays at `beta / ok` with `Denied: out_of_range` appended. +- [ ] **Range-aware cycle sanity:** press **Esc** to clear the lock (`Target: —`), then — while still inside beta's ring — press **Tab**. The client should skip alpha (out of range, ~18 m from here) and lock **beta** directly (`Target: prototype_target_beta`, `Validity: ok`). A plain cycle would pick alpha first and get denied with `out_of_range`; if that happens, the range-aware pick in `PrototypeTargetConstants.next_in_range_id_after` is not running (e.g. `set_freshness_kick` not wired). - [ ] **Freshness kick sanity:** walk to the edge of beta's ring, stop, and **immediately** press Tab. The select POST should succeed (`Validity: ok`) — the client kicks a `move-stream` with the current capsule position right before POSTing so the server is not validating against a stale 20 Hz sample. If this **denies** at a position visibly inside the ring, check `PlayerPositionLabel`'s `srv: …` line — a large `Δ` means the freshness kick is not landing before the select POST, and the race needs either a higher `move-stream` rate or a server-side client-position-hint contract. ## 6. Clear diff --git a/docs/plans/NEO-24-implementation-plan.md b/docs/plans/NEO-24-implementation-plan.md index ec1349c..9d61c86 100644 --- a/docs/plans/NEO-24-implementation-plan.md +++ b/docs/plans/NEO-24-implementation-plan.md @@ -107,3 +107,4 @@ No new **C#** tests (client-only story). **Bruno:** NEO-23 targeting requests al | 3 | **Signal for the refresh hook** | Review follow-up (see [2026-04-21-NEO-24 review](../reviews/2026-04-21-NEO-24.md)): the initial wiring hooked `authoritative_position_received`, which only fires on boot / rejection resync — normal `move-stream` 200s are intentionally silent to avoid RTT rubber-banding. A new `authoritative_ack(world)` signal was added to `PositionAuthorityClient` and emitted from both 200 paths so the movement-driven refresh is actually reachable during WASD. The snap signal kept its current semantics; the ack signal is pure heartbeat. | | 4 | **Post-merge UX: anchor visibility** | Users reported "targeting feels inconsistent" because the `PrototypeTargetRegistry` anchors (`alpha (-5,-5)` r=8, `beta (8,8)` r=4) have no visible meshes — the only visible console in the scene is the NEO-9 `PrototypeTerminal` at origin, which is unrelated to targeting. Added `client/scripts/prototype_target_markers.gd` + `PrototypeTargetMarkers` node in `main.tscn` (colored mast + translucent flat ring per anchor) and extended `TargetLockLabel` with per-anchor `: m / (in|out)` distance lines. Anchors/radii are mirrored in `prototype_target_constants.gd` as **display-only** data — server remains authoritative. If `PrototypeTargetRegistry.cs` moves, update both files in the same commit. | | 5 | **Post-merge UX: position-drift race** | Follow-up report: "when in range of beta, pressing Tab says denied." Root cause is a race between `move-stream` 20 Hz sampling and the target-select POST — a Tab press right after you stop moving is validated against the last sampled position, which can trail the visible capsule by several meters. Two changes: (a) `PlayerPositionLabel` now renders a `srv: (x,y,z) Δ=m age=` line from `PositionAuthorityClient.authoritative_ack`, making the server/client divergence directly visible; (b) `TargetSelectionClient.set_freshness_kick(authority, player)` (wired in `main.gd`) has the target client call `authority.submit_stream_targets([player.global_position])` immediately before every `POST /target/select`, so the server's stored snapshot is as fresh as possible when the range check runs. The freshness kick does not *wait* on the stream 200 — both requests race on separate HTTP connections — but it shrinks the window enough in practice that "stop, Tab, denied" stops firing. Both pieces are pure client changes; the server contract (NEO-23) is untouched. | +| 6 | **Post-merge UX: range-aware Tab cycle** | Follow-up report: "tab here does not acquire target" while standing 2.41 m from beta (inside 4 m) with `Δ=0` — server and client agreed on position but Tab still denied. Root cause: with no lock held, `request_tab_next()` returned `ORDERED_IDS[0]` = alpha (17.90 m away), not the anchor the user was visibly standing next to. Added `PrototypeTargetConstants.next_in_range_id_after(current, world)` which walks the cycle and returns the first anchor whose client-side horizontal distance ≤ radius; `TargetSelectionClient.request_tab_next()` prefers that pick when the player ref is wired (via the existing `set_freshness_kick` path) and falls back to `next_id_after(current)` when nothing is in range so the server still owns the denial reason. Pure client-side UX improvement; server contract untouched. | diff --git a/docs/reviews/2026-04-21-NEO-24.md b/docs/reviews/2026-04-21-NEO-24.md index 2c25849..93e0821 100644 --- a/docs/reviews/2026-04-21-NEO-24.md +++ b/docs/reviews/2026-04-21-NEO-24.md @@ -5,6 +5,8 @@ Scope: `NEO-24-e1m3-client-tab-target-lock-ui-synced-to-server` (`origin/main... Base: `origin/main` (merge-base `37f44a494400f4fdd369077c52dfc012df73a5d5`) Follow-up: blocking issue + suggestions below are **done** (strikethrough + **Done.**). See the [NEO-24 plan](../plans/NEO-24-implementation-plan.md) Decision 3 for the signal change. +**Post-merge UX follow-up #3 (2026-04-21):** user reported "tab here does not acquire target" while standing 2.41 m from beta (well inside the 4 m ring) with `Δ=0.00m age=2357ms` — i.e. client and server agreed on position. Server denied with `out_of_range`. Root cause was the Tab cycle: with no lock held, `request_tab_next()` returned `ORDERED_IDS[0]` = `prototype_target_alpha`, which was 17.90 m away. The user was visibly next to `beta` but Tab was always trying `alpha` first. Fix: `PrototypeTargetConstants.next_in_range_id_after(current, world)` walks the cycle and returns the first anchor whose client-side horizontal distance ≤ radius; `TargetSelectionClient.request_tab_next()` prefers that pick when the player ref is wired and falls back to `next_id_after(current)` when nothing is in range (so the server still owns the denial reason). Server contract untouched. Tests: `test_tab_from_no_lock_skips_out_of_range_and_picks_beta`, `test_tab_from_no_lock_with_nothing_in_range_falls_back_to_first_id`, `test_tab_without_player_ref_still_uses_plain_cycle_order`. + **Post-merge UX follow-up #2 (2026-04-21):** user reported "when in range of beta, pressing Tab says denied" — HUD distance line confirmed `prototype_target_beta: 0.58 m / 4.0 (in)` but server denied with `out_of_range` (and `prototype_target_alpha: 18.95 m` — almost exactly the spawn-to-current distance, strongly suggesting the server's stored position was still near spawn). Root cause is a race between the `move-stream` 20 Hz sampler and the target-select POST: a Tab press right after you stop moving validates against the last sampled position, which can trail the visible capsule. Two changes: - `PlayerPositionLabel` now shows the server-acknowledged position + horizontal Δ + ack age (cached from `PositionAuthorityClient.authoritative_ack` via a new `_on_authoritative_ack_for_hud` handler). Makes the divergence directly visible in future repro.