diff --git a/docs/plans/NEO-26-implementation-plan.md b/docs/plans/NEO-26-implementation-plan.md index a543a03..03609ce 100644 --- a/docs/plans/NEO-26-implementation-plan.md +++ b/docs/plans/NEO-26-implementation-plan.md @@ -15,13 +15,13 @@ ## Goal, scope, and out-of-scope -**Goal:** Every **material change** to the client’s **server-acknowledged combat target lock id** (`lockedTargetId` in the cached `TargetState` mirror) is observable as a **`SelectionEvent`** carrying **previous** id, **next** id, and **cause** (`tab`, `clear`, `server_correction`, …). Ship **one** built-in consumer (debug overlay, `print`, or thin HUD) so the bus is exercised in dev runs. +**Goal:** Every **material change** to the client’s **server-acknowledged combat target lock id** (`lockedTargetId` in the cached `TargetState` mirror) is observable as a **`SelectionEvent`** carrying **previous** id, **next** id, and **cause** (`tab`, `clear`, `server_correction`, …). Ship **one** built-in consumer: **dev-only `print`** behind an `@export` flag (planning choice 2026-04-25) so the bus is exercised without new HUD nodes. **In scope** - Minimal **GDScript** event shape (Dictionary or small typed helper) aligned with the illustrative table in E1.M3: `previous` (Variant `String` or `null`), `next` (same), `cause` (`String` enum-like literals). - **Cause attribution** at the authoritative update boundary inside the existing selection client (see Technical approach) so we do not guess causes from state diffs alone. -- **Consumer:** append-only **ring buffer** of last **N** events (e.g. 8–16) shown in UI **or** dev-only `print` gated by a `@export var` — pick one primary path in implementation; document the **subscription point** for E1.M4 (`signal` name + emitter node path pattern). +- **Consumer:** optional **`print`** when `@export var log_selection_events` (name TBD) is true on **`TargetSelectionClient`** (or tiny child node); document the **subscription point** for E1.M4 (`signal` name + emitter node path pattern). - **Tests:** tab vs clear vs movement-driven GET produce **distinct** `cause` strings when they each produce a lock-id transition (or documented no-op when id unchanged). **Out of scope (per Linear / backlog)** @@ -44,32 +44,29 @@ 2. **Intent / phase tags:** Before starting HTTP, set a private enum or String tag, e.g. `_pending_selection_cause`, cleared after handling the response: - `request_tab_next` → `tab` (even though it calls `request_select_target_id` internally, set cause at the tab entrypoint before POST). - `request_clear_target` → `clear`. - - `request_select_target_id` when **not** entered from `request_tab_next` → `direct_select` (reserved for tests / future UI; document). + - `request_select_target_id` when **not** entered from `request_tab_next` → use cause **`tab`** (same string as Tab path — planning choice 2026-04-25: no separate `direct_select`). - `request_sync_from_server` when invoked from **`on_authoritative_ack`** → mark refresh as **`server_correction`** for the next GET completion. - - Boot / one-shot sync from `main.gd` → use **`server_correction`** or **`initial_sync`** consistently; prefer **`initial_sync`** only if we want tests to ignore boot noise — **Decision (default):** use **`server_correction`** for any GET that is not tied to a tab/clear POST, including first sync, **only emit** if normalized `lockedTargetId` differs from prior snapshot (avoids spam on identical boot GET). + - Boot / one-shot sync from `main.gd` → **Decision (2026-04-25):** first and later **GET** completions use cause **`server_correction`**; **only emit** if normalized `lockedTargetId` differs from prior snapshot (avoids spam on identical boot GET). 3. **Emit rule:** After `_state` is updated in `_update_state_from_get` / `_update_state_from_post`, compare **previous** normalized `lockedTargetId` to **new**; if equal, **no** `SelectionEvent` (validity-only changes stay on `target_state_changed` only unless product later expands the contract). 4. **Signal:** Add e.g. **`selection_event(event: Dictionary)`** with keys `previous`, `next`, `cause` (and optional `sequence` int passthrough for debugging). Keep **`target_state_changed`** unchanged for existing HUD. -5. **Consumer:** New small script **`selection_event_debug_panel.gd`** (type `Control` or `Node` driving a `RichTextLabel`) under the HUD canvas, connected in **`main.gd`** only via `connect` — keeps [godot-client-script-organization](../../.cursor/rules/godot-client-script-organization.md) satisfied. +5. **Consumer:** No new scene nodes — **`TargetSelectionClient`** (or dedicated debug child) implements optional **`print`** when export flag enabled; ring buffer optional in-memory for test assertions only. -6. **Docs:** Update **`client/README.md`** “Target lock + tab cycle” area with a short **“SelectionEvent (NEO-26)”** subsection: signal name, payload keys, E1.M4 subscription note. +6. **Docs:** Update **`client/README.md`** “Target lock + tab cycle” area with a short **“SelectionEvent (NEO-26)”** subsection: signal name, payload keys, print flag, E1.M4 subscription note. ## Files to add | Path | Purpose | |------|---------| -| `client/scripts/selection_event_debug_panel.gd` | Thin HUD / debug list: listens for `selection_event`, shows last N events (timestamp optional). | | `client/test/selection_event_client_test.gd` | GdUnit4: assert causes for tab POST, clear POST, and GET-with-mock-body changing lock id (`TargetSelectionTestDouble` pattern). | ## Files to modify | Path | Rationale | |------|-----------| -| `client/scripts/target_selection_client.gd` | Track pending cause through GET/POST; compare old/new `lockedTargetId`; emit `selection_event`; document next to `target_state_changed`. | -| `client/scenes/main.tscn` | Add debug panel node (sibling under `UICanvas` or next to existing target HUD) and script attachment. | -| `client/scripts/main.gd` | Connect `TargetSelectionClient.selection_event` → debug panel; minimal wiring only. | +| `client/scripts/target_selection_client.gd` | Track pending cause through GET/POST; compare old/new `lockedTargetId`; emit `selection_event`; optional `print` behind `@export`; document next to `target_state_changed`. | | `client/README.md` | Document subscription point + payload for E1.M4. | | `docs/decomposition/modules/E1_M3_InteractionAndTargetingLayer.md` | **Implementation snapshot** bullet: replace “Still open — `SelectionEvent`” with pointer to NEO-26 + signal name (keeps decomposition aligned with shipped code). | @@ -84,7 +81,7 @@ ## Open questions / risks -- **Validity-only updates:** v1 emits only on **`lockedTargetId`** change; if design later needs `validity` transitions as events, extend shape with `kind: "lock_id" \| "validity"` in a follow-up. +- **Validity-only updates:** If v1 stays **lock-id-only** (recommended), extend later with `kind` or dual fields if telemetry needs **`validity`** transitions on the same bus. - **Linear blocked-by:** Relations may still show NEO-23/NEO-24 until closed; implementation assumes their client/server surfaces on `main` are present. ## Decisions @@ -92,5 +89,8 @@ | Topic | Decision | |--------|----------| | D1 — Emitter location | **`TargetSelectionClient`** owns `selection_event` (intent-aware). | -| D2 — Boot GET | Same emit rules as other GETs; use cause **`server_correction`** for GET path, **suppress** if id unchanged from empty initial snapshot. | -| D3 — `direct_select` | Expose cause for non-tab `request_select_target_id` callers (tests); not required for acceptance but avoids mis-labeling as `tab`. | +| D2 — Boot GET | Use cause **`server_correction`** for all GET completions (including first sync); **suppress** event if normalized `lockedTargetId` unchanged vs prior snapshot. | +| D3 — Non-tab `request_select_target_id` | Use cause **`tab`** (same literal as Tab path; no `direct_select`). | +| D4 — Consumer | **Print only** behind `@export` flag on emitter; no HUD panel in NEO-26. | +| D5 — Emit rule (v1) | **Lock id only:** emit `SelectionEvent` only when normalized **`lockedTargetId`** changes; **`validity`** / sequence updates without id change → no event (HUD keeps using `target_state_changed`). *Rationale (2026-04-25):* matches backlog “previous / next” id story, smallest payload, clear E1.M4 hook for cast target; avoids conflating soft-lock range telemetry with selection changes. Override if you want validity on this signal too. | +| D6 — Interactables | **Out of scope:** target lock only for NEO-26; interactable selection events belong with NEO-25 follow-up or a dedicated thin ticket. *Rationale:* E1M3-04 backlog is combat-target selection; folding interactables now overlaps catalog projection and slips scope. |