83 lines
8.1 KiB
Markdown
83 lines
8.1 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 (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.
|