From 435a32b3b33af224d2eb6b4927665eddb2f58fc1 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Tue, 21 Apr 2026 22:35:41 -0400 Subject: [PATCH] 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.