diff --git a/docs/plans/NEON-28-implementation-plan.md b/docs/plans/NEON-28-implementation-plan.md index 519c21b..4fd3482 100644 --- a/docs/plans/NEON-28-implementation-plan.md +++ b/docs/plans/NEON-28-implementation-plan.md @@ -5,95 +5,107 @@ | Field | Value | |--------|--------| | **Key** | NEON-28 | -| **Title** | E1.M2: Camera integration hardening + dependent contract notes (per [decomposition backlog](../decomposition/modules/E1_M2_IsometricCameraController.md#jira-backlog)) | +| **Title** | E1.M2: Camera integration hardening + dependent contract notes | | **Jira** | [NEON-28](https://neon-sprawl.atlassian.net/browse/NEON-28) | | **Parent** | [NEON-1 — Epic 1 — Core Player Runtime](https://neon-sprawl.atlassian.net/browse/NEON-1) | | **Module** | [E1.M2 — IsometricCameraController](../decomposition/modules/E1_M2_IsometricCameraController.md); umbrella [NEON-10](https://neon-sprawl.atlassian.net/browse/NEON-10) | -**Jira sync:** This plan was drafted from repo decomposition and code because the Atlassian MCP plugin is unavailable in this environment. **Reconcile the acceptance checklist below with the issue description in Jira** before merge; add or strike items so the plan matches Jira AC exactly. +**Jira source:** Description and acceptance criteria were pulled from Jira via Atlassian MCP (cloud `neon-sprawl`, issue **NEON-28**). This plan is the working implementation breakdown; if Jira edits after this date, re-sync the checklist. -**Board:** If the issue is still **To Do**, transition it to **In Progress** when implementation starts (Jira UI or MCP when available). +**Board:** Transitioned **To Do → In Progress** on kickoff (Atlassian MCP `transitionJiraIssue`, transition id `2` / name `prog`). ## Goal, scope, and out-of-scope -**Goal:** Close the E1.M2 “integration hardening” gap after follow + zoom + occlusion + pick-through shipped ([NEON-25](NEON-25-implementation-plan.md)–[NEON-27](NEON-27-implementation-plan.md), [NEON-30](NEON-30-implementation-plan.md)): make the camera stack **safe under edge cases** and write **explicit dependent contract notes** so downstream modules (notably **E6.M2 — ConsentAndRiskUxSignals**) know what they may rely on from the client camera layer. +**Goal (from Jira):** Close the **E1.M2 prototype slice** with stable contracts, documentation, and hooks so **E6.M2** (and other dependents) can rely on `CameraState` and presentation assumptions without spelunking implementation. -**In scope** +**In scope (from Jira)** -- **Runtime hardening** on `IsometricFollowCamera`: occluder override bookkeeping must not **leak or fault** when occluder bodies are **freed**, removed from the tree, or otherwise invalid while still listed in `_occluder_overrides`; periodic reconciliation should **drop invalid keys** and **restore** mesh overrides where the `MeshInstance3D` is still valid. -- **Contract documentation (dependent notes):** - - **E1.M2** module doc: short **“Consumer contract”** subsection — fields on `CameraState`, refresh cadence, meaning of `yaw` vs presentation yaw on the rig, client-only authority pointer to [client_server_authority.md](../decomposition/modules/client_server_authority.md#e1m2-isometriccameracontroller). - - **E6.M2** module doc: **“E1.M2 camera adjacency”** subsection — what risk UX may read today (`CameraState` / rig path), explicit statement that **server must not** use camera pose for gameplay (link authority doc), and placeholder for future flags if product adds them. -- **Alignment hygiene:** Update [module_dependency_register.md](../decomposition/modules/module_dependency_register.md) **E1.M2 note** and [documentation_and_implementation_alignment.md](../decomposition/modules/documentation_and_implementation_alignment.md) **E1.M2** row to reflect NEON-28 completion (integration hardened + contracts noted). -- **Tests** for hardening paths that stay **headless-friendly** (no full `.tscn` required). +- **Finalize `CameraState`:** follow target, **yaw** (prototype `0`; document semantics if orbit is enabled mid-project), zoom band / effective framing, **policy flags as needed** (add fields only when the documented consumer contract requires them; otherwise state explicitly what is **not** in state and where to read it on the rig). +- **Implementation snapshot in module doc:** **Mid-project rotation policy** and **post-release freeze** from `E1_M2_IsometricCameraController.md` (prototype **fixed yaw for players**; optional `allow_yaw` / limits seam; world-anchored gameplay preferred; telegraph readability). **Cross-link [NEON-10](https://neon-sprawl.atlassian.net/browse/NEON-10)** / module backlog for traceability. +- **Client-local authority** and **fixed pitch/roll** for isometric presentation — align [client_server_authority.md](../decomposition/modules/client_server_authority.md) references in E1.M2 / E6.M2 notes. +- **Optional telemetry:** document hooks for throttled `camera_zoom_changed` and occlusion/perf counters **when E9.M1** schema exists (no requirement to implement telemetry in this story unless already trivial TODO cleanup). +- **Dependency register + alignment:** E1.M2 **Ready** (or agreed status) with pointers to scripts/scenes per acceptance criteria. -**Out of scope** +**Additional in-repo hardening (engineering, supports “stable contracts”)** -- **Yaw orbit** input, new zoom/occlusion techniques, or server-side camera awareness. -- **E9.M1** throttled telemetry (`camera_zoom_changed`, occlusion stress telemetry) — remains TODO on the rig / policy scripts. -- **E6.M2 implementation** (HUD, `RiskPrompt`, etc.) — documentation and stability only here. +- **`IsometricFollowCamera` occluder lifecycle:** `_occluder_overrides` must not **leak or fault** when occluder bodies are **freed** or invalid; purge invalid keys and restore surviving `MeshInstance3D` overrides. + +**Out of scope (from Jira)** + +- New gameplay features; **E1.M3 / E1.M4** work. +- Committing to **yaw orbit in UX** without product sign-off (policy allows mid-project enablement only). ## Acceptance criteria checklist -Confirm against Jira; edit this section if Jira differs. +Jira acceptance criteria (verbatim intent): -- [ ] **Occluder lifecycle:** Fading an occluder then **queue_free** (or equivalent removal) does not leave permanent material overrides on surviving meshes and does not spam errors; `_occluder_overrides` does not retain invalid object keys. -- [ ] **Consumer contract documented:** E1.M2 documents **`CameraState`** semantics and cadence; E6.M2 documents what it may assume from E1.M2 today and points to authority rules. -- [ ] **Dependency / alignment docs** updated so E1.M2 no longer reads “integration hardening remains” for NEON-28’s scope (status/note row reflects shipped hardening + docs). -- [ ] **Automated tests** cover the new invalid-occluder / cleanup behavior (or document why impossible with current harness — prefer tests). +- [ ] **Module dependency register / module doc:** E1.M2 **Ready** (or agreed status) with pointers to scripts/scenes. +- [ ] **`CameraState` contract** summarized in one place and matches module doc (**includes yaw**). +- [ ] **E6.M2 note:** what consumers may read vs undefined; how **yaw** affects camera-adjacent UX if non-zero later. +- [ ] **Epic 1 Slice 2** acceptance satisfied: prototype keeps **yaw fixed for players**, zoom within bands, readable motion, occlusion policy active; aligns with updated module wording (not “camera has no yaw,” but **yaw not exposed / remains default in UX**). + +**Supporting engineering checks (not a substitute for Jira AC):** + +- [ ] Occluder invalid-key purge covered by tests or documented manual steps (see [Tests](#tests)). +- [ ] [epic_01_core_player_runtime.md](../decomposition/epics/epic_01_core_player_runtime.md) **Slice 2** bullets use the same **yaw / presentation** language as the module (replace over-simplified “no rotation” if it contradicts the rotation policy). ## Technical approach -1. **`isometric_follow_camera.gd`** - - When iterating `_occluder_overrides` (restore pass and/or a small **sanity pass** each frame or only when keys change): if an occluder key **`not is_instance_valid(body)`**, **purge** the entry: for any stored `{mesh, surface, original}` rows, call `set_surface_override_material` only when `is_instance_valid(mi)`; then **erase** the dictionary key. This mirrors “restore then forget” without requiring the body to still exist. - - Optionally run the same invalid-key purge at the **start** of `_update_occlusion` before computing `newly_blocking` to avoid freed keys participating in logic. - - Keep behavior **identical** on the happy path (no extra allocations in the common case if implemented with a single backward pass over keys). +1. **Contract documentation (primary)** + - **`camera_state.gd`:** Expand file header (and/or class-level comments) so it is a **concise, accurate** summary of fields: `yaw` = orbit delta vs presentation compass; `distance`; `zoom_band_index`; `focus_world`; `follow_target_path`; refresh **every `_process`** on the rig. + - **`E1_M2_IsometricCameraController.md`:** Single **Consumer contract** subsection: table linking each `CameraState` field + rig exports (`allow_yaw`, `presentation_yaw_deg`, resources). Include **mid-project rotation** + **post-release freeze** pointers; link **NEON-10**, **NEON-25–NEON-27**, **NEON-30** as appropriate. + - **`E6_M2_ConsentAndRiskUxSignals.md`:** **E1.M2 camera adjacency** — allowed reads (`CameraState` fields), **undefined** (what stays on rig only), **yaw non-zero later** (presentation / HUD implications in one short paragraph); link authority (server must not use camera pose for gameplay). + - **`module_dependency_register.md`:** Set E1.M2 **Status** to **Ready** (or agreed label) and replace “NEON-28 remains” narrative with **shipped** wording + script/scene pointers. + - **`documentation_and_implementation_alignment.md`:** E1.M2 row matches register + NEON-28 completion. + - **`epic_01_core_player_runtime.md`:** Adjust **Slice 2** scope/acceptance lines so they reflect **fixed yaw in UX** and documented orbit seam, not flat “camera never rotates” if that misleads. -2. **Documentation** - - **E1_M2_IsometricCameraController.md:** Add **NEON-28** bullet under implementation snapshot + **Consumer contract** (table or short list: `focus_world`, `distance`, `zoom_band_index`, `yaw` = orbit only, `follow_target_path`; refreshed every `_process` on the rig; presentation angle lives on rig exports, not duplicated in state unless we explicitly add a field later). - - **E6_M2_ConsentAndRiskUxSignals.md:** Add **E1.M2 camera adjacency** — read `CameraState` from the follow rig when implementing risk UI; do not send camera pose to server for eligibility; link E1.M2 + authority. - - **module_dependency_register.md** / **documentation_and_implementation_alignment.md:** Replace “hardening remains (NEON-28)” with **done** wording once this story merges. +2. **`CameraState` “finalize”** + - Reconcile code and docs. Add **policy flags** to `CameraState` only if the E6.M2 / E1.M2 contract text commits to them; otherwise explicitly document **“no policy flags in state yet; read rig exports.”** -3. **Tests** - - Prefer a **GdUnit** suite that instantiates the camera script in isolation (or minimal scene) if needed: register a fake occluder flow is hard without physics — **fallback:** unit-test a **new static helper** e.g. `prune_invalid_occluder_entries(overrides: Dictionary) -> void` extracted for testability, or test via internal method if extraction is too heavy. **Minimal approach:** extract a small `static func` that takes the override dict and validates keys (document in plan — if we keep logic inline only, use a test scene with `StaticBody3D` + mesh under test project — only if CI can run it headless). +3. **`isometric_follow_camera.gd` hardening** + - At start of `_update_occlusion` (and/or restore passes): drop dictionary keys whose body is **`not is_instance_valid`**; restore materials where `MeshInstance3D` is still valid; then erase entries. - **Decision during implementation:** Prefer **extracted static** `prune_freed_occluder_overrides(overrides: Dictionary) -> void` (or similar) that only uses `is_instance_valid` + restore loop so tests do not need `World3D`. Keeps `isometric_follow_camera.gd` as the single owner of restore semantics by having the static call the same restore logic via a callback — actually simpler: **instance method** `_purge_invalid_occluder_keys() -> void` called from `_update_occlusion` entry; test by subclassing or by scene-free test using `Engine.get_main_loop()` — hardest. +4. **Optional telemetry** + - Keep or add **TODO(E9.M1)** pointers in rig / policy scripts; no new telemetry pipeline unless out of scope exception is agreed. - **Pragmatic test path:** Add `isometric_follow_camera.gd` **static** `func occluder_override_key_is_valid(body: Variant) -> bool` returning `body is Node3D and is_instance_valid(body)` — test the static; implementation uses it when filtering keys. Low ceremony, proves the guard exists. +5. **Tests** + - Static helper tests for **occluder key validity** / purge behavior where headless-safe; manual **queue_free** occluder check in editor for full confidence. ## Decisions | Topic | Choice | Rationale | |-------|--------|-----------| -| **API surface for E6.M2** | **Docs only** in NEON-28 | No new `CameraState` fields until a risk UX story needs them; avoids speculative schema. | -| **Invalid occluder handling** | **Purge + restore surviving meshes** | Prevents stuck transparency and dictionary leaks when level chunks or props despawn. | -| **Jira AC** | **Reconcile after fetch** | Decomposition title is the source for this draft; Jira remains canonical. | +| **`CameraState` policy flags** | **Doc-first** | Add fields only if Jira/E6.M2 contract text requires a snapshot; avoid speculative API. | +| **Invalid occluder keys** | **Purge + restore** | Prevents stuck materials and leaks when occluders despawn. | +| **Epic Slice 2 wording** | **Align with rotation policy** | Satisfies Jira AC on “yaw not exposed / default in UX” vs misleading “no rotation.” | ## Files to add -**None** — changes land in existing `isometric_follow_camera.gd`, decomposition docs, and `client/test/isometric_follow_camera_test.gd`. If occlusion tests grow large during implementation, add `client/test/isometric_follow_camera_occlusion_hardening_test.gd` (listed as optional under [Tests](#tests)). +**None** unless occlusion tests are split out (optional `client/test/isometric_follow_camera_occlusion_hardening_test.gd`). ## Files to modify | Path | Rationale | |------|-----------| -| `client/scripts/isometric_follow_camera.gd` | Invalid occluder key purge + optional small static/helper for testability; keep happy-path behavior unchanged. | -| `docs/decomposition/modules/E1_M2_IsometricCameraController.md` | NEON-28 snapshot + consumer contract for `CameraState` / rig. | -| `docs/decomposition/modules/E6_M2_ConsentAndRiskUxSignals.md` | Dependent notes: E1.M2 adjacency + authority link. | -| `docs/decomposition/modules/module_dependency_register.md` | E1.M2 note: NEON-28 complete vs “remains.” | -| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | E1.M2 row: reflect hardening + contract notes shipped. | +| `client/scripts/camera_state.gd` | Finalize field documentation; optional policy flags if contract requires. | +| `client/scripts/isometric_follow_camera.gd` | Occluder invalid-key purge; keep TODO(E9.M1) aligned with Jira optional telemetry note. | +| `docs/decomposition/modules/E1_M2_IsometricCameraController.md` | Consumer contract, NEON-28 snapshot, NEON-10 / rotation policy cross-links. | +| `docs/decomposition/modules/E6_M2_ConsentAndRiskUxSignals.md` | E1.M2 adjacency + yaw UX note + authority. | +| `docs/decomposition/modules/module_dependency_register.md` | E1.M2 Ready + pointers; remove “hardening remains.” | +| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | E1.M2 row updated. | +| `docs/decomposition/epics/epic_01_core_player_runtime.md` | Slice 2 wording vs yaw/presentation policy. | +| `client/test/isometric_follow_camera_test.gd` | Tests for new static guards / occluder key validity if added. | ## Tests | File | Coverage | |------|----------| -| `client/test/isometric_follow_camera_test.gd` | **Change.** Add tests for any **new static guard** (e.g. invalid/freed `Node3D` key handling) or for **occluder override key validity** helper if introduced. | -| `client/test/isometric_follow_camera_occlusion_hardening_test.gd` | **Optional add.** Only if tests are too large for the existing suite; split occluder-lifecycle cases here. | +| `client/test/isometric_follow_camera_test.gd` | **Change.** Guards for occluder override dictionary key validity / purge helper if extracted. | +| `client/test/camera_state_test.gd` | **Change only if** new `CameraState` fields ship — round-trip defaults. | -**Manual verification:** Run main scene; stand behind `Obstacle` to trigger fade; use debugger or a one-off dev step to free the obstacle node and confirm no errors and materials on other objects look correct. Repeat zoom in/out and click-through (NEON-30) smoke check. +**Manual verification:** Main scene — zoom bands, occlusion fade, fixed yaw UX; free an occluder while faded and confirm no errors / stuck materials; spot-check Slice 2 behaviors (readable motion, occlusion active). ## Open questions / risks -- **Jira AC drift:** Issue text may list extra items (e.g. specific perf or UX gates). Re-read NEON-28 in Jira when the plugin works and **update this plan** before implementation lands. -- **Headless tests vs freed `Object`:** Freed instance behavior in GDScript is subtle; prefer **static** tests on `Variant` / validity helpers plus one manual free-node check in editor. +- **`CameraState` policy flags:** Product may later require explicit booleans for risk UX; if E6.M2 text stays non-committal, keep flags out and document **rig as source** until a follow-on story. - None otherwise.