NEO-23: code review follow-up (E1.M3 docs, GET 404 test, radius bounds)
parent
7e18ecedb6
commit
17f87aa747
|
|
@ -7,7 +7,7 @@
|
|||
| **Module ID** | E1.M3 |
|
||||
| **Epic** | [Epic 1 — Core Player Runtime](../epics/epic_01_core_player_runtime.md) |
|
||||
| **Stage target** | Prototype |
|
||||
| **Status** | Planned (see [dependency register](module_dependency_register.md)) |
|
||||
| **Status** | In Progress (see [dependency register](module_dependency_register.md)) |
|
||||
| **Linear** | [NEO-23](https://linear.app/neon-sprawl/issue/NEO-23) through [NEO-27](https://linear.app/neon-sprawl/issue/NEO-27) ([backlog](../../plans/E1M3-prototype-backlog.md)); label **`E1.M3`** per [decomposition README — Linear alignment](../README.md#linear-alignment) |
|
||||
|
||||
## Purpose
|
||||
|
|
@ -100,7 +100,7 @@ flowchart LR
|
|||
|
||||
## Implementation snapshot
|
||||
|
||||
- **E1.M3 contracts (`TargetState`, `InteractableDescriptor`, `SelectionEvent`):** not implemented as named, versioned surfaces yet; register status remains **Planned** until wire/proto or agreed JSON v1 lands with a minimal consumer. **Story backlog:** [E1M3 prototype backlog](../../plans/E1M3-prototype-backlog.md).
|
||||
- **`TargetState` (prototype JSON v1 — NEO-23):** server **`GET /game/players/{id}/target`** and **`POST /game/players/{id}/target/select`** with versioned **`PlayerTargetStateResponse`** / **`TargetSelectRequest`** / **`TargetSelectResponse`**; stub registry + in-memory lock under `server/NeonSprawl.Server/Game/Targeting/`; horizontal reach + soft lock semantics — [NEO-23](../../plans/NEO-23-implementation-plan.md), [server README — Targeting](../../../server/README.md#targeting-neo-23). **Still open on later issues:** **`InteractableDescriptor`**, **`SelectionEvent`**, Godot tab-target ([NEO-24](https://linear.app/neon-sprawl/issue/NEO-24)). **Story backlog:** [E1M3 prototype backlog](../../plans/E1M3-prototype-backlog.md).
|
||||
- **Precursor under E1.M1 (NEO-9):** server **`POST /game/players/{id}/interact`** with **`InteractionRequest`** / **`InteractionResponse`**; **`HorizontalReach`** (`server/NeonSprawl.Server/Game/World/HorizontalReach.cs`); prototype registry and API under `server/NeonSprawl.Server/Game/Interaction/`; client **`interaction_request_client.gd`**, **`interaction_radius_indicators.gd`**, **`prototype_interaction_constants.gd`** — see [NEO-9](../../plans/NEO-9-implementation-plan.md) and [server README — Interaction](../../../server/README.md#interaction-neo-9).
|
||||
- **Alignment:** [Documentation and implementation alignment](documentation_and_implementation_alignment.md).
|
||||
|
||||
|
|
|
|||
|
|
@ -48,7 +48,7 @@ Rows appear when work starts; default for unlisted modules is **Planned** / not
|
|||
|--------|-----------------|----------|-------------------|
|
||||
| E1.M1 | Ready | Prototype milestone **Done** ([E1.M1](https://linear.app/neon-sprawl/project/e1m1-inputandmovementruntime-20eb28dd3db4)). Authoritative `PositionState` + **MoveCommand** over HTTP (JSON v1 snap); **in-memory** store by default, **Postgres** when configured ([NEO-8](../../plans/NEO-8-implementation-plan.md)); shared **NpgsqlDataSource** disposed on host shutdown ([NEO-13](../../plans/NEO-13-implementation-plan.md)); Godot sync + path-follow ([NEO-7](../../plans/NEO-7-implementation-plan.md), [NEO-11](../../plans/NEO-11-implementation-plan.md)); **InteractionRequest** + horizontal range ([NEO-9](../../plans/NEO-9-implementation-plan.md)). Follow-on: prediction/reconciliation, Slice 1 telemetry, Protobuf wire per [contracts.md](contracts.md). | [NEO-6](../../plans/NEO-6-implementation-plan.md), [NEO-7](../../plans/NEO-7-implementation-plan.md), [NEO-8](../../plans/NEO-8-implementation-plan.md), [NEO-9](../../plans/NEO-9-implementation-plan.md), [NEO-10](../../plans/NEO-10-implementation-plan.md), [NEO-11](../../plans/NEO-11-implementation-plan.md), [NEO-13](../../plans/NEO-13-implementation-plan.md); `server/NeonSprawl.Server/Game/PositionState/`, `Game/Interaction/`; [server README](../../../server/README.md) |
|
||||
| E1.M2 | Ready | **Slice 2 prototype slice closed:** follow + `CameraState` ([NEO-15](https://linear.app/neon-sprawl/issue/NEO-15)); zoom bands ([NEO-16](https://linear.app/neon-sprawl/issue/NEO-16)); occlusion ([NEO-17](https://linear.app/neon-sprawl/issue/NEO-17)); occluder pick-through ([NEO-20](https://linear.app/neon-sprawl/issue/NEO-20)); contracts + hardening ([NEO-18](https://linear.app/neon-sprawl/issue/NEO-18)). Client-local; no server use of camera pose. | [NEO-15](../../plans/NEO-15-implementation-plan.md), [NEO-16](../../plans/NEO-16-implementation-plan.md), [NEO-17](../../plans/NEO-17-implementation-plan.md), [NEO-18](../../plans/NEO-18-implementation-plan.md), [NEO-20](../../plans/NEO-20-implementation-plan.md); `client/scripts/isometric_follow_camera.gd`, `camera_state.gd`, `zoom_band_config.gd`, `occlusion_policy.gd`, `ground_pick.gd`; [E1_M2_IsometricCameraController.md](E1_M2_IsometricCameraController.md) |
|
||||
| E1.M3 | Planned | **No E1.M3-named contracts in code yet** (`TargetState`, `InteractableDescriptor`, `SelectionEvent`). **Precursor** for interact range + `POST …/interact` is **E1.M1** ([NEO-9](../../plans/NEO-9-implementation-plan.md)); see [E1_M3_InteractionAndTargetingLayer.md](E1_M3_InteractionAndTargetingLayer.md#boundary-e1m1-vs-e1m3). Register stays **Planned** until first E1.M3-tagged implementation merges. | Linear [NEO-23](https://linear.app/neon-sprawl/issue/NEO-23) through [NEO-27](https://linear.app/neon-sprawl/issue/NEO-27) (Todo); register unchanged until code merges. [E1_M3_InteractionAndTargetingLayer.md](E1_M3_InteractionAndTargetingLayer.md); [E1M3 prototype backlog](../../plans/E1M3-prototype-backlog.md); [NEO-9](../../plans/NEO-9-implementation-plan.md); [server README — Interaction](../../../server/README.md#interaction-neo-9) |
|
||||
| E1.M3 | In Progress | **NEO-23 landed:** JSON v1 targeting read + select (`GET`/`POST …/target`, `Game/Targeting/`, [NEO-23](../../plans/NEO-23-implementation-plan.md)) — `PlayerTargetStateResponse` / soft lock / `reasonCode` denials; wire name differs from illustrative **`TargetState`** in module doc (called out in plan + README). **`InteractableDescriptor`**, **`SelectionEvent`**, and client tab-target ([NEO-24](https://linear.app/neon-sprawl/issue/NEO-24)) still open. **Precursor (E1.M1):** [NEO-9](../../plans/NEO-9-implementation-plan.md) `POST …/interact`. | Linear [NEO-24](https://linear.app/neon-sprawl/issue/NEO-24)–[NEO-27](https://linear.app/neon-sprawl/issue/NEO-27); [E1_M3_InteractionAndTargetingLayer.md](E1_M3_InteractionAndTargetingLayer.md); [E1M3 prototype backlog](../../plans/E1M3-prototype-backlog.md); [server README — Targeting](../../../server/README.md#targeting-neo-23), [Interaction](../../../server/README.md#interaction-neo-9) |
|
||||
|
||||
---
|
||||
|
||||
|
|
|
|||
|
|
@ -14,12 +14,12 @@ Fleshed-out scope, contracts, and integration notes live in **per-module documen
|
|||
|---|---|---|---|---|---|
|
||||
| E1.M1 | InputAndMovementRuntime | None | MoveCommand, PositionState, InteractionRequest | Prototype | Ready |
|
||||
| E1.M2 | IsometricCameraController | E1.M1 | CameraState, ZoomBandConfig, OcclusionPolicy | Prototype | Ready |
|
||||
| E1.M3 | InteractionAndTargetingLayer | E1.M1 | TargetState, InteractableDescriptor, SelectionEvent | Prototype | Planned |
|
||||
| E1.M3 | InteractionAndTargetingLayer | E1.M1 | TargetState, InteractableDescriptor, SelectionEvent | Prototype | In Progress |
|
||||
| E1.M4 | AbilityInputScaffold | E1.M3, E5.M1 | AbilityCastRequest, HotbarLoadout, CooldownSnapshot | Prototype | Planned |
|
||||
|
||||
**E1.M2 note:** Prototype slice **Ready**: `client/scripts/isometric_follow_camera.gd` + child `Camera3D` on **`World/IsometricFollowCamera`** in `client/scenes/main.tscn`; per-tick **`CameraState`** (`client/scripts/camera_state.gd`); **`ZoomBandConfig`** ([NEO-16](https://linear.app/neon-sprawl/issue/NEO-16)); **`OcclusionPolicy`** ([NEO-17](https://linear.app/neon-sprawl/issue/NEO-17)); pick-through **`"occluder"`** convention ([NEO-20](https://linear.app/neon-sprawl/issue/NEO-20)); consumer contract + occluder lifecycle hardening ([NEO-18](https://linear.app/neon-sprawl/issue/NEO-18)). See [E1_M2_IsometricCameraController.md](E1_M2_IsometricCameraController.md).
|
||||
|
||||
**E1.M3 note:** Decomposition expanded in [E1_M3_InteractionAndTargetingLayer.md](E1_M3_InteractionAndTargetingLayer.md) (E1.M1 vs E1.M3 boundary, contract sketches, Slice 3 alignment). **`TargetState` / `InteractableDescriptor` / `SelectionEvent`** are still **Planned** until published wire (Protobuf or agreed JSON v1) plus a minimal consumer. **Precursor:** authoritative **`InteractionRequest`** + horizontal reach lives under **E1.M1** ([NEO-9](../../plans/NEO-9-implementation-plan.md); `Game/Interaction/`, `Game/World/HorizontalReach.cs`).
|
||||
**E1.M3 note:** Decomposition expanded in [E1_M3_InteractionAndTargetingLayer.md](E1_M3_InteractionAndTargetingLayer.md) (E1.M1 vs E1.M3 boundary, contract sketches, Slice 3 alignment). **Partial (NEO-23):** versioned JSON v1 **`PlayerTargetStateResponse`** + **`POST …/target/select`** under `server/NeonSprawl.Server/Game/Targeting/` ([NEO-23](../../plans/NEO-23-implementation-plan.md); [server README — Targeting](../../../server/README.md#targeting-neo-23)) — throwaway HTTP spike per [contracts.md](contracts.md). **`InteractableDescriptor`** / **`SelectionEvent`** and a Godot consumer for targeting remain **in progress** on later Linear issues. **Precursor (E1.M1):** **`InteractionRequest`** + horizontal reach ([NEO-9](../../plans/NEO-9-implementation-plan.md); `Game/Interaction/`, `Game/World/HorizontalReach.cs`).
|
||||
|
||||
### Epic 2 — Skills and Progression Framework
|
||||
|
||||
|
|
|
|||
|
|
@ -19,12 +19,12 @@ The branch adds the NEO-23 targeting surface under `server/NeonSprawl.Server/Gam
|
|||
| Path | Result |
|
||||
|------|--------|
|
||||
| `docs/plans/NEO-23-implementation-plan.md` (branch) | **Matches** — routes, DTO names as documented (`PlayerTargetStateResponse`, `TargetSelectRequest`/`Response`), soft lock, sequence rules, README scope, and acceptance bullets align with the code under review. |
|
||||
| `docs/decomposition/modules/E1_M3_InteractionAndTargetingLayer.md` | **Partially matches** — authority, horizontal reach policy, denial `reasonCode` pattern, and “intent vs state” story align with the implementation. The module **Implementation snapshot** still states that versioned `TargetState` wire is not in the repo; after merge, that paragraph (and the Summary **Status** if tied to the register) should reflect NEO-23’s JSON v1 spike or stay accurate with “partial / prototype only” wording. |
|
||||
| `docs/decomposition/modules/module_dependency_register.md` | **Conflicts (post-merge drift risk)** — E1.M3 row remains **Planned** with text implying no first-party targeting contracts in code. This branch implements a deliberate JSON v1 slice. Per `documentation_and_implementation_alignment.md`, the register (and alignment table) should move to **In Progress** (or similar) when this merges, unless intentionally deferred in a follow-up PR called out in the PR description. |
|
||||
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Conflicts (post-merge drift risk)** — E1.M3 snapshot row still says no E1.M3-named contracts in code and register unchanged until NEO-23 merges; updating this file (and the register) in the same merge window avoids silent doc/code disagreement. |
|
||||
| `docs/decomposition/modules/E1_M3_InteractionAndTargetingLayer.md` | **Matches** (post–follow-up) — Summary **Status**, **Implementation snapshot**, and NEO-23 `Game/Targeting/` + README pointers reflect the JSON v1 spike; later contracts remain explicitly open. |
|
||||
| `docs/decomposition/modules/module_dependency_register.md` | **Matches** (post–follow-up) — E1.M3 **In Progress**; note text covers NEO-23 partial slice vs remaining contracts. |
|
||||
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** (post–follow-up) — E1.M3 row register + snapshot + plans cell aligned with NEO-23 merge and follow-on Linear issues. |
|
||||
| `docs/decomposition/modules/contracts.md` (versioned JSON spike discipline) | **Matches** — explicit `schemaVersion`, throwaway JSON over HTTP, consistent with other prototype routes. |
|
||||
|
||||
**Register / tracking table:** After merge, **should** update `module_dependency_register.md` and `documentation_and_implementation_alignment.md` (and optionally the E1.M3 module snapshot) so status and inventory match the new `Game/Targeting/` + README section.
|
||||
**Register / tracking table:** ~~After merge, **should** update `module_dependency_register.md` and `documentation_and_implementation_alignment.md` (and optionally the E1.M3 module snapshot) so status and inventory match the new `Game/Targeting/` + README section.~~ **Done** on branch (same PR window as NEO-23).
|
||||
|
||||
## Blocking issues
|
||||
|
||||
|
|
@ -32,11 +32,11 @@ The branch adds the NEO-23 targeting surface under `server/NeonSprawl.Server/Gam
|
|||
|
||||
## Suggestions
|
||||
|
||||
1. **Doc alignment in merge train:** Update `module_dependency_register.md`, `documentation_and_implementation_alignment.md`, and the **Implementation snapshot** in `E1_M3_InteractionAndTargetingLayer.md` so E1.M3 is not described as having zero implementation once NEO-23 lands (per this repo’s own alignment policy). A follow-up doc-only PR is acceptable if called out explicitly for the merge.
|
||||
1. ~~**Doc alignment in merge train:** Update `module_dependency_register.md`, `documentation_and_implementation_alignment.md`, and the **Implementation snapshot** in `E1_M3_InteractionAndTargetingLayer.md` so E1.M3 is not described as having zero implementation once NEO-23 lands (per this repo’s own alignment policy). A follow-up doc-only PR is acceptable if called out explicitly for the merge.~~ **Done.**
|
||||
|
||||
2. **Test gap — `GET` 404:** `PostSelect_ShouldReturnNotFound_WhenPlayerUnknown` covers unknown player on POST. Adding a parallel test for `GET /game/players/{id}/target` would lock the same contract as position/interact read paths.
|
||||
2. ~~**Test gap — `GET` 404:** `PostSelect_ShouldReturnNotFound_WhenPlayerUnknown` covers unknown player on POST. Adding a parallel test for `GET /game/players/{id}/target` would lock the same contract as position/interact read paths.~~ **Done.**
|
||||
|
||||
3. **`PlayerTargetStateReaderTests` depth:** The plan’s optional table called out inclusive-radius boundary cases (similar to `HorizontalReachTests`). Current unit tests cover `null` lock and unknown registry id only; adding one boundary assertion would better match the plan’s optional coverage intent.
|
||||
3. ~~**`PlayerTargetStateReaderTests` depth:** The plan’s optional table called out inclusive-radius boundary cases (similar to `HorizontalReachTests`). Current unit tests cover `null` lock and unknown registry id only; adding one boundary assertion would better match the plan’s optional coverage intent.~~ **Done.**
|
||||
|
||||
## Nits
|
||||
|
||||
|
|
|
|||
|
|
@ -19,4 +19,24 @@ public class PlayerTargetStateReaderTests
|
|||
var snap = new PositionSnapshot(0, 0, 0, 0);
|
||||
Assert.Equal(TargetValidity.None, PlayerTargetStateReader.ComputeValidity(null, in snap));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void ComputeValidity_AtExactLockRadius_ReturnsOk()
|
||||
{
|
||||
// Alpha anchor XZ (-5, -5), lockRadius 8 → horizontal distance 8 from (3, -5) is on boundary (inclusive).
|
||||
var snap = new PositionSnapshot(3, 0, -5, 0);
|
||||
Assert.Equal(
|
||||
TargetValidity.Ok,
|
||||
PlayerTargetStateReader.ComputeValidity(PrototypeTargetRegistry.PrototypeTargetAlphaId, in snap));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void ComputeValidity_JustOutsideLockRadius_ReturnsOutOfRange()
|
||||
{
|
||||
const double eps = 1e-6;
|
||||
var snap = new PositionSnapshot(3 + eps, 0, -5, 0);
|
||||
Assert.Equal(
|
||||
TargetValidity.OutOfRange,
|
||||
PlayerTargetStateReader.ComputeValidity(PrototypeTargetRegistry.PrototypeTargetAlphaId, in snap));
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -158,6 +158,16 @@ public class TargetingApiTests
|
|||
Assert.Equal(TargetValidity.OutOfRange, body.Validity);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task GetTarget_ShouldReturnNotFound_WhenPlayerUnknown()
|
||||
{
|
||||
await using var factory = new InMemoryWebApplicationFactory();
|
||||
var client = factory.CreateClient();
|
||||
|
||||
var response = await client.GetAsync("/game/players/missing-player/target");
|
||||
Assert.Equal(HttpStatusCode.NotFound, response.StatusCode);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task PostSelect_ShouldReturnNotFound_WhenPlayerUnknown()
|
||||
{
|
||||
|
|
|
|||
Loading…
Reference in New Issue