NEO-24: race-free target/select via optional positionHint

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.
pull/47/head
VinPropane 2026-04-21 23:52:20 -04:00
parent a7453eb82f
commit d6ef4fd811
11 changed files with 202 additions and 17 deletions

View File

@ -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.
}

View File

@ -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. - **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 `<id>: <dist> m / <radius> (in|out)` (computed client-side from the live capsule position against `prototype_target_constants.gd`), and (on denials) `Denied: <reasonCode>` 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. - **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 `<id>: <dist> m / <radius> (in|out)` (computed client-side from the live capsule position against `prototype_target_constants.gd`), and (on denials) `Denied: <reasonCode>` 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). - **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. - **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`. - **Inspector:** match **`base_url`** / **`dev_player_id`** on the `TargetSelectionClient` node to the other HTTP clients / `Game:DevPlayerId`.

View File

@ -36,10 +36,11 @@ var _phase: Phase = Phase.IDLE
## Optional "freshness kick": before each `POST /target/select` we ask the authority to ## 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 ## 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 ## catches up with the visible capsule. Since NEO-24 follow-up #5 the `target/select` decision
## this, a Tab press right after you stop moving can be validated against the last ## itself is made against the `positionHint` in the request body (race-free), so the kick is
## 20 Hz sample (which trailed the capsule) and deny at a position that is visibly in ## no longer load-bearing for denial correctness — its role is to keep the stored snap fresh
## range. Both refs are optional so tests that inject a plain script can still run. ## 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_authority: Node = null
var _freshness_player: Node3D = null var _freshness_player: Node3D = null
@ -114,7 +115,9 @@ func request_select_target_id(target_id: String) -> void:
if trimmed.is_empty(): if trimmed.is_empty():
push_warning("TargetSelectionClient: refusing empty targetId; use request_clear_target()") push_warning("TargetSelectionClient: refusing empty targetId; use request_clear_target()")
return 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: func request_clear_target() -> void:
@ -122,6 +125,19 @@ func request_clear_target() -> void:
_post_select({"schemaVersion": 1}) _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`. ## 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 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 ## (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 _phase = Phase.IDLE
## Nudges the authority with the current capsule position so the server's stored ## Nudges the authority with the current capsule position so the server's stored snapshot
## snapshot is as fresh as possible when the select POST lands. Does **not** wait ## catches up. Paired with the `positionHint` on the select POST (follow-up #5), this keeps
## for the move-stream response — both requests race on separate connections — but ## subsequent validity GETs honest; the select itself no longer depends on which request
## in practice this collapses the common "Tab right after stopping" false-denial ## arrives at the server first.
## window because the authority flushes queued crumbs synchronously on submit.
func _kick_freshness_stream() -> void: func _kick_freshness_stream() -> void:
if _freshness_authority == null or _freshness_player == null: if _freshness_authority == null or _freshness_player == null:
return return

View File

@ -251,9 +251,36 @@ func test_select_post_kicks_freshness_stream_before_posting() -> void:
assert_that(transport.last_url).contains("/target/select") 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: func test_select_post_without_freshness_wiring_is_a_noop() -> void:
# Regression: tests that construct the client without `set_freshness_kick(...)` must # 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() var transport := MockHttpTransport.new()
transport.enqueue( transport.enqueue(
HTTPRequest.RESULT_SUCCESS, 200, _select_response_json(true, ALPHA_ID, "ok", 1) 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) var c := _make_client(transport)
c.request_select_target_id(ALPHA_ID) c.request_select_target_id(ALPHA_ID)
assert_that(transport.last_url).contains("/target/select") 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) assert_that(c.cached_state().get("lockedTargetId")).is_equal(ALPHA_ID)

View File

@ -72,7 +72,8 @@ Pair the client (Godot **F5**) with the server (`cd server/NeonSprawl.Server &&
- `Seq` increments by 1 - `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). - [ ] 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). - [ ] **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 ## 7. Clear

View File

@ -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`**. 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): 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. - `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.

View File

@ -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) Δ=<d>m age=<ms>` 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. | | 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) Δ=<d>m age=<ms>` 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). | | 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. | | 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. |

View File

@ -5,6 +5,8 @@ Scope: `NEO-24-e1m3-client-tab-target-lock-ui-synced-to-server` (`origin/main...
Base: `origin/main` (merge-base `37f44a494400f4fdd369077c52dfc012df73a5d5`) 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. 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 #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 #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`.

View File

@ -96,6 +96,86 @@ public class TargetingApiTests
Assert.Null(body.TargetState.LockedTargetId); 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<TargetSelectResponse>();
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<TargetSelectResponse>();
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<PositionStateResponse>("/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<PositionStateResponse>("/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] [Fact]
public async Task PostSelect_ShouldClearLock_WhenTargetIdOmitted() public async Task PostSelect_ShouldClearLock_WhenTargetIdOmitted()
{ {

View File

@ -1,4 +1,5 @@
using System.Text.Json.Serialization; using System.Text.Json.Serialization;
using NeonSprawl.Server.Game.PositionState;
namespace NeonSprawl.Server.Game.Targeting; namespace NeonSprawl.Server.Game.Targeting;
@ -40,6 +41,18 @@ public sealed class TargetSelectRequest
/// <summary>Registry key after trim + case-insensitive lookup; <c>null</c> or omitted ⇒ clear intent.</summary> /// <summary>Registry key after trim + case-insensitive lookup; <c>null</c> or omitted ⇒ clear intent.</summary>
[JsonPropertyName("targetId")] [JsonPropertyName("targetId")]
public string? TargetId { get; init; } public string? TargetId { get; init; }
/// <summary>
/// Optional client-reported current position. When present the server uses these coordinates
/// for the range check instead of the stored <c>PositionState</c> snapshot, eliminating the
/// race between <c>move-stream</c> and <c>target/select</c> POSTs (NEO-24 follow-up #5). Purely
/// advisory: it does <em>not</em> write to the position store (<c>move-stream</c> 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.
/// </summary>
[JsonPropertyName("positionHint")]
[JsonIgnore(Condition = JsonIgnoreCondition.WhenWritingNull)]
public PositionVector? PositionHint { get; init; }
} }
/// <summary>POST response for target selection (NEO-23).</summary> /// <summary>POST response for target selection (NEO-23).</summary>

View File

@ -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 (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( return Results.Json(
new TargetSelectResponse new TargetSelectResponse
{ {
@ -90,7 +100,7 @@ public static class TargetingApi
} }
var applied = locks.ApplySet(id, lookupKey); 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( return Results.Json(
new TargetSelectResponse new TargetSelectResponse
{ {