neon-sprawl/docs/plans/NEO-18-implementation-plan.md

# NEO-18 — Implementation plan

## Story reference

| Field | Value |
|--------|--------|
| **Key** | NEO-18 |
| **Title** | E1.M2: Camera integration hardening + dependent contract notes |
| **Linear** | [NEO-18](https://linear.app/neon-sprawl/issue/NEO-18) |
| **Parent** | [Epic 1 — Core Player Runtime](https://linear.app/neon-sprawl/project/epic-1-core-player-runtime-client-controls-character-loop-66bd590cd016) |
| **Module** | [E1.M2 — IsometricCameraController](../decomposition/modules/E1_M2_IsometricCameraController.md); umbrella [E1.M2](https://linear.app/neon-sprawl/project/e1m2-isometriccameracontroller-deac8ef10395) |

**Linear source:** Description and acceptance criteria were pulled from Linear (issue **NEO-18**). This plan is the working implementation breakdown; if the Linear issue edits after this date, re-sync the checklist.

**Board:** Transitioned **To Do → In Progress** on kickoff (Linear MCP `save_issue` with **`state`** **In Progress**).

## Goal, scope, and out-of-scope

**Goal (from Linear):** 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 (from Linear)**

- **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 [E1.M2](https://linear.app/neon-sprawl/project/e1m2-isometriccameracontroller-deac8ef10395)** / 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.

**Additional in-repo hardening (engineering, supports “stable contracts”)**

- **`IsometricFollowCamera` occluder lifecycle:** `_occluder_overrides` must not **leak or fault** when occluder bodies are **freed** or invalid; **erase** stale dict keys without calling `set_surface_override_material` on a freed subtree (that errors); live bodies still restore when they leave the ray.

**Out of scope (from Linear)**

- 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

Acceptance criteria (verbatim intent):

- [x] **Module dependency register / module doc:** E1.M2 **Ready** (or agreed status) with pointers to scripts/scenes.
- [x] **`CameraState` contract** summarized in one place and matches module doc (**includes yaw**).
- [x] **E6.M2 note:** what consumers may read vs undefined; how **yaw** affects camera-adjacent UX if non-zero later.
- [x] **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 written acceptance criteria):**

- [x] Occluder invalid-key purge covered by tests or documented manual steps (see [Tests](#tests)).
- [x] [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. **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 `_physics_process`** tick on the rig (**`process_physics_priority`** after `Player` — see **Decisions**).
   - **`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 **E1.M2**, **NEO-15–NEO-17**, **NEO-20** 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 “NEO-18 remains” narrative with **shipped** wording + script/scene pointers.
   - **`documentation_and_implementation_alignment.md`:** E1.M2 row matches register + NEO-18 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. **`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. **`isometric_follow_camera.gd` hardening**
   - At start of `_update_occlusion` (and/or restore passes): **erase** dictionary keys whose body is **`not is_instance_valid`** (do **not** restore materials on that path — freed occluder’s meshes are gone). Harden `_restore_occluder_overrides_list` with `is_instance_valid` on mesh and non-null `original` for the live-body restore path.

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.

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 |
|-------|--------|-----------|
| **`CameraState` policy flags** | **Doc-first** | Add fields only if E6.M2 contract text requires a snapshot; avoid speculative API. |
| **Invalid occluder keys** | **Erase only** | Freed body ⇒ subtree is gone; restoring overrides hits freed `MeshInstance3D` / materials. Drop dict entry; live bodies restore when they leave the ray. |
| **Epic Slice 2 wording** | **Align with rotation policy** | Satisfies acceptance criteria on “yaw not exposed / default in UX” vs misleading “no rotation.” |
| **Rig tick (jitter follow-up)** | **`_physics_process` + `process_physics_priority = 1`** | `Player` default **0** runs before **1**; camera reads post–`move_and_slide`. Avoids `_process` vs 120 Hz physics jitter (camera was above `Player` in scene order). |
| **Runtime “remove obstacle” + `NavigationAgent3D`** | **Reparent out of `NavigationRegion3D`, then rebake + sync agent** | Default `NavigationMesh.geometry_parsed_geometry_type` is **`PARSED_GEOMETRY_BOTH`**: **mesh instances** under the region are baked as source geometry alongside static colliders. Disabling collision (or rebaking alone) does **not** reopen nav if the body’s **`MeshInstance3D` remains a child of the region** — the hole persists. For any **gameplay mechanic that removes or opens geometry** while using baked nav, either **exclude that mesh from source** (project/settings), **reparent** the subtree **outside** the region before rebake, **`queue_free`** (if safe), or switch to an explicit geometry pipeline. Prototype dev toggle: `main.gd` reparents `Obstacle` to `World` when hidden, rebakes, `player.sync_navigation_agent_after_map_rebuild`. |

### Runtime navigation (lesson for future mechanics)

If the client uses **`NavigationRegion3D`** with default parsing (**colliders + meshes**), **hiding** or **disabling collision** on a prop is **not** enough for **click-to-move** to cross that tile: **`bake_navigation_mesh`** will still see **`MeshInstance3D`** children of the region. **Rebake + `NavigationAgent3D` map sync** only fixes the path once that geometry is **no longer under the region** (or no longer parsed). Plan any **destructible door**, **removed barricade**, or **temporary wall** with this in mind — same pitfall as NEO-18 dev smoke.

## Files to add

**None** unless occlusion tests are split out (optional `client/test/isometric_follow_camera_occlusion_hardening_test.gd`).

## Files to modify

| Path | Rationale |
|------|-----------|
| `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 Linear optional telemetry note. |
| `docs/decomposition/modules/E1_M2_IsometricCameraController.md` | Consumer contract, NEO-18 snapshot, E1.M2 / 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.** 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:** Main scene — zoom bands, occlusion fade, fixed yaw UX; spot-check Slice 2 behaviors (readable motion, occlusion active).

**Occluder smoke (embedded editor):** Input action **`dev_toggle_occluder_obstacle`** (default **Ctrl+Shift+K** in `project.godot`) toggles `Obstacle` visibility + collision in `main.gd` (does not `queue_free`). **F9** / **F10** are debugger shortcuts (**Suspend/Resume**, **next frame**) when the embedded game has focus. Position the player so the obstacle is on the occlusion ray to test fade + `restore_all_occluder_materials_now` before hide. After hide, confirm **click-to-move through the former cell**: the implementation **reparents** the obstacle under **`World`**, **rebakes** the region mesh, and **`sync_navigation_agent_after_map_rebuild`** — see **Decisions** table and **Runtime navigation** note (**`PARSED_GEOMETRY_BOTH`**). Complements static `occluder_override_key_is_valid` tests, which do not execute the full purge path in CI.

## Open questions / risks

- **`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.

## Shipped (2026-04-10)

- `camera_state.gd` consumer contract header; `isometric_follow_camera.gd` invalid occluder purge + `occluder_override_key_is_valid`; tests in `isometric_follow_camera_test.gd`.
- Docs: `E1_M2_IsometricCameraController.md` (Ready, consumer contract, NEO-18 snapshot), `E6_M2_ConsentAndRiskUxSignals.md` (E1.M2 adjacency), `module_dependency_register.md`, `documentation_and_implementation_alignment.md`, `epic_01_core_player_runtime.md` Slice 2.
- Follow-up: rig **`_physics_process`** + priority (see **Decisions**).
- Client dev smoke (`main.gd`): obstacle toggle + **nav**: reparent out of `NavigationRegion3D`, rebake, agent sync — documents the **`PARSED_GEOMETRY_BOTH`** / mesh-as-source pitfall for future **runtime geometry removal** (see **Decisions** and **Runtime navigation**).