92 lines
11 KiB
Markdown
92 lines
11 KiB
Markdown
# 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.
|
||
|
||
**Post-merge UX follow-up #3 (2026-04-21):** user reported "tab here does not acquire target" while standing 2.41 m from beta (well inside the 4 m ring) with `Δ=0.00m age=2357ms` — i.e. client and server agreed on position. Server denied with `out_of_range`. Root cause was the Tab cycle: with no lock held, `request_tab_next()` returned `ORDERED_IDS[0]` = `prototype_target_alpha`, which was 17.90 m away. The user was visibly next to `beta` but Tab was always trying `alpha` first. Fix: `PrototypeTargetConstants.next_in_range_id_after(current, world)` walks the cycle and returns the first anchor whose client-side horizontal distance ≤ radius; `TargetSelectionClient.request_tab_next()` prefers that pick when the player ref is wired and falls back to `next_id_after(current)` when nothing is in range (so the server still owns the denial reason). Server contract untouched. Tests: `test_tab_from_no_lock_skips_out_of_range_and_picks_beta`, `test_tab_from_no_lock_with_nothing_in_range_falls_back_to_first_id`, `test_tab_without_player_ref_still_uses_plain_cycle_order`.
|
||
|
||
**Post-merge UX follow-up #2 (2026-04-21):** user reported "when in range of beta, pressing Tab says denied" — HUD distance line confirmed `prototype_target_beta: 0.58 m / 4.0 (in)` but server denied with `out_of_range` (and `prototype_target_alpha: 18.95 m` — almost exactly the spawn-to-current distance, strongly suggesting the server's stored position was still near spawn). Root cause is a race between the `move-stream` 20 Hz sampler and the target-select POST: a Tab press right after you stop moving validates against the last sampled position, which can trail the visible capsule. Two changes:
|
||
|
||
- `PlayerPositionLabel` now shows the server-acknowledged position + horizontal Δ + ack age (cached from `PositionAuthorityClient.authoritative_ack` via a new `_on_authoritative_ack_for_hud` handler). Makes the divergence directly visible in future repro.
|
||
- `TargetSelectionClient` exposes `set_freshness_kick(authority, player)`; `main.gd` wires it so every `POST /target/select` is immediately preceded by a `submit_stream_targets([player.global_position])` nudge on the authority. The select POST does not wait for the stream response, but the kick shrinks the race window enough in practice that "stop, Tab, denied" stops firing. Tests cover both the kick happening (`test_select_post_kicks_freshness_stream_before_posting`) and no regression when the wiring is absent (`test_select_post_without_freshness_wiring_is_a_noop`).
|
||
|
||
Server contract (NEO-23) untouched; this is pure client-side.
|
||
|
||
**Post-merge UX follow-up #1 (2026-04-21):** user reported "targeting result feels very inconsistent and doesn't seem to matter whether i'm within range of the console or not." Root cause was not a correctness bug — the `PrototypeTargetRegistry` anchors (`alpha (-5,-5)` r=8, `beta (8,8)` r=4) have **no visible scene meshes**, so players were reasoning about targeting against the `PrototypeTerminal` box at origin (which belongs to NEO-9 interaction, not NEO-23/24 targeting). Added:
|
||
|
||
- `client/scripts/prototype_target_markers.gd` — spawns an unshaded colored mast + flat radius ring per anchor (mirrors the server registry via `prototype_target_constants.gd`).
|
||
- Per-anchor distance readout on `UICanvas/TargetLockLabel`: `<id>: <d> m / <radius> (in|out)` computed client-side from the live capsule position. Labeled explicitly as a lag diagnostic — if `Validity: ok` shows while the distance line reads `(out)`, the server's position snapshot is trailing the visible capsule and the next `authoritative_ack` refresh reconciles it.
|
||
- Updated `client/README.md` and `docs/manual-qa/NEO-24.md` (Section 1 boot state and Section 4 locomotion check) to describe the markers and distance lines.
|
||
|
||
## Verdict
|
||
|
||
~~Request changes~~ — **Addressed.** Blocking issue fixed; integration test added; doc-alignment row updated.
|
||
|
||
## 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.
|
||
|
||
Follow-up verification confirms the earlier review findings were addressed. The movement-triggered soft-lock refresh is now reachable during normal locomotion via `PositionAuthorityClient.authoritative_ack`, the new integration test covers the real authority-to-target wiring, and the E1.M3 implementation-alignment row was updated to reflect NEO-24 landing.
|
||
|
||
## Documentation checked
|
||
|
||
- `docs/plans/NEO-24-implementation-plan.md` — **matches**. Decision 2 now uses `PositionAuthorityClient.authoritative_ack`, and Decision 3 records why the new signal was added.
|
||
- `docs/manual-qa/NEO-24.md` — **matches**. Section 4’s locomotion-driven refresh path is now implemented by the new authority ack signal and target refresh wiring.
|
||
- `docs/decomposition/modules/E1_M3_InteractionAndTargetingLayer.md` — **matches** for server-authoritative target state and denial reconciliation.
|
||
- `docs/decomposition/modules/client_server_authority.md` — **matches** for client-as-intent / server-as-truth behavior.
|
||
- `docs/decomposition/modules/module_dependency_register.md` — **matches**. `E1.M3` remains `In Progress`.
|
||
- `docs/decomposition/modules/documentation_and_implementation_alignment.md` — **matches**. The `E1.M3` tracking row now reflects NEO-24 landed (HUD + `authoritative_ack` refresh) and points back to the plan/manual QA docs.
|
||
|
||
## Blocking issues
|
||
|
||
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/position_authority_client_test.gd` from `client/` — suite passed, including the new `authoritative_ack` assertions.
|
||
- Ran `godot --headless --path . -s res://addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode -a res://test/target_selection_client_test.gd` from `client/` — suite passed, including the renamed ack-based refresh tests.
|
||
- Ran `godot --headless --path . -s res://addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode -a res://test/target_refresh_on_locomotion_test.gd` from `client/` — integration suite passed, confirming `move-stream` 200 triggers exactly one throttled target refresh while locked and none while unlocked.
|
||
- Read diagnostics for the touched client/scripts, client/test, plan, and review files — no linter errors found.
|
||
- I did not run the full manual client/server checklist; `docs/manual-qa/NEO-24.md` section 4 remains the highest-value end-to-end sanity check if you want one more runtime pass before merge.
|