NEO-26: address code review — tracking docs, sequence tests, review strikethrough
Align E1.M3 rows with landed SelectionEvent (NEO-26) vs follow-on scope. Assert selection_event sequence in GdUnit. Mark review suggestions/nits done.pull/51/head
parent
5127fe59a7
commit
1882476fd3
|
|
@ -84,6 +84,7 @@ func test_tab_lock_change_emits_tab() -> void:
|
|||
assert_that(last.get("cause", "")).is_equal("tab")
|
||||
assert_that(last.get("previous")).is_null()
|
||||
assert_that(last.get("next")).is_equal(ALPHA_ID)
|
||||
assert_that(int(last.get("sequence", -1))).is_equal(1)
|
||||
|
||||
|
||||
func test_clear_lock_change_emits_clear() -> void:
|
||||
|
|
@ -100,9 +101,11 @@ func test_clear_lock_change_emits_clear() -> void:
|
|||
c.request_tab_next()
|
||||
c.request_clear_target()
|
||||
assert_that(events.size()).is_equal(2)
|
||||
assert_that(int((events[0] as Dictionary).get("sequence", -1))).is_equal(1)
|
||||
assert_that((events[1] as Dictionary).get("cause", "")).is_equal("clear")
|
||||
assert_that((events[1] as Dictionary).get("previous")).is_equal(ALPHA_ID)
|
||||
assert_that((events[1] as Dictionary).get("next")).is_null()
|
||||
assert_that(int((events[1] as Dictionary).get("sequence", -1))).is_equal(2)
|
||||
|
||||
|
||||
func test_get_lock_id_change_emits_server_correction() -> void:
|
||||
|
|
@ -123,6 +126,7 @@ func test_get_lock_id_change_emits_server_correction() -> void:
|
|||
assert_that(last.get("cause", "")).is_equal("server_correction")
|
||||
assert_that(last.get("previous")).is_equal(ALPHA_ID)
|
||||
assert_that(last.get("next")).is_equal(BETA_ID)
|
||||
assert_that(int(last.get("sequence", -1))).is_equal(2)
|
||||
|
||||
|
||||
func test_get_validity_only_change_emits_nothing() -> void:
|
||||
|
|
@ -140,6 +144,7 @@ func test_get_validity_only_change_emits_nothing() -> void:
|
|||
c.request_sync_from_server()
|
||||
assert_that(events.size()).is_equal(1)
|
||||
assert_that((events[0] as Dictionary).get("cause", "")).is_equal("tab")
|
||||
assert_that(int((events[0] as Dictionary).get("sequence", -1))).is_equal(1)
|
||||
|
||||
|
||||
func test_denial_without_lock_id_change_emits_nothing() -> void:
|
||||
|
|
|
|||
|
|
@ -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 | 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). **NEO-24 landed:** client tab-target + lock HUD synced to server ([NEO-24](../../plans/NEO-24-implementation-plan.md)) — `TargetSelectionClient` ([`client/scripts/target_selection_client.gd`](../../../client/scripts/target_selection_client.gd)), Tab / Esc bindings, `TargetLockLabel` HUD, hybrid movement-triggered refresh via new `PositionAuthorityClient.authoritative_ack` signal (fires on every server-confirmed position, boot + `move-stream` 200) with a 250 ms cooldown; manual QA in [`docs/manual-qa/NEO-24.md`](../../manual-qa/NEO-24.md). **NEO-25 landed:** versioned **`GET /game/world/interactables`** ([NEO-25](../../plans/NEO-25-implementation-plan.md)) — `InteractablesListResponse` / `InteractablesWorldApi` in `server/NeonSprawl.Server/Game/Interaction/`; Godot `interactables_catalog_client.gd` + `interactable_world_builder.gd` (fetch-driven props + glow); [server README — Interactable descriptors (NEO-25)](../../../server/README.md#interactable-descriptors-neo-25). **Still open:** **`SelectionEvent`** and richer **`InteractableDescriptor`** consumers beyond list projection + existing `POST …/interact` (see [E1M3 prototype backlog](../../plans/E1M3-prototype-backlog.md)). **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), [Interactable descriptors (NEO-25)](../../../server/README.md#interactable-descriptors-neo-25), [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). **NEO-24 landed:** client tab-target + lock HUD synced to server ([NEO-24](../../plans/NEO-24-implementation-plan.md)) — `TargetSelectionClient` ([`client/scripts/target_selection_client.gd`](../../../client/scripts/target_selection_client.gd)), Tab / Esc bindings, `TargetLockLabel` HUD, hybrid movement-triggered refresh via new `PositionAuthorityClient.authoritative_ack` signal (fires on every server-confirmed position, boot + `move-stream` 200) with a 250 ms cooldown; manual QA in [`docs/manual-qa/NEO-24.md`](../../manual-qa/NEO-24.md). **NEO-25 landed:** versioned **`GET /game/world/interactables`** ([NEO-25](../../plans/NEO-25-implementation-plan.md)) — `InteractablesListResponse` / `InteractablesWorldApi` in `server/NeonSprawl.Server/Game/Interaction/`; Godot `interactables_catalog_client.gd` + `interactable_world_builder.gd` (fetch-driven props + glow); [server README — Interactable descriptors (NEO-25)](../../../server/README.md#interactable-descriptors-neo-25). **NEO-26 landed:** prototype **`SelectionEvent`** — **`TargetSelectionClient.selection_event`** ([NEO-26](../../plans/NEO-26-implementation-plan.md)) when **`lockedTargetId`** changes; optional **`log_selection_events`**. **Follow-on / still open:** richer **`InteractableDescriptor`** consumers beyond list projection + existing `POST …/interact`, Slice 3 telemetry (see [E1M3 prototype backlog](../../plans/E1M3-prototype-backlog.md)). **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), [Interactable descriptors (NEO-25)](../../../server/README.md#interactable-descriptors-neo-25), [Interaction](../../../server/README.md#interaction-neo-9) |
|
||||
|
||||
---
|
||||
|
||||
|
|
|
|||
|
|
@ -19,7 +19,7 @@ Fleshed-out scope, contracts, and integration notes live in **per-module documen
|
|||
|
||||
**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). **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). **Partial (NEO-25):** versioned **`GET /game/world/interactables`** + `InteractablesListDtos` / `InteractablesWorldApi` in `Game/Interaction/`; Godot `interactables_catalog_client.gd` + `interactable_world_builder.gd` ([NEO-25](../../plans/NEO-25-implementation-plan.md); [server README — Interactable descriptors](../../../server/README.md#interactable-descriptors-neo-25)). **`SelectionEvent`** and richer multi-consumer **`InteractableDescriptor`** work 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`).
|
||||
**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). **Partial (NEO-25):** versioned **`GET /game/world/interactables`** + `InteractablesListDtos` / `InteractablesWorldApi` in `Game/Interaction/`; Godot `interactables_catalog_client.gd` + `interactable_world_builder.gd` ([NEO-25](../../plans/NEO-25-implementation-plan.md); [server README — Interactable descriptors](../../../server/README.md#interactable-descriptors-neo-25)). **NEO-26 (prototype `SelectionEvent`):** Godot **`TargetSelectionClient.selection_event`** on **`lockedTargetId`** changes ([NEO-26](../../plans/NEO-26-implementation-plan.md)). **Follow-on / in progress:** richer multi-consumer **`InteractableDescriptor`** targeting 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
|
||||
|
||||
|
|
|
|||
|
|
@ -0,0 +1,39 @@
|
|||
# Code Review — NEO-26 SelectionEvent
|
||||
|
||||
- **Date:** 2026-04-25
|
||||
- **Scope:** Branch `NEO-26-selectionevent-surface-debug-consumer`; diff `origin/main...HEAD` (NEO-26 `SelectionEvent` surface + debug consumer)
|
||||
- **Base:** `origin/main`
|
||||
|
||||
## Verdict
|
||||
|
||||
Approve with nits.
|
||||
|
||||
## Summary
|
||||
|
||||
This branch adds a focused `SelectionEvent` surface in `TargetSelectionClient` and keeps it intentionally narrow to `lockedTargetId` transitions, with explicit `cause` attribution (`tab`, `clear`, `server_correction`). The implementation is coherent with the NEO-26 plan: cause tagging is set at request start, stale/failed paths clear the pending cause, and emit is suppressed for id-unchanged updates. Coverage in `client/test/selection_event_client_test.gd` exercises both positive and no-op rules, and the targeted suite passes locally. Risk is low and mostly around documentation tracking consistency rather than runtime behavior.
|
||||
|
||||
## Documentation checked
|
||||
|
||||
- `docs/plans/NEO-26-implementation-plan.md` — **matches** (signal shape, cause rules, lock-id-only emit rule, print consumer, QA artifact all implemented).
|
||||
- `docs/decomposition/modules/E1_M3_InteractionAndTargetingLayer.md` — **matches** (implementation snapshot updated to include NEO-26 `selection_event` prototype).
|
||||
- ~~`docs/decomposition/modules/module_dependency_register.md` — **partially matches** (E1.M3 note still says "`SelectionEvent` ... remain in progress"; …).~~ **Addressed:** E1.M3 note now records **NEO-26** prototype `SelectionEvent` and separates follow-on `InteractableDescriptor` / multi-consumer work.
|
||||
- ~~`docs/decomposition/modules/documentation_and_implementation_alignment.md` — **partially matches** (E1.M3 row still says "`SelectionEvent` ... still open"; …).~~ **Addressed:** E1.M3 snapshot row records **NEO-26** landed vs follow-on scope.
|
||||
|
||||
~~Implementation tracking update needed after merge: update the E1.M3 row wording in `documentation_and_implementation_alignment.md` (and optionally the E1.M3 note in `module_dependency_register.md`) so the docs reflect that prototype `SelectionEvent` now exists while richer downstream consumers remain open.~~ **Done.**
|
||||
|
||||
## Blocking issues
|
||||
|
||||
None.
|
||||
|
||||
## Suggestions
|
||||
|
||||
1. ~~Update the E1.M3 tracking row in `docs/decomposition/modules/documentation_and_implementation_alignment.md` to explicitly record NEO-26 as landed, replacing the blanket "`SelectionEvent` still open" phrasing with "prototype landed; richer consumers follow-on."~~ **Done.**
|
||||
|
||||
## Nits
|
||||
|
||||
- ~~Nit: Consider adding one assertion for `sequence` in `client/test/selection_event_client_test.gd` to lock payload stability for downstream subscribers.~~ **Done.** (`sequence` asserted on tab, clear, GET id-change, and validity-only cases.)
|
||||
|
||||
## Verification
|
||||
|
||||
- `godot --headless --path . -s res://addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode -a res://test/selection_event_client_test.gd` (from `client/`) — passed (`5 test cases`, `0 failures`).
|
||||
- Manual checks documented in `docs/manual-qa/NEO-26.md`.
|
||||
Loading…
Reference in New Issue