12 KiB

Raw Blame History

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 Decision 3 for the signal change.

Post-merge UX follow-up #4 (2026-04-21): user asked for a single shared radius across all prototype targets and overlapping rings in the test scene so Tab-flipping is observable without walking. PrototypeTargetRegistry now exposes a SharedLockRadius = 6.0 constant used by both anchors; alpha moved to (-3, 0.5, -3) and beta to (3, 0, 3) so the rings overlap ~3.5 m wide centered at origin. Spawn at (-5, -5) keeps alpha in range / beta out (existing soft-lock denial QA path untouched); walking to origin flips both rings in-range and the range-aware Tab picker swaps alpha ↔ beta on every press. PlayerTargetStateReaderTests updated to the new (3, -3) alpha boundary plus a new origin-in-both test. prototype_target_constants.gd mirrors the server's SHARED_LOCK_RADIUS. Picker tightened to skip the currently-locked id — so with only one target in range and that target already locked, Tab falls back to the cycle and the server denies, keeping NEO-23's "Tab = visible denial when no swap is possible" rule. New tests: test_tab_from_locked_alpha_at_origin_swaps_to_beta_when_both_in_range, test_tab_when_only_current_lock_is_in_range_falls_back_to_cycle_for_denial. Plan Decision 7.

Post-merge UX follow-up #3 (2026-04-21): user reported "tab here does not acquire target" while standing 2.41 m from beta (well inside the 4 m ring) with Δ=0.00m age=2357ms — i.e. client and server agreed on position. Server denied with out_of_range. Root cause was the Tab cycle: with no lock held, request_tab_next() returned ORDERED_IDS[0] = prototype_target_alpha, which was 17.90 m away. The user was visibly next to beta but Tab was always trying alpha first. Fix: PrototypeTargetConstants.next_in_range_id_after(current, world) walks the cycle and returns the first anchor whose client-side horizontal distance ≤ radius; TargetSelectionClient.request_tab_next() prefers that pick when the player ref is wired and falls back to next_id_after(current) when nothing is in range (so the server still owns the denial reason). Server contract untouched. Tests: test_tab_from_no_lock_skips_out_of_range_and_picks_beta, test_tab_from_no_lock_with_nothing_in_range_falls_back_to_first_id, test_tab_without_player_ref_still_uses_plain_cycle_order.

Post-merge UX follow-up #2 (2026-04-21): user reported "when in range of beta, pressing Tab says denied" — HUD distance line confirmed prototype_target_beta: 0.58 m / 4.0 (in) but server denied with out_of_range (and prototype_target_alpha: 18.95 m — almost exactly the spawn-to-current distance, strongly suggesting the server's stored position was still near spawn). Root cause is a race between the move-stream 20 Hz sampler and the target-select POST: a Tab press right after you stop moving validates against the last sampled position, which can trail the visible capsule. Two changes:

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

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:
```
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:
```
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

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.
~~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.

12 KiB Raw Blame History Unescape Escape