diff --git a/docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md b/docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md index b305970..34c489e 100644 --- a/docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md +++ b/docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md @@ -7,7 +7,7 @@ | **Module ID** | E5.M2 | | **Epic** | [Epic 5 — PvE Combat](../epics/epic_05_pve_combat.md) | | **Stage target** | Prototype | -| **Status** | In Progress — Slice 2 backlog [E5M2-prototype-backlog.md](../../plans/E5M2-prototype-backlog.md): **E5M2-01** [NEO-87](https://linear.app/neon-sprawl/issue/NEO-87) catalog + CI landed · **E5M2-02** [NEO-88](https://linear.app/neon-sprawl/issue/NEO-88) server load landed · **E5M2-03** [NEO-89](https://linear.app/neon-sprawl/issue/NEO-89) registry + DI landed · **E5M2-04** [NEO-90](https://linear.app/neon-sprawl/issue/NEO-90) behavior-definitions GET landed · **E5M2-05** [NEO-91](https://linear.app/neon-sprawl/issue/NEO-91) NPC instance registry + combat-target migration landed · **E5M2-06** [NEO-92](https://linear.app/neon-sprawl/issue/NEO-92) aggro rule + threat store landed · **E5M2-07** [NEO-93](https://linear.app/neon-sprawl/issue/NEO-93) NPC runtime state machine landed · **E5M2-08** [NEO-94](https://linear.app/neon-sprawl/issue/NEO-94) runtime snapshot GET landed · **E5M2-09** [NEO-95](https://linear.app/neon-sprawl/issue/NEO-95) NPC attack resolve + player combat HP landed → **E5M2-12** [NEO-98](https://linear.app/neon-sprawl/issue/NEO-98) | +| **Status** | In Progress — Slice 2 backlog [E5M2-prototype-backlog.md](../../plans/E5M2-prototype-backlog.md): **E5M2-01** [NEO-87](https://linear.app/neon-sprawl/issue/NEO-87) catalog + CI landed · **E5M2-02** [NEO-88](https://linear.app/neon-sprawl/issue/NEO-88) server load landed · **E5M2-03** [NEO-89](https://linear.app/neon-sprawl/issue/NEO-89) registry + DI landed · **E5M2-04** [NEO-90](https://linear.app/neon-sprawl/issue/NEO-90) behavior-definitions GET landed · **E5M2-05** [NEO-91](https://linear.app/neon-sprawl/issue/NEO-91) NPC instance registry + combat-target migration landed · **E5M2-06** [NEO-92](https://linear.app/neon-sprawl/issue/NEO-92) aggro rule + threat store landed · **E5M2-07** [NEO-93](https://linear.app/neon-sprawl/issue/NEO-93) NPC runtime state machine landed · **E5M2-08** [NEO-94](https://linear.app/neon-sprawl/issue/NEO-94) runtime snapshot GET landed · **E5M2-09** [NEO-95](https://linear.app/neon-sprawl/issue/NEO-95) NPC attack resolve + player combat HP landed · **E5M2-10** [NEO-96](https://linear.app/neon-sprawl/issue/NEO-96) NPC runtime telemetry hooks landed → **E5M2-12** [NEO-98](https://linear.app/neon-sprawl/issue/NEO-98) | | **Linear** | Label **`E5.M2`** · [E5M2-prototype-backlog.md](../../plans/E5M2-prototype-backlog.md) **E5M2-01** [NEO-87](https://linear.app/neon-sprawl/issue/NEO-87) · **E5M2-02** [NEO-88](https://linear.app/neon-sprawl/issue/NEO-88) · **E5M2-03** [NEO-89](https://linear.app/neon-sprawl/issue/NEO-89) · **E5M2-04** [NEO-90](https://linear.app/neon-sprawl/issue/NEO-90) · **E5M2-05** [NEO-91](https://linear.app/neon-sprawl/issue/NEO-91) → **E5M2-12** [NEO-98](https://linear.app/neon-sprawl/issue/NEO-98) | ## Purpose diff --git a/docs/plans/NEO-96-implementation-plan.md b/docs/plans/NEO-96-implementation-plan.md index a33c85c..bf091bb 100644 --- a/docs/plans/NEO-96-implementation-plan.md +++ b/docs/plans/NEO-96-implementation-plan.md @@ -52,7 +52,8 @@ ## Implementation reconciliation (shipped) - NEO-93 `(NEO-96)` stub comments replaced with full NEO-84-style hook blocks in **`NpcRuntimeOperations.AdvanceAll`**. -- **`telegraph_fired`** at aggro→telegraph and recover→telegraph entries; **`npc_state_transition`** on every state commit including holder clear → idle. +- **`telegraph_fired`** at aggro→telegraph and recover→telegraph entries; **`npc_state_transition`** on every state commit during **`AdvanceAll`** (holder clear → idle). +- Dev fixture **`ResetAllPrototypeRows`** documented as bypassing **`AdvanceOne`** hooks; optional E9.M1 anchor on reset wrapper. - **`server/README.md`**, **E5_M2** module doc, **E5M2** backlog, and alignment register updated. - No new tests (comment-only); existing NPC runtime tests unchanged. @@ -84,7 +85,9 @@ Immediately after every **`WriteState`** / **`WriteIdle`** commit: | Windup complete | `telegraph_windup` → `recover` (logical attack; no visible `attack_execute` phase) | | Defensive `AttackExecute` branch | `attack_execute` → `recover` | | Recover cooldown complete | `recover` → `telegraph_windup` | -| Holder cleared / fixture reset | any → `idle` | +| Holder cleared during **`AdvanceAll`** | any → `idle` | + +**Not instrumented in NEO-96:** dev fixture **`ResetAllPrototypeRows`** writes idle directly (bypasses **`AdvanceOne`**); optional E9.M1 anchor noted on reset wrapper. Comment block names future E9.M1 event **`npc_state_transition`**. **`TODO(E9.M1): catalog emit`** — once per committed transition. diff --git a/docs/reviews/2026-05-29-NEO-96.md b/docs/reviews/2026-05-29-NEO-96.md index 59d3b0f..17ba9e9 100644 --- a/docs/reviews/2026-05-29-NEO-96.md +++ b/docs/reviews/2026-05-29-NEO-96.md @@ -4,6 +4,8 @@ **Scope:** Branch `NEO-96-e5m2-10-npc-telemetry-hooks` vs `origin/main` — commits `dedd9a9` … `5e1e374` **Base:** `origin/main` +**Follow-up:** Code review suggestions addressed (2026-05-29). + ## Verdict **Approve with nits** @@ -18,7 +20,7 @@ NEO-96 replaces NEO-93 telemetry stubs in **`NpcRuntimeOperations.AdvanceAll`** |------|--------| | `docs/plans/NEO-96-implementation-plan.md` | **Matches** — AC checked; kickoff decisions (comments-only, engine-only, telegraph at windup start, every state commit, no manual QA) reflected in code and docs; reconciliation section accurate. | | `docs/plans/E5M2-prototype-backlog.md` (E5M2-10) | **Matches** — AC checked, landed note present; no client counterpart (NEO-97 deferred). | -| `docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md` | **Partially matches** — Related implementation slices landed line present; Summary **Status** row still jumps **E5M2-09** → **E5M2-12** without **E5M2-10 / NEO-96 landed** (same gap NEO-95 review closed for E5M2-09). | +| `docs/decomposition/modules/E5_M2_NpcAiAndBehaviorProfiles.md` | **Matches** — Related implementation slices landed line present; Summary **Status** row includes **E5M2-10 / NEO-96 landed**. | | `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E5.M2 row includes **NEO-96 landed** with README/plan links. | | `docs/decomposition/epics/epic_05_pve_combat.md` (Slice 2 telemetry) | **Matches** — hook vocabulary `telegraph_fired`, `npc_state_transition`. | | `docs/decomposition/modules/client_server_authority.md` | **N/A** — no authority change; server-only comment anchors. | @@ -30,13 +32,13 @@ None. ## Suggestions -1. **E5.M2 Summary status table** — Append **E5M2-10 / NEO-96 telemetry hooks landed** to the module Summary **Status** row (between E5M2-09 and E5M2-12), matching the pattern used when NEO-95 landed. +1. ~~**E5.M2 Summary status table** — Append **E5M2-10 / NEO-96 telemetry hooks landed** to the module Summary **Status** row (between E5M2-09 and E5M2-12), matching the pattern used when NEO-95 landed.~~ **Done.** Summary **Status** row updated. -2. **Fixture reset vs hook comments** — Plan transition table and the holder-clear hook comment cite “fixture reset,” but **`ResetAllPrototypeRows`** writes idle directly in the store (dev fixture API) without passing through **`AdvanceOne`** hook sites. Either add a comment-only anchor on the reset path when E9.M1 lands, or narrow comment/plan wording to “holder cleared during **`AdvanceAll`**” so implementers do not assume fixture reset is already instrumented. +2. ~~**Fixture reset vs hook comments** — Plan transition table and the holder-clear hook comment cite “fixture reset,” but **`ResetAllPrototypeRows`** writes idle directly in the store (dev fixture API) without passing through **`AdvanceOne`** hook sites. Either add a comment-only anchor on the reset path when E9.M1 lands, or narrow comment/plan wording to “holder cleared during **`AdvanceAll`**” so implementers do not assume fixture reset is already instrumented.~~ **Done.** Wording narrowed to “holder cleared during **`AdvanceAll`**”; reset wrapper documents bypass + optional E9.M1 anchor; plan and README updated. ## Nits -- Nit: Mid-cycle holder-clear path (inside the **`while`** loop) adds **`TryGet`** before **`WriteIdle`** — slightly exceeds “comments only” AC; **`snapshot`** from the top of the same iteration should already be valid, so the guard is redundant unless retained for future emit code that reads **`fromState`** from **`snapshot`**. +- ~~Nit: Mid-cycle holder-clear path (inside the **`while`** loop) adds **`TryGet`** before **`WriteIdle`** — slightly exceeds “comments only” AC; **`snapshot`** from the top of the same iteration should already be valid, so the guard is redundant unless retained for future emit code that reads **`fromState`** from **`snapshot`**.~~ **Done.** Redundant **`TryGet`** removed. - Nit: **`telegraph_fired`** payload comments list **`archetypeKind`** but engine scope has **`behaviorDefId`** only — fine for planned fields; E9.M1 implementer will map via behavior catalog (same as snapshot DTO projection). diff --git a/server/NeonSprawl.Server/Game/Npc/NpcRuntimeOperations.cs b/server/NeonSprawl.Server/Game/Npc/NpcRuntimeOperations.cs index c43ae3a..e138945 100644 --- a/server/NeonSprawl.Server/Game/Npc/NpcRuntimeOperations.cs +++ b/server/NeonSprawl.Server/Game/Npc/NpcRuntimeOperations.cs @@ -85,8 +85,12 @@ public static class NpcRuntimeOperations } /// Resets all prototype NPC runtime rows to idle (dev fixture). - public static void ResetAllPrototypeRows(INpcRuntimeStateStore runtimeStore) => + public static void ResetAllPrototypeRows(INpcRuntimeStateStore runtimeStore) + { + // NEO-96: dev fixture reset writes idle directly — bypasses AdvanceOne npc_state_transition hooks. + // TODO(E9.M1): optional catalog emit anchor here if fixture reset transitions must be tracked. runtimeStore.ResetAllPrototypeRows(); + } private static void AdvanceOne( string npcInstanceId, @@ -119,7 +123,7 @@ public static class NpcRuntimeOperations { WriteIdle(runtimeStore, npcInstanceId); // --- Telemetry hook site (NEO-96): future E9.M1 catalog event `npc_state_transition` --- - // TODO(E9.M1): catalog emit — holder cleared or fixture reset (any → idle). + // TODO(E9.M1): catalog emit — holder cleared during AdvanceAll (any → idle). // Planned payload fields: npcInstanceId, behaviorDefId, fromState, toState (idle), // phaseStartedUtc, optional aggroHolderPlayerId (previous holder when known). } @@ -150,14 +154,9 @@ public static class NpcRuntimeOperations { if (!TryHasAggroHolder(npcInstanceId, threatStore, out aggroHolderPlayerId)) { - if (!runtimeStore.TryGet(npcInstanceId, out snapshot)) - { - return; - } - WriteIdle(runtimeStore, npcInstanceId); // --- Telemetry hook site (NEO-96): future E9.M1 catalog event `npc_state_transition` --- - // TODO(E9.M1): catalog emit — holder cleared mid-cycle (any → idle). + // TODO(E9.M1): catalog emit — holder cleared mid-cycle during AdvanceAll (any → idle). // Planned payload fields: npcInstanceId, behaviorDefId, fromState, toState (idle), // phaseStartedUtc, optional aggroHolderPlayerId (previous holder). return; diff --git a/server/README.md b/server/README.md index d5fb166..9fb9939 100644 --- a/server/README.md +++ b/server/README.md @@ -190,7 +190,7 @@ Plan: [NEO-93 implementation plan](../../docs/plans/NEO-93-implementation-plan.m ### NPC runtime telemetry hooks (NEO-96) -Comment-only hook sites in **`NpcRuntimeOperations.AdvanceAll`** for future E9.M1 catalog events — **`telegraph_fired`** at each **`telegraph_windup`** entry (aggro→telegraph and recover→telegraph); **`npc_state_transition`** on every committed state change (including holder clear → **`idle`**). **`TODO(E9.M1)`** — no production ingest. Snapshot GET invokes **`AdvanceAll`** before read (no duplicate API-layer hooks). Plan: [NEO-96 implementation plan](../../docs/plans/NEO-96-implementation-plan.md). +Comment-only hook sites in **`NpcRuntimeOperations.AdvanceAll`** for future E9.M1 catalog events — **`telegraph_fired`** at each **`telegraph_windup`** entry (aggro→telegraph and recover→telegraph); **`npc_state_transition`** on every committed state change during **`AdvanceAll`** (including holder clear → **`idle`**). Dev fixture **`ResetAllPrototypeRows`** writes idle directly and bypasses **`AdvanceOne`** hooks (optional E9.M1 anchor noted on reset path). **`TODO(E9.M1)`** — no production ingest. Snapshot GET invokes **`AdvanceAll`** before read (no duplicate API-layer hooks). Plan: [NEO-96 implementation plan](../../docs/plans/NEO-96-implementation-plan.md). ## NPC runtime snapshot (NEO-94)