From 71ae8294826541380f3432751abd59007ed3d704 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Tue, 21 Apr 2026 23:04:03 -0400 Subject: [PATCH] NEO-24: add visible target-anchor markers + HUD distance lines MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Players reported targeting "feels inconsistent" because the server PrototypeTargetRegistry anchors (alpha (-5,-5) r=8, beta (8,8) r=4) have no visible meshes — the only console visible in the scene is the NEO-9 PrototypeTerminal at origin, which is unrelated to NEO-23/24 targeting. Adds a display-only mirror of the server registry: - client/scripts/prototype_target_markers.gd spawns a colored mast + translucent flat radius ring per anchor so the lock radius is visible in-world. - client/scripts/prototype_target_constants.gd gains ANCHORS with position/radius/color, matching PrototypeTargetRegistry.cs. Marked display-only; server remains authoritative for validity. - TargetLockLabel now renders a per-anchor distance line each physics tick (": m / (in|out)") so players can see the relationship between the visible capsule and the lock radius and immediately spot client/server position drift. Also updates README, plan (Decision 4), manual QA (Section 1 boot + Section 4 locomotion), and the review file's follow-up section to describe the UX fix. --- client/README.md | 3 +- client/scenes/main.tscn | 8 +- client/scripts/main.gd | 50 ++++++-- client/scripts/prototype_target_constants.gd | 52 +++++++++ client/scripts/prototype_target_markers.gd | 109 ++++++++++++++++++ .../scripts/prototype_target_markers.gd.uid | 1 + .../target_refresh_on_locomotion_test.gd.uid | 1 + docs/manual-qa/NEO-24.md | 4 + docs/plans/NEO-24-implementation-plan.md | 7 +- docs/reviews/2026-04-21-NEO-24.md | 21 +++- 10 files changed, 233 insertions(+), 23 deletions(-) create mode 100644 client/scripts/prototype_target_markers.gd create mode 100644 client/scripts/prototype_target_markers.gd.uid create mode 100644 client/test/target_refresh_on_locomotion_test.gd.uid diff --git a/client/README.md b/client/README.md index f3d411b..d5621b4 100644 --- a/client/README.md +++ b/client/README.md @@ -96,7 +96,8 @@ The main scene includes a **prototype terminal** at the map center (same world * `scripts/target_selection_client.gd` wires the server targeting API from [NEO-23](../docs/plans/NEO-23-implementation-plan.md) to a tab-cycle input and a HUD overlay. - **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. +- **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 `: m / (in|out)` (computed client-side from the live capsule position against `prototype_target_constants.gd`), and (on denials) `Denied: ` 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: 10.3 m / 8.0 (out)`, the server's position snapshot is lagging the visible capsule, and the next `authoritative_ack` refresh will reconcile. +- **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. The script and `prototype_target_constants.gd` mirror the server's `PrototypeTargetRegistry.cs` anchors/radii; change them together in the same commit so the markers never drift from server authority. - **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`. diff --git a/client/scenes/main.tscn b/client/scenes/main.tscn index 6db5e59..d8e3903 100644 --- a/client/scenes/main.tscn +++ b/client/scenes/main.tscn @@ -5,6 +5,7 @@ [ext_resource type="Script" uid="uid://ds5fkbscljkxi" path="res://scripts/position_authority_client.gd" id="4_auth"] [ext_resource type="Script" uid="uid://bv0xprp660hib" path="res://scripts/interaction_request_client.gd" id="5_ix"] [ext_resource type="Script" uid="uid://gh6yv5p1y0vq" path="res://scripts/target_selection_client.gd" id="11_tgt_sel"] +[ext_resource type="Script" uid="uid://crd46axw4gkjn" path="res://scripts/prototype_target_markers.gd" id="12_tgt_mk"] [ext_resource type="Script" uid="uid://n3p4f3fky3nv" path="res://scripts/interaction_radius_indicators.gd" id="6_rad"] [ext_resource type="Script" uid="uid://caaj3ohikfdx2" path="res://scripts/isometric_follow_camera.gd" id="7_iso_cam"] [ext_resource type="Resource" path="res://resources/isometric_zoom_bands.tres" id="8_zoom_bands"] @@ -1070,6 +1071,9 @@ surface_material_override/0 = SubResource("Mat_physics_ramp_test") [node name="CollisionShape3D" type="CollisionShape3D" parent="World/PhysicsRampTestSteepRamp" unique_id=4201032] shape = SubResource("BoxShape3D_physics_ramp_test_steep") +[node name="PrototypeTargetMarkers" type="Node3D" parent="World" unique_id=1700010] +script = ExtResource("12_tgt_mk") + [node name="InteractionMarkers" type="Node3D" parent="World" unique_id=1700003] script = ExtResource("6_rad") @@ -1147,8 +1151,8 @@ z: —" [node name="TargetLockLabel" type="Label" parent="UICanvas" unique_id=9000004] offset_left = 8.0 offset_top = 104.0 -offset_right = 320.0 -offset_bottom = 224.0 +offset_right = 340.0 +offset_bottom = 288.0 grow_horizontal = 0 grow_vertical = 0 theme_override_colors/font_color = Color(0.86, 0.94, 1, 1) diff --git a/client/scripts/main.gd b/client/scripts/main.gd index aaf0633..d150adf 100644 --- a/client/scripts/main.gd +++ b/client/scripts/main.gd @@ -37,9 +37,14 @@ const AUTH_SNAP_LOCOMOTION_SKIP_MAX_DH: float = 0.55 ## Horizontal speed² above this (m²/s²) counts as “moving” for snap suppression (~0.35 m/s). const AUTH_SNAP_LOCOMOTION_VEL_HZ_SQ: float = 0.12 +const PrototypeTargetConstants := preload("res://scripts/prototype_target_constants.gd") + ## Bump on each rejection so older one-shot timers do not clear a newer message. var _move_reject_msg_token: int = 0 var _stream_physics_counter: int = 0 +## Cached latest `target_state_changed` payload so the HUD can re-render with live +## distance readouts on every physics tick without missing server-side fields. +var _last_target_state: Dictionary = {} ## After the first `apply_as_snap` authority update, micro-snaps may be skipped ## (boot always applies). var _allow_authority_soft_snap_skip: bool = false @@ -87,10 +92,7 @@ func _ready() -> void: # 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_ack", - Callable(_target_client, "on_authoritative_ack") - ) + _authority.connect("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") _target_client.call("request_sync_from_server") @@ -115,6 +117,7 @@ func _physics_process(_delta: float) -> void: _stream_physics_counter = 0 var p: Vector3 = _player.global_position _player_pos_label.text = "x: %.3f\ny: %.3f\nz: %.3f" % [p.x, p.y, p.z] + _render_target_lock_label(p) func _locomotion_wish_world_xz_from_camera() -> Vector3: @@ -158,24 +161,47 @@ func _on_authoritative_position(world: Vector3, apply_as_snap: bool) -> void: _player.snap_to_server(world) -## NEO-24: render the last server-acknowledged target lock to the HUD label. -## `state` carries `lockedTargetId` (String or `null`), `validity`, `sequence`, and -## (on POST responses) `selectionApplied` + optional `reasonCode`. +## NEO-24: cache the last server-acknowledged target state; the label text is +## re-rendered every physics tick in [method _render_target_lock_label] so the +## live distance readouts stay in sync with the player capsule even while +## the server state is idle. func _on_target_state_changed(state: Dictionary) -> void: + _last_target_state = state.duplicate() + if is_instance_valid(_player): + _render_target_lock_label(_player.global_position) + + +## Combines cached server state (`_last_target_state`) with per-anchor horizontal +## distance computed from the live capsule position. Distances are a **display-only** +## hint — the server remains authoritative for `validity`. If the HUD shows +## `Validity: ok` but `alpha: 10.3 m` (> 8 m radius), that means the server's +## position snapshot is lagging the visible capsule (NEO-23 `move-stream` 20 Hz + +## one-way latency); the refresh GET on `authoritative_ack` will reconcile within +## ~250 ms cooldown the next time a `move-stream` lands. +func _render_target_lock_label(world: Vector3) -> void: if not is_instance_valid(_target_lock_label): return + var state: Dictionary = _last_target_state var locked_variant: Variant = state.get("lockedTargetId", null) var locked_text: String = "—" if locked_variant is String and not (locked_variant as String).is_empty(): locked_text = locked_variant as String var validity: String = str(state.get("validity", "none")) var sequence: int = int(state.get("sequence", 0)) - var suffix: String = "" + var lines: PackedStringArray = [ + "Target: %s" % locked_text, + "Validity: %s" % validity, + "Seq: %d" % sequence, + ] + for id_variant: Variant in PrototypeTargetConstants.ordered_ids(): + var tid: String = id_variant as String + var dist: float = PrototypeTargetConstants.horizontal_distance_to(tid, world) + var radius: float = PrototypeTargetConstants.anchor_radius(tid) + var marker: String = "in" if dist <= radius else "out" + lines.append("%s: %.2f m / %.1f (%s)" % [tid, dist, radius, marker]) if state.has("reasonCode"): - suffix = "\nDenied: %s" % str(state["reasonCode"]) - _target_lock_label.text = ( - "Target: %s\nValidity: %s\nSeq: %d%s" % [locked_text, validity, sequence, suffix] - ) + lines.append("Denied: %s" % str(state["reasonCode"])) + _target_lock_label.text = "\n".join(lines) func _on_move_rejected(reason_code: String) -> void: diff --git a/client/scripts/prototype_target_constants.gd b/client/scripts/prototype_target_constants.gd index e47e477..803be22 100644 --- a/client/scripts/prototype_target_constants.gd +++ b/client/scripts/prototype_target_constants.gd @@ -3,11 +3,63 @@ extends Object ## NEO-24: prototype combat target ids in **tab cycle order** — must match ## `server/NeonSprawl.Server/Game/Targeting/PrototypeTargetRegistry.cs` ## (`PrototypeTargetAlphaId`, `PrototypeTargetBetaId`; ascending id order). +## Anchors + radii are a **display-only** mirror of the C# registry so the scene markers +## (`prototype_target_markers.gd`) and HUD distance readouts match server-authoritative +## range decisions. If the server registry moves, update this file in the same commit — +## the two must stay in lock-step to avoid confusing "ok at 10 m / denied at 5 m" bugs. ## Static accessors only (no `class_name`; headless / CI loads before editor import — see client ## README Godot CLI section). const ORDERED_IDS: Array[String] = ["prototype_target_alpha", "prototype_target_beta"] +## Keys match [constant ORDERED_IDS]; values mirror `PrototypeTargetRegistry.cs` anchors +## (world space) and horizontal lock radii (XZ meters). +const ANCHORS: Dictionary = { + "prototype_target_alpha": + {"position": Vector3(-5.0, 0.5, -5.0), "radius": 8.0, "color": Color(1.0, 0.55, 0.2, 1.0)}, + "prototype_target_beta": + {"position": Vector3(8.0, 0.0, 8.0), "radius": 4.0, "color": Color(0.7, 0.45, 1.0, 1.0)}, +} + + +## Returns the world anchor position for [param target_id], or [code]Vector3.ZERO[/code] +## if the id is not in [constant ANCHORS]. +static func anchor_position(target_id: String) -> Vector3: + var data: Variant = ANCHORS.get(target_id, null) + if not data is Dictionary: + return Vector3.ZERO + var pos: Variant = (data as Dictionary).get("position", Vector3.ZERO) + return pos if pos is Vector3 else Vector3.ZERO + + +## Horizontal (XZ) lock radius for [param target_id], or [code]0.0[/code] if unknown. +static func anchor_radius(target_id: String) -> float: + var data: Variant = ANCHORS.get(target_id, null) + if not data is Dictionary: + return 0.0 + return float((data as Dictionary).get("radius", 0.0)) + + +## Marker tint for [param target_id]; falls back to white if unknown. +static func anchor_color(target_id: String) -> Color: + var data: Variant = ANCHORS.get(target_id, null) + if not data is Dictionary: + return Color.WHITE + var c: Variant = (data as Dictionary).get("color", Color.WHITE) + return c if c is Color else Color.WHITE + + +## Horizontal (XZ) distance from [param world] to the anchor of [param target_id], or +## [code]INF[/code] if the id is unknown. Display-only helper — **not** authoritative for +## range decisions (server owns that; see `PlayerTargetStateReader.ComputeValidity`). +static func horizontal_distance_to(target_id: String, world: Vector3) -> float: + if not ANCHORS.has(target_id): + return INF + var anchor: Vector3 = anchor_position(target_id) + var dx: float = world.x - anchor.x + var dz: float = world.z - anchor.z + return sqrt(dx * dx + dz * dz) + static func ordered_ids() -> Array[String]: return ORDERED_IDS.duplicate() diff --git a/client/scripts/prototype_target_markers.gd b/client/scripts/prototype_target_markers.gd new file mode 100644 index 0000000..2ed84cb --- /dev/null +++ b/client/scripts/prototype_target_markers.gd @@ -0,0 +1,109 @@ +extends Node3D + +## NEO-24 UX: the targeting registry anchors (`prototype_target_alpha`, +## `prototype_target_beta`) are invisible in the world, so "Tab says ok but I feel +## far away" is impossible to reason about without debug markers. This node spawns +## one small mast + a flat radius ring per id in +## `PrototypeTargetConstants.ANCHORS`, so the player can see exactly where each +## anchor sits and how big its lock radius is. +## +## Display-only; no game logic hangs off these nodes. If the server registry moves, +## update `prototype_target_constants.gd` — markers respawn on next scene load. + +const PrototypeTargetConstants := preload("res://scripts/prototype_target_constants.gd") + +## Visual tuning — keep markers unmistakable but not dominant. +const _MAST_HEIGHT: float = 2.5 +const _MAST_RADIUS: float = 0.18 +const _RING_RADIUS_SEGMENTS: int = 72 +const _RING_Y: float = 0.05 +const _RING_THICKNESS: float = 0.06 + + +func _ready() -> void: + for id_variant: Variant in PrototypeTargetConstants.ANCHORS.keys(): + if not id_variant is String: + continue + _spawn_marker(id_variant as String) + + +func _spawn_marker(target_id: String) -> void: + var anchor: Vector3 = PrototypeTargetConstants.anchor_position(target_id) + var radius: float = PrototypeTargetConstants.anchor_radius(target_id) + var tint: Color = PrototypeTargetConstants.anchor_color(target_id) + if radius <= 0.0: + return + + var group := Node3D.new() + group.name = "Marker_%s" % target_id + group.position = anchor + add_child(group) + + var mast := MeshInstance3D.new() + mast.name = "Mast" + var mast_mesh := CylinderMesh.new() + mast_mesh.top_radius = _MAST_RADIUS + mast_mesh.bottom_radius = _MAST_RADIUS + mast_mesh.height = _MAST_HEIGHT + mast.mesh = mast_mesh + # CylinderMesh is Y-centered at its origin; raise so the base sits on the anchor Y. + mast.position = Vector3(0.0, _MAST_HEIGHT * 0.5, 0.0) + mast.material_override = _make_unshaded_material(tint, 1.0) + group.add_child(mast) + + var ring: MeshInstance3D = _make_radius_ring(radius, tint) + ring.name = "RadiusRing" + ring.position = Vector3(0.0, _RING_Y - anchor.y, 0.0) + group.add_child(ring) + + +func _make_unshaded_material(tint: Color, alpha: float) -> StandardMaterial3D: + var m := StandardMaterial3D.new() + m.shading_mode = BaseMaterial3D.SHADING_MODE_UNSHADED + var c := tint + c.a = alpha + m.albedo_color = c + if alpha < 0.999: + m.transparency = BaseMaterial3D.TRANSPARENCY_ALPHA + return m + + +## Builds a thin flat ring (annulus) by triangulating the band between `radius - t/2` +## and `radius + t/2`. `ImmediateMesh` with `SurfaceTool` would also work, but an +## `ArrayMesh` keeps this a static resource with no per-frame cost. +func _make_radius_ring(radius: float, tint: Color) -> MeshInstance3D: + var mi := MeshInstance3D.new() + var arr_mesh := ArrayMesh.new() + var verts := PackedVector3Array() + var normals := PackedVector3Array() + var indices := PackedInt32Array() + + var inner: float = max(0.01, radius - _RING_THICKNESS * 0.5) + var outer: float = radius + _RING_THICKNESS * 0.5 + var up := Vector3.UP + for i in range(_RING_RADIUS_SEGMENTS): + var a: float = TAU * float(i) / float(_RING_RADIUS_SEGMENTS) + var c: float = cos(a) + var s: float = sin(a) + verts.append(Vector3(inner * c, 0.0, inner * s)) + verts.append(Vector3(outer * c, 0.0, outer * s)) + normals.append(up) + normals.append(up) + + for i in range(_RING_RADIUS_SEGMENTS): + var i_next: int = (i + 1) % _RING_RADIUS_SEGMENTS + var a0: int = i * 2 + var a1: int = i * 2 + 1 + var b0: int = i_next * 2 + var b1: int = i_next * 2 + 1 + indices.append_array([a0, a1, b1, a0, b1, b0]) + + var surface: Array = [] + surface.resize(Mesh.ARRAY_MAX) + surface[Mesh.ARRAY_VERTEX] = verts + surface[Mesh.ARRAY_NORMAL] = normals + surface[Mesh.ARRAY_INDEX] = indices + arr_mesh.add_surface_from_arrays(Mesh.PRIMITIVE_TRIANGLES, surface) + mi.mesh = arr_mesh + mi.material_override = _make_unshaded_material(tint, 0.45) + return mi diff --git a/client/scripts/prototype_target_markers.gd.uid b/client/scripts/prototype_target_markers.gd.uid new file mode 100644 index 0000000..ac67ce0 --- /dev/null +++ b/client/scripts/prototype_target_markers.gd.uid @@ -0,0 +1 @@ +uid://crd46axw4gkjn diff --git a/client/test/target_refresh_on_locomotion_test.gd.uid b/client/test/target_refresh_on_locomotion_test.gd.uid new file mode 100644 index 0000000..50a0c94 --- /dev/null +++ b/client/test/target_refresh_on_locomotion_test.gd.uid @@ -0,0 +1 @@ +uid://b3jgb0gf52s24 diff --git a/docs/manual-qa/NEO-24.md b/docs/manual-qa/NEO-24.md index 137bbbf..0422e92 100644 --- a/docs/manual-qa/NEO-24.md +++ b/docs/manual-qa/NEO-24.md @@ -23,6 +23,9 @@ Pair the client (Godot **F5**) with the server (`cd server/NeonSprawl.Server && - `Target: —` - `Validity: none` - `Seq: 0` + - `prototype_target_alpha: 0.00 m / 8.0 (in)` (spawn `(-5,-5)` is exactly on alpha's XZ anchor) + - `prototype_target_beta: 18.38 m / 4.0 (out)` +- [ ] **Visible anchors** are rendered in-world: an **orange** mast + flat ring at `(-5, 0.5, -5)` (alpha, 8 m radius) and a **violet** mast + ring at `(8, 0, 8)` (beta, 4 m radius). Both rings lie flat on the floor; the player capsule should sit inside the alpha ring at spawn. - [ ] Godot **Output** has no `TargetSelectionClient:` warnings. Server console shows a single `GET /game/players/dev-local-1/target` from the boot sync. ## 2. Tab cycle — happy path @@ -50,6 +53,7 @@ Pair the client (Godot **F5**) with the server (`cd server/NeonSprawl.Server && - [ ] 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). +- [ ] **Distance lines match the ring**: while moving, the `prototype_target_alpha: m / 8.0 (in|out)` line flips `in`/`out` as you cross the visible orange ring edge. Any time the HUD shows `Validity: ok` but the distance line reads `(out)`, the server position snapshot is lagging the visible capsule — the next `authoritative_ack` refresh should reconcile within 250 ms. ## 5. Swap to beta when in range diff --git a/docs/plans/NEO-24-implementation-plan.md b/docs/plans/NEO-24-implementation-plan.md index 1dbeea9..6d26140 100644 --- a/docs/plans/NEO-24-implementation-plan.md +++ b/docs/plans/NEO-24-implementation-plan.md @@ -75,8 +75,10 @@ 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; connect `authoritative_ack` → `on_authoritative_ack` (see Decision 2). | +| `client/scenes/main.tscn` | Add `TargetSelectionClient` node + **`TargetLockLabel`** (or reuse a debug panel) bound to scripts. Post-merge follow-up (Decision 4) also adds `World/PrototypeTargetMarkers` (colored mast + translucent radius ring per anchor). | +| `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). Re-render `TargetLockLabel` each physics tick with live per-anchor distance lines from `PrototypeTargetConstants` (Decision 4). | +| `client/scripts/prototype_target_constants.gd` | Display-only mirror of `PrototypeTargetRegistry.cs` anchors + radii used by the markers and HUD distance readouts (Decision 4). | +| `client/scripts/prototype_target_markers.gd` | Spawns a visible mast + flat ring at every `PrototypeTargetConstants.ANCHORS` entry so players can reason about range visually (Decision 4). | | `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). | @@ -103,3 +105,4 @@ No new **C#** tests (client-only story). **Bruno:** NEO-23 targeting requests al | 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_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. | +| 4 | **Post-merge UX: anchor visibility** | Users reported "targeting feels inconsistent" because the `PrototypeTargetRegistry` anchors (`alpha (-5,-5)` r=8, `beta (8,8)` r=4) have no visible meshes — the only visible console in the scene is the NEO-9 `PrototypeTerminal` at origin, which is unrelated to targeting. Added `client/scripts/prototype_target_markers.gd` + `PrototypeTargetMarkers` node in `main.tscn` (colored mast + translucent flat ring per anchor) and extended `TargetLockLabel` with per-anchor `: m / (in|out)` distance lines. Anchors/radii are mirrored in `prototype_target_constants.gd` as **display-only** data — server remains authoritative. If `PrototypeTargetRegistry.cs` moves, update both files in the same commit. | diff --git a/docs/reviews/2026-04-21-NEO-24.md b/docs/reviews/2026-04-21-NEO-24.md index 45fbb94..d402088 100644 --- a/docs/reviews/2026-04-21-NEO-24.md +++ b/docs/reviews/2026-04-21-NEO-24.md @@ -5,6 +5,12 @@ Scope: `NEO-24-e1m3-client-tab-target-lock-ui-synced-to-server` (`origin/main... 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`: `: m / (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. @@ -13,16 +19,16 @@ Follow-up: blocking issue + suggestions below are **done** (strikethrough + **Do 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. +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` — **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/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` — **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. +- `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 @@ -69,5 +75,8 @@ 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. +- 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.