From 8a399821462484bec90525bfd9de1bbafabd8f3a Mon Sep 17 00:00:00 2001 From: VinPropane Date: Tue, 21 Apr 2026 21:58:13 -0400 Subject: [PATCH 01/13] NEO-24: add implementation plan for client tab-target UI --- docs/plans/NEO-24-implementation-plan.md | 97 ++++++++++++++++++++++++ 1 file changed, 97 insertions(+) create mode 100644 docs/plans/NEO-24-implementation-plan.md diff --git a/docs/plans/NEO-24-implementation-plan.md b/docs/plans/NEO-24-implementation-plan.md new file mode 100644 index 0000000..91c057a --- /dev/null +++ b/docs/plans/NEO-24-implementation-plan.md @@ -0,0 +1,97 @@ +# NEO-24 — Implementation plan + +## Story reference + +| Field | Value | +|--------|--------| +| **Key** | NEO-24 | +| **Title** | E1.M3: Client tab-target + lock UI synced to server | +| **Linear** | [NEO-24](https://linear.app/neon-sprawl/issue/NEO-24/e1m3-client-tab-target-lock-ui-synced-to-server) | +| **Slug** | E1M3-02 | +| **Git branch** | `NEO-24-e1m3-client-tab-target-lock-ui-synced-to-server` | +| **Parent context** | [Epic 1 — Core Player Runtime](https://linear.app/neon-sprawl/project/epic-1-core-player-runtime-client-controls-character-loop-66bd590cd016) · [E1M3 prototype backlog](E1M3-prototype-backlog.md) | +| **Depends on** | [NEO-23](NEO-23-implementation-plan.md) (targeting HTTP v1) | +| **Decomposition** | [E1.M3 — InteractionAndTargetingLayer](../decomposition/modules/E1_M3_InteractionAndTargetingLayer.md); authority notes: [client_server_authority — E1.M3](../decomposition/modules/client_server_authority.md#e1m3-interactionandtargetinglayer) | + +## Goal, scope, and out-of-scope + +**Goal:** **Tab** (or agreed input action) cycles **server-eligible** stub targets in a **documented deterministic order**; **HUD / debug overlay** reflects the last **server-acknowledged** `TargetState` (`lockedTargetId`, `validity`, `sequence`). Client sends **selection intent** via existing **`POST …/target/select`**; **optional optimistic** presentation **must** reconcile to the **authoritative** `targetState` in each **200** response (including denials), per E1.M3 authority doc. + +**In scope** + +- Godot: input → **`TargetSelectRequest`** v1 → parse **`TargetSelectResponse`** / **`PlayerTargetStateResponse`**; maintain **last acknowledged** target state for UI. +- **Tab cycle order:** match server stub contract — lexicographic ascending id over the **prototype registry ids** (`prototype_target_alpha`, then `prototype_target_beta`), wrapping from last → first; same ordering as [server README — Targeting](../../server/README.md#targeting-neo-23) / `PrototypeTargetRegistry` comments. +- **Clear lock:** **`POST`** with **`targetId`** omitted or JSON **`null`** (NEO-23 rule); binding documented in `client/project.godot` (proposed: **Esc** via `target_clear`). +- **UI:** readable label (or small overlay) showing **server** lock id (or **none** / empty), **`validity`**, and optionally **`sequence`** for debugging. +- **Soft lock / movement:** when the player moves, **`validity`** can become **`out_of_range`** without clearing **`lockedTargetId`**. Client must **refresh** authoritative state periodically or on hooks so the overlay stays truthful (see Technical approach). +- **Shared constants:** duplicate **stub target id list + order** in a small GDScript constants module aligned with `PrototypeTargetRegistry` (NEO-23 plan expectation for NEO-24 preview/cycle). + +**Out of scope (per Linear / backlog)** + +- Nearest-in-cone auto-target; sticky soft-target under latency. +- **`AbilityCastRequest`** / hotbar — [E1.M4](../decomposition/modules/E1_M4_AbilityInputScaffold.md). +- Server route or DTO changes (covered by NEO-23); **no** new ASP.NET endpoints unless a client bug reveals a contract gap (then narrow fix + plan update). + +## Acceptance criteria checklist + +- [ ] With **≥2** stub targets, **tab** advances selection intent in **ascending id** order and **wraps** at the end of the ordered list. +- [ ] After each successful **round-trip**, UI shows **`lockedTargetId`** (or equivalent **none**) and **`validity`** from the **response body** (`targetState` on POST, body on GET). +- [ ] On **`selectionApplied`: `false`**, UI shows **authoritative** `targetState` from the same response (no “stuck optimistic” lock id that the server denied). +- [ ] **Clear** action clears lock server-side and UI updates from the response. +- [ ] Input bindings for **tab cycle** and **clear** are documented in **`client/project.godot`** (and briefly in **`client/README.md`** if other prototype keys are documented there). + +## Technical approach + +1. **`prototype_target_constants.gd` (or rename if clearer):** export ordered `Array[String]` of stub ids matching `PrototypeTargetRegistry` (`alpha`, `beta`) for tab math only; single source for “eligible cycle list” on the client. + +2. **`target_selection_client.gd`:** Node analogous to `interaction_request_client.gd` / `position_authority_client.gd`: owns injectable HTTP transport, `base_url`, `dev_player_id`, `_busy` guard. Responsibilities: + - **`request_sync_from_server()`** — `GET …/target` → parse `PlayerTargetStateResponse` v1 → cache + emit signal. + - **`request_select_target_id(target_id: String)`** — `POST …/target/select` with body `{"schemaVersion":1,"targetId":…}`. + - **`request_clear_target()`** — `POST` with no `targetId` / `null` per NEO-23. + - **`request_tab_next()`** — read cached `lockedTargetId` + `validity`; compute **next** id in the ordered list (if no lock, choose **first** id in order); POST select; on deny, signal carries **server** `targetState` only (no client-only lock). + - Signal e.g. **`target_state_changed(state: Dictionary)`** or typed fields for consumers (`lockedTargetId` Variant string-or-null, `validity` string, `sequence` int, `selection_applied` bool optional for last POST). + - **Polling:** while last known `lockedTargetId` is non-null, issue **`GET …/target`** on a **throttled** timer (e.g. every **0.25–0.5 s**) so **`out_of_range`** updates after locomotion without requiring new tab presses. Stop or slow polling when no lock if desired for noise (document choice in plan **Decisions** during implementation). + +3. **Optimistic highlight (optional slice):** If implemented, keep **pending** selection separate from **acknowledged** state; on **200** denial, drop pending and paint from **`targetState`**. If timeboxed, **v1** may ship **ack-only** UI (still satisfies “optional optimistic” — none used). + +4. **`main.tscn` / `main.gd`:** Add child node with `target_selection_client.gd`; connect signal to a **Label** (new `TargetLockLabel` under `UICanvas`). Call **`request_sync_from_server()`** once after boot position sync (or same frame as first authority ready) so HUD matches server on start. + +5. **`project.godot`:** `target_tab` → **Tab**; `target_clear` → **Escape** (document; user may request alternatives before implementation). + +6. **Manual QA:** Run server + client; tab between stubs; walk out of radius and confirm **`validity`** flips to **`out_of_range`** on overlay while id remains; clear with Esc. + +## Files to add + +| Path | Purpose | +|------|---------| +| `client/scripts/prototype_target_constants.gd` | Ordered stub **`targetId`** list + helpers for “next in cycle” matching server registry. | +| `client/scripts/target_selection_client.gd` | HTTP GET/POST targeting APIs; tab/clear entrypoints; throttled refresh; signals for UI. | +| `client/test/target_selection_test_double.gd` | Test subclass injecting mock HTTP (pattern from `position_authority_test_double.gd`). | +| `client/test/target_selection_client_test.gd` | GdUnit4: tab order wrap, POST success path, denial keeps server `lockedTargetId`, clear POST, GET parse. | + +## Files to modify + +| Path | Rationale | +|------|-----------| +| `client/scenes/main.tscn` | Add `TargetSelectionClient` node + **`TargetLockLabel`** (or reuse a debug panel) bound to scripts. | +| `client/scripts/main.gd` | Wire `@onready` target client + label; connect `target_state_changed`; trigger initial `request_sync_from_server()` after authority boot path. | +| `client/project.godot` | Register **`target_tab`** and **`target_clear`** input actions with defaults above. | +| `client/README.md` | One short subsection: tab-target prototype, bindings, and how to read the lock overlay (parity with interaction/move docs). | + +## Tests + +| Test file | What to cover | +|-----------|----------------| +| `client/test/target_selection_client_test.gd` | **Mock transport** enqueues JSON: GET returns a v1 envelope; POST **apply** returns `selectionApplied: true` + `targetState`; POST **deny** returns `selectionApplied: false` + `reasonCode` + authoritative `targetState`; **tab** from no lock selects first id in order; **tab** from `alpha` requests `beta`; wrap from `beta` to `alpha`; **clear** issues POST without `targetId`. Use `MockHttpTransport` pattern from `position_authority_client_test.gd`. | + +No new **C#** tests (client-only story). **Bruno:** NEO-23 targeting requests already live under `bruno/neon-sprawl-server/targeting/`; **no** new `.bru` required unless implementation discovers a **server** contract change (then add per repo [testing expectations](../../.cursor/rules/testing-expectations.md) for HTTP contract changes). + +## Open questions / risks + +- **Input bindings:** Plan assumes **Tab** + **Esc**. Confirm or specify alternatives (e.g. mouse thumb buttons) before locking `project.godot`. +- **Polling cost:** Throttled GET while locked is simple but adds HTTP traffic; acceptable for prototype. If objection, switch to polling only when HUD visible or tie to `move-stream` cadence with plan update. +- **Scene visuals:** This story does **not** require 3D reticles on stub meshes; overlay text satisfies AC. World-space highlights can be a follow-up. + +## Decisions + +*(None locked yet — update this table during implementation chat / PR.)* From ddfc78a0448b864e933d7832e9018aa7a5858a54 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Tue, 21 Apr 2026 22:02:33 -0400 Subject: [PATCH 02/13] NEO-24: lock kickoff decisions (Tab/Esc; hybrid movement-triggered refresh) --- docs/plans/NEO-24-implementation-plan.md | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/docs/plans/NEO-24-implementation-plan.md b/docs/plans/NEO-24-implementation-plan.md index 91c057a..5c448f4 100644 --- a/docs/plans/NEO-24-implementation-plan.md +++ b/docs/plans/NEO-24-implementation-plan.md @@ -50,13 +50,13 @@ - **`request_clear_target()`** — `POST` with no `targetId` / `null` per NEO-23. - **`request_tab_next()`** — read cached `lockedTargetId` + `validity`; compute **next** id in the ordered list (if no lock, choose **first** id in order); POST select; on deny, signal carries **server** `targetState` only (no client-only lock). - Signal e.g. **`target_state_changed(state: Dictionary)`** or typed fields for consumers (`lockedTargetId` Variant string-or-null, `validity` string, `sequence` int, `selection_applied` bool optional for last POST). - - **Polling:** while last known `lockedTargetId` is non-null, issue **`GET …/target`** on a **throttled** timer (e.g. every **0.25–0.5 s**) so **`out_of_range`** updates after locomotion without requiring new tab presses. Stop or slow polling when no lock if desired for noise (document choice in plan **Decisions** during implementation). + - **Refresh policy (hybrid, movement-triggered — Decision 2):** no periodic polling. Issue **`GET …/target`** when (a) boot sync runs, (b) any `POST …/target/select` completes (covered by response body already — no extra GET needed), and (c) **while a lock is currently held**, the `PositionAuthorityClient` emits **`authoritative_position_received`** *after* boot (i.e. movement-driven snaps, not the initial boot snap). A small **cooldown** (e.g. **250 ms**) coalesces bursts so many consecutive `move-stream` acks cannot produce a GET per ack. No timer otherwise. Rationale: stays honest about `out_of_range` without duplicating radius math on the client or holding an always-on polling timer. Trade-off: will not pick up purely server-driven state changes (none exist in NEO-23; revisit if combat/PvP adds them). 3. **Optimistic highlight (optional slice):** If implemented, keep **pending** selection separate from **acknowledged** state; on **200** denial, drop pending and paint from **`targetState`**. If timeboxed, **v1** may ship **ack-only** UI (still satisfies “optional optimistic” — none used). -4. **`main.tscn` / `main.gd`:** Add child node with `target_selection_client.gd`; connect signal to a **Label** (new `TargetLockLabel` under `UICanvas`). Call **`request_sync_from_server()`** once after boot position sync (or same frame as first authority ready) so HUD matches server on start. +4. **`main.tscn` / `main.gd`:** Add child node with `target_selection_client.gd`; connect signal to a **Label** (new `TargetLockLabel` under `UICanvas`). Call **`request_sync_from_server()`** once after boot position sync (or same frame as first authority ready) so HUD matches server on start. **Also** connect `PositionAuthorityClient.authoritative_position_received` → `target_selection_client.on_authoritative_position_snap(...)` (the client ignores the boot snap per `apply_as_snap` flag and only fires a GET when a lock is currently held; see step 2 refresh policy). -5. **`project.godot`:** `target_tab` → **Tab**; `target_clear` → **Escape** (document; user may request alternatives before implementation). +5. **`project.godot`:** `target_tab` → **Tab**; `target_clear` → **Escape** (locked — Decision 1). 6. **Manual QA:** Run server + client; tab between stubs; walk out of radius and confirm **`validity`** flips to **`out_of_range`** on overlay while id remains; clear with Esc. @@ -82,16 +82,18 @@ | Test file | What to cover | |-----------|----------------| -| `client/test/target_selection_client_test.gd` | **Mock transport** enqueues JSON: GET returns a v1 envelope; POST **apply** returns `selectionApplied: true` + `targetState`; POST **deny** returns `selectionApplied: false` + `reasonCode` + authoritative `targetState`; **tab** from no lock selects first id in order; **tab** from `alpha` requests `beta`; wrap from `beta` to `alpha`; **clear** issues POST without `targetId`. Use `MockHttpTransport` pattern from `position_authority_client_test.gd`. | +| `client/test/target_selection_client_test.gd` | **Mock transport** enqueues JSON: GET returns a v1 envelope; POST **apply** returns `selectionApplied: true` + `targetState`; POST **deny** returns `selectionApplied: false` + `reasonCode` + authoritative `targetState`; **tab** from no lock selects first id in order; **tab** from `alpha` requests `beta`; wrap from `beta` to `alpha`; **clear** issues POST without `targetId`; **movement-triggered refresh:** calling `on_authoritative_position_snap(...)` while a lock is held fires a GET, while **no** lock is held fires **nothing**, and two back-to-back snaps within cooldown produce **one** GET. Use `MockHttpTransport` pattern from `position_authority_client_test.gd`. | No new **C#** tests (client-only story). **Bruno:** NEO-23 targeting requests already live under `bruno/neon-sprawl-server/targeting/`; **no** new `.bru` required unless implementation discovers a **server** contract change (then add per repo [testing expectations](../../.cursor/rules/testing-expectations.md) for HTTP contract changes). ## Open questions / risks -- **Input bindings:** Plan assumes **Tab** + **Esc**. Confirm or specify alternatives (e.g. mouse thumb buttons) before locking `project.godot`. -- **Polling cost:** Throttled GET while locked is simple but adds HTTP traffic; acceptable for prototype. If objection, switch to polling only when HUD visible or tie to `move-stream` cadence with plan update. - **Scene visuals:** This story does **not** require 3D reticles on stub meshes; overlay text satisfies AC. World-space highlights can be a follow-up. +- **Server-driven state changes:** The hybrid refresh policy (Decision 2) **will not** pick up purely server-driven flips to `invalid_target` or future auto-clears — none exist in NEO-23, but if combat/PvP (E5.M1 / E6.M1) adds them later, revisit (either add a narrow server push or reintroduce a throttled poll). ## Decisions -*(None locked yet — update this table during implementation chat / PR.)* +| # | Topic | Choice | +|---|-------|--------| +| 1 | **Input bindings** | `target_tab` → **Tab**; `target_clear` → **Escape**. Registered in `client/project.godot`. | +| 2 | **Soft-lock refresh policy** | **Hybrid, movement-triggered.** No periodic polling. Refresh sources: boot `GET`, each `POST` response echo, **and** `GET` fired from `PositionAuthorityClient.authoritative_position_received` (non-boot snaps) **only while a lock is currently held**, with a **~250 ms** cooldown to coalesce bursts. Does **not** duplicate radius math on the client; does **not** cover future purely server-driven state changes (revisit when E5.M1 / E6.M1 introduce them). | From ee572e366111e98d9b7087913f084c6475bf5cab Mon Sep 17 00:00:00 2001 From: VinPropane Date: Tue, 21 Apr 2026 22:14:16 -0400 Subject: [PATCH 03/13] NEO-24: client tab-target + lock HUD synced to server Adds `TargetSelectionClient` (Godot, HTTP to NEO-23 targeting endpoints): - Tab cycles ids in ascending order matching `PrototypeTargetRegistry` (`prototype_target_alpha`, `prototype_target_beta`) and wraps. - Esc clears by POSTing with `targetId` omitted. - UI always paints from the server's authoritative `targetState` (success and denials alike); `UICanvas/TargetLockLabel` shows `lockedTargetId`, `validity`, `sequence`, and (on deny) `reasonCode`. - Movement-triggered refresh: while a lock is held, subsequent `authoritative_position_received` snaps fire a throttled GET (~250 ms cooldown) so `validity` flips to `out_of_range` after locomotion without duplicating server radius math on the client. Registers `target_tab` (Tab) / `target_clear` (Esc) in `project.godot`; documents bindings + manual QA in `client/README.md`. Tests: `target_selection_client_test.gd` covers GET parse, tab ordering from empty/alpha/wrap, POST body shape, denial preserves server state, clear omits targetId, movement refresh fires only while locked, cooldown collapses bursts to one GET, and `apply_as_snap=false` is ignored. All 85 client GdUnit cases pass; gdlint and gdformat clean. See `docs/plans/NEO-24-implementation-plan.md`. --- client/README.md | 18 ++ client/project.godot | 10 + client/scenes/main.tscn | 19 ++ client/scripts/main.gd | 30 +++ client/scripts/prototype_target_constants.gd | 31 +++ .../scripts/prototype_target_constants.gd.uid | 1 + client/scripts/target_selection_client.gd | 228 ++++++++++++++++++ client/scripts/target_selection_client.gd.uid | 1 + client/test/target_selection_client_test.gd | 223 +++++++++++++++++ .../test/target_selection_client_test.gd.uid | 1 + client/test/target_selection_test_double.gd | 9 + .../test/target_selection_test_double.gd.uid | 1 + 12 files changed, 572 insertions(+) create mode 100644 client/scripts/prototype_target_constants.gd create mode 100644 client/scripts/prototype_target_constants.gd.uid create mode 100644 client/scripts/target_selection_client.gd create mode 100644 client/scripts/target_selection_client.gd.uid create mode 100644 client/test/target_selection_client_test.gd create mode 100644 client/test/target_selection_client_test.gd.uid create mode 100644 client/test/target_selection_test_double.gd create mode 100644 client/test/target_selection_test_double.gd.uid diff --git a/client/README.md b/client/README.md index e0bf5ae..49c07ae 100644 --- a/client/README.md +++ b/client/README.md @@ -91,6 +91,24 @@ The main scene includes a **prototype terminal** at the map center (same world * 2. **F5** in Godot: default spawn is out of range of the terminal; markers should stay **dim**. **WASD** toward the center until markers **brighten** (within **3** m on the floor plane). 3. Press **E** (input action **`interact`** in `project.godot`): Output should show **`allowed=true`** when markers glow, **`allowed=false`** with **`reasonCode=out_of_range`** when dim (if you walk back out). Interaction uses **`_input`**, not `_unhandled_input`, so keys register reliably in the embedded **Game** dock; click the game view if the editor had focus elsewhere. +## Target lock + tab cycle (NEO-24) + +`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. +- **HUD:** `UICanvas/TargetLockLabel` shows the last **server-acknowledged** state: `lockedTargetId` (or **`—`** when no lock), `validity` (`none` / `ok` / `out_of_range` / `invalid_target`), `sequence`, and (on denials) `Denied: ` on the next line. **UI never shows an optimistic lock the server denied** — it always paints from the authoritative `targetState` echoed by each `200` response. +- **Soft lock refresh:** the client does **not** poll on a timer. It refreshes via `GET …/target` on boot, on every `POST …/target/select` response (body already carries `targetState`), and — while a lock is currently held — on **`authoritative_position_received`** from `PositionAuthorityClient`, throttled to at most one GET per **250 ms**. Boot is naturally skipped because no lock exists yet. Purely server-driven state flips (future PvP / combat categories) would require revisiting this policy; today NEO-23 has none. +- **Inspector:** match **`base_url`** / **`dev_player_id`** on the `TargetSelectionClient` node to the other HTTP clients / `Game:DevPlayerId`. + +### Manual check (NEO-24) + +1. Run the server and client as in NEO-7 / NEO-9; confirm spawn snap. +2. Press **Tab**: HUD → `Target: prototype_target_alpha`, `Validity: ok` (spawn is inside alpha's 8 m radius). +3. Press **Tab** again: server denies (beta anchor is **(8, 8)** with a **4 m** radius, so from spawn it is out of range). Per NEO-23 soft-lock policy the persisted lock does not change, so HUD stays at `Target: prototype_target_alpha, Validity: ok` with `Denied: out_of_range` appended. +4. **WASD** away from spawn past ~8 m horizontally from alpha; HUD flips to `Validity: out_of_range` on the next movement-triggered refresh (≤250 ms cadence) while `lockedTargetId` stays `alpha` — this is the **soft lock** round-tripped from the server. +5. Walk toward beta (inside **(8, 8)** within 4 m) and press **Tab**: swap succeeds, HUD shows `Target: prototype_target_beta, Validity: ok`. A further **Tab** from here wraps to `alpha` (and will itself succeed or deny based on distance). +6. Press **Esc**: HUD → `Target: —`, `Validity: none`, sequence advances. + ## Movement prototype (NEO-5 → NEO-11) **`player.gd`** steers **XZ** toward the authoritative goal and uses **`NavigationAgent3D`**’s **current path** only for the **vertical-routing** case above (on sufficiently flat floor); **`move_and_slide()`** does the motion. **`snap_to_server()`** remains for **boot** (and would apply for any future hard reconcile). diff --git a/client/project.godot b/client/project.godot index 05e57ce..29dfcbd 100644 --- a/client/project.godot +++ b/client/project.godot @@ -44,6 +44,16 @@ interact={ "events": [Object(InputEventKey,"resource_local_to_scene":false,"resource_name":"","device":-1,"window_id":0,"alt_pressed":false,"shift_pressed":false,"ctrl_pressed":false,"meta_pressed":false,"pressed":false,"keycode":69,"physical_keycode":69,"key_label":0,"unicode":101,"location":0,"echo":false,"script":null) ] } +target_tab={ +"deadzone": 0.5, +"events": [Object(InputEventKey,"resource_local_to_scene":false,"resource_name":"","device":-1,"window_id":0,"alt_pressed":false,"shift_pressed":false,"ctrl_pressed":false,"meta_pressed":false,"pressed":false,"keycode":4194306,"physical_keycode":4194306,"key_label":0,"unicode":0,"location":0,"echo":false,"script":null) +] +} +target_clear={ +"deadzone": 0.5, +"events": [Object(InputEventKey,"resource_local_to_scene":false,"resource_name":"","device":-1,"window_id":0,"alt_pressed":false,"shift_pressed":false,"ctrl_pressed":false,"meta_pressed":false,"pressed":false,"keycode":4194305,"physical_keycode":4194305,"key_label":0,"unicode":0,"location":0,"echo":false,"script":null) +] +} camera_zoom_in={ "deadzone": 0.5, "events": [Object(InputEventMouseButton,"resource_local_to_scene":false,"resource_name":"","device":-1,"window_id":0,"alt_pressed":false,"shift_pressed":false,"ctrl_pressed":false,"meta_pressed":false,"button_mask":0,"position":Vector2(0, 0),"global_position":Vector2(0, 0),"factor":1.0,"button_index":4,"canceled":false,"pressed":false,"double_click":false,"script":null) diff --git a/client/scenes/main.tscn b/client/scenes/main.tscn index cd0fc52..47ec26b 100644 --- a/client/scenes/main.tscn +++ b/client/scenes/main.tscn @@ -4,6 +4,7 @@ [ext_resource type="Script" uid="uid://1jimgt3d4bjj" path="res://scripts/player.gd" id="2_player"] [ext_resource type="Script" uid="uid://ds5fkbscljkxi" path="res://scripts/position_authority_client.gd" id="4_auth"] [ext_resource type="Script" uid="uid://bv0xprp660hib" path="res://scripts/interaction_request_client.gd" id="5_ix"] +[ext_resource type="Script" uid="uid://gh6yv5p1y0vq" path="res://scripts/target_selection_client.gd" id="11_tgt_sel"] [ext_resource type="Script" uid="uid://n3p4f3fky3nv" path="res://scripts/interaction_radius_indicators.gd" id="6_rad"] [ext_resource type="Script" uid="uid://caaj3ohikfdx2" path="res://scripts/isometric_follow_camera.gd" id="7_iso_cam"] [ext_resource type="Resource" path="res://resources/isometric_zoom_bands.tres" id="8_zoom_bands"] @@ -1111,6 +1112,9 @@ script = ExtResource("4_auth") [node name="InteractionRequestClient" type="Node" parent="." unique_id=2500003] script = ExtResource("5_ix") +[node name="TargetSelectionClient" type="Node" parent="." unique_id=2500004] +script = ExtResource("11_tgt_sel") + [node name="UICanvas" type="CanvasLayer" parent="." unique_id=9000001] [node name="MoveRejectLabel" type="Label" parent="UICanvas" unique_id=9000002] @@ -1139,3 +1143,18 @@ theme_override_font_sizes/font_size = 15 text = "x: — y: — z: —" + +[node name="TargetLockLabel" type="Label" parent="UICanvas" unique_id=9000004] +offset_left = 8.0 +offset_top = 82.0 +offset_right = 280.0 +offset_bottom = 150.0 +grow_horizontal = 0 +grow_vertical = 0 +theme_override_colors/font_color = Color(0.86, 0.94, 1, 1) +theme_override_colors/font_outline_color = Color(0.08, 0.08, 0.1, 1) +theme_override_constants/outline_size = 6 +theme_override_font_sizes/font_size = 15 +text = "Target: — +Validity: — +Seq: —" diff --git a/client/scripts/main.gd b/client/scripts/main.gd index 438bf09..63c69c3 100644 --- a/client/scripts/main.gd +++ b/client/scripts/main.gd @@ -62,6 +62,8 @@ var _dev_obstacle_smoke: Node3D @onready var _radius_preview: Node3D = $World/InteractionMarkers @onready var _move_reject_label: Label = $UICanvas/MoveRejectLabel @onready var _player_pos_label: Label = $UICanvas/PlayerPositionLabel +@onready var _target_lock_label: Label = $UICanvas/TargetLockLabel +@onready var _target_client: Node = $TargetSelectionClient @onready var _floor: StaticBody3D = $World/NavigationRegion3D/Floor @@ -81,7 +83,15 @@ func _ready() -> void: "authoritative_position_received", Callable(self, "_on_authoritative_position") ) _authority.connect("move_rejected", Callable(self, "_on_move_rejected")) + # NEO-24: movement-driven refresh — `TargetSelectionClient` no-ops on boot (no lock yet) + # and throttles bursts (see Decision 2 in NEO-24 plan). + _authority.connect( + "authoritative_position_received", + Callable(_target_client, "on_authoritative_position_snap") + ) + _target_client.connect("target_state_changed", Callable(self, "_on_target_state_changed")) _authority.call("sync_from_server") + _target_client.call("request_sync_from_server") if _radius_preview.has_method("setup_player"): _radius_preview.call("setup_player", _player) @@ -146,6 +156,26 @@ func _on_authoritative_position(world: Vector3, apply_as_snap: bool) -> void: _player.snap_to_server(world) +## NEO-24: render the last server-acknowledged target lock to the HUD label. +## `state` carries `lockedTargetId` (String or `null`), `validity`, `sequence`, and +## (on POST responses) `selectionApplied` + optional `reasonCode`. +func _on_target_state_changed(state: Dictionary) -> void: + if not is_instance_valid(_target_lock_label): + return + var locked_variant: Variant = state.get("lockedTargetId", null) + var locked_text: String = "—" + if locked_variant is String and not (locked_variant as String).is_empty(): + locked_text = locked_variant as String + var validity: String = str(state.get("validity", "none")) + var sequence: int = int(state.get("sequence", 0)) + var suffix: String = "" + if state.has("reasonCode"): + suffix = "\nDenied: %s" % str(state["reasonCode"]) + _target_lock_label.text = ( + "Target: %s\nValidity: %s\nSeq: %d%s" % [locked_text, validity, sequence, suffix] + ) + + func _on_move_rejected(reason_code: String) -> void: _authority_force_snap_next = true # Rejected stream: server state may differ; next snap is forced. diff --git a/client/scripts/prototype_target_constants.gd b/client/scripts/prototype_target_constants.gd new file mode 100644 index 0000000..e47e477 --- /dev/null +++ b/client/scripts/prototype_target_constants.gd @@ -0,0 +1,31 @@ +extends Object + +## NEO-24: prototype combat target ids in **tab cycle order** — must match +## `server/NeonSprawl.Server/Game/Targeting/PrototypeTargetRegistry.cs` +## (`PrototypeTargetAlphaId`, `PrototypeTargetBetaId`; ascending id order). +## Static accessors only (no `class_name`; headless / CI loads before editor import — see client +## README Godot CLI section). + +const ORDERED_IDS: Array[String] = ["prototype_target_alpha", "prototype_target_beta"] + + +static func ordered_ids() -> Array[String]: + return ORDERED_IDS.duplicate() + + +## 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. +static func next_id_after(current: Variant) -> String: + if ORDERED_IDS.is_empty(): + return "" + if not current is String: + return ORDERED_IDS[0] + var current_str: String = (current as String).strip_edges() + if current_str.is_empty(): + return ORDERED_IDS[0] + var idx: int = ORDERED_IDS.find(current_str) + if idx < 0: + return ORDERED_IDS[0] + var next_idx: int = (idx + 1) % ORDERED_IDS.size() + return ORDERED_IDS[next_idx] diff --git a/client/scripts/prototype_target_constants.gd.uid b/client/scripts/prototype_target_constants.gd.uid new file mode 100644 index 0000000..dfdcbbd --- /dev/null +++ b/client/scripts/prototype_target_constants.gd.uid @@ -0,0 +1 @@ +uid://bvcj2yn4v6nrh diff --git a/client/scripts/target_selection_client.gd b/client/scripts/target_selection_client.gd new file mode 100644 index 0000000..3bc86b2 --- /dev/null +++ b/client/scripts/target_selection_client.gd @@ -0,0 +1,228 @@ +extends Node + +## NEO-24 (E1M3-02): client tab-target + lock UI synced to server (NEO-23 `TargetState` v1). +## +## Flow: +## - `request_sync_from_server()` → `GET …/target` returns `PlayerTargetStateResponse` v1. +## - `request_tab_next()` picks the next id from `PrototypeTargetConstants.ORDERED_IDS` +## (first id if no lock) and POSTs `TargetSelectRequest` v1. +## - `request_select_target_id(id)` POSTs a specific id. +## - `request_clear_target()` POSTs with `targetId` omitted (NEO-23 clear rule). +## - Movement-triggered refresh: while a lock is held, `on_authoritative_position_snap(...)` +## fires a throttled `GET` so `validity` flips to `out_of_range` after locomotion without +## duplicating server radius math on the client (plan Decision 2). +## +## Not an `HTTPRequest` subclass — tests inject a `Node` exposing `request()` + +## `request_completed` (same pattern as `position_authority_client.gd`). + +signal target_state_changed(state: Dictionary) + +enum Phase { IDLE, GET, POST_SELECT } + +const PrototypeTargetConstants := preload("res://scripts/prototype_target_constants.gd") + +## Cooldown between movement-triggered refresh GETs. Drops further snaps inside the window so +## one `move-stream` burst does not produce one GET per snap. +const _REFRESH_COOLDOWN_MSEC: int = 250 + +@export var base_url: String = "http://127.0.0.1:5253" +@export var dev_player_id: String = "dev-local-1" + +var _http: Node +var _busy: bool = false +var _phase: Phase = Phase.IDLE + +## Last acknowledged state from the server. Mirrors `PlayerTargetStateResponse` v1: +## `schemaVersion` (int), `playerId` (String), `lockedTargetId` (String or `null`), +## `validity` (String: `none` | `ok` | `out_of_range` | `invalid_target`), +## `sequence` (int). POST responses additionally carry `selectionApplied` (bool) and an optional +## `reasonCode` (String). +var _state: Dictionary = {} + +## `-_REFRESH_COOLDOWN_MSEC` so the first snap is never blocked by the window on boot. +var _last_refresh_msec: int = -_REFRESH_COOLDOWN_MSEC + + +func _create_http_request() -> Node: + return HTTPRequest.new() + + +func _ready() -> void: + _http = _create_http_request() + add_child(_http) + if _http is HTTPRequest: + (_http as HTTPRequest).timeout = 30.0 + @warning_ignore("unsafe_method_access") + _http.request_completed.connect(_on_request_completed) + + +## Matches `interaction_request_client.gd`: `_input` is more reliable than `_unhandled_input` +## inside the embedded Game dock / focus quirks. Tab is captured before UI focus navigation, +## which is fine while the prototype HUD has no focusable controls. +func _input(event: InputEvent) -> void: + if event.is_action_pressed("target_tab"): + request_tab_next() + elif event.is_action_pressed("target_clear"): + request_clear_target() + + +func request_sync_from_server() -> void: + if _busy: + return + _busy = true + _phase = Phase.GET + _last_refresh_msec = Time.get_ticks_msec() + var url := "%s/game/players/%s/target" % [_base_root(), _player_path_segment()] + var err: Error = _http.request(url) + if err != OK: + push_warning("TargetSelectionClient: GET failed to start (%s)" % err) + _busy = false + _phase = Phase.IDLE + + +func request_tab_next() -> void: + var current: Variant = _state.get("lockedTargetId", null) + var next_id: String = PrototypeTargetConstants.next_id_after(current) + if next_id.is_empty(): + return + request_select_target_id(next_id) + + +func request_select_target_id(target_id: String) -> void: + var trimmed := target_id.strip_edges() + if trimmed.is_empty(): + push_warning("TargetSelectionClient: refusing empty targetId; use request_clear_target()") + return + _post_select({"schemaVersion": 1, "targetId": trimmed}) + + +func request_clear_target() -> void: + # NEO-23: omit `targetId` to clear. JSON `null` also works; omission keeps the body minimal. + _post_select({"schemaVersion": 1}) + + +## Connected to `PositionAuthorityClient.authoritative_position_received` in `main.gd`. +## Boot is naturally skipped because no lock is held yet; once a lock exists, subsequent +## snaps fire a refresh GET subject to a cooldown so bursts collapse to one GET. +func on_authoritative_position_snap(_world: Vector3, apply_as_snap: bool) -> void: + if not apply_as_snap: + return + if not _has_lock(): + return + if _busy: + return + var now_msec: int = Time.get_ticks_msec() + if now_msec - _last_refresh_msec < _REFRESH_COOLDOWN_MSEC: + return + request_sync_from_server() + + +func cached_state() -> Dictionary: + return _state.duplicate() + + +func _has_lock() -> bool: + if _state.is_empty(): + return false + var v: Variant = _state.get("lockedTargetId", null) + return v is String and not (v as String).is_empty() + + +func _post_select(payload: Dictionary) -> void: + if _busy: + return + _busy = true + _phase = Phase.POST_SELECT + var url := "%s/game/players/%s/target/select" % [_base_root(), _player_path_segment()] + var body := JSON.stringify(payload) + var headers := PackedStringArray(["Content-Type: application/json"]) + var err: Error = _http.request(url, headers, HTTPClient.METHOD_POST, body) + if err != OK: + push_warning("TargetSelectionClient: POST failed to start (%s)" % err) + _busy = false + _phase = Phase.IDLE + + +func _base_root() -> String: + return base_url.strip_edges().rstrip("/") + + +func _player_path_segment() -> String: + # ASCII-safe ids (e.g. `dev-local-1`); percent-encode if ids gain reserved URL characters. + return dev_player_id.strip_edges() + + +func _on_request_completed( + _result: int, response_code: int, _headers: PackedStringArray, body: PackedByteArray +) -> void: + var phase: Phase = _phase + _busy = false + _phase = Phase.IDLE + + if _result != HTTPRequest.RESULT_SUCCESS: + push_warning("TargetSelectionClient: HTTP failed (result=%s)" % _result) + return + + var text := body.get_string_from_utf8() + var parsed: Variant = JSON.parse_string(text) + if not parsed is Dictionary: + if response_code != 200: + push_warning("TargetSelectionClient: HTTP %s non-JSON body" % response_code) + return + var data: Dictionary = parsed + + match phase: + Phase.GET: + if response_code == 200: + _update_state_from_get(data) + Phase.POST_SELECT: + # Server includes authoritative `targetState` on 200 (apply and denial). 400 / 404 + # bodies may lack it — swallow quietly; `_state` keeps last-good. + if response_code == 200: + _update_state_from_post(data) + _: + pass + + +func _update_state_from_get(data: Dictionary) -> void: + var state := _extract_target_state_fields(data) + if state.is_empty(): + return + _state = state + target_state_changed.emit(_state.duplicate()) + + +func _update_state_from_post(data: Dictionary) -> void: + var target_state_variant: Variant = data.get("targetState", null) + if not target_state_variant is Dictionary: + push_warning("TargetSelectionClient: POST 200 missing targetState") + return + var state := _extract_target_state_fields(target_state_variant as Dictionary) + if state.is_empty(): + return + state["selectionApplied"] = bool(data.get("selectionApplied", false)) + var reason_variant: Variant = data.get("reasonCode", null) + if reason_variant is String and not (reason_variant as String).is_empty(): + state["reasonCode"] = reason_variant as String + _state = state + target_state_changed.emit(_state.duplicate()) + + +## Shape-only parse of the v1 target state payload (works for GET body and POST echo). +## `lockedTargetId` is normalized to either a non-empty [String] or Variant [code]null[/code]. +func _extract_target_state_fields(data: Dictionary) -> Dictionary: + if not data.has("validity"): + return {} + var out: Dictionary = {} + out["schemaVersion"] = int(data.get("schemaVersion", 1)) + var pid_variant: Variant = data.get("playerId", "") + out["playerId"] = (pid_variant as String) if pid_variant is String else "" + var locked_variant: Variant = data.get("lockedTargetId", null) + if locked_variant is String and not (locked_variant as String).is_empty(): + out["lockedTargetId"] = locked_variant as String + else: + out["lockedTargetId"] = null + var validity_variant: Variant = data.get("validity", "none") + out["validity"] = (validity_variant as String) if validity_variant is String else "none" + out["sequence"] = int(data.get("sequence", 0)) + return out diff --git a/client/scripts/target_selection_client.gd.uid b/client/scripts/target_selection_client.gd.uid new file mode 100644 index 0000000..b1b0e9c --- /dev/null +++ b/client/scripts/target_selection_client.gd.uid @@ -0,0 +1 @@ +uid://gh6yv5p1y0vq diff --git a/client/test/target_selection_client_test.gd b/client/test/target_selection_client_test.gd new file mode 100644 index 0000000..3e9a802 --- /dev/null +++ b/client/test/target_selection_client_test.gd @@ -0,0 +1,223 @@ +extends GdUnitTestSuite + +## NEO-24: GdUnit4 suite for `TargetSelectionClient`. Mirrors +## `position_authority_client_test.gd` — mock HTTP transport enqueues JSON responses and +## (optionally) tracks request count so the movement-triggered refresh cooldown is testable. + +const TargetSelectionTestDouble := preload("res://test/target_selection_test_double.gd") + +const ALPHA_ID := "prototype_target_alpha" +const BETA_ID := "prototype_target_beta" + + +# In-process transport: not an HTTPRequest subclass (Godot 4 will not call script `request()` +# on those). Counts requests so "no lock → no GET" and "cooldown collapses snaps" are testable. +class MockHttpTransport: + extends Node + signal request_completed( + result: int, response_code: int, headers: PackedStringArray, body: PackedByteArray + ) + var request_count: int = 0 + var last_url: String = "" + var last_body: String = "" + var last_method: HTTPClient.Method = HTTPClient.METHOD_GET + var _queue: Array[Dictionary] = [] + + func enqueue(result: int, code: int, body: String) -> void: + _queue.append({"result": result, "code": code, "body": body}) + + func request( + url: String, + _custom_headers: PackedStringArray = PackedStringArray(), + method: HTTPClient.Method = HTTPClient.METHOD_GET, + request_data: String = "" + ) -> Error: + request_count += 1 + last_url = url + last_method = method + last_body = request_data + if _queue.is_empty(): + return ERR_UNAVAILABLE + var r: Dictionary = _queue.pop_front() + var body_bytes: PackedByteArray = str(r["body"]).to_utf8_buffer() + request_completed.emit(r["result"], r["code"], PackedStringArray(), body_bytes) + return OK + + +func _make_client(http_transport: Node) -> Node: + var c: Node = TargetSelectionTestDouble.new() + c.set("injected_http", http_transport) + auto_free(http_transport) + auto_free(c) + add_child(c) + return c + + +func _target_state_json(locked_id_or_null: Variant, validity: String, sequence: int = 0) -> String: + var locked := "null" + if locked_id_or_null is String and not (locked_id_or_null as String).is_empty(): + locked = '"%s"' % (locked_id_or_null as String) + return ( + '{"schemaVersion":1,"playerId":"dev-local-1","lockedTargetId":%s,' % locked + + '"validity":"%s","sequence":%d}' % [validity, sequence] + ) + + +func _select_response_json( + applied: bool, + locked_id_or_null: Variant, + validity: String, + sequence: int = 0, + reason: String = "" +) -> String: + var target_state := _target_state_json(locked_id_or_null, validity, sequence) + if applied: + return '{"schemaVersion":1,"selectionApplied":true,"targetState":%s}' % target_state + var reason_segment := "" + if not reason.is_empty(): + reason_segment = ',"reasonCode":"%s"' % reason + return ( + '{"schemaVersion":1,"selectionApplied":false%s,"targetState":%s}' + % [reason_segment, target_state] + ) + + +func test_sync_get_200_emits_state() -> void: + var transport := MockHttpTransport.new() + transport.enqueue(HTTPRequest.RESULT_SUCCESS, 200, _target_state_json(null, "none")) + var c := _make_client(transport) + monitor_signals(c) + c.request_sync_from_server() + assert_signal(c).is_emitted("target_state_changed") + var state: Dictionary = c.cached_state() + assert_that(state.get("validity", "")).is_equal("none") + assert_that(state.get("lockedTargetId", "non-null-sentinel")).is_null() + + +func test_tab_from_no_lock_selects_first_id() -> void: + var transport := MockHttpTransport.new() + transport.enqueue( + HTTPRequest.RESULT_SUCCESS, 200, _select_response_json(true, ALPHA_ID, "ok", 1) + ) + var c := _make_client(transport) + monitor_signals(c) + c.request_tab_next() + assert_that(transport.last_url).contains("/target/select") + assert_that(transport.last_method).is_equal(HTTPClient.METHOD_POST) + assert_that(transport.last_body).contains('"targetId":"%s"' % ALPHA_ID) + assert_signal(c).is_emitted("target_state_changed") + assert_that(c.cached_state().get("lockedTargetId")).is_equal(ALPHA_ID) + + +func test_tab_from_alpha_requests_beta() -> void: + var transport := MockHttpTransport.new() + transport.enqueue( + HTTPRequest.RESULT_SUCCESS, 200, _select_response_json(true, ALPHA_ID, "ok", 1) + ) + transport.enqueue( + HTTPRequest.RESULT_SUCCESS, 200, _select_response_json(true, BETA_ID, "ok", 2) + ) + var c := _make_client(transport) + c.request_tab_next() + 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_wraps_from_beta_to_alpha() -> void: + var transport := MockHttpTransport.new() + transport.enqueue( + HTTPRequest.RESULT_SUCCESS, 200, _select_response_json(true, ALPHA_ID, "ok", 1) + ) + transport.enqueue( + HTTPRequest.RESULT_SUCCESS, 200, _select_response_json(true, BETA_ID, "ok", 2) + ) + transport.enqueue( + HTTPRequest.RESULT_SUCCESS, 200, _select_response_json(true, ALPHA_ID, "ok", 3) + ) + var c := _make_client(transport) + c.request_tab_next() + c.request_tab_next() + c.request_tab_next() + assert_that(transport.last_body).contains('"targetId":"%s"' % ALPHA_ID) + assert_that(c.cached_state().get("lockedTargetId")).is_equal(ALPHA_ID) + + +func test_denial_reflects_authoritative_target_state() -> void: + var transport := MockHttpTransport.new() + # Server denies: selectionApplied=false, lockedTargetId=null, reasonCode=out_of_range. + transport.enqueue( + HTTPRequest.RESULT_SUCCESS, + 200, + _select_response_json(false, null, "none", 0, "out_of_range") + ) + var c := _make_client(transport) + c.request_select_target_id(ALPHA_ID) + var state: Dictionary = c.cached_state() + assert_that(bool(state.get("selectionApplied", true))).is_false() + assert_that(state.get("reasonCode", "")).is_equal("out_of_range") + assert_that(state.get("lockedTargetId", "sentinel")).is_null() + + +func test_clear_issues_post_without_target_id() -> void: + var transport := MockHttpTransport.new() + transport.enqueue(HTTPRequest.RESULT_SUCCESS, 200, _select_response_json(true, null, "none", 4)) + var c := _make_client(transport) + c.request_clear_target() + assert_that(transport.last_url).contains("/target/select") + assert_that(transport.last_method).is_equal(HTTPClient.METHOD_POST) + assert_that(transport.last_body).not_contains("targetId") + + +func test_position_snap_while_locked_fires_refresh_get() -> void: + var transport := MockHttpTransport.new() + # Step 1: lock alpha via POST. + transport.enqueue( + HTTPRequest.RESULT_SUCCESS, 200, _select_response_json(true, ALPHA_ID, "ok", 1) + ) + # Step 2: movement-triggered GET shows out_of_range (soft lock). + transport.enqueue( + HTTPRequest.RESULT_SUCCESS, 200, _target_state_json(ALPHA_ID, "out_of_range", 1) + ) + var c := _make_client(transport) + c.request_select_target_id(ALPHA_ID) + var count_before_snap: int = transport.request_count + c.on_authoritative_position_snap(Vector3.ZERO, true) + assert_that(transport.request_count).is_equal(count_before_snap + 1) + assert_that(c.cached_state().get("validity")).is_equal("out_of_range") + assert_that(c.cached_state().get("lockedTargetId")).is_equal(ALPHA_ID) + + +func test_position_snap_without_lock_fires_nothing() -> void: + var transport := MockHttpTransport.new() + var c := _make_client(transport) + c.on_authoritative_position_snap(Vector3.ZERO, true) + assert_that(transport.request_count).is_equal(0) + + +func test_two_snaps_within_cooldown_collapse_to_one_get() -> void: + var transport := MockHttpTransport.new() + transport.enqueue( + HTTPRequest.RESULT_SUCCESS, 200, _select_response_json(true, ALPHA_ID, "ok", 1) + ) + transport.enqueue( + HTTPRequest.RESULT_SUCCESS, 200, _target_state_json(ALPHA_ID, "out_of_range", 1) + ) + var c := _make_client(transport) + c.request_select_target_id(ALPHA_ID) + var count_before: int = transport.request_count + c.on_authoritative_position_snap(Vector3.ZERO, true) + c.on_authoritative_position_snap(Vector3.ZERO, true) + assert_that(transport.request_count).is_equal(count_before + 1) + + +func test_position_snap_apply_as_snap_false_is_ignored() -> void: + 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_select_target_id(ALPHA_ID) + var count_before: int = transport.request_count + c.on_authoritative_position_snap(Vector3.ZERO, false) + assert_that(transport.request_count).is_equal(count_before) diff --git a/client/test/target_selection_client_test.gd.uid b/client/test/target_selection_client_test.gd.uid new file mode 100644 index 0000000..5a94851 --- /dev/null +++ b/client/test/target_selection_client_test.gd.uid @@ -0,0 +1 @@ +uid://csqopvd66fsp0 diff --git a/client/test/target_selection_test_double.gd b/client/test/target_selection_test_double.gd new file mode 100644 index 0000000..8c2105c --- /dev/null +++ b/client/test/target_selection_test_double.gd @@ -0,0 +1,9 @@ +extends "res://scripts/target_selection_client.gd" + +## Test-only: inject transport node before `add_child` (NEO-24; mirrors +## `position_authority_test_double.gd`). +var injected_http: Node + + +func _create_http_request() -> Node: + return injected_http diff --git a/client/test/target_selection_test_double.gd.uid b/client/test/target_selection_test_double.gd.uid new file mode 100644 index 0000000..19d5433 --- /dev/null +++ b/client/test/target_selection_test_double.gd.uid @@ -0,0 +1 @@ +uid://bq6776s8atb8 From e5bbfd3ee72190ba9f4e0f76c69eb5488ecaeaf6 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Tue, 21 Apr 2026 22:14:49 -0400 Subject: [PATCH 04/13] =?UTF-8?q?NEO-24:=20reconcile=20plan=20=E2=80=94=20?= =?UTF-8?q?tick=20acceptance=20criteria=20shipped?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/plans/NEO-24-implementation-plan.md | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/docs/plans/NEO-24-implementation-plan.md b/docs/plans/NEO-24-implementation-plan.md index 5c448f4..85de5d8 100644 --- a/docs/plans/NEO-24-implementation-plan.md +++ b/docs/plans/NEO-24-implementation-plan.md @@ -34,11 +34,13 @@ ## Acceptance criteria checklist -- [ ] With **≥2** stub targets, **tab** advances selection intent in **ascending id** order and **wraps** at the end of the ordered list. -- [ ] After each successful **round-trip**, UI shows **`lockedTargetId`** (or equivalent **none**) and **`validity`** from the **response body** (`targetState` on POST, body on GET). -- [ ] On **`selectionApplied`: `false`**, UI shows **authoritative** `targetState` from the same response (no “stuck optimistic” lock id that the server denied). -- [ ] **Clear** action clears lock server-side and UI updates from the response. -- [ ] Input bindings for **tab cycle** and **clear** are documented in **`client/project.godot`** (and briefly in **`client/README.md`** if other prototype keys are documented there). +- [x] With **≥2** stub targets, **tab** advances selection intent in **ascending id** order and **wraps** at the end of the ordered list. (`PrototypeTargetConstants.next_id_after` + `test_tab_from_no_lock_selects_first_id` / `test_tab_from_alpha_requests_beta` / `test_tab_wraps_from_beta_to_alpha`.) +- [x] After each successful **round-trip**, UI shows **`lockedTargetId`** (or **`—`** for none) and **`validity`** from the **response body** (`targetState` on POST, body on GET). (`TargetSelectionClient._extract_target_state_fields` + `main.gd:_on_target_state_changed`; `test_sync_get_200_emits_state`.) +- [x] On **`selectionApplied`: `false`**, UI shows **authoritative** `targetState` from the same response (no “stuck optimistic” lock id that the server denied). (`_update_state_from_post` replaces cached state from `targetState` on every 200, success or deny; `test_denial_reflects_authoritative_target_state`.) +- [x] **Clear** action clears lock server-side and UI updates from the response. (`request_clear_target` omits `targetId`; `test_clear_issues_post_without_target_id`.) +- [x] Input bindings for **tab cycle** and **clear** are documented in **`client/project.godot`** and in **`client/README.md`** (new “Target lock + tab cycle (NEO-24)” section). + +Server-up manual QA steps live in the README section above; run the server + client to exercise the soft-lock out-of-range flow end to end. ## Technical approach From f87ff2c99a574bb7d46b9cc47c8d2f38368c5f86 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Tue, 21 Apr 2026 22:21:44 -0400 Subject: [PATCH 05/13] chore: require manual QA checklist per story in planning-implementation-docs rule Add docs/manual-qa/{LINEAR_ISSUE_ID}.md as a required implementation-time artifact for stories with user-visible behavior. Documents the when-to- create / skip criteria, required header, section structure, coverage cues, and that the agent generates but does not tick the boxes. Cross-references from AGENTS.md alongside plans and reviews. --- .cursor/rules/planning-implementation-docs.md | 17 ++++++++++++++++- AGENTS.md | 2 +- 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/.cursor/rules/planning-implementation-docs.md b/.cursor/rules/planning-implementation-docs.md index 749fe96..50410d6 100644 --- a/.cursor/rules/planning-implementation-docs.md +++ b/.cursor/rules/planning-implementation-docs.md @@ -13,8 +13,22 @@ Whenever you **decide** something during **story planning** or **implementation* |-----------|--------| | Ticketed story work | **`docs/plans/{LINEAR_ISSUE_ID}-implementation-plan.md`** (e.g. `NEO-7-…`) — add or edit a **Decisions** subsection, **resolve** former open questions, refresh **Technical approach** / **Acceptance criteria** checkboxes, and **Files to add/modify** if reality diverged from kickoff. | | Cross-cutting or module contract | Relevant **`docs/decomposition/modules/*.md`**, **`docs/game-design/`**, or **`README`** / **`server/README.md`** / **`client/README.md`** when the decision affects how others integrate. | +| Manual QA steps for ticketed story work | **`docs/manual-qa/{LINEAR_ISSUE_ID}.md`** — see **Manual QA checklist** below. | | Review findings | **`docs/reviews/…`** when using the code-review agent; link back to the plan if the review changed direction. When feedback is fixed, **strike through + `Done.`** on the original bullets in that review file — see **Code review follow-up** below. | +## Manual QA checklist + +For ticketed story work that produces **user-visible** behavior (UI, input bindings, HTTP surfaces, scene wiring, runtime config, etc.), generate a checklist at **`docs/manual-qa/{LINEAR_ISSUE_ID}.md`** (example: `docs/manual-qa/NEO-24.md`) during **implementation**—not after handoff. The checklist complements automated tests; it captures the things a human needs to exercise end-to-end against real processes (server + client, database, input hardware) that unit / integration tests do not cover. + +- **When to create:** at the end of implementation, once code is stable enough to exercise. Commit it on the **story branch** alongside the implementation commits; do not defer to a separate PR. +- **When to skip:** pure refactors, docs-only changes, or stories whose acceptance is fully covered by automated tests (state that reason in one line in the plan's **Tests** section instead of creating an empty checklist). +- **File header:** a small table with **Key**, **Title**, **Linear** link, **Plan** link back to `docs/plans/{KEY}-implementation-plan.md`, and the story **Branch**. +- **Body:** **numbered sections** grouped by concern (e.g. *Setup sanity*, *Happy path*, *Edge cases / failure modes*, *No regressions*), each with **`- [ ]` checkbox bullets** that are **concrete and observable**—name the UI label, the server route, the console message, the expected state. Avoid vague asks like "test it works." +- **Coverage cues:** include at least (a) a **boot / baseline** section, (b) each **acceptance criterion** from the plan exercised by a concrete user action, (c) relevant **denial / failure** paths (HTTP 4xx, server down, invalid input), (d) **coexistence / no regressions** with neighboring features the story touches. +- **Keep it current:** if implementation reveals a new failure mode or a changed binding, update the checklist in the same commit. Any `Done.` / `Deferred` follow-ups from a QA pass belong back in the plan's **Decisions** or **Open questions**, not as strikethroughs on the checklist. + +The checklist is for the **user** (or a reviewer) to execute; the agent generates it but does **not** tick the boxes on the user's behalf unless the user explicitly confirms a section was exercised. + ## Code review follow-up (resolved suggestions) When **blocking issues**, **suggestions**, or agreed **nits** from a saved **`docs/reviews/YYYY-MM-DD-*.md`** are **fixed** in code or other docs, **edit that same review file** in the **same** change-set or the **next** commit—do **not** only fix the code and leave the review implying feedback is still open. @@ -33,7 +47,8 @@ When **blocking issues**, **suggestions**, or agreed **nits** from a saved **`do ## Agent behavior - After **kickoff**, the plan is living: **amend it** when implementation choices differ from the draft. -- Before **closing** or **handing off** a story, skim the plan: open questions should be **resolved** or restated as follow-up tickets with pointers. +- During **implementation**, generate **`docs/manual-qa/{KEY}.md`** per **Manual QA checklist** above for any story with user-visible behavior; commit it on the story branch before handoff. +- Before **closing** or **handing off** a story, skim the plan: open questions should be **resolved** or restated as follow-up tickets with pointers, and the manual QA checklist should exist (or be explicitly declined in the plan). - After **implementing** review feedback, **edit the corresponding `docs/reviews/…` file** with **strikethrough + Done** on the original bullets per **Code review follow-up** above—this step is easy to skip; treat it as **required** whenever suggestions were listed in that review. This complements [story-kickoff](story-kickoff.md) (plan creation) and [code-review-agent](code-review-agent.md) (documentation checked vs plans and decomposition). diff --git a/AGENTS.md b/AGENTS.md index 18817e1..b8db36f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -7,6 +7,6 @@ Optional **personas** for Cursor. Enable by **@ mentioning** the rule in chat or | **Code review** | [`.cursor/rules/code-review-agent.md`](.cursor/rules/code-review-agent.md) | PR / diff / pre-merge review; **always writes** `docs/reviews/YYYY-MM-DD-{slug}.md` with **Documentation checked** vs `docs/plans/` and `docs/decomposition/modules/`; **when suggestions are fixed, strike through those bullets + `Done.` in that file** ([planning-implementation-docs](.cursor/rules/planning-implementation-docs.md)); short chat pointer | | **Docs review** | [`.cursor/rules/docs-review-agent.md`](.cursor/rules/docs-review-agent.md) | Coherence / links / dev-guide fitness for `docs/` (especially `docs/game-design/` + decomposition); **writes** `docs/reviews/YYYY-MM-DD-{slug}.md`; **when suggestions are fixed, strike through + `Done.` in that file** ([planning-implementation-docs](.cursor/rules/planning-implementation-docs.md)); use when working in **documents** or @ mention | -Project-wide conventions live under [`.cursor/rules/`](.cursor/rules/) (many are `alwaysApply`). **Planning / implementation decisions** must be written into `docs/plans/` (and related docs)—[`.cursor/rules/planning-implementation-docs.md`](.cursor/rules/planning-implementation-docs.md). **Godot client layout** (thin `main.gd`, split by concern): [`.cursor/rules/godot-client-script-organization.md`](.cursor/rules/godot-client-script-organization.md). **Git:** agents may **commit** at discretion on story/ticket work; **never** `git push` unless the user asks — [`.cursor/rules/commit-and-review.md`](.cursor/rules/commit-and-review.md). **Story lifecycle:** kickoff [`.cursor/rules/story-kickoff.md`](.cursor/rules/story-kickoff.md); after merge / end of story — `checkout main`, `pull`, delete local story branch — [`.cursor/rules/story-end.md`](.cursor/rules/story-end.md). **Linear:** ask which **state** to set (e.g. **In Test** vs **Done**), or wait for the user to say which in the same message, **before** updating the issue; do not guess. **PR / push text:** no “Made-with: Cursor” boilerplate (same file). +Project-wide conventions live under [`.cursor/rules/`](.cursor/rules/) (many are `alwaysApply`). **Planning / implementation decisions** must be written into `docs/plans/` (and related docs)—[`.cursor/rules/planning-implementation-docs.md`](.cursor/rules/planning-implementation-docs.md). **Manual QA checklists** for user-visible ticketed work are generated during implementation at `docs/manual-qa/{KEY}.md` (same rule). **Godot client layout** (thin `main.gd`, split by concern): [`.cursor/rules/godot-client-script-organization.md`](.cursor/rules/godot-client-script-organization.md). **Git:** agents may **commit** at discretion on story/ticket work; **never** `git push` unless the user asks — [`.cursor/rules/commit-and-review.md`](.cursor/rules/commit-and-review.md). **Story lifecycle:** kickoff [`.cursor/rules/story-kickoff.md`](.cursor/rules/story-kickoff.md); after merge / end of story — `checkout main`, `pull`, delete local story branch — [`.cursor/rules/story-end.md`](.cursor/rules/story-end.md). **Linear:** ask which **state** to set (e.g. **In Test** vs **Done**), or wait for the user to say which in the same message, **before** updating the issue; do not guess. **PR / push text:** no “Made-with: Cursor” boilerplate (same file). **Commits tied to a Linear issue** must start the subject with the **issue id** and a colon (e.g. `NEO-8: …`). Branch naming and exceptions (`chore:` when there is no ticket) — [`.cursor/rules/linear-git-naming.md`](.cursor/rules/linear-git-naming.md). From cbcb4e3d639cc6e8499cdb7ca31b62a562e0bb2a Mon Sep 17 00:00:00 2001 From: VinPropane Date: Tue, 21 Apr 2026 22:21:50 -0400 Subject: [PATCH 06/13] NEO-24: add manual QA checklist First instance of the new docs/manual-qa/ convention. Covers boot sync, Tab happy path, out-of-range soft-lock (both by Tab and by locomotion per Decision 2), swap-to-beta, Esc clear, denial UI hygiene, input behavior, move-rejection coexistence, transport failure, and no- regression checks against NEO-7/9/19/22. --- docs/manual-qa/NEO-24.md | 105 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 105 insertions(+) create mode 100644 docs/manual-qa/NEO-24.md diff --git a/docs/manual-qa/NEO-24.md b/docs/manual-qa/NEO-24.md new file mode 100644 index 0000000..7da6fc7 --- /dev/null +++ b/docs/manual-qa/NEO-24.md @@ -0,0 +1,105 @@ +# NEO-24 — Manual QA checklist + +| Field | Value | +|--------|--------| +| **Key** | NEO-24 | +| **Title** | E1.M3: Client tab-target + lock UI synced to server | +| **Linear** | [NEO-24](https://linear.app/neon-sprawl/issue/NEO-24/e1m3-client-tab-target-lock-ui-synced-to-server) | +| **Plan** | [`docs/plans/NEO-24-implementation-plan.md`](../plans/NEO-24-implementation-plan.md) | +| **Branch** | `NEO-24-e1m3-client-tab-target-lock-ui-synced-to-server` | + +Pair the client (Godot **F5**) with the server (`cd server/NeonSprawl.Server && dotnet run`). Keep the server console visible so you can see route hits, and the Godot **Output** dock visible for client warnings. + +## 0. Setup sanity + +- [ ] Server prints a URL around `http://localhost:5253` on boot; `GET /health` returns JSON. +- [ ] In Godot, select `TargetSelectionClient` in the scene tree: **`base_url`** matches the server URL and **`dev_player_id`** matches `Game:DevPlayerId` (default `dev-local-1`). +- [ ] (Optional) Bruno collection `bruno/neon-sprawl-server/targeting/` is configured for the same port — use it to cross-check server state after client actions. + +## 1. Boot state + +- [ ] On run, the player snaps to spawn (`-5, 0.9, -5`) — existing NEO-7 behavior, unchanged. +- [ ] `UICanvas/TargetLockLabel` (top-left, under `PlayerPositionLabel`) shows: + - `Target: —` + - `Validity: none` + - `Seq: 0` +- [ ] Godot **Output** has no `TargetSelectionClient:` warnings. Server console shows a single `GET /game/players/dev-local-1/target` from the boot sync. + +## 2. Tab cycle — happy path + +- [ ] Press **Tab**. HUD flips to: + - `Target: prototype_target_alpha` + - `Validity: ok` + - `Seq: 1` +- [ ] Server log shows `POST /game/players/dev-local-1/target/select` with `{"schemaVersion":1,"targetId":"prototype_target_alpha"}`. +- [ ] No `Denied:` line is appended to the HUD (success ⇒ no `reasonCode`). + +## 3. Tab to out-of-range target — soft lock preserved + +- [ ] With alpha locked, press **Tab** again. The server denies the swap to beta (spawn is ~18 m from `(8, 8)`; beta radius is 4 m). +- [ ] HUD **stays** at `Target: prototype_target_alpha`, `Validity: ok`, `Seq: 1` with **`Denied: out_of_range`** appended. That is the NEO-23 soft-lock rule: a failed swap does not change the persisted lock or its sequence. +- [ ] Server log shows the denial: `POST` → `200` with `selectionApplied: false`. +- [ ] Press **Tab** several more times from here. Each press cleanly denies; HUD never drifts or shows a `beta` / optimistic lock between responses. + +## 4. Soft-lock out-of-range via locomotion (plan Decision 2) + +- [ ] Still locked to alpha, **WASD** away from spawn until you are ~8 m horizontally from `(-5, -5)`. Within ~250 ms of the next authority snap, HUD flips to: + - `Target: prototype_target_alpha` (unchanged — soft lock) + - `Validity: out_of_range` + - `Seq: 1` (unchanged — no lock id change) +- [ ] Walk back inside the radius. HUD returns to `Validity: ok` within ~250 ms of the next snap. +- [ ] While walking, Godot Output is **not** spammed with targeting GETs. Server log shows no more than roughly one targeting `GET` per 250 ms during sustained motion. +- [ ] Stand still with no lock changes. Server log shows **no** targeting GETs at idle (no timer polling). + +## 5. Swap to beta when in range + +- [ ] WASD toward beta at **(8, 8)** until you are within 4 m. Press **Tab**. HUD swaps cleanly: + - `Target: prototype_target_beta` + - `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. + +## 6. Clear + +- [ ] Press **Esc** at any point with a lock held. HUD flips to: + - `Target: —` + - `Validity: none` + - `Seq` increments by 1 +- [ ] Any `Denied: …` line from the previous state is gone. +- [ ] Server log shows `POST /target/select` with body `{"schemaVersion":1}` (no `targetId` key). +- [ ] Press **Esc** again with no lock. Server returns the same cleared state; HUD stays `—` / `none` (server may or may not bump sequence — whatever it returns is what the HUD shows). + +## 7. Denial UI hygiene + +- [ ] Press **Tab** from `—` while far from both targets. If the server denies (possible depending on exact spawn distance), HUD shows `Target: —`, `Validity: none`, `Denied: out_of_range`. The lock id does **not** get "stuck" at something the server refused. +- [ ] After any successful Tab / Esc, the `Denied: …` line goes away on the next response. + +## 8. Input behavior + +- [ ] **Tab** does **not** make focus jump around UI during play (we intercept in `_input` before UI focus navigation). +- [ ] **Esc** does not quit or minimize the game. +- [ ] Holding **Tab** does not auto-repeat into a firehose of POSTs (pressed action only fires on key press, not echo). +- [ ] **E** (NEO-9 interact) and **WASD** (NEO-22 move) still work independently of the target lock; having a lock does not block interact or movement. + +## 9. Coexistence with movement rejection (NEO-19 / move-stream 400) + +- [ ] Walk at the orange `MoveRejectPedestal` (~(7.5, −6.5)) while holding a lock. When `move-stream` returns **400** (`vertical_step_exceeded`), `PositionAuthorityClient` runs a boot-style re-sync which emits `authoritative_position_received`. +- [ ] HUD's `MoveRejectLabel` shows the rejection message as before (NEO-19). +- [ ] `TargetSelectionClient` treats that snap like any other: if a lock is held, it fires at most one refresh GET (subject to cooldown). No duplicate requests, no stuck busy state. + +## 10. Server-down / transport failure + +- [ ] Stop the server. Press **Tab** / **Esc**. Godot **Output** prints `TargetSelectionClient: HTTP failed …` warnings; the client is not wedged — you can press again (busy flag is released on failure). +- [ ] Restart the server and confirm the next **Tab** succeeds; HUD updates from the response. + +## 11. No regressions + +- [ ] Boot snap still works (NEO-7). +- [ ] WASD locomotion + `move-stream` still works (NEO-22); `PlayerPositionLabel` updates every physics tick. +- [ ] `E` interact with the prototype terminal still prints `allowed=true/false` in Output (NEO-9). +- [ ] Camera zoom / occluder obstacle toggle (Ctrl+Shift+K) still work. +- [ ] No new **red** lines in Godot Output during a typical 1–2 minute session. Benign warnings from other systems are fine; nothing from `TargetSelectionClient` during normal play. + +--- + +When sections 1–10 are ticked and 11 shows no regressions, the story is ready for **In Test** / PR. Record any deviations or follow-ups in the **Decisions** or **Open questions** sections of the [implementation plan](../plans/NEO-24-implementation-plan.md). From 435a32b3b33af224d2eb6b4927665eddb2f58fc1 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Tue, 21 Apr 2026 22:35:41 -0400 Subject: [PATCH 07/13] NEO-24: fix movement-driven target refresh via authoritative_ack signal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review 2026-04-21 caught that the hybrid soft-lock refresh path is unreachable during normal WASD locomotion. `main.gd` was wiring `TargetSelectionClient` to `PositionAuthorityClient.authoritative_position_received`, which only fires on boot sync and move-rejection resync — successful `move-stream` 200s are intentionally silent to avoid RTT rubber-banding. So walking out of an alpha/beta radius never triggered the throttled `GET /target` the HUD depends on, and validity stayed stuck at `ok`. Add a separate `authoritative_ack(world)` signal on `PositionAuthorityClient` emitted from both `BOOT_GET` 200 and successful `move-stream` 200, and switch `TargetSelectionClient` (+ `main.gd`) to it. The snap signal keeps its current semantics so rubber-band suppression is untouched; the ack signal is a pure heartbeat that cooldown-throttled consumers can hook. Also adds an integration test that wires both real clients together (per the review's suggestion 1) so renames or mis-wires between the two scripts fail loudly rather than silently skipping the end-to-end path. - `client/scripts/position_authority_client.gd`: new `authoritative_ack` signal; extracted `_parse_world_from_response` helper to share JSON parse across `BOOT_GET` and `STREAM_POST`; emit ack from both. - `client/scripts/target_selection_client.gd`: rename `on_authoritative_position_snap` to `on_authoritative_ack`; drop `apply_as_snap` gate (no longer meaningful). - `client/scripts/main.gd`: connect the new signal. - `client/test/position_authority_client_test.gd`: assert ack on boot 200 and stream 200. - `client/test/target_selection_client_test.gd`: rename tests for new handler. - `client/test/target_refresh_on_locomotion_test.gd`: new integration suite wiring real authority + target clients. - Plan Decision 3 + Tests row + Files-to-modify row updated. - `docs/reviews/2026-04-21-NEO-24.md`: committed with blocking issue + suggestions struck through with `Done.` notes per planning-implementation-docs rule. - `docs/decomposition/modules/documentation_and_implementation_alignment.md`: E1.M3 row now reflects NEO-24 landed. - `client/README.md` + `docs/manual-qa/NEO-24.md`: refreshed signal names so the soft-lock refresh description matches the wiring. Tests: 88/88 GdUnit headless passed locally. --- client/README.md | 2 +- client/scripts/main.gd | 8 +- client/scripts/position_authority_client.gd | 58 +++++-- client/scripts/target_selection_client.gd | 19 +-- client/test/position_authority_client_test.gd | 28 ++++ .../test/target_refresh_on_locomotion_test.gd | 148 ++++++++++++++++++ client/test/target_selection_client_test.gd | 30 ++-- ...umentation_and_implementation_alignment.md | 2 +- docs/manual-qa/NEO-24.md | 12 +- docs/plans/NEO-24-implementation-plan.md | 14 +- docs/reviews/2026-04-21-NEO-24.md | 73 +++++++++ 11 files changed, 335 insertions(+), 59 deletions(-) create mode 100644 client/test/target_refresh_on_locomotion_test.gd create mode 100644 docs/reviews/2026-04-21-NEO-24.md diff --git a/client/README.md b/client/README.md index 49c07ae..f3d411b 100644 --- a/client/README.md +++ b/client/README.md @@ -97,7 +97,7 @@ The main scene includes a **prototype terminal** at the map center (same world * - **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. - **HUD:** `UICanvas/TargetLockLabel` shows the last **server-acknowledged** state: `lockedTargetId` (or **`—`** when no lock), `validity` (`none` / `ok` / `out_of_range` / `invalid_target`), `sequence`, and (on denials) `Denied: ` on the next line. **UI never shows an optimistic lock the server denied** — it always paints from the authoritative `targetState` echoed by each `200` response. -- **Soft lock refresh:** the client does **not** poll on a timer. It refreshes via `GET …/target` on boot, on every `POST …/target/select` response (body already carries `targetState`), and — while a lock is currently held — on **`authoritative_position_received`** from `PositionAuthorityClient`, throttled to at most one GET per **250 ms**. Boot is naturally skipped because no lock exists yet. Purely server-driven state flips (future PvP / combat categories) would require revisiting this policy; today NEO-23 has none. +- **Soft lock refresh:** the client does **not** poll on a timer. It refreshes via `GET …/target` on boot, on every `POST …/target/select` response (body already carries `targetState`), and — while a lock is currently held — on **`authoritative_ack`** from `PositionAuthorityClient`, throttled to at most one GET per **250 ms**. `authoritative_ack` fires on every server-confirmed position (boot `GET` 200 **and** successful `move-stream` 200), so a held lock revalidates during normal WASD locomotion without re-introducing snap rubber-banding. Boot is naturally skipped because no lock exists yet. Purely server-driven state flips (future PvP / combat categories) would require revisiting this policy; today NEO-23 has none. - **Inspector:** match **`base_url`** / **`dev_player_id`** on the `TargetSelectionClient` node to the other HTTP clients / `Game:DevPlayerId`. ### Manual check (NEO-24) diff --git a/client/scripts/main.gd b/client/scripts/main.gd index 63c69c3..aaf0633 100644 --- a/client/scripts/main.gd +++ b/client/scripts/main.gd @@ -84,10 +84,12 @@ func _ready() -> void: ) _authority.connect("move_rejected", Callable(self, "_on_move_rejected")) # NEO-24: movement-driven refresh — `TargetSelectionClient` no-ops on boot (no lock yet) - # and throttles bursts (see Decision 2 in NEO-24 plan). + # and throttles bursts (see Decision 2 in NEO-24 plan). `authoritative_ack` fires on + # every server-confirmed position (boot GET 200 + `move-stream` 200) so a held lock is + # revalidated during ordinary WASD locomotion without re-introducing snap rubber-banding. _authority.connect( - "authoritative_position_received", - Callable(_target_client, "on_authoritative_position_snap") + "authoritative_ack", + Callable(_target_client, "on_authoritative_ack") ) _target_client.connect("target_state_changed", Callable(self, "_on_target_state_changed")) _authority.call("sync_from_server") diff --git a/client/scripts/position_authority_client.gd b/client/scripts/position_authority_client.gd index 6805ecb..3289576 100644 --- a/client/scripts/position_authority_client.gd +++ b/client/scripts/position_authority_client.gd @@ -3,10 +3,14 @@ extends Node ## `GET` position for boot snap; `submit_stream_targets` POSTs `move-stream` (WASD locomotion). ## NS-19: failed validation (HTTP 400 + rejection body) emits `move_rejected`. ## NS-23: second signal arg on `authoritative_position_received` — always `true` (boot snap only). -## A successful `move-stream` 200 is ack-only (no position signal; avoids RTT rubber-band). +## A successful `move-stream` 200 is ack-only (no snap signal; avoids RTT rubber-band). +## NEO-24: `authoritative_ack` fires on **every** server-confirmed position (BOOT_GET 200 **and** +## successful move-stream 200), giving target / cooldown-throttled systems a heartbeat during +## normal locomotion without re-introducing snap rubber-banding. ## (No `class_name` so headless/CI can load the project before `.godot` import exists.) signal authoritative_position_received(world: Vector3, apply_as_snap: bool) +signal authoritative_ack(world: Vector3) signal move_rejected(reason_code: String) enum Phase { BOOT_GET, STREAM_POST } @@ -148,11 +152,12 @@ func _on_request_completed( return _stream_in_flight_count = 0 _busy = false - if response_code == 200: - # Do not emit `authoritative_position_received` here. The body echoes server position - # after applying the batch, which typically lags the client's integrated capsule by - # roughly one RTT — snapping caused visible rubber-banding during WASD. - pass + # Do not emit `authoritative_position_received` here. The body echoes server position + # after applying the batch, which typically lags the client's integrated capsule by + # roughly one RTT — snapping caused visible rubber-banding during WASD. We do emit + # `authoritative_ack` (NEO-24) so cooldown-throttled consumers like + # `TargetSelectionClient` get a heartbeat during normal locomotion. + _emit_ack_if_position_present(text) _try_flush_pending() @@ -172,20 +177,47 @@ func _emit_move_rejection_if_present(json_text: String) -> void: func _emit_position_from_response(json_text: String, apply_as_snap: bool) -> void: + var world_variant: Variant = _parse_world_from_response(json_text, true) + if world_variant == null: + return + var world: Vector3 = world_variant + authoritative_position_received.emit(world, apply_as_snap) + # NEO-24: boot / rejection-resync is also an authoritative ack for cooldown-throttled + # consumers. Emit both so subscribers do not have to dedupe across two code paths. + authoritative_ack.emit(world) + + +## NEO-24: `move-stream` 200 path — echo position is authoritative but intentionally NOT snapped +## (see rubber-band comment in `_on_request_completed`). Still emit `authoritative_ack` so +## `TargetSelectionClient` can run its throttled refresh during ordinary locomotion. +func _emit_ack_if_position_present(json_text: String) -> void: + var world_variant: Variant = _parse_world_from_response(json_text, false) + if world_variant == null: + return + authoritative_ack.emit(world_variant as Vector3) + + +## Shared `{"position":{"x","y","z"}}` parser. `warn_on_missing` lets BOOT_GET complain about +## malformed boot responses while STREAM_POST stays quiet (some 200 bodies may legitimately +## omit `position` in future response shapes; the ack then simply does not fire). +func _parse_world_from_response(json_text: String, warn_on_missing: bool) -> Variant: var parsed: Variant = JSON.parse_string(json_text) if parsed == null: - push_warning("PositionAuthorityClient: position response JSON parse failed") - return + if warn_on_missing: + push_warning("PositionAuthorityClient: position response JSON parse failed") + return null if not parsed is Dictionary: - push_warning("PositionAuthorityClient: position response is not a JSON object") - return + if warn_on_missing: + push_warning("PositionAuthorityClient: position response is not a JSON object") + return null var data: Dictionary = parsed var pos_variant: Variant = data.get("position", null) if not pos_variant is Dictionary: - push_warning("PositionAuthorityClient: position response missing `position` object") - return + if warn_on_missing: + push_warning("PositionAuthorityClient: position response missing `position` object") + return null var pos: Dictionary = pos_variant var x: float = float(pos.get("x", 0.0)) var y: float = float(pos.get("y", 0.0)) var z: float = float(pos.get("z", 0.0)) - authoritative_position_received.emit(Vector3(x, y, z), apply_as_snap) + return Vector3(x, y, z) diff --git a/client/scripts/target_selection_client.gd b/client/scripts/target_selection_client.gd index 3bc86b2..41f2fae 100644 --- a/client/scripts/target_selection_client.gd +++ b/client/scripts/target_selection_client.gd @@ -8,9 +8,11 @@ extends Node ## (first id if no lock) and POSTs `TargetSelectRequest` v1. ## - `request_select_target_id(id)` POSTs a specific id. ## - `request_clear_target()` POSTs with `targetId` omitted (NEO-23 clear rule). -## - Movement-triggered refresh: while a lock is held, `on_authoritative_position_snap(...)` -## fires a throttled `GET` so `validity` flips to `out_of_range` after locomotion without -## duplicating server radius math on the client (plan Decision 2). +## - Movement-triggered refresh: while a lock is held, `on_authoritative_ack(...)` fires a +## throttled `GET` so `validity` flips to `out_of_range` after locomotion without duplicating +## server radius math on the client (plan Decision 2). The ack signal fires on every +## server-confirmed position — both boot and successful `move-stream` — so a held lock is +## revalidated during normal WASD locomotion without re-introducing snap rubber-banding. ## ## Not an `HTTPRequest` subclass — tests inject a `Node` exposing `request()` + ## `request_completed` (same pattern as `position_authority_client.gd`). @@ -101,12 +103,11 @@ func request_clear_target() -> void: _post_select({"schemaVersion": 1}) -## Connected to `PositionAuthorityClient.authoritative_position_received` in `main.gd`. -## Boot is naturally skipped because no lock is held yet; once a lock exists, subsequent -## snaps fire a refresh GET subject to a cooldown so bursts collapse to one GET. -func on_authoritative_position_snap(_world: Vector3, apply_as_snap: bool) -> void: - if not apply_as_snap: - return +## Connected to `PositionAuthorityClient.authoritative_ack` in `main.gd`. +## Boot is naturally skipped because no lock is held yet; once a lock exists, subsequent acks +## (boot resync or `move-stream` 200) fire a refresh GET subject to a cooldown so bursts +## collapse to one GET per window. +func on_authoritative_ack(_world: Vector3) -> void: if not _has_lock(): return if _busy: diff --git a/client/test/position_authority_client_test.gd b/client/test/position_authority_client_test.gd index 5e954c2..cbd3f19 100644 --- a/client/test/position_authority_client_test.gd +++ b/client/test/position_authority_client_test.gd @@ -65,6 +65,16 @@ func test_sync_boot_get_200_emits_snap() -> void: assert_signal(c).is_emitted("authoritative_position_received", Vector3(1.0, 0.9, -5.0), true) +# NEO-24: boot sync also emits the cooldown-throttled heartbeat. +func test_sync_boot_get_200_emits_authoritative_ack() -> void: + var transport := MockHttpTransport.new() + transport.enqueue(HTTPRequest.RESULT_SUCCESS, 200, '{"position":{"x":1,"y":0.9,"z":-5}}') + var c := _make_client(transport) + monitor_signals(c) + c.sync_from_server() + assert_signal(c).is_emitted("authoritative_ack", Vector3(1.0, 0.9, -5.0)) + + func test_sync_boot_get_200_malformed_position_does_not_emit() -> void: var transport := MockHttpTransport.new() transport.enqueue(HTTPRequest.RESULT_SUCCESS, 200, '{"position":"invalid"}') @@ -118,3 +128,21 @@ func test_move_stream_post_200_does_not_emit_position() -> void: monitor_signals(c) c.submit_stream_targets([Vector3(-4.9, 0.9, -5.0)]) assert_signal(c).wait_until(400).is_not_emitted("authoritative_position_received") + + +# NEO-24: move-stream 200 must emit `authoritative_ack` so a held target lock can re-validate +# during normal locomotion (without re-introducing the snap rubber-band). +func test_move_stream_post_200_emits_authoritative_ack() -> void: + var transport := MockHttpTransport.new() + transport.enqueue( + HTTPRequest.RESULT_SUCCESS, + 200, + ( + '{"schemaVersion":1,"playerId":"dev-local-1","position":{"x":-4.9,"y":0.9,"z":-5},' + + '"sequence":3}' + ) + ) + var c := _make_client(transport) + monitor_signals(c) + c.submit_stream_targets([Vector3(-4.9, 0.9, -5.0)]) + assert_signal(c).is_emitted("authoritative_ack", Vector3(-4.9, 0.9, -5.0)) diff --git a/client/test/target_refresh_on_locomotion_test.gd b/client/test/target_refresh_on_locomotion_test.gd new file mode 100644 index 0000000..1d642f3 --- /dev/null +++ b/client/test/target_refresh_on_locomotion_test.gd @@ -0,0 +1,148 @@ +extends GdUnitTestSuite + +## NEO-24 integration test (code-review follow-up to the blocking issue): +## +## `main.gd` connects `PositionAuthorityClient.authoritative_ack` → +## `TargetSelectionClient.on_authoritative_ack`. The unit suites cover each side +## in isolation, but do not prove the two actually meet during normal locomotion +## (a successful `move-stream` POST, not a boot resync). This suite wires the +## **real** scripts together with mock HTTP transports and exercises that path +## end-to-end: with a lock held, a `move-stream` 200 must produce a throttled +## `GET /target` refresh on the target client's transport. + +const PositionAuthorityTestDouble := preload("res://test/position_authority_test_double.gd") +const TargetSelectionTestDouble := preload("res://test/target_selection_test_double.gd") + +const ALPHA_ID := "prototype_target_alpha" + + +# Mirrors the transports from the per-client suites: a queue-backed `Node` with a +# `request()` that synchronously emits `request_completed`. Counts requests so the +# test can assert exactly how many GETs reached the target-client transport. +class MockHttpTransport: + extends Node + signal request_completed( + result: int, response_code: int, headers: PackedStringArray, body: PackedByteArray + ) + var request_count: int = 0 + var last_url: String = "" + var last_method: HTTPClient.Method = HTTPClient.METHOD_GET + var _queue: Array[Dictionary] = [] + + func enqueue(result: int, code: int, body: String) -> void: + _queue.append({"result": result, "code": code, "body": body}) + + func request( + url: String, + _custom_headers: PackedStringArray = PackedStringArray(), + method: HTTPClient.Method = HTTPClient.METHOD_GET, + _request_data: String = "" + ) -> Error: + request_count += 1 + last_url = url + last_method = method + if _queue.is_empty(): + return ERR_UNAVAILABLE + var r: Dictionary = _queue.pop_front() + var body_bytes: PackedByteArray = str(r["body"]).to_utf8_buffer() + request_completed.emit(r["result"], r["code"], PackedStringArray(), body_bytes) + return OK + + +func _make_authority(transport: Node) -> Node: + var a: Node = PositionAuthorityTestDouble.new() + a.set("injected_http", transport) + auto_free(transport) + auto_free(a) + add_child(a) + return a + + +func _make_target_client(transport: Node) -> Node: + var c: Node = TargetSelectionTestDouble.new() + c.set("injected_http", transport) + auto_free(transport) + auto_free(c) + add_child(c) + return c + + +func _select_response_json(locked_id: String, validity: String, sequence: int) -> String: + var state := ( + '{"schemaVersion":1,"playerId":"dev-local-1","lockedTargetId":"%s",' % locked_id + + '"validity":"%s","sequence":%d}' % [validity, sequence] + ) + return '{"schemaVersion":1,"selectionApplied":true,"targetState":%s}' % state + + +func _target_state_json(locked_id: String, validity: String, sequence: int) -> String: + return ( + '{"schemaVersion":1,"playerId":"dev-local-1","lockedTargetId":"%s",' % locked_id + + '"validity":"%s","sequence":%d}' % [validity, sequence] + ) + + +func test_move_stream_200_triggers_target_refresh_while_locked() -> void: + var auth_transport := MockHttpTransport.new() + var target_transport := MockHttpTransport.new() + + # 1) Target client locks alpha via POST. + target_transport.enqueue( + HTTPRequest.RESULT_SUCCESS, 200, _select_response_json(ALPHA_ID, "ok", 1) + ) + # 2) After the move-stream POST, the target client refreshes with GET. + target_transport.enqueue( + HTTPRequest.RESULT_SUCCESS, 200, _target_state_json(ALPHA_ID, "out_of_range", 1) + ) + # 3) The authority's move-stream POST echoes position (triggers `authoritative_ack`). + auth_transport.enqueue( + HTTPRequest.RESULT_SUCCESS, + 200, + ( + '{"schemaVersion":1,"playerId":"dev-local-1","position":{"x":-20,"y":0.9,"z":-20},' + + '"sequence":3}' + ) + ) + + var authority := _make_authority(auth_transport) + var target_client := _make_target_client(target_transport) + + # Same wiring as `main.gd` so this test fails loudly if someone renames the signal + # or handler without updating both ends. + authority.connect("authoritative_ack", Callable(target_client, "on_authoritative_ack")) + + # Establish the lock first. + target_client.request_select_target_id(ALPHA_ID) + assert_that(target_client.cached_state().get("lockedTargetId")).is_equal(ALPHA_ID) + var get_count_baseline: int = target_transport.request_count + + # Simulate WASD locomotion: authority POSTs move-stream, server echoes 200. + authority.submit_stream_targets([Vector3(-20.0, 0.9, -20.0)]) + + # The target client must have issued exactly one refresh GET in response to the ack. + var new_requests: int = target_transport.request_count - get_count_baseline + assert_that(new_requests).is_equal(1) + assert_that(target_transport.last_url).contains("/game/players/dev-local-1/target") + assert_that(target_transport.last_method).is_equal(HTTPClient.METHOD_GET) + assert_that(target_client.cached_state().get("validity")).is_equal("out_of_range") + # Soft lock is preserved on out-of-range. + assert_that(target_client.cached_state().get("lockedTargetId")).is_equal(ALPHA_ID) + + +func test_move_stream_200_does_not_refresh_without_lock() -> void: + var auth_transport := MockHttpTransport.new() + var target_transport := MockHttpTransport.new() + + auth_transport.enqueue( + HTTPRequest.RESULT_SUCCESS, + 200, + '{"schemaVersion":1,"playerId":"dev-local-1","position":{"x":0,"y":0.9,"z":0},"sequence":1}' + ) + + var authority := _make_authority(auth_transport) + var target_client := _make_target_client(target_transport) + authority.connect("authoritative_ack", Callable(target_client, "on_authoritative_ack")) + + authority.submit_stream_targets([Vector3.ZERO]) + + assert_that(target_transport.request_count).is_equal(0) diff --git a/client/test/target_selection_client_test.gd b/client/test/target_selection_client_test.gd index 3e9a802..eabcfb7 100644 --- a/client/test/target_selection_client_test.gd +++ b/client/test/target_selection_client_test.gd @@ -169,7 +169,7 @@ func test_clear_issues_post_without_target_id() -> void: assert_that(transport.last_body).not_contains("targetId") -func test_position_snap_while_locked_fires_refresh_get() -> void: +func test_authoritative_ack_while_locked_fires_refresh_get() -> void: var transport := MockHttpTransport.new() # Step 1: lock alpha via POST. transport.enqueue( @@ -181,21 +181,21 @@ func test_position_snap_while_locked_fires_refresh_get() -> void: ) var c := _make_client(transport) c.request_select_target_id(ALPHA_ID) - var count_before_snap: int = transport.request_count - c.on_authoritative_position_snap(Vector3.ZERO, true) - assert_that(transport.request_count).is_equal(count_before_snap + 1) + var count_before_ack: int = transport.request_count + c.on_authoritative_ack(Vector3.ZERO) + assert_that(transport.request_count).is_equal(count_before_ack + 1) assert_that(c.cached_state().get("validity")).is_equal("out_of_range") assert_that(c.cached_state().get("lockedTargetId")).is_equal(ALPHA_ID) -func test_position_snap_without_lock_fires_nothing() -> void: +func test_authoritative_ack_without_lock_fires_nothing() -> void: var transport := MockHttpTransport.new() var c := _make_client(transport) - c.on_authoritative_position_snap(Vector3.ZERO, true) + c.on_authoritative_ack(Vector3.ZERO) assert_that(transport.request_count).is_equal(0) -func test_two_snaps_within_cooldown_collapse_to_one_get() -> void: +func test_two_acks_within_cooldown_collapse_to_one_get() -> void: var transport := MockHttpTransport.new() transport.enqueue( HTTPRequest.RESULT_SUCCESS, 200, _select_response_json(true, ALPHA_ID, "ok", 1) @@ -206,18 +206,6 @@ func test_two_snaps_within_cooldown_collapse_to_one_get() -> void: var c := _make_client(transport) c.request_select_target_id(ALPHA_ID) var count_before: int = transport.request_count - c.on_authoritative_position_snap(Vector3.ZERO, true) - c.on_authoritative_position_snap(Vector3.ZERO, true) + c.on_authoritative_ack(Vector3.ZERO) + c.on_authoritative_ack(Vector3.ZERO) assert_that(transport.request_count).is_equal(count_before + 1) - - -func test_position_snap_apply_as_snap_false_is_ignored() -> void: - 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_select_target_id(ALPHA_ID) - var count_before: int = transport.request_count - c.on_authoritative_position_snap(Vector3.ZERO, false) - assert_that(transport.request_count).is_equal(count_before) diff --git a/docs/decomposition/modules/documentation_and_implementation_alignment.md b/docs/decomposition/modules/documentation_and_implementation_alignment.md index 21d2a69..05413b1 100644 --- a/docs/decomposition/modules/documentation_and_implementation_alignment.md +++ b/docs/decomposition/modules/documentation_and_implementation_alignment.md @@ -48,7 +48,7 @@ Rows appear when work starts; default for unlisted modules is **Planned** / not |--------|-----------------|----------|-------------------| | E1.M1 | Ready | Prototype milestone **Done** ([E1.M1](https://linear.app/neon-sprawl/project/e1m1-inputandmovementruntime-20eb28dd3db4)). Authoritative `PositionState` + **MoveCommand** over HTTP (JSON v1 snap); **in-memory** store by default, **Postgres** when configured ([NEO-8](../../plans/NEO-8-implementation-plan.md)); shared **NpgsqlDataSource** disposed on host shutdown ([NEO-13](../../plans/NEO-13-implementation-plan.md)); Godot sync + path-follow ([NEO-7](../../plans/NEO-7-implementation-plan.md), [NEO-11](../../plans/NEO-11-implementation-plan.md)); **InteractionRequest** + horizontal range ([NEO-9](../../plans/NEO-9-implementation-plan.md)). Follow-on: prediction/reconciliation, Slice 1 telemetry, Protobuf wire per [contracts.md](contracts.md). | [NEO-6](../../plans/NEO-6-implementation-plan.md), [NEO-7](../../plans/NEO-7-implementation-plan.md), [NEO-8](../../plans/NEO-8-implementation-plan.md), [NEO-9](../../plans/NEO-9-implementation-plan.md), [NEO-10](../../plans/NEO-10-implementation-plan.md), [NEO-11](../../plans/NEO-11-implementation-plan.md), [NEO-13](../../plans/NEO-13-implementation-plan.md); `server/NeonSprawl.Server/Game/PositionState/`, `Game/Interaction/`; [server README](../../../server/README.md) | | E1.M2 | Ready | **Slice 2 prototype slice closed:** follow + `CameraState` ([NEO-15](https://linear.app/neon-sprawl/issue/NEO-15)); zoom bands ([NEO-16](https://linear.app/neon-sprawl/issue/NEO-16)); occlusion ([NEO-17](https://linear.app/neon-sprawl/issue/NEO-17)); occluder pick-through ([NEO-20](https://linear.app/neon-sprawl/issue/NEO-20)); contracts + hardening ([NEO-18](https://linear.app/neon-sprawl/issue/NEO-18)). Client-local; no server use of camera pose. | [NEO-15](../../plans/NEO-15-implementation-plan.md), [NEO-16](../../plans/NEO-16-implementation-plan.md), [NEO-17](../../plans/NEO-17-implementation-plan.md), [NEO-18](../../plans/NEO-18-implementation-plan.md), [NEO-20](../../plans/NEO-20-implementation-plan.md); `client/scripts/isometric_follow_camera.gd`, `camera_state.gd`, `zoom_band_config.gd`, `occlusion_policy.gd`, `ground_pick.gd`; [E1_M2_IsometricCameraController.md](E1_M2_IsometricCameraController.md) | -| E1.M3 | In Progress | **NEO-23 landed:** JSON v1 targeting read + select (`GET`/`POST …/target`, `Game/Targeting/`, [NEO-23](../../plans/NEO-23-implementation-plan.md)) — `PlayerTargetStateResponse` / soft lock / `reasonCode` denials; wire name differs from illustrative **`TargetState`** in module doc (called out in plan + README). **`InteractableDescriptor`**, **`SelectionEvent`**, and client tab-target ([NEO-24](https://linear.app/neon-sprawl/issue/NEO-24)) still open. **Precursor (E1.M1):** [NEO-9](../../plans/NEO-9-implementation-plan.md) `POST …/interact`. | Linear [NEO-24](https://linear.app/neon-sprawl/issue/NEO-24)–[NEO-27](https://linear.app/neon-sprawl/issue/NEO-27); [E1_M3_InteractionAndTargetingLayer.md](E1_M3_InteractionAndTargetingLayer.md); [E1M3 prototype backlog](../../plans/E1M3-prototype-backlog.md); [server README — Targeting](../../../server/README.md#targeting-neo-23), [Interaction](../../../server/README.md#interaction-neo-9) | +| E1.M3 | In Progress | **NEO-23 landed:** JSON v1 targeting read + select (`GET`/`POST …/target`, `Game/Targeting/`, [NEO-23](../../plans/NEO-23-implementation-plan.md)) — `PlayerTargetStateResponse` / soft lock / `reasonCode` denials; wire name differs from illustrative **`TargetState`** in module doc (called out in plan + README). **NEO-24 landed:** client tab-target + lock HUD synced to server ([NEO-24](../../plans/NEO-24-implementation-plan.md)) — `TargetSelectionClient` ([`client/scripts/target_selection_client.gd`](../../../client/scripts/target_selection_client.gd)), Tab / Esc bindings, `TargetLockLabel` HUD, hybrid movement-triggered refresh via new `PositionAuthorityClient.authoritative_ack` signal (fires on every server-confirmed position, boot + `move-stream` 200) with a 250 ms cooldown; manual QA in [`docs/manual-qa/NEO-24.md`](../../manual-qa/NEO-24.md). **`InteractableDescriptor`** and **`SelectionEvent`** still open. **Precursor (E1.M1):** [NEO-9](../../plans/NEO-9-implementation-plan.md) `POST …/interact`. | Linear [NEO-24](https://linear.app/neon-sprawl/issue/NEO-24)–[NEO-27](https://linear.app/neon-sprawl/issue/NEO-27); [E1_M3_InteractionAndTargetingLayer.md](E1_M3_InteractionAndTargetingLayer.md); [E1M3 prototype backlog](../../plans/E1M3-prototype-backlog.md); [server README — Targeting](../../../server/README.md#targeting-neo-23), [Interaction](../../../server/README.md#interaction-neo-9) | --- diff --git a/docs/manual-qa/NEO-24.md b/docs/manual-qa/NEO-24.md index 7da6fc7..137bbbf 100644 --- a/docs/manual-qa/NEO-24.md +++ b/docs/manual-qa/NEO-24.md @@ -43,13 +43,13 @@ Pair the client (Godot **F5**) with the server (`cd server/NeonSprawl.Server && ## 4. Soft-lock out-of-range via locomotion (plan Decision 2) -- [ ] Still locked to alpha, **WASD** away from spawn until you are ~8 m horizontally from `(-5, -5)`. Within ~250 ms of the next authority snap, HUD flips to: +- [ ] Still locked to alpha, **WASD** away from spawn until you are ~8 m horizontally from `(-5, -5)`. Within ~250 ms of the next `move-stream` 200 (each successful POST emits `authoritative_ack` → throttled target `GET`), HUD flips to: - `Target: prototype_target_alpha` (unchanged — soft lock) - `Validity: out_of_range` - `Seq: 1` (unchanged — no lock id change) -- [ ] Walk back inside the radius. HUD returns to `Validity: ok` within ~250 ms of the next snap. -- [ ] While walking, Godot Output is **not** spammed with targeting GETs. Server log shows no more than roughly one targeting `GET` per 250 ms during sustained motion. -- [ ] Stand still with no lock changes. Server log shows **no** targeting GETs at idle (no timer polling). +- [ ] Walk back inside the radius. HUD returns to `Validity: ok` within ~250 ms of the next `move-stream` ack. +- [ ] While walking, Godot Output is **not** spammed with targeting GETs. Server log shows no more than roughly one targeting `GET` per 250 ms during sustained motion, even though `move-stream` POSTs fire at ~20 Hz. +- [ ] Stand still with no lock changes. Server log shows **no** targeting GETs at idle (no `move-stream` is sent while idle, so there is no ack to trigger a refresh). ## 5. Swap to beta when in range @@ -83,9 +83,9 @@ Pair the client (Godot **F5**) with the server (`cd server/NeonSprawl.Server && ## 9. Coexistence with movement rejection (NEO-19 / move-stream 400) -- [ ] Walk at the orange `MoveRejectPedestal` (~(7.5, −6.5)) while holding a lock. When `move-stream` returns **400** (`vertical_step_exceeded`), `PositionAuthorityClient` runs a boot-style re-sync which emits `authoritative_position_received`. +- [ ] Walk at the orange `MoveRejectPedestal` (~(7.5, −6.5)) while holding a lock. When `move-stream` returns **400** (`vertical_step_exceeded`), `PositionAuthorityClient` runs a boot-style re-sync which emits both `authoritative_position_received` (for the snap) and `authoritative_ack` (for cooldown-throttled consumers). - [ ] HUD's `MoveRejectLabel` shows the rejection message as before (NEO-19). -- [ ] `TargetSelectionClient` treats that snap like any other: if a lock is held, it fires at most one refresh GET (subject to cooldown). No duplicate requests, no stuck busy state. +- [ ] `TargetSelectionClient` treats that ack like any other: if a lock is held, it fires at most one refresh GET (subject to cooldown). No duplicate requests, no stuck busy state. ## 10. Server-down / transport failure diff --git a/docs/plans/NEO-24-implementation-plan.md b/docs/plans/NEO-24-implementation-plan.md index 85de5d8..1dbeea9 100644 --- a/docs/plans/NEO-24-implementation-plan.md +++ b/docs/plans/NEO-24-implementation-plan.md @@ -52,11 +52,11 @@ Server-up manual QA steps live in the README section above; run the server + cli - **`request_clear_target()`** — `POST` with no `targetId` / `null` per NEO-23. - **`request_tab_next()`** — read cached `lockedTargetId` + `validity`; compute **next** id in the ordered list (if no lock, choose **first** id in order); POST select; on deny, signal carries **server** `targetState` only (no client-only lock). - Signal e.g. **`target_state_changed(state: Dictionary)`** or typed fields for consumers (`lockedTargetId` Variant string-or-null, `validity` string, `sequence` int, `selection_applied` bool optional for last POST). - - **Refresh policy (hybrid, movement-triggered — Decision 2):** no periodic polling. Issue **`GET …/target`** when (a) boot sync runs, (b) any `POST …/target/select` completes (covered by response body already — no extra GET needed), and (c) **while a lock is currently held**, the `PositionAuthorityClient` emits **`authoritative_position_received`** *after* boot (i.e. movement-driven snaps, not the initial boot snap). A small **cooldown** (e.g. **250 ms**) coalesces bursts so many consecutive `move-stream` acks cannot produce a GET per ack. No timer otherwise. Rationale: stays honest about `out_of_range` without duplicating radius math on the client or holding an always-on polling timer. Trade-off: will not pick up purely server-driven state changes (none exist in NEO-23; revisit if combat/PvP adds them). + - **Refresh policy (hybrid, movement-triggered — Decision 2):** no periodic polling. Issue **`GET …/target`** when (a) boot sync runs, (b) any `POST …/target/select` completes (covered by response body already — no extra GET needed), and (c) **while a lock is currently held**, `PositionAuthorityClient` emits a new **`authoritative_ack(world)`** signal — fired on **every** server-confirmed position (boot `GET` 200 **and** successful `move-stream` 200). A small **cooldown** (e.g. **250 ms**) coalesces bursts so many consecutive `move-stream` acks cannot produce a GET per ack. No timer otherwise. Rationale: stays honest about `out_of_range` without duplicating radius math on the client or holding an always-on polling timer. Trade-off: will not pick up purely server-driven state changes (none exist in NEO-23; revisit if combat/PvP adds them). **Why `authoritative_ack` instead of `authoritative_position_received`:** the latter only fires on boot / move-rejection resync — normal `move-stream` 200s are intentionally silent to avoid RTT rubber-banding — so hooking the target refresh to it leaves the soft-lock path unreachable during WASD locomotion (blocking issue raised in [2026-04-21-NEO-24 review](../reviews/2026-04-21-NEO-24.md)). The new signal carries only the world position (no `apply_as_snap` flag) and is emitted after every authoritative position regardless of whether the client snaps. 3. **Optimistic highlight (optional slice):** If implemented, keep **pending** selection separate from **acknowledged** state; on **200** denial, drop pending and paint from **`targetState`**. If timeboxed, **v1** may ship **ack-only** UI (still satisfies “optional optimistic” — none used). -4. **`main.tscn` / `main.gd`:** Add child node with `target_selection_client.gd`; connect signal to a **Label** (new `TargetLockLabel` under `UICanvas`). Call **`request_sync_from_server()`** once after boot position sync (or same frame as first authority ready) so HUD matches server on start. **Also** connect `PositionAuthorityClient.authoritative_position_received` → `target_selection_client.on_authoritative_position_snap(...)` (the client ignores the boot snap per `apply_as_snap` flag and only fires a GET when a lock is currently held; see step 2 refresh policy). +4. **`main.tscn` / `main.gd`:** Add child node with `target_selection_client.gd`; connect signal to a **Label** (new `TargetLockLabel` under `UICanvas`). Call **`request_sync_from_server()`** once after boot position sync (or same frame as first authority ready) so HUD matches server on start. **Also** connect `PositionAuthorityClient.authoritative_ack` → `target_selection_client.on_authoritative_ack(...)` (the handler is a no-op when no lock is held, so boot is naturally skipped; see step 2 refresh policy). 5. **`project.godot`:** `target_tab` → **Tab**; `target_clear` → **Escape** (locked — Decision 1). @@ -76,7 +76,8 @@ Server-up manual QA steps live in the README section above; run the server + cli | Path | Rationale | |------|-----------| | `client/scenes/main.tscn` | Add `TargetSelectionClient` node + **`TargetLockLabel`** (or reuse a debug panel) bound to scripts. | -| `client/scripts/main.gd` | Wire `@onready` target client + label; connect `target_state_changed`; trigger initial `request_sync_from_server()` after authority boot path. | +| `client/scripts/main.gd` | Wire `@onready` target client + label; connect `target_state_changed`; trigger initial `request_sync_from_server()` after authority boot path; connect `authoritative_ack` → `on_authoritative_ack` (see Decision 2). | +| `client/scripts/position_authority_client.gd` | Add `authoritative_ack(world)` signal and emit it from both `BOOT_GET` 200 and `STREAM_POST` 200 so held locks re-validate during normal WASD locomotion without re-introducing snap rubber-banding. Extracted `_parse_world_from_response(...)` helper to avoid duplicating JSON parse across the two paths. | | `client/project.godot` | Register **`target_tab`** and **`target_clear`** input actions with defaults above. | | `client/README.md` | One short subsection: tab-target prototype, bindings, and how to read the lock overlay (parity with interaction/move docs). | @@ -84,7 +85,9 @@ Server-up manual QA steps live in the README section above; run the server + cli | Test file | What to cover | |-----------|----------------| -| `client/test/target_selection_client_test.gd` | **Mock transport** enqueues JSON: GET returns a v1 envelope; POST **apply** returns `selectionApplied: true` + `targetState`; POST **deny** returns `selectionApplied: false` + `reasonCode` + authoritative `targetState`; **tab** from no lock selects first id in order; **tab** from `alpha` requests `beta`; wrap from `beta` to `alpha`; **clear** issues POST without `targetId`; **movement-triggered refresh:** calling `on_authoritative_position_snap(...)` while a lock is held fires a GET, while **no** lock is held fires **nothing**, and two back-to-back snaps within cooldown produce **one** GET. Use `MockHttpTransport` pattern from `position_authority_client_test.gd`. | +| `client/test/target_selection_client_test.gd` | **Mock transport** enqueues JSON: GET returns a v1 envelope; POST **apply** returns `selectionApplied: true` + `targetState`; POST **deny** returns `selectionApplied: false` + `reasonCode` + authoritative `targetState`; **tab** from no lock selects first id in order; **tab** from `alpha` requests `beta`; wrap from `beta` to `alpha`; **clear** issues POST without `targetId`; **movement-triggered refresh:** calling `on_authoritative_ack(...)` while a lock is held fires a GET, while **no** lock is held fires **nothing**, and two back-to-back acks within cooldown produce **one** GET. Use `MockHttpTransport` pattern from `position_authority_client_test.gd`. | +| `client/test/position_authority_client_test.gd` | Extend the existing suite to assert that `authoritative_ack` is emitted on both `BOOT_GET` 200 and successful `move-stream` 200 (existing no-snap-on-stream-200 assertion stays). | +| `client/test/target_refresh_on_locomotion_test.gd` | **Integration** (added during review follow-up): wires a real `PositionAuthorityClient` + `TargetSelectionClient` with mock transports and the same signal wiring `main.gd` uses; asserts that with a lock held, a `move-stream` 200 produces exactly one throttled refresh `GET` on the target client, and that the same path is a no-op with no lock. Catches renames / mis-wires between the two scripts that the unit suites miss. | No new **C#** tests (client-only story). **Bruno:** NEO-23 targeting requests already live under `bruno/neon-sprawl-server/targeting/`; **no** new `.bru` required unless implementation discovers a **server** contract change (then add per repo [testing expectations](../../.cursor/rules/testing-expectations.md) for HTTP contract changes). @@ -98,4 +101,5 @@ No new **C#** tests (client-only story). **Bruno:** NEO-23 targeting requests al | # | Topic | Choice | |---|-------|--------| | 1 | **Input bindings** | `target_tab` → **Tab**; `target_clear` → **Escape**. Registered in `client/project.godot`. | -| 2 | **Soft-lock refresh policy** | **Hybrid, movement-triggered.** No periodic polling. Refresh sources: boot `GET`, each `POST` response echo, **and** `GET` fired from `PositionAuthorityClient.authoritative_position_received` (non-boot snaps) **only while a lock is currently held**, with a **~250 ms** cooldown to coalesce bursts. Does **not** duplicate radius math on the client; does **not** cover future purely server-driven state changes (revisit when E5.M1 / E6.M1 introduce them). | +| 2 | **Soft-lock refresh policy** | **Hybrid, movement-triggered.** No periodic polling. Refresh sources: boot `GET`, each `POST` response echo, **and** `GET` fired from `PositionAuthorityClient.authoritative_ack` (new signal — fires on every server-confirmed position, including `move-stream` 200) **only while a lock is currently held**, with a **~250 ms** cooldown to coalesce bursts. Does **not** duplicate radius math on the client; does **not** cover future purely server-driven state changes (revisit when E5.M1 / E6.M1 introduce them). | +| 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. | diff --git a/docs/reviews/2026-04-21-NEO-24.md b/docs/reviews/2026-04-21-NEO-24.md new file mode 100644 index 0000000..45fbb94 --- /dev/null +++ b/docs/reviews/2026-04-21-NEO-24.md @@ -0,0 +1,73 @@ +# Review — NEO-24 + +Date: 2026-04-21 +Scope: `NEO-24-e1m3-client-tab-target-lock-ui-synced-to-server` (`origin/main...HEAD`) +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. + +## Verdict + +~~Request changes~~ — **Addressed.** Blocking issue fixed; integration test added; doc-alignment row updated. + +## Summary + +This branch adds the client-side target selection node, HUD label, input bindings, tests, and story docs for NEO-24. The overall direction matches the E1.M3 plan well: selection intent is POSTed to the server, the HUD paints only server-acknowledged target state, and denial handling is intentionally authoritative. + +The main risk is that the advertised movement-driven soft-lock refresh is not actually wired to normal locomotion. The new unit suite passes, but it only proves `TargetSelectionClient` reacts correctly when its refresh hook is called directly; it does not cover the real signal behavior of `PositionAuthorityClient`, so the branch can merge with a broken end-to-end acceptance criterion. + +## Documentation checked + +- `docs/plans/NEO-24-implementation-plan.md` — **conflicts**. Decision 2 and the acceptance criteria require movement-triggered refresh while a lock is held, but the current wiring only refreshes on boot sync / rejection resync, not on ordinary movement. +- `docs/manual-qa/NEO-24.md` — **conflicts**. Section 4 expects walking out of range to flip HUD validity within ~250 ms during normal locomotion; that flow is not reachable with the current signal source. +- `docs/decomposition/modules/E1_M3_InteractionAndTargetingLayer.md` — **matches** for server-authoritative target state and denial reconciliation. +- `docs/decomposition/modules/client_server_authority.md` — **matches** for client-as-intent / server-as-truth behavior. +- `docs/decomposition/modules/module_dependency_register.md` — **matches**. `E1.M3` remains `In Progress`. +- `docs/decomposition/modules/documentation_and_implementation_alignment.md` — **partially matches**. The `E1.M3` tracking row still says client tab-target is open; if/when this story lands, that snapshot should be updated to reflect NEO-24 progress. + +## Blocking issues + +1. ~~The movement-driven refresh path described by the plan, README, and manual QA is not reachable during normal locomotion. `main.gd` connects `TargetSelectionClient.on_authoritative_position_snap()` to `PositionAuthorityClient.authoritative_position_received`, but `position_authority_client.gd` only emits that signal from `_emit_position_from_response()` during `sync_from_server()` and the move-rejection recovery path. Successful `move-stream` POSTs explicitly do not emit the signal, so walking around with a held lock never triggers the throttled `GET /target` refresh that NEO-24 depends on. As a result, the HUD can stay stuck at `Validity: ok` after the player walks out of range, which breaks the plan's Decision 2, the acceptance criterion for soft-lock refresh, and manual QA section 4.~~ **Done.** Added a separate `authoritative_ack(world)` signal on `PositionAuthorityClient` that fires on both `BOOT_GET` 200 and successful `move-stream` 200 (extracted a shared `_parse_world_from_response` helper). `authoritative_position_received` keeps its current snap-only semantics so rubber-band suppression is untouched. `TargetSelectionClient.on_authoritative_ack(world)` replaces `on_authoritative_position_snap`; cooldown + lock gate unchanged. `main.gd` connects to the new signal. See `client/scripts/position_authority_client.gd`, `target_selection_client.gd`, `main.gd`, and plan Decision 3. + + Relevant behavior in `client/scripts/position_authority_client.gd`: + + ```text + match _phase: + Phase.BOOT_GET: + _busy = false + if response_code == 200: + _emit_position_from_response(text, true) + Phase.STREAM_POST: + ... + if response_code == 200: + # Do not emit `authoritative_position_received` here. + pass + ``` + + Meanwhile `client/scripts/target_selection_client.gd` only refreshes from that signal: + + ```text + func on_authoritative_position_snap(_world: Vector3, apply_as_snap: bool) -> void: + if not apply_as_snap: + return + if not _has_lock(): + return + ... + request_sync_from_server() + ``` + + The fix needs to either emit a suitable movement/reconciliation signal during normal locomotion or hook the target refresh to another authoritative movement event that actually occurs while moving. + +## Suggestions + +1. ~~Add an integration-level test for the real movement/targeting wiring. The current `client/test/target_selection_client_test.gd` suite calls `on_authoritative_position_snap()` directly, which is why it misses the broken connection to `PositionAuthorityClient`. A focused test around `main.gd` wiring or `PositionAuthorityClient` signal behavior would prevent this regression.~~ **Done.** Added `client/test/target_refresh_on_locomotion_test.gd`: wires a real `PositionAuthorityClient` and `TargetSelectionClient` with mock transports and the same `authoritative_ack` → `on_authoritative_ack` connection `main.gd` uses. Verifies (a) a `move-stream` 200 with a held lock produces exactly one throttled `GET /target` and the cached state flips to `out_of_range`, and (b) the same path is a no-op when no lock is held. Also added `test_sync_boot_get_200_emits_authoritative_ack` and `test_move_stream_post_200_emits_authoritative_ack` to `position_authority_client_test.gd` so renames on either side will break the per-script suites too. +2. ~~Update `docs/decomposition/modules/documentation_and_implementation_alignment.md` if this story lands. Its `E1.M3` snapshot still says client tab-target is open, which will be stale after merge.~~ **Done.** E1.M3 row now reflects NEO-24 landed (HUD + `authoritative_ack` refresh) with links to the plan and manual QA file. + +## Nits + +None. + +## Verification + +- Ran `godot --headless --import --path . --quit-after 10` from `client/`. +- Ran `godot --headless --path . -s res://addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode -a res://test/target_selection_client_test.gd` from `client/` — suite passed. +- I did not run the full manual client/server checklist, but `docs/manual-qa/NEO-24.md` section 4 is the critical end-to-end check to rerun after fixing the movement refresh trigger. From 33ebfc31c4817a480d3d1ad092b37c9541588c58 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Tue, 21 Apr 2026 22:44:25 -0400 Subject: [PATCH 08/13] NEO-24: fix HUD overlap between PlayerPositionLabel and TargetLockLabel PlayerPositionLabel's 3 lines at font_size=15 + outline=6 overran its 64 px rect, so TargetLockLabel (offset_top=82) rendered the `Target:` row on top of the `z:` row. Bump position-label bottom to 88 and push target-label top to 104 for clean separation; widen target-label rect to 320x120 so the optional 4th `Denied: ` line fits without clipping. --- client/scenes/main.tscn | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/client/scenes/main.tscn b/client/scenes/main.tscn index 47ec26b..6db5e59 100644 --- a/client/scenes/main.tscn +++ b/client/scenes/main.tscn @@ -1133,7 +1133,7 @@ autowrap_mode = 3 offset_left = 8.0 offset_top = 8.0 offset_right = 220.0 -offset_bottom = 72.0 +offset_bottom = 88.0 grow_horizontal = 0 grow_vertical = 0 theme_override_colors/font_color = Color(0.92, 0.95, 0.98, 1) @@ -1146,9 +1146,9 @@ z: —" [node name="TargetLockLabel" type="Label" parent="UICanvas" unique_id=9000004] offset_left = 8.0 -offset_top = 82.0 -offset_right = 280.0 -offset_bottom = 150.0 +offset_top = 104.0 +offset_right = 320.0 +offset_bottom = 224.0 grow_horizontal = 0 grow_vertical = 0 theme_override_colors/font_color = Color(0.86, 0.94, 1, 1) From 71ae8294826541380f3432751abd59007ed3d704 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Tue, 21 Apr 2026 23:04:03 -0400 Subject: [PATCH 09/13] NEO-24: add visible target-anchor markers + HUD distance lines MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Players reported targeting "feels inconsistent" because the server PrototypeTargetRegistry anchors (alpha (-5,-5) r=8, beta (8,8) r=4) have no visible meshes — the only console visible in the scene is the NEO-9 PrototypeTerminal at origin, which is unrelated to NEO-23/24 targeting. Adds a display-only mirror of the server registry: - client/scripts/prototype_target_markers.gd spawns a colored mast + translucent flat radius ring per anchor so the lock radius is visible in-world. - client/scripts/prototype_target_constants.gd gains ANCHORS with position/radius/color, matching PrototypeTargetRegistry.cs. Marked display-only; server remains authoritative for validity. - TargetLockLabel now renders a per-anchor distance line each physics tick (": m / (in|out)") so players can see the relationship between the visible capsule and the lock radius and immediately spot client/server position drift. Also updates README, plan (Decision 4), manual QA (Section 1 boot + Section 4 locomotion), and the review file's follow-up section to describe the UX fix. --- client/README.md | 3 +- client/scenes/main.tscn | 8 +- client/scripts/main.gd | 50 ++++++-- client/scripts/prototype_target_constants.gd | 52 +++++++++ client/scripts/prototype_target_markers.gd | 109 ++++++++++++++++++ .../scripts/prototype_target_markers.gd.uid | 1 + .../target_refresh_on_locomotion_test.gd.uid | 1 + docs/manual-qa/NEO-24.md | 4 + docs/plans/NEO-24-implementation-plan.md | 7 +- docs/reviews/2026-04-21-NEO-24.md | 21 +++- 10 files changed, 233 insertions(+), 23 deletions(-) create mode 100644 client/scripts/prototype_target_markers.gd create mode 100644 client/scripts/prototype_target_markers.gd.uid create mode 100644 client/test/target_refresh_on_locomotion_test.gd.uid diff --git a/client/README.md b/client/README.md index f3d411b..d5621b4 100644 --- a/client/README.md +++ b/client/README.md @@ -96,7 +96,8 @@ 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. -- **HUD:** `UICanvas/TargetLockLabel` shows the last **server-acknowledged** state: `lockedTargetId` (or **`—`** when no lock), `validity` (`none` / `ok` / `out_of_range` / `invalid_target`), `sequence`, and (on denials) `Denied: ` on the next line. **UI never shows an optimistic lock the server denied** — it always paints from the authoritative `targetState` echoed by each `200` response. +- **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. +- **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. - **Soft lock refresh:** the client does **not** poll on a timer. It refreshes via `GET …/target` on boot, on every `POST …/target/select` response (body already carries `targetState`), and — while a lock is currently held — on **`authoritative_ack`** from `PositionAuthorityClient`, throttled to at most one GET per **250 ms**. `authoritative_ack` fires on every server-confirmed position (boot `GET` 200 **and** successful `move-stream` 200), so a held lock revalidates during normal WASD locomotion without re-introducing snap rubber-banding. Boot is naturally skipped because no lock exists yet. Purely server-driven state flips (future PvP / combat categories) would require revisiting this policy; today NEO-23 has none. - **Inspector:** match **`base_url`** / **`dev_player_id`** on the `TargetSelectionClient` node to the other HTTP clients / `Game:DevPlayerId`. diff --git a/client/scenes/main.tscn b/client/scenes/main.tscn index 6db5e59..d8e3903 100644 --- a/client/scenes/main.tscn +++ b/client/scenes/main.tscn @@ -5,6 +5,7 @@ [ext_resource type="Script" uid="uid://ds5fkbscljkxi" path="res://scripts/position_authority_client.gd" id="4_auth"] [ext_resource type="Script" uid="uid://bv0xprp660hib" path="res://scripts/interaction_request_client.gd" id="5_ix"] [ext_resource type="Script" uid="uid://gh6yv5p1y0vq" path="res://scripts/target_selection_client.gd" id="11_tgt_sel"] +[ext_resource type="Script" uid="uid://crd46axw4gkjn" path="res://scripts/prototype_target_markers.gd" id="12_tgt_mk"] [ext_resource type="Script" uid="uid://n3p4f3fky3nv" path="res://scripts/interaction_radius_indicators.gd" id="6_rad"] [ext_resource type="Script" uid="uid://caaj3ohikfdx2" path="res://scripts/isometric_follow_camera.gd" id="7_iso_cam"] [ext_resource type="Resource" path="res://resources/isometric_zoom_bands.tres" id="8_zoom_bands"] @@ -1070,6 +1071,9 @@ surface_material_override/0 = SubResource("Mat_physics_ramp_test") [node name="CollisionShape3D" type="CollisionShape3D" parent="World/PhysicsRampTestSteepRamp" unique_id=4201032] shape = SubResource("BoxShape3D_physics_ramp_test_steep") +[node name="PrototypeTargetMarkers" type="Node3D" parent="World" unique_id=1700010] +script = ExtResource("12_tgt_mk") + [node name="InteractionMarkers" type="Node3D" parent="World" unique_id=1700003] script = ExtResource("6_rad") @@ -1147,8 +1151,8 @@ z: —" [node name="TargetLockLabel" type="Label" parent="UICanvas" unique_id=9000004] offset_left = 8.0 offset_top = 104.0 -offset_right = 320.0 -offset_bottom = 224.0 +offset_right = 340.0 +offset_bottom = 288.0 grow_horizontal = 0 grow_vertical = 0 theme_override_colors/font_color = Color(0.86, 0.94, 1, 1) diff --git a/client/scripts/main.gd b/client/scripts/main.gd index aaf0633..d150adf 100644 --- a/client/scripts/main.gd +++ b/client/scripts/main.gd @@ -37,9 +37,14 @@ const AUTH_SNAP_LOCOMOTION_SKIP_MAX_DH: float = 0.55 ## Horizontal speed² above this (m²/s²) counts as “moving” for snap suppression (~0.35 m/s). const AUTH_SNAP_LOCOMOTION_VEL_HZ_SQ: float = 0.12 +const PrototypeTargetConstants := preload("res://scripts/prototype_target_constants.gd") + ## Bump on each rejection so older one-shot timers do not clear a newer message. var _move_reject_msg_token: int = 0 var _stream_physics_counter: int = 0 +## Cached latest `target_state_changed` payload so the HUD can re-render with live +## distance readouts on every physics tick without missing server-side fields. +var _last_target_state: Dictionary = {} ## After the first `apply_as_snap` authority update, micro-snaps may be skipped ## (boot always applies). var _allow_authority_soft_snap_skip: bool = false @@ -87,10 +92,7 @@ func _ready() -> void: # and throttles bursts (see Decision 2 in NEO-24 plan). `authoritative_ack` fires on # every server-confirmed position (boot GET 200 + `move-stream` 200) so a held lock is # revalidated during ordinary WASD locomotion without re-introducing snap rubber-banding. - _authority.connect( - "authoritative_ack", - Callable(_target_client, "on_authoritative_ack") - ) + _authority.connect("authoritative_ack", Callable(_target_client, "on_authoritative_ack")) _target_client.connect("target_state_changed", Callable(self, "_on_target_state_changed")) _authority.call("sync_from_server") _target_client.call("request_sync_from_server") @@ -115,6 +117,7 @@ func _physics_process(_delta: float) -> void: _stream_physics_counter = 0 var p: Vector3 = _player.global_position _player_pos_label.text = "x: %.3f\ny: %.3f\nz: %.3f" % [p.x, p.y, p.z] + _render_target_lock_label(p) func _locomotion_wish_world_xz_from_camera() -> Vector3: @@ -158,24 +161,47 @@ func _on_authoritative_position(world: Vector3, apply_as_snap: bool) -> void: _player.snap_to_server(world) -## NEO-24: render the last server-acknowledged target lock to the HUD label. -## `state` carries `lockedTargetId` (String or `null`), `validity`, `sequence`, and -## (on POST responses) `selectionApplied` + optional `reasonCode`. +## NEO-24: cache the last server-acknowledged target state; the label text is +## re-rendered every physics tick in [method _render_target_lock_label] so the +## live distance readouts stay in sync with the player capsule even while +## the server state is idle. func _on_target_state_changed(state: Dictionary) -> void: + _last_target_state = state.duplicate() + if is_instance_valid(_player): + _render_target_lock_label(_player.global_position) + + +## Combines cached server state (`_last_target_state`) with per-anchor horizontal +## distance computed from the live capsule position. Distances are a **display-only** +## hint — the server remains authoritative for `validity`. If the HUD shows +## `Validity: ok` but `alpha: 10.3 m` (> 8 m radius), that means the server's +## position snapshot is lagging the visible capsule (NEO-23 `move-stream` 20 Hz + +## one-way latency); the refresh GET on `authoritative_ack` will reconcile within +## ~250 ms cooldown the next time a `move-stream` lands. +func _render_target_lock_label(world: Vector3) -> void: if not is_instance_valid(_target_lock_label): return + var state: Dictionary = _last_target_state var locked_variant: Variant = state.get("lockedTargetId", null) var locked_text: String = "—" if locked_variant is String and not (locked_variant as String).is_empty(): locked_text = locked_variant as String var validity: String = str(state.get("validity", "none")) var sequence: int = int(state.get("sequence", 0)) - var suffix: String = "" + var lines: PackedStringArray = [ + "Target: %s" % locked_text, + "Validity: %s" % validity, + "Seq: %d" % sequence, + ] + for id_variant: Variant in PrototypeTargetConstants.ordered_ids(): + var tid: String = id_variant as String + var dist: float = PrototypeTargetConstants.horizontal_distance_to(tid, world) + var radius: float = PrototypeTargetConstants.anchor_radius(tid) + var marker: String = "in" if dist <= radius else "out" + lines.append("%s: %.2f m / %.1f (%s)" % [tid, dist, radius, marker]) if state.has("reasonCode"): - suffix = "\nDenied: %s" % str(state["reasonCode"]) - _target_lock_label.text = ( - "Target: %s\nValidity: %s\nSeq: %d%s" % [locked_text, validity, sequence, suffix] - ) + lines.append("Denied: %s" % str(state["reasonCode"])) + _target_lock_label.text = "\n".join(lines) func _on_move_rejected(reason_code: String) -> void: diff --git a/client/scripts/prototype_target_constants.gd b/client/scripts/prototype_target_constants.gd index e47e477..803be22 100644 --- a/client/scripts/prototype_target_constants.gd +++ b/client/scripts/prototype_target_constants.gd @@ -3,11 +3,63 @@ extends Object ## NEO-24: prototype combat target ids in **tab cycle order** — must match ## `server/NeonSprawl.Server/Game/Targeting/PrototypeTargetRegistry.cs` ## (`PrototypeTargetAlphaId`, `PrototypeTargetBetaId`; ascending id order). +## Anchors + radii are a **display-only** mirror of the C# registry so the scene markers +## (`prototype_target_markers.gd`) and HUD distance readouts match server-authoritative +## range decisions. If the server registry moves, update this file in the same commit — +## the two must stay in lock-step to avoid confusing "ok at 10 m / denied at 5 m" bugs. ## Static accessors only (no `class_name`; headless / CI loads before editor import — see client ## README Godot CLI section). const ORDERED_IDS: Array[String] = ["prototype_target_alpha", "prototype_target_beta"] +## Keys match [constant ORDERED_IDS]; values mirror `PrototypeTargetRegistry.cs` anchors +## (world space) and horizontal lock radii (XZ meters). +const ANCHORS: Dictionary = { + "prototype_target_alpha": + {"position": Vector3(-5.0, 0.5, -5.0), "radius": 8.0, "color": Color(1.0, 0.55, 0.2, 1.0)}, + "prototype_target_beta": + {"position": Vector3(8.0, 0.0, 8.0), "radius": 4.0, "color": Color(0.7, 0.45, 1.0, 1.0)}, +} + + +## Returns the world anchor position for [param target_id], or [code]Vector3.ZERO[/code] +## if the id is not in [constant ANCHORS]. +static func anchor_position(target_id: String) -> Vector3: + var data: Variant = ANCHORS.get(target_id, null) + if not data is Dictionary: + return Vector3.ZERO + var pos: Variant = (data as Dictionary).get("position", Vector3.ZERO) + return pos if pos is Vector3 else Vector3.ZERO + + +## Horizontal (XZ) lock radius for [param target_id], or [code]0.0[/code] if unknown. +static func anchor_radius(target_id: String) -> float: + var data: Variant = ANCHORS.get(target_id, null) + if not data is Dictionary: + return 0.0 + return float((data as Dictionary).get("radius", 0.0)) + + +## Marker tint for [param target_id]; falls back to white if unknown. +static func anchor_color(target_id: String) -> Color: + var data: Variant = ANCHORS.get(target_id, null) + if not data is Dictionary: + return Color.WHITE + var c: Variant = (data as Dictionary).get("color", Color.WHITE) + return c if c is Color else Color.WHITE + + +## Horizontal (XZ) distance from [param world] to the anchor of [param target_id], or +## [code]INF[/code] if the id is unknown. Display-only helper — **not** authoritative for +## range decisions (server owns that; see `PlayerTargetStateReader.ComputeValidity`). +static func horizontal_distance_to(target_id: String, world: Vector3) -> float: + if not ANCHORS.has(target_id): + return INF + var anchor: Vector3 = anchor_position(target_id) + var dx: float = world.x - anchor.x + var dz: float = world.z - anchor.z + return sqrt(dx * dx + dz * dz) + static func ordered_ids() -> Array[String]: return ORDERED_IDS.duplicate() diff --git a/client/scripts/prototype_target_markers.gd b/client/scripts/prototype_target_markers.gd new file mode 100644 index 0000000..2ed84cb --- /dev/null +++ b/client/scripts/prototype_target_markers.gd @@ -0,0 +1,109 @@ +extends Node3D + +## NEO-24 UX: the targeting registry anchors (`prototype_target_alpha`, +## `prototype_target_beta`) are invisible in the world, so "Tab says ok but I feel +## far away" is impossible to reason about without debug markers. This node spawns +## one small mast + a flat radius ring per id in +## `PrototypeTargetConstants.ANCHORS`, so the player can see exactly where each +## anchor sits and how big its lock radius is. +## +## Display-only; no game logic hangs off these nodes. If the server registry moves, +## update `prototype_target_constants.gd` — markers respawn on next scene load. + +const PrototypeTargetConstants := preload("res://scripts/prototype_target_constants.gd") + +## Visual tuning — keep markers unmistakable but not dominant. +const _MAST_HEIGHT: float = 2.5 +const _MAST_RADIUS: float = 0.18 +const _RING_RADIUS_SEGMENTS: int = 72 +const _RING_Y: float = 0.05 +const _RING_THICKNESS: float = 0.06 + + +func _ready() -> void: + for id_variant: Variant in PrototypeTargetConstants.ANCHORS.keys(): + if not id_variant is String: + continue + _spawn_marker(id_variant as String) + + +func _spawn_marker(target_id: String) -> void: + var anchor: Vector3 = PrototypeTargetConstants.anchor_position(target_id) + var radius: float = PrototypeTargetConstants.anchor_radius(target_id) + var tint: Color = PrototypeTargetConstants.anchor_color(target_id) + if radius <= 0.0: + return + + var group := Node3D.new() + group.name = "Marker_%s" % target_id + group.position = anchor + add_child(group) + + var mast := MeshInstance3D.new() + mast.name = "Mast" + var mast_mesh := CylinderMesh.new() + mast_mesh.top_radius = _MAST_RADIUS + mast_mesh.bottom_radius = _MAST_RADIUS + mast_mesh.height = _MAST_HEIGHT + mast.mesh = mast_mesh + # CylinderMesh is Y-centered at its origin; raise so the base sits on the anchor Y. + mast.position = Vector3(0.0, _MAST_HEIGHT * 0.5, 0.0) + mast.material_override = _make_unshaded_material(tint, 1.0) + group.add_child(mast) + + var ring: MeshInstance3D = _make_radius_ring(radius, tint) + ring.name = "RadiusRing" + ring.position = Vector3(0.0, _RING_Y - anchor.y, 0.0) + group.add_child(ring) + + +func _make_unshaded_material(tint: Color, alpha: float) -> StandardMaterial3D: + var m := StandardMaterial3D.new() + m.shading_mode = BaseMaterial3D.SHADING_MODE_UNSHADED + var c := tint + c.a = alpha + m.albedo_color = c + if alpha < 0.999: + m.transparency = BaseMaterial3D.TRANSPARENCY_ALPHA + return m + + +## Builds a thin flat ring (annulus) by triangulating the band between `radius - t/2` +## and `radius + t/2`. `ImmediateMesh` with `SurfaceTool` would also work, but an +## `ArrayMesh` keeps this a static resource with no per-frame cost. +func _make_radius_ring(radius: float, tint: Color) -> MeshInstance3D: + var mi := MeshInstance3D.new() + var arr_mesh := ArrayMesh.new() + var verts := PackedVector3Array() + var normals := PackedVector3Array() + var indices := PackedInt32Array() + + var inner: float = max(0.01, radius - _RING_THICKNESS * 0.5) + var outer: float = radius + _RING_THICKNESS * 0.5 + var up := Vector3.UP + for i in range(_RING_RADIUS_SEGMENTS): + var a: float = TAU * float(i) / float(_RING_RADIUS_SEGMENTS) + var c: float = cos(a) + var s: float = sin(a) + verts.append(Vector3(inner * c, 0.0, inner * s)) + verts.append(Vector3(outer * c, 0.0, outer * s)) + normals.append(up) + normals.append(up) + + for i in range(_RING_RADIUS_SEGMENTS): + var i_next: int = (i + 1) % _RING_RADIUS_SEGMENTS + var a0: int = i * 2 + var a1: int = i * 2 + 1 + var b0: int = i_next * 2 + var b1: int = i_next * 2 + 1 + indices.append_array([a0, a1, b1, a0, b1, b0]) + + var surface: Array = [] + surface.resize(Mesh.ARRAY_MAX) + surface[Mesh.ARRAY_VERTEX] = verts + surface[Mesh.ARRAY_NORMAL] = normals + surface[Mesh.ARRAY_INDEX] = indices + arr_mesh.add_surface_from_arrays(Mesh.PRIMITIVE_TRIANGLES, surface) + mi.mesh = arr_mesh + mi.material_override = _make_unshaded_material(tint, 0.45) + return mi diff --git a/client/scripts/prototype_target_markers.gd.uid b/client/scripts/prototype_target_markers.gd.uid new file mode 100644 index 0000000..ac67ce0 --- /dev/null +++ b/client/scripts/prototype_target_markers.gd.uid @@ -0,0 +1 @@ +uid://crd46axw4gkjn diff --git a/client/test/target_refresh_on_locomotion_test.gd.uid b/client/test/target_refresh_on_locomotion_test.gd.uid new file mode 100644 index 0000000..50a0c94 --- /dev/null +++ b/client/test/target_refresh_on_locomotion_test.gd.uid @@ -0,0 +1 @@ +uid://b3jgb0gf52s24 diff --git a/docs/manual-qa/NEO-24.md b/docs/manual-qa/NEO-24.md index 137bbbf..0422e92 100644 --- a/docs/manual-qa/NEO-24.md +++ b/docs/manual-qa/NEO-24.md @@ -23,6 +23,9 @@ Pair the client (Godot **F5**) with the server (`cd server/NeonSprawl.Server && - `Target: —` - `Validity: none` - `Seq: 0` + - `prototype_target_alpha: 0.00 m / 8.0 (in)` (spawn `(-5,-5)` is exactly on alpha's XZ anchor) + - `prototype_target_beta: 18.38 m / 4.0 (out)` +- [ ] **Visible anchors** are rendered in-world: an **orange** mast + flat ring at `(-5, 0.5, -5)` (alpha, 8 m radius) and a **violet** mast + ring at `(8, 0, 8)` (beta, 4 m radius). Both rings lie flat on the floor; the player capsule should sit inside the alpha ring at spawn. - [ ] Godot **Output** has no `TargetSelectionClient:` warnings. Server console shows a single `GET /game/players/dev-local-1/target` from the boot sync. ## 2. Tab cycle — happy path @@ -50,6 +53,7 @@ Pair the client (Godot **F5**) with the server (`cd server/NeonSprawl.Server && - [ ] Walk back inside the radius. HUD returns to `Validity: ok` within ~250 ms of the next `move-stream` ack. - [ ] While walking, Godot Output is **not** spammed with targeting GETs. Server log shows no more than roughly one targeting `GET` per 250 ms during sustained motion, even though `move-stream` POSTs fire at ~20 Hz. - [ ] Stand still with no lock changes. Server log shows **no** targeting GETs at idle (no `move-stream` is sent while idle, so there is no ack to trigger a refresh). +- [ ] **Distance lines match the ring**: while moving, the `prototype_target_alpha: m / 8.0 (in|out)` line flips `in`/`out` as you cross the visible orange ring edge. Any time the HUD shows `Validity: ok` but the distance line reads `(out)`, the server position snapshot is lagging the visible capsule — the next `authoritative_ack` refresh should reconcile within 250 ms. ## 5. Swap to beta when in range diff --git a/docs/plans/NEO-24-implementation-plan.md b/docs/plans/NEO-24-implementation-plan.md index 1dbeea9..6d26140 100644 --- a/docs/plans/NEO-24-implementation-plan.md +++ b/docs/plans/NEO-24-implementation-plan.md @@ -75,8 +75,10 @@ Server-up manual QA steps live in the README section above; run the server + cli | Path | Rationale | |------|-----------| -| `client/scenes/main.tscn` | Add `TargetSelectionClient` node + **`TargetLockLabel`** (or reuse a debug panel) bound to scripts. | -| `client/scripts/main.gd` | Wire `@onready` target client + label; connect `target_state_changed`; trigger initial `request_sync_from_server()` after authority boot path; connect `authoritative_ack` → `on_authoritative_ack` (see Decision 2). | +| `client/scenes/main.tscn` | Add `TargetSelectionClient` node + **`TargetLockLabel`** (or reuse a debug panel) bound to scripts. Post-merge follow-up (Decision 4) also adds `World/PrototypeTargetMarkers` (colored mast + translucent radius ring per anchor). | +| `client/scripts/main.gd` | Wire `@onready` target client + label; connect `target_state_changed`; trigger initial `request_sync_from_server()` after authority boot path; connect `authoritative_ack` → `on_authoritative_ack` (see Decision 2). Re-render `TargetLockLabel` each physics tick with live per-anchor distance lines from `PrototypeTargetConstants` (Decision 4). | +| `client/scripts/prototype_target_constants.gd` | Display-only mirror of `PrototypeTargetRegistry.cs` anchors + radii used by the markers and HUD distance readouts (Decision 4). | +| `client/scripts/prototype_target_markers.gd` | Spawns a visible mast + flat ring at every `PrototypeTargetConstants.ANCHORS` entry so players can reason about range visually (Decision 4). | | `client/scripts/position_authority_client.gd` | Add `authoritative_ack(world)` signal and emit it from both `BOOT_GET` 200 and `STREAM_POST` 200 so held locks re-validate during normal WASD locomotion without re-introducing snap rubber-banding. Extracted `_parse_world_from_response(...)` helper to avoid duplicating JSON parse across the two paths. | | `client/project.godot` | Register **`target_tab`** and **`target_clear`** input actions with defaults above. | | `client/README.md` | One short subsection: tab-target prototype, bindings, and how to read the lock overlay (parity with interaction/move docs). | @@ -103,3 +105,4 @@ No new **C#** tests (client-only story). **Bruno:** NEO-23 targeting requests al | 1 | **Input bindings** | `target_tab` → **Tab**; `target_clear` → **Escape**. Registered in `client/project.godot`. | | 2 | **Soft-lock refresh policy** | **Hybrid, movement-triggered.** No periodic polling. Refresh sources: boot `GET`, each `POST` response echo, **and** `GET` fired from `PositionAuthorityClient.authoritative_ack` (new signal — fires on every server-confirmed position, including `move-stream` 200) **only while a lock is currently held**, with a **~250 ms** cooldown to coalesce bursts. Does **not** duplicate radius math on the client; does **not** cover future purely server-driven state changes (revisit when E5.M1 / E6.M1 introduce them). | | 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. | diff --git a/docs/reviews/2026-04-21-NEO-24.md b/docs/reviews/2026-04-21-NEO-24.md index 45fbb94..d402088 100644 --- a/docs/reviews/2026-04-21-NEO-24.md +++ b/docs/reviews/2026-04-21-NEO-24.md @@ -5,6 +5,12 @@ 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 (2026-04-21):** user reported "targeting result feels very inconsistent and doesn't seem to matter whether i'm within range of the console or not." Root cause was not a correctness bug — the `PrototypeTargetRegistry` anchors (`alpha (-5,-5)` r=8, `beta (8,8)` r=4) have **no visible scene meshes**, so players were reasoning about targeting against the `PrototypeTerminal` box at origin (which belongs to NEO-9 interaction, not NEO-23/24 targeting). Added: + +- `client/scripts/prototype_target_markers.gd` — spawns an unshaded colored mast + flat radius ring per anchor (mirrors the server registry via `prototype_target_constants.gd`). +- Per-anchor distance readout on `UICanvas/TargetLockLabel`: `: m / (in|out)` computed client-side from the live capsule position. Labeled explicitly as a lag diagnostic — if `Validity: ok` shows while the distance line reads `(out)`, the server's position snapshot is trailing the visible capsule and the next `authoritative_ack` refresh reconciles it. +- Updated `client/README.md` and `docs/manual-qa/NEO-24.md` (Section 1 boot state and Section 4 locomotion check) to describe the markers and distance lines. + ## Verdict ~~Request changes~~ — **Addressed.** Blocking issue fixed; integration test added; doc-alignment row updated. @@ -13,16 +19,16 @@ Follow-up: blocking issue + suggestions below are **done** (strikethrough + **Do This branch adds the client-side target selection node, HUD label, input bindings, tests, and story docs for NEO-24. The overall direction matches the E1.M3 plan well: selection intent is POSTed to the server, the HUD paints only server-acknowledged target state, and denial handling is intentionally authoritative. -The main risk is that the advertised movement-driven soft-lock refresh is not actually wired to normal locomotion. The new unit suite passes, but it only proves `TargetSelectionClient` reacts correctly when its refresh hook is called directly; it does not cover the real signal behavior of `PositionAuthorityClient`, so the branch can merge with a broken end-to-end acceptance criterion. +Follow-up verification confirms the earlier review findings were addressed. The movement-triggered soft-lock refresh is now reachable during normal locomotion via `PositionAuthorityClient.authoritative_ack`, the new integration test covers the real authority-to-target wiring, and the E1.M3 implementation-alignment row was updated to reflect NEO-24 landing. ## Documentation checked -- `docs/plans/NEO-24-implementation-plan.md` — **conflicts**. Decision 2 and the acceptance criteria require movement-triggered refresh while a lock is held, but the current wiring only refreshes on boot sync / rejection resync, not on ordinary movement. -- `docs/manual-qa/NEO-24.md` — **conflicts**. Section 4 expects walking out of range to flip HUD validity within ~250 ms during normal locomotion; that flow is not reachable with the current signal source. +- `docs/plans/NEO-24-implementation-plan.md` — **matches**. Decision 2 now uses `PositionAuthorityClient.authoritative_ack`, and Decision 3 records why the new signal was added. +- `docs/manual-qa/NEO-24.md` — **matches**. Section 4’s locomotion-driven refresh path is now implemented by the new authority ack signal and target refresh wiring. - `docs/decomposition/modules/E1_M3_InteractionAndTargetingLayer.md` — **matches** for server-authoritative target state and denial reconciliation. - `docs/decomposition/modules/client_server_authority.md` — **matches** for client-as-intent / server-as-truth behavior. - `docs/decomposition/modules/module_dependency_register.md` — **matches**. `E1.M3` remains `In Progress`. -- `docs/decomposition/modules/documentation_and_implementation_alignment.md` — **partially matches**. The `E1.M3` tracking row still says client tab-target is open; if/when this story lands, that snapshot should be updated to reflect NEO-24 progress. +- `docs/decomposition/modules/documentation_and_implementation_alignment.md` — **matches**. The `E1.M3` tracking row now reflects NEO-24 landed (HUD + `authoritative_ack` refresh) and points back to the plan/manual QA docs. ## Blocking issues @@ -69,5 +75,8 @@ None. ## Verification - Ran `godot --headless --import --path . --quit-after 10` from `client/`. -- Ran `godot --headless --path . -s res://addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode -a res://test/target_selection_client_test.gd` from `client/` — suite passed. -- I did not run the full manual client/server checklist, but `docs/manual-qa/NEO-24.md` section 4 is the critical end-to-end check to rerun after fixing the movement refresh trigger. +- Ran `godot --headless --path . -s res://addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode -a res://test/position_authority_client_test.gd` from `client/` — suite passed, including the new `authoritative_ack` assertions. +- Ran `godot --headless --path . -s res://addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode -a res://test/target_selection_client_test.gd` from `client/` — suite passed, including the renamed ack-based refresh tests. +- Ran `godot --headless --path . -s res://addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode -a res://test/target_refresh_on_locomotion_test.gd` from `client/` — integration suite passed, confirming `move-stream` 200 triggers exactly one throttled target refresh while locked and none while unlocked. +- Read diagnostics for the touched client/scripts, client/test, plan, and review files — no linter errors found. +- I did not run the full manual client/server checklist; `docs/manual-qa/NEO-24.md` section 4 remains the highest-value end-to-end sanity check if you want one more runtime pass before merge. From e31cd2df684528b24272075698041a1434a25699 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Tue, 21 Apr 2026 23:21:16 -0400 Subject: [PATCH 10/13] NEO-24: freshen server position before target-select + surface divergence on HUD MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix "Tab says denied even though I'm in range of beta": root cause is the race between the 20 Hz move-stream sampler and the target-select POST. A Tab press right after the capsule stops can be validated against the server's last sampled position, which trails the visible capsule by several meters. Client-side fixes (server contract unchanged): - TargetSelectionClient.set_freshness_kick(authority, player): before each POST /target/select, submit a move-stream with the current player position so the server's stored snapshot is as fresh as possible when the range check runs. Wired in main.gd; optional so tests without the wiring still pass (regression-covered). - PlayerPositionLabel now shows a "srv: (x,y,z) Δ=m age=" line populated from PositionAuthorityClient.authoritative_ack, so any residual client/server divergence is visible in the HUD and future reports can point at it directly. - Two new tests: select POST kicks exactly one freshness stream with the current player position before POSTing, and the kick is a no-op when the wiring is absent. Docs: plan Decision 5, review follow-up #2, manual QA section 5 and section 1, and client README updated to describe the new behaviors. --- client/README.md | 3 +- client/scenes/main.tscn | 13 ++--- client/scripts/main.gd | 41 +++++++++++++++- client/scripts/target_selection_client.gd | 33 +++++++++++++ client/test/target_selection_client_test.gd | 53 +++++++++++++++++++++ docs/manual-qa/NEO-24.md | 4 +- docs/plans/NEO-24-implementation-plan.md | 1 + docs/reviews/2026-04-21-NEO-24.md | 9 +++- 8 files changed, 147 insertions(+), 10 deletions(-) diff --git a/client/README.md b/client/README.md index d5621b4..d097614 100644 --- a/client/README.md +++ b/client/README.md @@ -96,7 +96,8 @@ 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. -- **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. +- **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. - **Soft lock refresh:** the client does **not** poll on a timer. It refreshes via `GET …/target` on boot, on every `POST …/target/select` response (body already carries `targetState`), and — while a lock is currently held — on **`authoritative_ack`** from `PositionAuthorityClient`, throttled to at most one GET per **250 ms**. `authoritative_ack` fires on every server-confirmed position (boot `GET` 200 **and** successful `move-stream` 200), so a held lock revalidates during normal WASD locomotion without re-introducing snap rubber-banding. Boot is naturally skipped because no lock exists yet. Purely server-driven state flips (future PvP / combat categories) would require revisiting this policy; today NEO-23 has none. - **Inspector:** match **`base_url`** / **`dev_player_id`** on the `TargetSelectionClient` node to the other HTTP clients / `Game:DevPlayerId`. diff --git a/client/scenes/main.tscn b/client/scenes/main.tscn index d8e3903..866dcbd 100644 --- a/client/scenes/main.tscn +++ b/client/scenes/main.tscn @@ -1136,8 +1136,8 @@ autowrap_mode = 3 [node name="PlayerPositionLabel" type="Label" parent="UICanvas" unique_id=9000003] offset_left = 8.0 offset_top = 8.0 -offset_right = 220.0 -offset_bottom = 88.0 +offset_right = 360.0 +offset_bottom = 106.0 grow_horizontal = 0 grow_vertical = 0 theme_override_colors/font_color = Color(0.92, 0.95, 0.98, 1) @@ -1146,13 +1146,14 @@ theme_override_constants/outline_size = 6 theme_override_font_sizes/font_size = 15 text = "x: — y: — -z: —" +z: — +srv: —" [node name="TargetLockLabel" type="Label" parent="UICanvas" unique_id=9000004] offset_left = 8.0 -offset_top = 104.0 -offset_right = 340.0 -offset_bottom = 288.0 +offset_top = 122.0 +offset_right = 360.0 +offset_bottom = 306.0 grow_horizontal = 0 grow_vertical = 0 theme_override_colors/font_color = Color(0.86, 0.94, 1, 1) diff --git a/client/scripts/main.gd b/client/scripts/main.gd index d150adf..2050318 100644 --- a/client/scripts/main.gd +++ b/client/scripts/main.gd @@ -45,6 +45,12 @@ var _stream_physics_counter: int = 0 ## Cached latest `target_state_changed` payload so the HUD can re-render with live ## distance readouts on every physics tick without missing server-side fields. var _last_target_state: Dictionary = {} +## Last server-acknowledged world position (from `PositionAuthorityClient.authoritative_ack`; +## boot GET 200 or `move-stream` 200). Used purely to render the server-vs-client divergence +## line in the HUD so "target says denied but I'm obviously in range" becomes self-explanatory. +var _last_ack_world: Vector3 = Vector3.ZERO +var _have_last_ack: bool = false +var _last_ack_msec: int = 0 ## After the first `apply_as_snap` authority update, micro-snaps may be skipped ## (boot always applies). var _allow_authority_soft_snap_skip: bool = false @@ -93,7 +99,13 @@ func _ready() -> void: # every server-confirmed position (boot GET 200 + `move-stream` 200) so a held lock is # revalidated during ordinary WASD locomotion without re-introducing snap rubber-banding. _authority.connect("authoritative_ack", Callable(_target_client, "on_authoritative_ack")) + _authority.connect("authoritative_ack", Callable(self, "_on_authoritative_ack_for_hud")) _target_client.connect("target_state_changed", Callable(self, "_on_target_state_changed")) + # NEO-24 post-merge: tell the target client how to nudge the authority with the current + # capsule position before each POST /target/select. Shrinks the common "Tab right after + # stopping" false-denial race by making the server's stored snapshot as fresh as possible. + if _target_client.has_method("set_freshness_kick"): + _target_client.call("set_freshness_kick", _authority, _player) _authority.call("sync_from_server") _target_client.call("request_sync_from_server") if _radius_preview.has_method("setup_player"): @@ -116,10 +128,27 @@ func _physics_process(_delta: float) -> void: else: _stream_physics_counter = 0 var p: Vector3 = _player.global_position - _player_pos_label.text = "x: %.3f\ny: %.3f\nz: %.3f" % [p.x, p.y, p.z] + _player_pos_label.text = _build_player_position_text(p) _render_target_lock_label(p) +## HUD: client position on top, and (once a `move-stream` / boot GET has landed) the +## **server**-acknowledged position + horizontal delta underneath. When the server line +## is far from the client line the capsule has outrun the authoritative state — any +## targeting denials are being computed against the stale server position, and the next +## `move-stream` 200 (or the recovery resync on a 400) will reconcile. +func _build_player_position_text(p: Vector3) -> String: + var lines: PackedStringArray = ["x: %.3f" % p.x, "y: %.3f" % p.y, "z: %.3f" % p.z] + if _have_last_ack: + var s: Vector3 = _last_ack_world + var dh: float = Vector2(p.x - s.x, p.z - s.z).length() + var age_msec: int = max(0, Time.get_ticks_msec() - _last_ack_msec) + lines.append("srv: (%.2f, %.2f, %.2f) Δ=%.2fm age=%dms" % [s.x, s.y, s.z, dh, age_msec]) + else: + lines.append("srv: — (no ack yet)") + return "\n".join(lines) + + func _locomotion_wish_world_xz_from_camera() -> Vector3: var v: Vector2 = Input.get_vector("move_left", "move_right", "move_forward", "move_back") if v.length_squared() < 1e-8: @@ -161,6 +190,16 @@ func _on_authoritative_position(world: Vector3, apply_as_snap: bool) -> void: _player.snap_to_server(world) +## NEO-24 diagnostic: cache the server's last authoritative position so the HUD can +## surface client-vs-server divergence. This is the same signal `TargetSelectionClient` +## hooks for movement-driven lock refresh — we do not snap the capsule to this value +## (that is `authoritative_position_received`'s job on boot + move rejection). +func _on_authoritative_ack_for_hud(world: Vector3) -> void: + _last_ack_world = world + _have_last_ack = true + _last_ack_msec = Time.get_ticks_msec() + + ## NEO-24: cache the last server-acknowledged target state; the label text is ## re-rendered every physics tick in [method _render_target_lock_label] so the ## live distance readouts stay in sync with the player capsule even while diff --git a/client/scripts/target_selection_client.gd b/client/scripts/target_selection_client.gd index 41f2fae..3e408fd 100644 --- a/client/scripts/target_selection_client.gd +++ b/client/scripts/target_selection_client.gd @@ -34,6 +34,15 @@ var _http: Node var _busy: bool = false var _phase: Phase = Phase.IDLE +## Optional "freshness kick": before each `POST /target/select` we ask the authority to +## submit a `move-stream` with the current player position so the server's stored snapshot +## is as close to the visible capsule as possible when it runs the radius check. Without +## this, a Tab press right after you stop moving can be validated against the last +## 20 Hz sample (which trailed the capsule) and deny at a position that is visibly in +## range. Both refs are optional so tests that inject a plain script can still run. +var _freshness_authority: Node = null +var _freshness_player: Node3D = null + ## Last acknowledged state from the server. Mirrors `PlayerTargetStateResponse` v1: ## `schemaVersion` (int), `playerId` (String), `lockedTargetId` (String or `null`), ## `validity` (String: `none` | `ok` | `out_of_range` | `invalid_target`), @@ -122,6 +131,14 @@ func cached_state() -> Dictionary: return _state.duplicate() +## Wire the authority + player so each `POST /target/select` is preceded by a fresh +## `move-stream` submit (see `_freshness_authority`). Pass `null` for either to disable. +## Safe to call multiple times; the last wiring wins. +func set_freshness_kick(authority: Node, player: Node3D) -> void: + _freshness_authority = authority + _freshness_player = player + + func _has_lock() -> bool: if _state.is_empty(): return false @@ -132,6 +149,7 @@ func _has_lock() -> bool: func _post_select(payload: Dictionary) -> void: if _busy: return + _kick_freshness_stream() _busy = true _phase = Phase.POST_SELECT var url := "%s/game/players/%s/target/select" % [_base_root(), _player_path_segment()] @@ -144,6 +162,21 @@ func _post_select(payload: Dictionary) -> void: _phase = Phase.IDLE +## Nudges the authority with the current capsule position so the server's stored +## snapshot is as fresh as possible when the select POST lands. Does **not** wait +## for the move-stream response — both requests race on separate connections — but +## in practice this collapses the common "Tab right after stopping" false-denial +## window because the authority flushes queued crumbs synchronously on submit. +func _kick_freshness_stream() -> void: + if _freshness_authority == null or _freshness_player == null: + return + if not is_instance_valid(_freshness_authority) or not is_instance_valid(_freshness_player): + return + if not _freshness_authority.has_method("submit_stream_targets"): + return + _freshness_authority.call("submit_stream_targets", [_freshness_player.global_position]) + + func _base_root() -> String: return base_url.strip_edges().rstrip("/") diff --git a/client/test/target_selection_client_test.gd b/client/test/target_selection_client_test.gd index eabcfb7..ecfc193 100644 --- a/client/test/target_selection_client_test.gd +++ b/client/test/target_selection_client_test.gd @@ -209,3 +209,56 @@ func test_two_acks_within_cooldown_collapse_to_one_get() -> void: c.on_authoritative_ack(Vector3.ZERO) c.on_authoritative_ack(Vector3.ZERO) assert_that(transport.request_count).is_equal(count_before + 1) + + +# Authority stub that captures the last `submit_stream_targets` batch so we can verify +# `TargetSelectionClient._post_select` kicks a freshness stream before POSTing. +class FreshnessAuthorityStub: + extends Node + var last_batch: Array = [] + var submit_count: int = 0 + + func submit_stream_targets(targets: Array) -> void: + submit_count += 1 + last_batch = targets.duplicate() + + +func _make_player(pos: Vector3) -> Node3D: + var n := Node3D.new() + n.global_position = pos + auto_free(n) + add_child(n) + return n + + +func test_select_post_kicks_freshness_stream_before_posting() -> void: + var transport := MockHttpTransport.new() + transport.enqueue( + HTTPRequest.RESULT_SUCCESS, 200, _select_response_json(true, ALPHA_ID, "ok", 1) + ) + var c := _make_client(transport) + var authority := FreshnessAuthorityStub.new() + auto_free(authority) + add_child(authority) + var player := _make_player(Vector3(1.5, 0.5, -2.25)) + c.set_freshness_kick(authority, player) + c.request_select_target_id(ALPHA_ID) + # Exactly one freshness submit, matching the pre-POST capsule position. + assert_that(authority.submit_count).is_equal(1) + assert_that(authority.last_batch.size()).is_equal(1) + assert_that(authority.last_batch[0]).is_equal(Vector3(1.5, 0.5, -2.25)) + # And the select POST still went out as usual. + assert_that(transport.last_url).contains("/target/select") + + +func test_select_post_without_freshness_wiring_is_a_noop() -> void: + # Regression: tests that construct the client without `set_freshness_kick(...)` must + # still POST normally. Confirms the kick is fully optional. + 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_select_target_id(ALPHA_ID) + assert_that(transport.last_url).contains("/target/select") + assert_that(c.cached_state().get("lockedTargetId")).is_equal(ALPHA_ID) diff --git a/docs/manual-qa/NEO-24.md b/docs/manual-qa/NEO-24.md index 0422e92..5112492 100644 --- a/docs/manual-qa/NEO-24.md +++ b/docs/manual-qa/NEO-24.md @@ -25,6 +25,7 @@ Pair the client (Godot **F5**) with the server (`cd server/NeonSprawl.Server && - `Seq: 0` - `prototype_target_alpha: 0.00 m / 8.0 (in)` (spawn `(-5,-5)` is exactly on alpha's XZ anchor) - `prototype_target_beta: 18.38 m / 4.0 (out)` +- [ ] `UICanvas/PlayerPositionLabel` shows an `srv: (x, y, z) Δ=m age=` line within ~1 s of boot (boot `GET /position` populates it). At rest the `Δ` should be near 0. - [ ] **Visible anchors** are rendered in-world: an **orange** mast + flat ring at `(-5, 0.5, -5)` (alpha, 8 m radius) and a **violet** mast + ring at `(8, 0, 8)` (beta, 4 m radius). Both rings lie flat on the floor; the player capsule should sit inside the alpha ring at spawn. - [ ] Godot **Output** has no `TargetSelectionClient:` warnings. Server console shows a single `GET /game/players/dev-local-1/target` from the boot sync. @@ -57,11 +58,12 @@ Pair the client (Godot **F5**) with the server (`cd server/NeonSprawl.Server && ## 5. Swap to beta when in range -- [ ] WASD toward beta at **(8, 8)** until you are within 4 m. Press **Tab**. HUD swaps cleanly: +- [ ] WASD toward beta at **(8, 8)** until you are inside the violet ring (≤4 m). Press **Tab**. HUD swaps cleanly: - `Target: prototype_target_beta` - `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. +- [ ] **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 6d26140..ec1349c 100644 --- a/docs/plans/NEO-24-implementation-plan.md +++ b/docs/plans/NEO-24-implementation-plan.md @@ -106,3 +106,4 @@ No new **C#** tests (client-only story). **Bruno:** NEO-23 targeting requests al | 2 | **Soft-lock refresh policy** | **Hybrid, movement-triggered.** No periodic polling. Refresh sources: boot `GET`, each `POST` response echo, **and** `GET` fired from `PositionAuthorityClient.authoritative_ack` (new signal — fires on every server-confirmed position, including `move-stream` 200) **only while a lock is currently held**, with a **~250 ms** cooldown to coalesce bursts. Does **not** duplicate radius math on the client; does **not** cover future purely server-driven state changes (revisit when E5.M1 / E6.M1 introduce them). | | 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. | diff --git a/docs/reviews/2026-04-21-NEO-24.md b/docs/reviews/2026-04-21-NEO-24.md index d402088..2c25849 100644 --- a/docs/reviews/2026-04-21-NEO-24.md +++ b/docs/reviews/2026-04-21-NEO-24.md @@ -5,7 +5,14 @@ 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 (2026-04-21):** user reported "targeting result feels very inconsistent and doesn't seem to matter whether i'm within range of the console or not." Root cause was not a correctness bug — the `PrototypeTargetRegistry` anchors (`alpha (-5,-5)` r=8, `beta (8,8)` r=4) have **no visible scene meshes**, so players were reasoning about targeting against the `PrototypeTerminal` box at origin (which belongs to NEO-9 interaction, not NEO-23/24 targeting). Added: +**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. +- `TargetSelectionClient` exposes `set_freshness_kick(authority, player)`; `main.gd` wires it so every `POST /target/select` is immediately preceded by a `submit_stream_targets([player.global_position])` nudge on the authority. The select POST does not wait for the stream response, but the kick shrinks the race window enough in practice that "stop, Tab, denied" stops firing. Tests cover both the kick happening (`test_select_post_kicks_freshness_stream_before_posting`) and no regression when the wiring is absent (`test_select_post_without_freshness_wiring_is_a_noop`). + +Server contract (NEO-23) untouched; this is pure client-side. + +**Post-merge UX follow-up #1 (2026-04-21):** user reported "targeting result feels very inconsistent and doesn't seem to matter whether i'm within range of the console or not." Root cause was not a correctness bug — the `PrototypeTargetRegistry` anchors (`alpha (-5,-5)` r=8, `beta (8,8)` r=4) have **no visible scene meshes**, so players were reasoning about targeting against the `PrototypeTerminal` box at origin (which belongs to NEO-9 interaction, not NEO-23/24 targeting). Added: - `client/scripts/prototype_target_markers.gd` — spawns an unshaded colored mast + flat radius ring per anchor (mirrors the server registry via `prototype_target_constants.gd`). - Per-anchor distance readout on `UICanvas/TargetLockLabel`: `: m / (in|out)` computed client-side from the live capsule position. Labeled explicitly as a lag diagnostic — if `Validity: ok` shows while the distance line reads `(out)`, the server's position snapshot is trailing the visible capsule and the next `authoritative_ack` refresh reconciles it. From 63fecb10996f857727b3385f0a8f53c9cb70baee Mon Sep 17 00:00:00 2001 From: VinPropane Date: Tue, 21 Apr 2026 23:26:56 -0400 Subject: [PATCH 11/13] NEO-24: range-aware Tab cycle skips out-of-range anchors before POST Previously Tab from no-lock always picked `ORDERED_IDS[0]` (alpha), so standing next to beta and pressing Tab was denied with `out_of_range` even though the client HUD clearly showed beta in range. Added `PrototypeTargetConstants.next_in_range_id_after(current, world)` which walks the cycle and returns the first anchor whose client-side horizontal distance is within the anchor radius. `TargetSelectionClient.request_tab_next()` prefers that pick when the player reference is wired (same path `set_freshness_kick` uses) and falls back to the plain cycle order when nothing is in range so the server still owns the denial reason. Server contract (NEO-23) untouched. Tests cover the beta pick, the nothing-in-range fallback, and the no-player-ref regression. Plan Decision 6 + review follow-up #3 + README + manual QA updated. --- client/README.md | 2 +- client/scripts/prototype_target_constants.gd | 31 ++++++++++++ client/scripts/target_selection_client.gd | 12 ++++- client/test/target_selection_client_test.gd | 50 ++++++++++++++++++++ docs/manual-qa/NEO-24.md | 1 + docs/plans/NEO-24-implementation-plan.md | 1 + docs/reviews/2026-04-21-NEO-24.md | 2 + 7 files changed, 97 insertions(+), 2 deletions(-) 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. From a7453eb82fb11d2d13b6f66317e8cea03808a4a5 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Tue, 21 Apr 2026 23:37:42 -0400 Subject: [PATCH 12/13] NEO-24: unify prototype target radius at 6 m, overlap rings at origin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All prototype targets now share `PrototypeTargetRegistry.SharedLockRadius` (= 6 m). Anchors moved to alpha `(-3, 0.5, -3)` and beta `(3, 0, 3)` so the two 6 m rings overlap ~3.5 m wide centered at origin, letting Tab flip between targets without locomotion. Spawn `(-5, -5)` keeps alpha in range (~2.83 m) and beta out (~11.31 m) so the existing soft-lock denial QA path at spawn still holds. `prototype_target_constants.gd` mirrors the shared radius + new anchors. The range-aware Tab picker now **skips the currently-locked id** so Tab always expresses a swap intent — if the only in-range target is already locked, the fallback picks the next cycle id and the server denies (preserves NEO-23's "Tab = visible denial when no swap possible" rule). Tests: - Server: new `PlayerTargetStateReaderTests.ComputeValidity_AtOrigin_…` asserts both targets report `Ok` at origin. Existing boundary test rewritten for the new alpha anchor. - Client: new `test_tab_from_locked_alpha_at_origin_swaps_to_beta_when_both_in_range` and `test_tab_when_only_current_lock_is_in_range_falls_back_to_cycle_for_denial`. Existing in-range/out-of-range positions updated for the new layout. Server 59/59 and client 95/95 suites pass. Plan Decision 7, review follow-up #4, manual QA section 5 (new "Tab-flip at origin" overlap sanity), and README updated. Per-target radii will return with real combat design (E5.M1) — this is a prototype stub. --- client/README.md | 17 +++--- client/scripts/prototype_target_constants.gd | 33 ++++++++--- client/test/target_selection_client_test.gd | 57 ++++++++++++++++++- docs/manual-qa/NEO-24.md | 44 ++++++++------ docs/plans/NEO-24-implementation-plan.md | 3 +- docs/reviews/2026-04-21-NEO-24.md | 2 + .../Targeting/PlayerTargetStateReaderTests.cs | 20 ++++++- .../Game/Targeting/PrototypeTargetRegistry.cs | 18 ++++-- 8 files changed, 149 insertions(+), 45 deletions(-) diff --git a/client/README.md b/client/README.md index cb799e3..f427c61 100644 --- a/client/README.md +++ b/client/README.md @@ -96,20 +96,21 @@ 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. 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. +- **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: 7.1 m / 6.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. +- **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. All prototype targets share a single radius (`PrototypeTargetConstants.SHARED_LOCK_RADIUS` = `PrototypeTargetRegistry.SharedLockRadius` = **6 m**); anchors are placed at `(-3, 0.5, -3)` and `(3, 0, 3)` so the two rings overlap at origin (~3.5 m wide), letting you flip between targets with Tab without locomotion. The script and `prototype_target_constants.gd` mirror the server's `PrototypeTargetRegistry.cs` — change them together in the same commit so the markers never drift from server authority. Per-target radii will return with real combat design (E5.M1). - **Soft lock refresh:** the client does **not** poll on a timer. It refreshes via `GET …/target` on boot, on every `POST …/target/select` response (body already carries `targetState`), and — while a lock is currently held — on **`authoritative_ack`** from `PositionAuthorityClient`, throttled to at most one GET per **250 ms**. `authoritative_ack` fires on every server-confirmed position (boot `GET` 200 **and** successful `move-stream` 200), so a held lock revalidates during normal WASD locomotion without re-introducing snap rubber-banding. Boot is naturally skipped because no lock exists yet. Purely server-driven state flips (future PvP / combat categories) would require revisiting this policy; today NEO-23 has none. - **Inspector:** match **`base_url`** / **`dev_player_id`** on the `TargetSelectionClient` node to the other HTTP clients / `Game:DevPlayerId`. ### Manual check (NEO-24) -1. Run the server and client as in NEO-7 / NEO-9; confirm spawn snap. -2. Press **Tab**: HUD → `Target: prototype_target_alpha`, `Validity: ok` (spawn is inside alpha's 8 m radius). -3. Press **Tab** again: server denies (beta anchor is **(8, 8)** with a **4 m** radius, so from spawn it is out of range). Per NEO-23 soft-lock policy the persisted lock does not change, so HUD stays at `Target: prototype_target_alpha, Validity: ok` with `Denied: out_of_range` appended. -4. **WASD** away from spawn past ~8 m horizontally from alpha; HUD flips to `Validity: out_of_range` on the next movement-triggered refresh (≤250 ms cadence) while `lockedTargetId` stays `alpha` — this is the **soft lock** round-tripped from the server. -5. Walk toward beta (inside **(8, 8)** within 4 m) and press **Tab**: swap succeeds, HUD shows `Target: prototype_target_beta, Validity: ok`. A further **Tab** from here wraps to `alpha` (and will itself succeed or deny based on distance). -6. Press **Esc**: HUD → `Target: —`, `Validity: none`, sequence advances. +1. Run the server and client as in NEO-7 / NEO-9; confirm spawn snap at `(-5, 0.9, -5)`. +2. Press **Tab**: HUD → `Target: prototype_target_alpha`, `Validity: ok` (spawn is ~2.83 m from alpha's anchor `(-3, -3)`, well inside the shared 6 m radius). +3. Press **Tab** again: server denies (beta anchor `(3, 3)` is ~11.3 m from spawn, outside 6 m). The range-aware picker skips the currently-locked alpha and falls back to the cycle so the server gets a clean "swap to beta" request and can deny it; per NEO-23 soft-lock policy the persisted lock does not change, so HUD stays at `Target: prototype_target_alpha, Validity: ok` with `Denied: out_of_range` appended. +4. **WASD** away from spawn past ~6 m horizontally from alpha; HUD flips to `Validity: out_of_range` on the next movement-triggered refresh (≤250 ms cadence) while `lockedTargetId` stays `alpha` — this is the **soft lock** round-tripped from the server. +5. Walk to the **origin** (both rings visibly overlap): HUD shows `alpha` and `beta` distance lines both `(in)` (~4.24 m each). Press **Tab** to flip alpha → beta → alpha → … without moving. This is the main overlap sanity. +6. Walk toward beta (inside `(3, 3)` within 6 m, outside alpha's ring) and press **Tab** from a cleared lock: the range-aware picker skips alpha and locks beta directly. A further **Tab** from beta-only-in-range wraps to `alpha` and the server denies (same soft-lock behavior as step 3). +7. Press **Esc**: HUD → `Target: —`, `Validity: none`, sequence advances. ## Movement prototype (NEO-5 → NEO-11) diff --git a/client/scripts/prototype_target_constants.gd b/client/scripts/prototype_target_constants.gd index d91f2b2..a986e68 100644 --- a/client/scripts/prototype_target_constants.gd +++ b/client/scripts/prototype_target_constants.gd @@ -12,13 +12,27 @@ extends Object const ORDERED_IDS: Array[String] = ["prototype_target_alpha", "prototype_target_beta"] +## All prototype targets share a single radius (`PrototypeTargetRegistry.SharedLockRadius` +## on the server). Per-target radii will come back with real combat design (E5.M1). +const SHARED_LOCK_RADIUS: float = 6.0 + ## Keys match [constant ORDERED_IDS]; values mirror `PrototypeTargetRegistry.cs` anchors -## (world space) and horizontal lock radii (XZ meters). +## (world space) and horizontal lock radii (XZ meters). Anchors are placed so the two rings +## overlap at origin (~3.5 m wide), letting Tab flip between targets without locomotion. +## Keep in lock-step with the C# registry. const ANCHORS: Dictionary = { "prototype_target_alpha": - {"position": Vector3(-5.0, 0.5, -5.0), "radius": 8.0, "color": Color(1.0, 0.55, 0.2, 1.0)}, + { + "position": Vector3(-3.0, 0.5, -3.0), + "radius": SHARED_LOCK_RADIUS, + "color": Color(1.0, 0.55, 0.2, 1.0), + }, "prototype_target_beta": - {"position": Vector3(8.0, 0.0, 8.0), "radius": 4.0, "color": Color(0.7, 0.45, 1.0, 1.0)}, + { + "position": Vector3(3.0, 0.0, 3.0), + "radius": SHARED_LOCK_RADIUS, + "color": Color(0.7, 0.45, 1.0, 1.0), + }, } @@ -66,9 +80,11 @@ static func ordered_ids() -> Array[String]: ## 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-side horizontal distance ≤ anchor radius) **and not equal to [param current]**, +## starting after [param current] and wrapping last → first. Returns [code]""[/code] when +## no such anchor is in range so callers can fall back to [method next_id_after] and let +## the server pick the denial reason (this preserves the "Tab on an out-of-reach swap = +## visible denial" UX from NEO-23's soft-lock rule). ## ## Client math here is a UX convenience (same purpose as the HUD distance lines): the ## server remains authoritative for the actual lock decision in @@ -78,9 +94,10 @@ static func ordered_ids() -> Array[String]: static func next_in_range_id_after(current: Variant, world: Vector3) -> String: if ORDERED_IDS.is_empty(): return "" + var current_str: String = "" var start_idx: int = 0 if current is String: - var current_str: String = (current as String).strip_edges() + current_str = (current as String).strip_edges() if not current_str.is_empty(): var found: int = ORDERED_IDS.find(current_str) if found >= 0: @@ -88,6 +105,8 @@ static func next_in_range_id_after(current: Variant, world: Vector3) -> String: 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] + if probe_id == current_str: + continue var radius: float = anchor_radius(probe_id) if radius <= 0.0: continue diff --git a/client/test/target_selection_client_test.gd b/client/test/target_selection_client_test.gd index afdac45..77241b1 100644 --- a/client/test/target_selection_client_test.gd +++ b/client/test/target_selection_client_test.gd @@ -265,7 +265,7 @@ func test_select_post_without_freshness_wiring_is_a_noop() -> void: 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 + # Player stands next to beta (well outside alpha's 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( @@ -275,8 +275,8 @@ func test_tab_from_no_lock_skips_out_of_range_and_picks_beta() -> void: 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)) + # Near beta anchor (3, 3), ~1.4 m horizontal — inside 6 m. Far from alpha at (-3, -3). + var player := _make_player(Vector3(4.0, 0.5, 4.0)) c.set_freshness_kick(authority, player) c.request_tab_next() assert_that(transport.last_body).contains('"targetId":"%s"' % BETA_ID) @@ -302,6 +302,57 @@ func test_tab_from_no_lock_with_nothing_in_range_falls_back_to_first_id() -> voi assert_that(c.cached_state().get("reasonCode", "")).is_equal("out_of_range") +func test_tab_from_locked_alpha_at_origin_swaps_to_beta_when_both_in_range() -> void: + # With alpha (-3,-3) and beta (3,3) sharing r=6, origin is in both rings. Locked to + # alpha, Tab should request beta (normal cycle behavior; range-aware skip-current + # does not interfere when the next id is also in range). + var transport := MockHttpTransport.new() + transport.enqueue( + HTTPRequest.RESULT_SUCCESS, 200, _select_response_json(true, ALPHA_ID, "ok", 1) + ) + transport.enqueue( + HTTPRequest.RESULT_SUCCESS, 200, _select_response_json(true, BETA_ID, "ok", 2) + ) + var c := _make_client(transport) + var authority := FreshnessAuthorityStub.new() + auto_free(authority) + add_child(authority) + var player := _make_player(Vector3(0.0, 0.5, 0.0)) + c.set_freshness_kick(authority, player) + c.request_tab_next() + 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_when_only_current_lock_is_in_range_falls_back_to_cycle_for_denial() -> void: + # Locked to alpha, only alpha is in range — Tab should express a *swap* intent and + # fall through to the plain cycle so the server denies with `out_of_range` (NEO-23 + # soft-lock rule). If the picker returned alpha instead, the user would see no + # visible reaction to Tab. + var transport := MockHttpTransport.new() + transport.enqueue( + HTTPRequest.RESULT_SUCCESS, 200, _select_response_json(true, ALPHA_ID, "ok", 1) + ) + transport.enqueue( + HTTPRequest.RESULT_SUCCESS, + 200, + _select_response_json(false, ALPHA_ID, "ok", 1, "out_of_range") + ) + var c := _make_client(transport) + var authority := FreshnessAuthorityStub.new() + auto_free(authority) + add_child(authority) + # Near alpha (-3, -3), ~1.4 m in — beta (3, 3) is ~8.5 m out. + var player := _make_player(Vector3(-2.0, 0.5, -2.0)) + c.set_freshness_kick(authority, player) + c.request_select_target_id(ALPHA_ID) + c.request_tab_next() + assert_that(transport.last_body).contains('"targetId":"%s"' % BETA_ID) + assert_that(c.cached_state().get("reasonCode", "")).is_equal("out_of_range") + assert_that(c.cached_state().get("lockedTargetId")).is_equal(ALPHA_ID) + + 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. diff --git a/docs/manual-qa/NEO-24.md b/docs/manual-qa/NEO-24.md index 1a156d4..884351e 100644 --- a/docs/manual-qa/NEO-24.md +++ b/docs/manual-qa/NEO-24.md @@ -23,10 +23,10 @@ Pair the client (Godot **F5**) with the server (`cd server/NeonSprawl.Server && - `Target: —` - `Validity: none` - `Seq: 0` - - `prototype_target_alpha: 0.00 m / 8.0 (in)` (spawn `(-5,-5)` is exactly on alpha's XZ anchor) - - `prototype_target_beta: 18.38 m / 4.0 (out)` + - `prototype_target_alpha: 2.83 m / 6.0 (in)` (spawn `(-5,-5)` is ~2.83 m from alpha anchor `(-3,-3)`) + - `prototype_target_beta: 11.31 m / 6.0 (out)` - [ ] `UICanvas/PlayerPositionLabel` shows an `srv: (x, y, z) Δ=m age=` line within ~1 s of boot (boot `GET /position` populates it). At rest the `Δ` should be near 0. -- [ ] **Visible anchors** are rendered in-world: an **orange** mast + flat ring at `(-5, 0.5, -5)` (alpha, 8 m radius) and a **violet** mast + ring at `(8, 0, 8)` (beta, 4 m radius). Both rings lie flat on the floor; the player capsule should sit inside the alpha ring at spawn. +- [ ] **Visible anchors** are rendered in-world with a **shared 6 m radius**: an **orange** mast + flat ring at `(-3, 0.5, -3)` (alpha) and a **violet** mast + ring at `(3, 0, 3)` (beta). Both rings lie flat on the floor and **visibly overlap** in a band of ~3.5 m centered at origin. The player capsule should sit inside the alpha ring (only) at spawn. - [ ] Godot **Output** has no `TargetSelectionClient:` warnings. Server console shows a single `GET /game/players/dev-local-1/target` from the boot sync. ## 2. Tab cycle — happy path @@ -40,33 +40,41 @@ Pair the client (Godot **F5**) with the server (`cd server/NeonSprawl.Server && ## 3. Tab to out-of-range target — soft lock preserved -- [ ] With alpha locked, press **Tab** again. The server denies the swap to beta (spawn is ~18 m from `(8, 8)`; beta radius is 4 m). +- [ ] With alpha locked at spawn, press **Tab** again. The server denies the swap to beta (spawn is ~11.3 m from `(3, 3)`; shared radius is 6 m). Because the range-aware picker **skips the currently-locked id**, Tab falls back to the plain cycle (→ beta), the server rejects, and the soft lock is preserved. - [ ] HUD **stays** at `Target: prototype_target_alpha`, `Validity: ok`, `Seq: 1` with **`Denied: out_of_range`** appended. That is the NEO-23 soft-lock rule: a failed swap does not change the persisted lock or its sequence. - [ ] Server log shows the denial: `POST` → `200` with `selectionApplied: false`. - [ ] Press **Tab** several more times from here. Each press cleanly denies; HUD never drifts or shows a `beta` / optimistic lock between responses. ## 4. Soft-lock out-of-range via locomotion (plan Decision 2) -- [ ] Still locked to alpha, **WASD** away from spawn until you are ~8 m horizontally from `(-5, -5)`. Within ~250 ms of the next `move-stream` 200 (each successful POST emits `authoritative_ack` → throttled target `GET`), HUD flips to: +- [ ] Still locked to alpha, **WASD** away from spawn until you are ~6 m horizontally from `(-3, -3)` (easiest route: head toward the -x/-z side of the map away from both rings). Within ~250 ms of the next `move-stream` 200 (each successful POST emits `authoritative_ack` → throttled target `GET`), HUD flips to: - `Target: prototype_target_alpha` (unchanged — soft lock) - `Validity: out_of_range` - `Seq: 1` (unchanged — no lock id change) -- [ ] Walk back inside the radius. HUD returns to `Validity: ok` within ~250 ms of the next `move-stream` ack. +- [ ] Walk back inside the alpha ring. HUD returns to `Validity: ok` within ~250 ms of the next `move-stream` ack. - [ ] While walking, Godot Output is **not** spammed with targeting GETs. Server log shows no more than roughly one targeting `GET` per 250 ms during sustained motion, even though `move-stream` POSTs fire at ~20 Hz. - [ ] Stand still with no lock changes. Server log shows **no** targeting GETs at idle (no `move-stream` is sent while idle, so there is no ack to trigger a refresh). -- [ ] **Distance lines match the ring**: while moving, the `prototype_target_alpha: m / 8.0 (in|out)` line flips `in`/`out` as you cross the visible orange ring edge. Any time the HUD shows `Validity: ok` but the distance line reads `(out)`, the server position snapshot is lagging the visible capsule — the next `authoritative_ack` refresh should reconcile within 250 ms. +- [ ] **Distance lines match the ring**: while moving, the `prototype_target_alpha: m / 6.0 (in|out)` line flips `in`/`out` as you cross the visible orange ring edge. Any time the HUD shows `Validity: ok` but the distance line reads `(out)`, the server position snapshot is lagging the visible capsule — the next `authoritative_ack` refresh should reconcile within 250 ms. -## 5. Swap to beta when in range +## 5. Tab-flip at origin (overlap zone) — shared-radius sanity -- [ ] WASD toward beta at **(8, 8)** until you are inside the violet ring (≤4 m). Press **Tab**. HUD swaps cleanly: +- [ ] WASD toward the **origin** `(0, 0)`. Both rings overlap there; the HUD should show **both** distance lines with `(in)` (each ~4.24 m / 6.0). +- [ ] Press **Tab** from no-lock (if Esc'd first): HUD flips to `Target: prototype_target_alpha`, `Validity: ok`, `Seq` +1. Cycle-first. +- [ ] Press **Tab** again: HUD flips to `Target: prototype_target_beta`, `Validity: ok`, `Seq` +1. The range-aware picker returns beta (next in cycle, still in range). +- [ ] Press **Tab** a third time: HUD flips back to `Target: prototype_target_alpha`. This is the core "flip between targets without moving" sanity — if it does **not** flip cleanly, check the server log; you should see a successful `POST /target/select` per press. +- [ ] **Walk slightly off-center** so only one ring covers you (e.g. toward alpha's anchor): distance lines flip `(in)` / `(out)` accordingly, and Tab swaps behave like Section 3 (denial on the out-of-range direction, preserved soft lock). + +## 6. Swap to beta when in range (via walking, not the overlap) + +- [ ] WASD from spawn straight toward beta at **(3, 3)** until the capsule is inside the violet ring (≤6 m). Press **Tab**. HUD swaps cleanly: - `Target: prototype_target_beta` - `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). +- [ ] Press **Tab** once more from deep inside beta (alpha out of range). Server denies — HUD stays at `beta / ok` with `Denied: out_of_range` appended (same soft-lock UX as Section 3, other direction). +- [ ] **Range-aware cycle sanity:** press **Esc** to clear the lock (`Target: —`), then — while still inside beta's ring but outside alpha's — press **Tab**. The client skips alpha (out of range) and locks **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 +## 7. Clear - [ ] Press **Esc** at any point with a lock held. HUD flips to: - `Target: —` @@ -76,30 +84,30 @@ Pair the client (Godot **F5**) with the server (`cd server/NeonSprawl.Server && - [ ] Server log shows `POST /target/select` with body `{"schemaVersion":1}` (no `targetId` key). - [ ] Press **Esc** again with no lock. Server returns the same cleared state; HUD stays `—` / `none` (server may or may not bump sequence — whatever it returns is what the HUD shows). -## 7. Denial UI hygiene +## 8. Denial UI hygiene - [ ] Press **Tab** from `—` while far from both targets. If the server denies (possible depending on exact spawn distance), HUD shows `Target: —`, `Validity: none`, `Denied: out_of_range`. The lock id does **not** get "stuck" at something the server refused. - [ ] After any successful Tab / Esc, the `Denied: …` line goes away on the next response. -## 8. Input behavior +## 9. Input behavior - [ ] **Tab** does **not** make focus jump around UI during play (we intercept in `_input` before UI focus navigation). - [ ] **Esc** does not quit or minimize the game. - [ ] Holding **Tab** does not auto-repeat into a firehose of POSTs (pressed action only fires on key press, not echo). - [ ] **E** (NEO-9 interact) and **WASD** (NEO-22 move) still work independently of the target lock; having a lock does not block interact or movement. -## 9. Coexistence with movement rejection (NEO-19 / move-stream 400) +## 10. Coexistence with movement rejection (NEO-19 / move-stream 400) - [ ] Walk at the orange `MoveRejectPedestal` (~(7.5, −6.5)) while holding a lock. When `move-stream` returns **400** (`vertical_step_exceeded`), `PositionAuthorityClient` runs a boot-style re-sync which emits both `authoritative_position_received` (for the snap) and `authoritative_ack` (for cooldown-throttled consumers). - [ ] HUD's `MoveRejectLabel` shows the rejection message as before (NEO-19). - [ ] `TargetSelectionClient` treats that ack like any other: if a lock is held, it fires at most one refresh GET (subject to cooldown). No duplicate requests, no stuck busy state. -## 10. Server-down / transport failure +## 11. Server-down / transport failure - [ ] Stop the server. Press **Tab** / **Esc**. Godot **Output** prints `TargetSelectionClient: HTTP failed …` warnings; the client is not wedged — you can press again (busy flag is released on failure). - [ ] Restart the server and confirm the next **Tab** succeeds; HUD updates from the response. -## 11. No regressions +## 12. No regressions - [ ] Boot snap still works (NEO-7). - [ ] WASD locomotion + `move-stream` still works (NEO-22); `PlayerPositionLabel` updates every physics tick. @@ -109,4 +117,4 @@ Pair the client (Godot **F5**) with the server (`cd server/NeonSprawl.Server && --- -When sections 1–10 are ticked and 11 shows no regressions, the story is ready for **In Test** / PR. Record any deviations or follow-ups in the **Decisions** or **Open questions** sections of the [implementation plan](../plans/NEO-24-implementation-plan.md). +When sections 1–11 are ticked and 12 shows no regressions, the story is ready for **In Test** / PR. Record any deviations or follow-ups in the **Decisions** or **Open questions** sections of the [implementation plan](../plans/NEO-24-implementation-plan.md). diff --git a/docs/plans/NEO-24-implementation-plan.md b/docs/plans/NEO-24-implementation-plan.md index 9d61c86..bbaf3bc 100644 --- a/docs/plans/NEO-24-implementation-plan.md +++ b/docs/plans/NEO-24-implementation-plan.md @@ -107,4 +107,5 @@ 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. | +| 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. The picker also **skips the currently-locked id** so Tab always expresses a swap intent — if the only in-range target is what you already have, the fallback picks the next cycle id and lets the server deny (preserves the NEO-23 soft-lock "Tab = visible denial when no swap possible" rule). | +| 7 | **Prototype target geometry: shared radius + overlap** | Follow-up request: make all targetable objects share a single radius and make the rings overlap in the test scene so Tab-flipping is observable without locomotion. Introduced `PrototypeTargetRegistry.SharedLockRadius = 6.0` and moved anchors to **alpha `(-3, 0.5, -3)`** and **beta `(3, 0, 3)`** (distance ~8.49 m ⇒ ~3.5 m-wide overlap centered at origin). Spawn `(-5, -5)` keeps alpha in range (~2.83 m) and beta out (~11.31 m) so the existing manual QA "Tab-to-out-of-range = soft-lock denial" path still holds; walking to the origin puts both rings in range and Tab cleanly flips alpha ↔ beta. `client/scripts/prototype_target_constants.gd` mirrors the new values as display-only data (+ a `SHARED_LOCK_RADIUS` constant to match the server). Keep the two in lock-step; NEO-23 reader tests updated to the new boundary (`(3, -3)` for alpha radius 6). Per-target radii will return with real combat design (E5.M1); this is a prototype stub only. | diff --git a/docs/reviews/2026-04-21-NEO-24.md b/docs/reviews/2026-04-21-NEO-24.md index 93e0821..43780c8 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 #4 (2026-04-21):** user asked for a single shared radius across all prototype targets and overlapping rings in the test scene so Tab-flipping is observable without walking. `PrototypeTargetRegistry` now exposes a `SharedLockRadius = 6.0` constant used by both anchors; alpha moved to `(-3, 0.5, -3)` and beta to `(3, 0, 3)` so the rings overlap ~3.5 m wide centered at origin. Spawn at `(-5, -5)` keeps alpha in range / beta out (existing soft-lock denial QA path untouched); walking to origin flips both rings in-range and the range-aware Tab picker swaps alpha ↔ beta on every press. `PlayerTargetStateReaderTests` updated to the new `(3, -3)` alpha boundary plus a new origin-in-both test. `prototype_target_constants.gd` mirrors the server's `SHARED_LOCK_RADIUS`. Picker tightened to **skip the currently-locked id** — so with only one target in range and that target already locked, Tab falls back to the cycle and the server denies, keeping NEO-23's "Tab = visible denial when no swap is possible" rule. New tests: `test_tab_from_locked_alpha_at_origin_swaps_to_beta_when_both_in_range`, `test_tab_when_only_current_lock_is_in_range_falls_back_to_cycle_for_denial`. Plan Decision 7. + **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: diff --git a/server/NeonSprawl.Server.Tests/Game/Targeting/PlayerTargetStateReaderTests.cs b/server/NeonSprawl.Server.Tests/Game/Targeting/PlayerTargetStateReaderTests.cs index 4868ff0..f7c8cd7 100644 --- a/server/NeonSprawl.Server.Tests/Game/Targeting/PlayerTargetStateReaderTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Targeting/PlayerTargetStateReaderTests.cs @@ -23,8 +23,8 @@ public class PlayerTargetStateReaderTests [Fact] public void ComputeValidity_AtExactLockRadius_ReturnsOk() { - // Alpha anchor XZ (-5, -5), lockRadius 8 → horizontal distance 8 from (3, -5) is on boundary (inclusive). - var snap = new PositionSnapshot(3, 0, -5, 0); + // Alpha anchor XZ (-3, -3), lockRadius 6 → horizontal distance 6 from (3, -3) is on boundary (inclusive). + var snap = new PositionSnapshot(3, 0, -3, 0); Assert.Equal( TargetValidity.Ok, PlayerTargetStateReader.ComputeValidity(PrototypeTargetRegistry.PrototypeTargetAlphaId, in snap)); @@ -34,9 +34,23 @@ public class PlayerTargetStateReaderTests public void ComputeValidity_JustOutsideLockRadius_ReturnsOutOfRange() { const double eps = 1e-6; - var snap = new PositionSnapshot(3 + eps, 0, -5, 0); + var snap = new PositionSnapshot(3 + eps, 0, -3, 0); Assert.Equal( TargetValidity.OutOfRange, PlayerTargetStateReader.ComputeValidity(PrototypeTargetRegistry.PrototypeTargetAlphaId, in snap)); } + + [Fact] + public void ComputeValidity_AtOrigin_ShouldReturnOk_ForBothPrototypeTargets() + { + // NEO-24 post-merge: alpha (-3,-3) r=6 and beta (3,3) r=6 overlap at origin so Tab can + // flip without locomotion. Origin is ~4.24 m from each anchor (< 6 m). + var snap = new PositionSnapshot(0, 0, 0, 0); + Assert.Equal( + TargetValidity.Ok, + PlayerTargetStateReader.ComputeValidity(PrototypeTargetRegistry.PrototypeTargetAlphaId, in snap)); + Assert.Equal( + TargetValidity.Ok, + PlayerTargetStateReader.ComputeValidity(PrototypeTargetRegistry.PrototypeTargetBetaId, in snap)); + } } diff --git a/server/NeonSprawl.Server/Game/Targeting/PrototypeTargetRegistry.cs b/server/NeonSprawl.Server/Game/Targeting/PrototypeTargetRegistry.cs index f339bdb..e477ff9 100644 --- a/server/NeonSprawl.Server/Game/Targeting/PrototypeTargetRegistry.cs +++ b/server/NeonSprawl.Server/Game/Targeting/PrototypeTargetRegistry.cs @@ -3,16 +3,24 @@ namespace NeonSprawl.Server.Game.Targeting; /// Prototype combat target ids → world anchor + horizontal lock radius (NEO-23). Keys are lowercase. public static class PrototypeTargetRegistry { - /// Near default dev spawn (-5, -5) on XZ. + /// + /// All prototype combat targets share a single LockRadius. Scoped to prototype + /// stubs — per-target radii will come back with real combat design (E5.M1). Changing + /// this requires updating client/scripts/prototype_target_constants.gd in the + /// same commit so markers/HUD match server range checks. + /// + public const double SharedLockRadius = 6.0; + + /// On the -x/-z side of spawn; overlaps 's ring at origin so Tab can flip without moving. public const string PrototypeTargetAlphaId = "prototype_target_alpha"; - /// Farther stub for out-of-range tests; ascending id order for NEO-24 tab cycle. + /// On the +x/+z side; mirrors alpha across the origin for symmetric overlap (NEO-24 Tab cycle). public const string PrototypeTargetBetaId = "prototype_target_beta"; - /// Alpha anchor aligned with spawn horizontal cell; on XZ only. - public static readonly PrototypeTargetEntry PrototypeTargetAlpha = new(-5, 0.5, -5, 8.0); + /// Alpha anchor; is XZ-only. Distance to beta ~8.49 m ⇒ ~3.5 m overlap at origin. + public static readonly PrototypeTargetEntry PrototypeTargetAlpha = new(-3, 0.5, -3, SharedLockRadius); - public static readonly PrototypeTargetEntry PrototypeTargetBeta = new(8, 0, 8, 4.0); + public static readonly PrototypeTargetEntry PrototypeTargetBeta = new(3, 0, 3, SharedLockRadius); private static readonly Dictionary ById = new(StringComparer.Ordinal) { From d6ef4fd81131b17e73c54491948bc0759495de95 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Tue, 21 Apr 2026 23:52:20 -0400 Subject: [PATCH 13/13] NEO-24: race-free target/select via optional positionHint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Followup #5: after locking alpha, walking into beta's ring, stopping and pressing Tab could still return `out_of_range` even when client and server agreed on position (Δ=0). The freshness-kick move-stream POST and the target/select POST race on separate HTTP connections so the select can be validated against a stale stored snap. Extend TargetSelectRequest v1 with an optional positionHint {x,y,z}. Server uses the hint for the radius check and for the echoed targetState.validity; the hint is advisory and does not write to the position store (move-stream stays the sole write path). Client attaches the hint automatically whenever set_freshness_kick(...) is wired, so headless unit tests that skip the wiring fall through to the no-hint behavior. Freshness kick is kept in place to refresh the stored snap for later validity GETs but is no longer load-bearing for denial correctness. Server: 3 new tests (hint accepts when stored is stale, hint-far denies, hint never writes to position store). Client: 1 new test asserting the hint is in the POST body when wired. All 62 server + 96 client tests pass. Plan Decision 8, review follow-up #5, manual QA section 6, and NEO-23 contract notes updated. New bruno request exercises the hint path. --- .../Post target select beta with hint.bru | 30 +++++++ client/README.md | 3 +- client/scripts/target_selection_client.gd | 35 +++++--- client/test/target_selection_client_test.gd | 34 +++++++- docs/manual-qa/NEO-24.md | 3 +- docs/plans/NEO-23-implementation-plan.md | 2 +- docs/plans/NEO-24-implementation-plan.md | 1 + docs/reviews/2026-04-21-NEO-24.md | 2 + .../Game/Targeting/TargetingApiTests.cs | 80 +++++++++++++++++++ .../Game/Targeting/TargetStateDtos.cs | 13 +++ .../Game/Targeting/TargetingApi.cs | 16 +++- 11 files changed, 202 insertions(+), 17 deletions(-) create mode 100644 bruno/neon-sprawl-server/targeting/Post target select beta with hint.bru diff --git a/bruno/neon-sprawl-server/targeting/Post target select beta with hint.bru b/bruno/neon-sprawl-server/targeting/Post target select beta with hint.bru new file mode 100644 index 0000000..88b9361 --- /dev/null +++ b/bruno/neon-sprawl-server/targeting/Post target select beta with hint.bru @@ -0,0 +1,30 @@ +meta { + name: POST target select - prototype_target_beta (with positionHint) + type: http + seq: 13 +} + +post { + url: {{baseUrl}}/game/players/{{playerId}}/target/select + body: json + auth: none +} + +headers { + content-type: application/json +} + +body:json { + { + "schemaVersion": 1, + "targetId": "prototype_target_beta", + "positionHint": { "x": 3.0, "y": 0.5, "z": 5.0 } + } +} + +docs { + NEO-24 follow-up #5: the optional `positionHint` lets the server run the radius check against + the client's live capsule position instead of the stored `move-stream` snap. Useful to verify + a "stopped near beta but store is stale" case without having to drive the capsule manually. + Server treats the hint as advisory — it is NOT written to the position store. +} diff --git a/client/README.md b/client/README.md index f427c61..ca66072 100644 --- a/client/README.md +++ b/client/README.md @@ -97,7 +97,8 @@ The main scene includes a **prototype terminal** at the map center (same world * - **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: 7.1 m / 6.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. +- **`positionHint` on select (NEO-24 follow-up #5):** when `TargetSelectionClient.set_freshness_kick(authority, player)` is wired (as `main.gd` does), every `POST /target/select` body also carries `positionHint: {x,y,z}` = the live capsule position. The server uses the hint for the radius check and for the `validity` field in the echoed `targetState`, eliminating the cross-request race between `move-stream` and `target/select` POSTs — a Tab pressed several seconds after stopping (with an old stored snap on the server) now still succeeds when you are visibly in range. The hint is advisory (server never writes it to the position store); `move-stream` remains the sole write path. Tests that omit `set_freshness_kick(...)` fall back to the no-hint code path. +- **Freshness kick (ancillary):** alongside the hint, `set_freshness_kick(...)` also has the target client nudge the authority with `submit_stream_targets([player.global_position])` before each select POST. That write is no longer load-bearing for denial correctness (the hint handles that), but it keeps the stored snap reasonably fresh so the movement-triggered validity GET that runs on the next `authoritative_ack` sees current data. - **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. All prototype targets share a single radius (`PrototypeTargetConstants.SHARED_LOCK_RADIUS` = `PrototypeTargetRegistry.SharedLockRadius` = **6 m**); anchors are placed at `(-3, 0.5, -3)` and `(3, 0, 3)` so the two rings overlap at origin (~3.5 m wide), letting you flip between targets with Tab without locomotion. The script and `prototype_target_constants.gd` mirror the server's `PrototypeTargetRegistry.cs` — change them together in the same commit so the markers never drift from server authority. Per-target radii will return with real combat design (E5.M1). - **Soft lock refresh:** the client does **not** poll on a timer. It refreshes via `GET …/target` on boot, on every `POST …/target/select` response (body already carries `targetState`), and — while a lock is currently held — on **`authoritative_ack`** from `PositionAuthorityClient`, throttled to at most one GET per **250 ms**. `authoritative_ack` fires on every server-confirmed position (boot `GET` 200 **and** successful `move-stream` 200), so a held lock revalidates during normal WASD locomotion without re-introducing snap rubber-banding. Boot is naturally skipped because no lock exists yet. Purely server-driven state flips (future PvP / combat categories) would require revisiting this policy; today NEO-23 has none. - **Inspector:** match **`base_url`** / **`dev_player_id`** on the `TargetSelectionClient` node to the other HTTP clients / `Game:DevPlayerId`. diff --git a/client/scripts/target_selection_client.gd b/client/scripts/target_selection_client.gd index 0168128..56b96a3 100644 --- a/client/scripts/target_selection_client.gd +++ b/client/scripts/target_selection_client.gd @@ -36,10 +36,11 @@ var _phase: Phase = Phase.IDLE ## Optional "freshness kick": before each `POST /target/select` we ask the authority to ## submit a `move-stream` with the current player position so the server's stored snapshot -## is as close to the visible capsule as possible when it runs the radius check. Without -## this, a Tab press right after you stop moving can be validated against the last -## 20 Hz sample (which trailed the capsule) and deny at a position that is visibly in -## range. Both refs are optional so tests that inject a plain script can still run. +## catches up with the visible capsule. Since NEO-24 follow-up #5 the `target/select` decision +## itself is made against the `positionHint` in the request body (race-free), so the kick is +## no longer load-bearing for denial correctness — its role is to keep the stored snap fresh +## for the movement-triggered validity GET that runs on the next `authoritative_ack`. Both +## refs are optional so tests that inject a plain script can still run without wiring. var _freshness_authority: Node = null var _freshness_player: Node3D = null @@ -114,7 +115,9 @@ func request_select_target_id(target_id: String) -> void: if trimmed.is_empty(): push_warning("TargetSelectionClient: refusing empty targetId; use request_clear_target()") return - _post_select({"schemaVersion": 1, "targetId": trimmed}) + var payload: Dictionary = {"schemaVersion": 1, "targetId": trimmed} + _maybe_attach_position_hint(payload) + _post_select(payload) func request_clear_target() -> void: @@ -122,6 +125,19 @@ func request_clear_target() -> void: _post_select({"schemaVersion": 1}) +## NEO-24 follow-up #5: attach the live capsule position as an advisory `positionHint` so the +## server's range check runs against what the player sees *right now* instead of the last +## `move-stream` sample the server happens to have stored. Only populated when the player ref +## is wired (via [method set_freshness_kick]) — tests and other consumers that skip the wiring +## fall through to today's stored-snap behavior. Server remains authoritative (hint is advisory +## and does not write to the position store). +func _maybe_attach_position_hint(payload: Dictionary) -> void: + if _freshness_player == null or not is_instance_valid(_freshness_player): + return + var p: Vector3 = _freshness_player.global_position + payload["positionHint"] = {"x": p.x, "y": p.y, "z": p.z} + + ## Connected to `PositionAuthorityClient.authoritative_ack` in `main.gd`. ## Boot is naturally skipped because no lock is held yet; once a lock exists, subsequent acks ## (boot resync or `move-stream` 200) fire a refresh GET subject to a cooldown so bursts @@ -172,11 +188,10 @@ func _post_select(payload: Dictionary) -> void: _phase = Phase.IDLE -## Nudges the authority with the current capsule position so the server's stored -## snapshot is as fresh as possible when the select POST lands. Does **not** wait -## for the move-stream response — both requests race on separate connections — but -## in practice this collapses the common "Tab right after stopping" false-denial -## window because the authority flushes queued crumbs synchronously on submit. +## Nudges the authority with the current capsule position so the server's stored snapshot +## catches up. Paired with the `positionHint` on the select POST (follow-up #5), this keeps +## subsequent validity GETs honest; the select itself no longer depends on which request +## arrives at the server first. func _kick_freshness_stream() -> void: if _freshness_authority == null or _freshness_player == null: return diff --git a/client/test/target_selection_client_test.gd b/client/test/target_selection_client_test.gd index 77241b1..f8fa9e8 100644 --- a/client/test/target_selection_client_test.gd +++ b/client/test/target_selection_client_test.gd @@ -251,9 +251,36 @@ func test_select_post_kicks_freshness_stream_before_posting() -> void: assert_that(transport.last_url).contains("/target/select") +func test_select_post_includes_position_hint_when_player_wired() -> void: + # NEO-24 follow-up #5: the select POST body must carry the live capsule position as + # `positionHint` so the server's range check can bypass any stale stored snap. + 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) + # Values chosen so `JSON.stringify` emits exact decimal representations (no float precision + # drift): 3.0 → "3", 0.5 → "0.5", 5.0 → "5". Re-parse the JSON for precise assertions. + var player := _make_player(Vector3(3.0, 0.5, 5.0)) + c.set_freshness_kick(authority, player) + c.request_select_target_id(BETA_ID) + var parsed: Variant = JSON.parse_string(transport.last_body) + assert_that(parsed is Dictionary).is_true() + var body: Dictionary = parsed + assert_that(body.get("targetId")).is_equal(BETA_ID) + assert_that(body.has("positionHint")).is_true() + var hint: Dictionary = body["positionHint"] + assert_float(hint.get("x")).is_equal_approx(3.0, 0.0001) + assert_float(hint.get("y")).is_equal_approx(0.5, 0.0001) + assert_float(hint.get("z")).is_equal_approx(5.0, 0.0001) + + func test_select_post_without_freshness_wiring_is_a_noop() -> void: # Regression: tests that construct the client without `set_freshness_kick(...)` must - # still POST normally. Confirms the kick is fully optional. + # still POST normally, *without* a `positionHint` (hint is opt-in per wiring). var transport := MockHttpTransport.new() transport.enqueue( HTTPRequest.RESULT_SUCCESS, 200, _select_response_json(true, ALPHA_ID, "ok", 1) @@ -261,6 +288,11 @@ func test_select_post_without_freshness_wiring_is_a_noop() -> void: var c := _make_client(transport) c.request_select_target_id(ALPHA_ID) assert_that(transport.last_url).contains("/target/select") + # No `positionHint` key in the body when no player is wired — re-parse so the assertion + # is robust against JSON whitespace / key ordering. + var parsed: Variant = JSON.parse_string(transport.last_body) + assert_that(parsed is Dictionary).is_true() + assert_that((parsed as Dictionary).has("positionHint")).is_false() assert_that(c.cached_state().get("lockedTargetId")).is_equal(ALPHA_ID) diff --git a/docs/manual-qa/NEO-24.md b/docs/manual-qa/NEO-24.md index 884351e..db86722 100644 --- a/docs/manual-qa/NEO-24.md +++ b/docs/manual-qa/NEO-24.md @@ -72,7 +72,8 @@ Pair the client (Godot **F5**) with the server (`cd server/NeonSprawl.Server && - `Seq` increments by 1 - [ ] Press **Tab** once more from deep inside beta (alpha out of range). Server denies — HUD stays at `beta / ok` with `Denied: out_of_range` appended (same soft-lock UX as Section 3, other direction). - [ ] **Range-aware cycle sanity:** press **Esc** to clear the lock (`Target: —`), then — while still inside beta's ring but outside alpha's — press **Tab**. The client skips alpha (out of range) and locks **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. +- [ ] **`positionHint` sanity (NEO-24 follow-up #5):** walk to the edge of beta's ring, stop, and **immediately** press Tab. The select POST should succeed (`Validity: ok`). The client now attaches `positionHint: {x,y,z}` to the `target/select` body, so the server runs the radius check against the live capsule position instead of the stored `move-stream` snap — even when `PlayerPositionLabel`'s `age=…` is several seconds old or `Δ>0`. Denials here would mean the hint is not being attached (confirm `set_freshness_kick` is wired in `main.gd`) or the client's sense of "in range" genuinely disagrees with the server's radius math (check the per-target distance lines). +- [ ] **Soft-lock roam regression (follow-up #5 repro):** lock alpha at spawn, walk out of alpha's ring and into beta's ring (HUD shows `Validity: out_of_range`, beta distance `< 6 m (in)`), **stop**, wait until `age=…` climbs past ~3 s, and press **Tab**. Select swaps to beta successfully (`Target: prototype_target_beta`, `Validity: ok`). Regression before follow-up #5: this path denied with `out_of_range` because the select POST raced the freshness kick's `move-stream` POST on separate HTTP connections. ## 7. Clear diff --git a/docs/plans/NEO-23-implementation-plan.md b/docs/plans/NEO-23-implementation-plan.md index c791182..5f70308 100644 --- a/docs/plans/NEO-23-implementation-plan.md +++ b/docs/plans/NEO-23-implementation-plan.md @@ -46,7 +46,7 @@ 4. **`TargetState` fields (v1 JSON, shared by GET body and nested POST echo):** `schemaVersion`, `playerId` (echo), **`lockedTargetId`** (string or JSON **`null`** — key **always present**), `validity` (`ok`, `out_of_range`, `invalid_target`, `none`), `sequence` (int). **Validity** is **computed** on each read: no lock ⇒ `validity: none` and `lockedTargetId: null`; unknown id in store (should not happen) ⇒ `invalid_target`; else horizontal distance vs **`lockRadius`**. -5. **`TargetSelectRequest` / `TargetSelectResponse` v1:** Request: `schemaVersion`, optional `targetId`. Response: `schemaVersion`, **`selectionApplied`** (bool), **`targetState`** (object matching the GET field set above — always present), **`reasonCode`** (required when `selectionApplied` is `false`, omitted when `true` — same spirit as `InteractionResponse`). Successful **clear** ⇒ `selectionApplied: true`, `targetState` shows `lockedTargetId: null`, `validity: none`. +5. **`TargetSelectRequest` / `TargetSelectResponse` v1:** Request: `schemaVersion`, optional `targetId`, optional `positionHint` (`{x,y,z}` — NEO-24 follow-up #5, non-breaking additive field). When `positionHint` is present the server uses it for the radius check and for the echoed `targetState.validity`, making the select race-free against `move-stream`. The hint is advisory: it does **not** write to `IPositionStateStore` (`move-stream` remains the only position-write path). Response: `schemaVersion`, **`selectionApplied`** (bool), **`targetState`** (object matching the GET field set above — always present), **`reasonCode`** (required when `selectionApplied` is `false`, omitted when `true` — same spirit as `InteractionResponse`). Successful **clear** ⇒ `selectionApplied: true`, `targetState` shows `lockedTargetId: null`, `validity: none`. 6. **Routes** (exact paths in README; suggested): - `GET /game/players/{id}/target` → **envelope** aligned with **`PositionStateResponse`**: top-level `schemaVersion`, `playerId`, plus lock fields (`lockedTargetId`, `validity`, `sequence`) so read and move APIs share the same “version + player echo + payload” pattern. diff --git a/docs/plans/NEO-24-implementation-plan.md b/docs/plans/NEO-24-implementation-plan.md index bbaf3bc..eebf4f0 100644 --- a/docs/plans/NEO-24-implementation-plan.md +++ b/docs/plans/NEO-24-implementation-plan.md @@ -109,3 +109,4 @@ No new **C#** tests (client-only story). **Bruno:** NEO-23 targeting requests al | 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. The picker also **skips the currently-locked id** so Tab always expresses a swap intent — if the only in-range target is what you already have, the fallback picks the next cycle id and lets the server deny (preserves the NEO-23 soft-lock "Tab = visible denial when no swap possible" rule). | | 7 | **Prototype target geometry: shared radius + overlap** | Follow-up request: make all targetable objects share a single radius and make the rings overlap in the test scene so Tab-flipping is observable without locomotion. Introduced `PrototypeTargetRegistry.SharedLockRadius = 6.0` and moved anchors to **alpha `(-3, 0.5, -3)`** and **beta `(3, 0, 3)`** (distance ~8.49 m ⇒ ~3.5 m-wide overlap centered at origin). Spawn `(-5, -5)` keeps alpha in range (~2.83 m) and beta out (~11.31 m) so the existing manual QA "Tab-to-out-of-range = soft-lock denial" path still holds; walking to the origin puts both rings in range and Tab cleanly flips alpha ↔ beta. `client/scripts/prototype_target_constants.gd` mirrors the new values as display-only data (+ a `SHARED_LOCK_RADIUS` constant to match the server). Keep the two in lock-step; NEO-23 reader tests updated to the new boundary (`(3, -3)` for alpha radius 6). Per-target radii will return with real combat design (E5.M1); this is a prototype stub only. | +| 8 | **Post-merge UX: `positionHint` on `target/select` (NEO-23 contract extension)** | Follow-up report: "targeted A, moved out of A range and into B range — Tab gets denied incorrectly" at `Δ=0`, `age≈8 s`. Root cause: the freshness kick from Decision 5 kicks a `move-stream` POST *and* the select POST on separate HTTP connections — nothing guarantees the server processes the stream first, so the select's range check can still run against the stale stored snap. Fix: extend `TargetSelectRequest` v1 with an **optional** `positionHint: {x,y,z}`; when present, the server uses it for the radius check and for the `validity` field in the response (both the soft-lock and applied-lock paths). The hint is purely advisory — it does **not** write to `IPositionStateStore` (`move-stream` stays the only write path) and remains trust-on-client for the prototype; E5.M1 combat will bound the hint against the stored snap / movement budget before shipping to players. Client wires the hint automatically when `set_freshness_kick(...)` is called (same player ref), so existing headless unit tests that skip that wiring continue to omit the hint. The freshness kick is kept in place to refresh the stored snap for subsequent validity GETs, but is no longer load-bearing for denial correctness. | diff --git a/docs/reviews/2026-04-21-NEO-24.md b/docs/reviews/2026-04-21-NEO-24.md index 43780c8..4ee0f49 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 #5 (2026-04-21):** user reported "targeted A, moved out of A range and into B range — Tab gets denied incorrectly." HUD confirmed `Δ=0.00m` with `age≈7964ms` and beta distance `2.04 m / 6.0 (in)`, so client and server *agreed* on the player's stopped position and beta was clearly in range — but the select POST still denied. Root cause: follow-up #2's freshness kick fires a `move-stream` POST and the select POST on separate HTTP connections; nothing guarantees the server processes the stream first, so the select's range check can still race against a stale stored snap (or, for that matter, an ASP.NET request executing on another thread while the stream is still in flight). Fix: extend the NEO-23 `TargetSelectRequest` v1 with an **optional** `positionHint: {x,y,z}`. When present, `TargetingApi` uses the hint for both the radius check and the `validity` field in the echoed `targetState` (so an accepted select never reports a stale `out_of_range`). The hint is advisory — it does **not** write to `IPositionStateStore`, so `move-stream` remains the sole position-write path. `TargetSelectionClient` attaches the hint automatically whenever `set_freshness_kick(...)` is called (same player ref), keeping the headless unit suites that skip that wiring on the unchanged code path. The freshness kick stays in place to keep the stored snap fresh for subsequent validity GETs, but is no longer load-bearing for denial correctness. Tests: `test_select_post_includes_position_hint_when_player_wired` (client), `PostSelect_ShouldUsePositionHint_WhenStoredSnapIsStale` + `PostSelect_ShouldDenyOutOfRange_WhenHintIsFar` + `PostSelect_PositionHint_ShouldNotWriteToPositionStore` (server). Plan Decision 8 + NEO-23 contract notes updated. + **Post-merge UX follow-up #4 (2026-04-21):** user asked for a single shared radius across all prototype targets and overlapping rings in the test scene so Tab-flipping is observable without walking. `PrototypeTargetRegistry` now exposes a `SharedLockRadius = 6.0` constant used by both anchors; alpha moved to `(-3, 0.5, -3)` and beta to `(3, 0, 3)` so the rings overlap ~3.5 m wide centered at origin. Spawn at `(-5, -5)` keeps alpha in range / beta out (existing soft-lock denial QA path untouched); walking to origin flips both rings in-range and the range-aware Tab picker swaps alpha ↔ beta on every press. `PlayerTargetStateReaderTests` updated to the new `(3, -3)` alpha boundary plus a new origin-in-both test. `prototype_target_constants.gd` mirrors the server's `SHARED_LOCK_RADIUS`. Picker tightened to **skip the currently-locked id** — so with only one target in range and that target already locked, Tab falls back to the cycle and the server denies, keeping NEO-23's "Tab = visible denial when no swap is possible" rule. New tests: `test_tab_from_locked_alpha_at_origin_swaps_to_beta_when_both_in_range`, `test_tab_when_only_current_lock_is_in_range_falls_back_to_cycle_for_denial`. Plan Decision 7. **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`. diff --git a/server/NeonSprawl.Server.Tests/Game/Targeting/TargetingApiTests.cs b/server/NeonSprawl.Server.Tests/Game/Targeting/TargetingApiTests.cs index 871b15e..3d28883 100644 --- a/server/NeonSprawl.Server.Tests/Game/Targeting/TargetingApiTests.cs +++ b/server/NeonSprawl.Server.Tests/Game/Targeting/TargetingApiTests.cs @@ -96,6 +96,86 @@ public class TargetingApiTests Assert.Null(body.TargetState.LockedTargetId); } + [Fact] + public async Task PostSelect_ShouldUsePositionHint_WhenStoredSnapIsStale() + { + // NEO-24 follow-up #5: without the hint, spawn's stored position (-5, -5) denies beta (r=6, + // anchor (3, 3)) — ~11.3 m out. With an advisory `positionHint` pinning the capsule inside + // beta's ring, the server must accept. This is exactly the race-free path the client uses. + await using var factory = new InMemoryWebApplicationFactory(); + var client = factory.CreateClient(); + + var response = await client.PostAsJsonAsync( + "/game/players/dev-local-1/target/select", + new TargetSelectRequest + { + SchemaVersion = TargetSelectRequest.CurrentSchemaVersion, + TargetId = PrototypeTargetRegistry.PrototypeTargetBetaId, + PositionHint = new PositionVector { X = 3.0, Y = 0.5, Z = 5.0 }, + }); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + var body = await response.Content.ReadFromJsonAsync(); + Assert.NotNull(body); + Assert.True(body!.SelectionApplied); + Assert.Null(body.ReasonCode); + Assert.Equal(PrototypeTargetRegistry.PrototypeTargetBetaId, body.TargetState.LockedTargetId); + } + + [Fact] + public async Task PostSelect_ShouldDenyOutOfRange_WhenHintIsFar() + { + // Hint that places the capsule outside beta's ring must deny — hint is advisory only; + // it does not bypass the radius check. + await using var factory = new InMemoryWebApplicationFactory(); + var client = factory.CreateClient(); + + var response = await client.PostAsJsonAsync( + "/game/players/dev-local-1/target/select", + new TargetSelectRequest + { + SchemaVersion = TargetSelectRequest.CurrentSchemaVersion, + TargetId = PrototypeTargetRegistry.PrototypeTargetBetaId, + PositionHint = new PositionVector { X = 50.0, Y = 0.0, Z = 50.0 }, + }); + + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + var body = await response.Content.ReadFromJsonAsync(); + Assert.NotNull(body); + Assert.False(body!.SelectionApplied); + Assert.Equal(TargetingApi.ReasonOutOfRange, body.ReasonCode); + } + + [Fact] + public async Task PostSelect_PositionHint_ShouldNotWriteToPositionStore() + { + // Hint must be purely advisory for the range check. Confirm the stored `PositionState` + // is untouched by a select-with-hint by reading it back through `GET /position`. + await using var factory = new InMemoryWebApplicationFactory(); + var client = factory.CreateClient(); + + var before = await client.GetFromJsonAsync("/game/players/dev-local-1/position"); + Assert.NotNull(before); + + Assert.Equal( + HttpStatusCode.OK, + (await client.PostAsJsonAsync( + "/game/players/dev-local-1/target/select", + new TargetSelectRequest + { + SchemaVersion = TargetSelectRequest.CurrentSchemaVersion, + TargetId = PrototypeTargetRegistry.PrototypeTargetBetaId, + PositionHint = new PositionVector { X = 3.0, Y = 0.5, Z = 5.0 }, + })).StatusCode); + + var after = await client.GetFromJsonAsync("/game/players/dev-local-1/position"); + Assert.NotNull(after); + Assert.Equal(before!.Position.X, after!.Position.X); + Assert.Equal(before.Position.Y, after.Position.Y); + Assert.Equal(before.Position.Z, after.Position.Z); + Assert.Equal(before.Sequence, after.Sequence); + } + [Fact] public async Task PostSelect_ShouldClearLock_WhenTargetIdOmitted() { diff --git a/server/NeonSprawl.Server/Game/Targeting/TargetStateDtos.cs b/server/NeonSprawl.Server/Game/Targeting/TargetStateDtos.cs index 99b865c..d94b125 100644 --- a/server/NeonSprawl.Server/Game/Targeting/TargetStateDtos.cs +++ b/server/NeonSprawl.Server/Game/Targeting/TargetStateDtos.cs @@ -1,4 +1,5 @@ using System.Text.Json.Serialization; +using NeonSprawl.Server.Game.PositionState; namespace NeonSprawl.Server.Game.Targeting; @@ -40,6 +41,18 @@ public sealed class TargetSelectRequest /// Registry key after trim + case-insensitive lookup; null or omitted ⇒ clear intent. [JsonPropertyName("targetId")] public string? TargetId { get; init; } + + /// + /// Optional client-reported current position. When present the server uses these coordinates + /// for the range check instead of the stored PositionState snapshot, eliminating the + /// race between move-stream and target/select POSTs (NEO-24 follow-up #5). Purely + /// advisory: it does not write to the position store (move-stream remains the + /// only write path). Real combat (E5.M1) will bound the hint against the stored snap / movement + /// budget so this cannot be used to "teleport" the radius check — prototype does not yet. + /// + [JsonPropertyName("positionHint")] + [JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)] + public PositionVector? PositionHint { get; init; } } /// POST response for target selection (NEO-23). diff --git a/server/NeonSprawl.Server/Game/Targeting/TargetingApi.cs b/server/NeonSprawl.Server/Game/Targeting/TargetingApi.cs index efca25b..172c2bc 100644 --- a/server/NeonSprawl.Server/Game/Targeting/TargetingApi.cs +++ b/server/NeonSprawl.Server/Game/Targeting/TargetingApi.cs @@ -75,10 +75,20 @@ public static class TargetingApi }); } - if (!HorizontalReach.IsWithinHorizontalRadius(snap.X, snap.Z, entry.X, entry.Z, entry.LockRadius)) + // Prefer the client-reported `positionHint` when present: the stored `snap` can + // trail the visible capsule by several seconds when `move-stream` has been idle, + // which caused false `out_of_range` denials (NEO-24 follow-up #5). The hint is + // advisory — position writes stay the sole responsibility of `move-stream`. Real + // combat (E5.M1) will bound the hint vs. stored snap before trusting it. + // The hinted snap is used for the radius check *and* for building the response's + // `validity` so a hint-accepted select does not echo a stale `out_of_range`. + var effective = body.PositionHint is { } hint + ? new PositionSnapshot(hint.X, hint.Y, hint.Z, snap.Sequence) + : snap; + if (!HorizontalReach.IsWithinHorizontalRadius(effective.X, effective.Z, entry.X, entry.Z, entry.LockRadius)) { var (lockFar, seqFar) = locks.GetLockState(id); - var stateFar = PlayerTargetStateReader.Build(id, lockFar, seqFar, in snap); + var stateFar = PlayerTargetStateReader.Build(id, lockFar, seqFar, in effective); return Results.Json( new TargetSelectResponse { @@ -90,7 +100,7 @@ public static class TargetingApi } var applied = locks.ApplySet(id, lookupKey); - var stateOk = PlayerTargetStateReader.Build(id, applied.LockIdLowercase, applied.Sequence, in snap); + var stateOk = PlayerTargetStateReader.Build(id, applied.LockIdLowercase, applied.Sequence, in effective); return Results.Json( new TargetSelectResponse {