From 92104b0b534261fb8aff4533f2ee16b07f42a47c Mon Sep 17 00:00:00 2001 From: VinPropane Date: Fri, 10 Apr 2026 21:56:44 -0400 Subject: [PATCH 01/12] NEON-28: add implementation plan for camera hardening --- docs/plans/NEON-28-implementation-plan.md | 99 +++++++++++++++++++++++ 1 file changed, 99 insertions(+) create mode 100644 docs/plans/NEON-28-implementation-plan.md diff --git a/docs/plans/NEON-28-implementation-plan.md b/docs/plans/NEON-28-implementation-plan.md new file mode 100644 index 0000000..519c21b --- /dev/null +++ b/docs/plans/NEON-28-implementation-plan.md @@ -0,0 +1,99 @@ +# NEON-28 — Implementation plan + +## Story reference + +| 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)) | +| **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. + +**Board:** If the issue is still **To Do**, transition it to **In Progress** when implementation starts (Jira UI or MCP when available). + +## 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. + +**In scope** + +- **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). + +**Out of scope** + +- **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. + +## Acceptance criteria checklist + +Confirm against Jira; edit this section if Jira differs. + +- [ ] **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). + +## 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). + +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. + +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). + + **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. + + **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. + +## 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. | + +## 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)). + +## 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. | + +## 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. | + +**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. + +## 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. +- None otherwise. From 7e674c371038c39a3be78a34f08816e78c10d8ee Mon Sep 17 00:00:00 2001 From: VinPropane Date: Fri, 10 Apr 2026 21:59:35 -0400 Subject: [PATCH 02/12] NEON-28: reconcile plan with Jira (MCP sync) --- docs/plans/NEON-28-implementation-plan.md | 104 ++++++++++++---------- 1 file changed, 58 insertions(+), 46 deletions(-) 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. From 15d304fa2c233e1d232263574d82bbe4a81cd990 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Fri, 10 Apr 2026 22:11:40 -0400 Subject: [PATCH 03/12] NEON-28: camera contracts, E6.M2 adjacency, occluder purge --- client/scripts/camera_state.gd | 19 +++++++-- client/scripts/isometric_follow_camera.gd | 39 ++++++++++++++++--- client/test/isometric_follow_camera_test.gd | 22 ++++++++++- .../epics/epic_01_core_player_runtime.md | 4 +- .../E1_M2_IsometricCameraController.md | 35 +++++++++++++++-- .../modules/E6_M2_ConsentAndRiskUxSignals.md | 9 +++++ ...umentation_and_implementation_alignment.md | 2 +- .../modules/module_dependency_register.md | 4 +- docs/plans/NEON-28-implementation-plan.md | 17 +++++--- 9 files changed, 126 insertions(+), 25 deletions(-) diff --git a/client/scripts/camera_state.gd b/client/scripts/camera_state.gd index b71f310..2f2980e 100644 --- a/client/scripts/camera_state.gd +++ b/client/scripts/camera_state.gd @@ -1,12 +1,23 @@ extends RefCounted -## Client-local camera snapshot for consumers (risk UX, UI, debug). Refreshed each frame by -## `isometric_follow_camera.gd`. No `class_name` — see repo Godot headless / CI notes. +## Client-local **per-frame** snapshot from the follow rig (`isometric_follow_camera.gd`). +## The rig creates **one** instance and assigns fields every `_process` after pose is final. +## No `class_name` — see repo Godot headless / CI notes. +## +## **Consumer contract (NEON-28):** Risk UX / HUD may read this object from the rig’s +## `camera_state` getter. **Policy-style data** (occlusion enabled, +## zoom resource identity, pitch, presentation compass) is **not** duplicated here — read the +## rig’s exports (`allow_yaw`, `presentation_yaw_deg`, resources) when needed. Future E6.M2 work +## may promote selected flags into this type; until then, keep a single source on the rig. +## +## **Authority:** Camera pose is client-only; the server must not use it for gameplay checks +## (`docs/decomposition/modules/client_server_authority.md` — E1.M2). ## Orbit yaw (rad) around the follow target **vertical axis**, **relative to** the rig’s fixed ## presentation compass (`presentation_yaw_deg` on `isometric_follow_camera.gd`). -## This is **not** total camera compass—only the optional orbit delta. -## Prototype UX keeps this at **0** (`allow_yaw` false). +## **Not** total camera compass — only the optional orbit delta. Prototype UX keeps this at **0** +## (`allow_yaw` false). If orbit is enabled mid-project, HUD and world-anchored gameplay should +## treat framing as presentation + this delta (see E1.M2 rotation policy). var yaw: float = 0.0 ## Scene path to the follow anchor (typically `CharacterBody3D` / `Node3D`). diff --git a/client/scripts/isometric_follow_camera.gd b/client/scripts/isometric_follow_camera.gd index e548fbd..3760af8 100644 --- a/client/scripts/isometric_follow_camera.gd +++ b/client/scripts/isometric_follow_camera.gd @@ -2,12 +2,14 @@ extends Node3D ## NEON-25: client-local isometric follow; updates [CameraState] each `_process`. ## NEON-26: discrete zoom bands via [member zoom_band_config]; wheel / `camera_zoom_*` actions. +## NEON-28: purge freed occluder keys from [member _occluder_overrides] before ray pass (see +## [method occluder_override_key_is_valid]). ## Pitch / roll are fixed by presentation exports; **orbit yaw** in state stays **0** while ## [member allow_yaw] is false (no rotate input bound). Future orbit: read relative input here, ## add to `_orbit_yaw_rad`, clamp with [member max_yaw_deg]. -## TODO(E9.M1): map throttled product telemetry (for example `camera_zoom_changed`) -## to this signal when schema exists. +## TODO(E9.M1): map throttled product telemetry (for example `camera_zoom_changed`) to +## [signal zoom_band_changed] when schema exists; optional occlusion/perf counters per module doc. signal zoom_band_changed(new_index: int, distance: float) const CameraStateScript := preload("res://scripts/camera_state.gd") @@ -223,6 +225,31 @@ static func occlusion_policy_is_valid(policy: Resource) -> bool: return policy != null and policy.get_script() == OcclusionPolicyScript and policy.is_valid() +## True if [param body] is a live [Node3D] suitable as an [member _occluder_overrides] key. +## Used before occlusion ray work and from tests (NEON-28). +static func occluder_override_key_is_valid(body: Variant) -> bool: + return body is Node3D and is_instance_valid(body) + + +func _restore_occluder_overrides_list(overrides: Array) -> void: + for entry in overrides: + var mi: MeshInstance3D = entry["mesh"] + if is_instance_valid(mi): + mi.set_surface_override_material(entry["surface"], entry["original"]) + + +## Drop invalid keys from [member _occluder_overrides] and restore any surviving meshes. +func _purge_invalid_occluder_override_keys() -> void: + var keys: Array = _occluder_overrides.keys() + for body in keys: + if occluder_override_key_is_valid(body): + continue + var overrides: Variant = _occluder_overrides.get(body) + if overrides != null: + _restore_occluder_overrides_list(overrides) + _occluder_overrides.erase(body) + + ## Cast iterative rays from the smoothed eye to the player focus. ## Bodies in the [member occluder_group] that intersect the ray are faded; ## bodies that leave the path have their materials restored. @@ -231,6 +258,8 @@ func _update_occlusion(focus: Vector3) -> void: _restore_all_occluders() return + _purge_invalid_occluder_override_keys() + var policy = occlusion_policy var space_state := get_world_3d().direct_space_state var newly_blocking: Array[Node3D] = [] @@ -309,14 +338,12 @@ func _apply_occluder_fade(body: Node3D) -> void: func _restore_occluder(body: Node3D) -> void: if not _occluder_overrides.has(body): return - for entry in _occluder_overrides[body]: - var mi: MeshInstance3D = entry["mesh"] - if is_instance_valid(mi): - mi.set_surface_override_material(entry["surface"], entry["original"]) + _restore_occluder_overrides_list(_occluder_overrides[body]) _occluder_overrides.erase(body) ## Restore all currently faded occluders (called on policy disable or node exit). func _restore_all_occluders() -> void: + _purge_invalid_occluder_override_keys() for body in _occluder_overrides.keys(): _restore_occluder(body) diff --git a/client/test/isometric_follow_camera_test.gd b/client/test/isometric_follow_camera_test.gd index 542d935..c630d03 100644 --- a/client/test/isometric_follow_camera_test.gd +++ b/client/test/isometric_follow_camera_test.gd @@ -1,4 +1,4 @@ -# Static helpers for res://scripts/isometric_follow_camera.gd (NEON-25 framing, NEON-27 guard). +# Static helpers for res://scripts/isometric_follow_camera.gd (NEON-25 framing, NEON-27/28 guards). extends GdUnitTestSuite const IsoScript := preload("res://scripts/isometric_follow_camera.gd") @@ -62,3 +62,23 @@ func test_occlusion_policy_is_valid_false_when_disabled() -> void: var p = OcclusionPolicyScript.new() p.enabled = false assert_that(IsoScript.occlusion_policy_is_valid(p)).is_false() + + +func test_occluder_override_key_is_valid_null() -> void: + assert_that(IsoScript.occluder_override_key_is_valid(null)).is_false() + + +func test_occluder_override_key_is_valid_non_node() -> void: + assert_that(IsoScript.occluder_override_key_is_valid(42)).is_false() + + +func test_occluder_override_key_is_valid_live_node3d() -> void: + var n := Node3D.new() + assert_that(IsoScript.occluder_override_key_is_valid(n)).is_true() + n.free() + + +func test_occluder_override_key_is_valid_false_after_free() -> void: + var n := Node3D.new() + n.free() + assert_that(IsoScript.occluder_override_key_is_valid(n)).is_false() diff --git a/docs/decomposition/epics/epic_01_core_player_runtime.md b/docs/decomposition/epics/epic_01_core_player_runtime.md index 5582480..a26939e 100644 --- a/docs/decomposition/epics/epic_01_core_player_runtime.md +++ b/docs/decomposition/epics/epic_01_core_player_runtime.md @@ -58,10 +58,10 @@ Deliver the foundational client runtime: movement, locked isometric camera, worl ### Slice 2 - Locked isometric camera -- Scope: E1.M2 end-to-end: follow-center, zoom bands, occlusion policy, no rotation. +- Scope: E1.M2 end-to-end: follow-center, zoom bands, occlusion policy, **yaw fixed in prototype UX** with **`CameraState.yaw`** and rig seam (`allow_yaw` / `presentation_yaw_deg`) documented for optional mid-project orbit ([E1_M2 — rotation policy](../modules/E1_M2_IsometricCameraController.md#mid-project-rotation-policy-practical-compromise)). - Dependencies: E1.M1 - Acceptance criteria: - - Camera never rotates; zoom stays within configured bands; player remains readable during motion. + - **Orbit not exposed in UX** for players in prototype (`allow_yaw` default off); zoom stays within configured bands; player remains readable during motion; occlusion policy active; `CameraState` contract documented for dependents. - Telemetry hooks: `camera_zoom_changed` (optional throttled), occluder stress markers if needed for perf. ### Slice 3 - Interaction, targeting, and ability input wiring diff --git a/docs/decomposition/modules/E1_M2_IsometricCameraController.md b/docs/decomposition/modules/E1_M2_IsometricCameraController.md index d886726..3b43981 100644 --- a/docs/decomposition/modules/E1_M2_IsometricCameraController.md +++ b/docs/decomposition/modules/E1_M2_IsometricCameraController.md @@ -7,7 +7,7 @@ | **Module ID** | E1.M2 | | **Epic** | [Epic 1 — Core Player Runtime](../epics/epic_01_core_player_runtime.md) | | **Stage target** | Prototype | -| **Status** | In progress — follow + `CameraState` + **discrete zoom bands** + **occlusion policy** shipped ([NEON-25](https://neon-sprawl.atlassian.net/browse/NEON-25)–[NEON-27](https://neon-sprawl.atlassian.net/browse/NEON-27)); hardening open ([NEON-28](https://neon-sprawl.atlassian.net/browse/NEON-28)); see [dependency register](module_dependency_register.md) | +| **Status** | **Ready** (prototype slice) — follow + `CameraState` + **discrete zoom bands** + **occlusion** + pick-through ([NEON-25](https://neon-sprawl.atlassian.net/browse/NEON-25)–[NEON-27](https://neon-sprawl.atlassian.net/browse/NEON-27), [NEON-30](https://neon-sprawl.atlassian.net/browse/NEON-30)); contracts + hardening ([NEON-28](https://neon-sprawl.atlassian.net/browse/NEON-28)); see [dependency register](module_dependency_register.md) | | **Jira** | Feature [NEON-10](https://neon-sprawl.atlassian.net/browse/NEON-10); prototype backlog stories under parent [NEON-1](https://neon-sprawl.atlassian.net/browse/NEON-1) — [Jira backlog](#jira-backlog) | ## Purpose @@ -39,10 +39,37 @@ Delivers an isometric follow camera that keeps the player readable during motion | Contract | Role | |----------|------| -| `CameraState` | Current follow target, zoom level, **yaw** (default `0`; optional future orbit), and policy flags for consumers (e.g. risk UX). | +| `CameraState` | Per-frame snapshot: follow path, effective distance, zoom band index, focus, **yaw** (orbit delta; prototype `0`). **Policy / presentation exports are not duplicated** in this type for the prototype — read the rig when needed ([Consumer contract](#consumer-contract-neon-28)). | | `ZoomBandConfig` | Data-driven min/max or discrete zoom steps. | | `OcclusionPolicy` | Rules for fading, dithering, or offset when geometry blocks the view. | +## Consumer contract (NEON-28) + +Single place for dependents (e.g. **E6.M2**): what is on `CameraState` vs what stays on the rig. **Authority:** [E1.M2 — client vs server](client_server_authority.md#e1m2-isometriccameracontroller). + +### `CameraState` fields (`client/scripts/camera_state.gd`) + +Refreshed **every `_process`** on `IsometricFollowCamera` after pose is final. Same object each frame (rig-owned). + +| Field | Meaning | +|--------|---------| +| `yaw` | Orbit delta (rad) around the follow target **relative to** `presentation_yaw_deg` — **not** full compass. **0** while `allow_yaw` is false (prototype UX). | +| `follow_target_path` | `NodePath` to the follow anchor (e.g. player). | +| `distance` | Effective follow distance this tick (active band or fallback). | +| `zoom_band_index` | Active discrete band index, or **0** when zoom config is invalid / unused. | +| `focus_world` | World look-at focus last tick. | + +### Rig-only reads (`client/scripts/isometric_follow_camera.gd`, `World/IsometricFollowCamera` in `main.tscn`) + +| Export / node | Meaning | +|----------------|---------| +| `allow_yaw`, `max_yaw_deg` | Orbit seam (no rotate input bound in prototype). | +| `presentation_yaw_deg`, `pitch_elevation_deg`, `follow_distance`, `focus_vertical_offset` | Fixed isometric presentation (pitch/roll not exposed as free axes). | +| `zoom_band_config`, `occlusion_policy` | Resources driving bands and occlusion. | +| Child `Camera3D` | Active render camera (`ground_pick` uses viewport / `fallback_camera` from `main.gd`). | + +**Telemetry:** Throttled `camera_zoom_changed` and occlusion/perf hooks are **TODO(E9.M1)** until an event schema exists (`zoom_band_changed` signal on the rig today). + ## Module dependencies - **E1.M1** — InputAndMovementRuntime: camera follows the player anchor derived from movement/position. @@ -55,7 +82,7 @@ Delivers an isometric follow camera that keeps the player readable during motion See Epic 1 **Slice 2 — Locked isometric camera**: follow-center, zoom bands, occlusion policy, **yaw fixed for players in prototype** (implementation still models yaw per [mid-project rotation policy](#mid-project-rotation-policy-practical-compromise)); optional telemetry such as throttled `camera_zoom_changed` and perf stress markers for occluders. -## Implementation snapshot (NEON-25 + NEON-26 + NEON-27 + NEON-30, 2026-04-08 / 2026-04-09) +## Implementation snapshot (NEON-25 + NEON-26 + NEON-27 + NEON-30 + NEON-28, 2026-04-08 / 2026-04-10) - **Godot:** `client/scripts/isometric_follow_camera.gd` on **`World/IsometricFollowCamera`**; child **`Camera3D`** (current). **`scripts/camera_state.gd`** — `RefCounted` tick snapshot (`yaw` = orbit component, **0** while **`allow_yaw`** is false; **`distance`** = effective follow distance; **`zoom_band_index`**; **`focus_world`**, **`follow_target_path`**). - **Seam:** **`allow_yaw`** / **`max_yaw_deg`** on the rig; orbit would adjust **`_orbit_yaw_rad`** (no input bound yet). **Presentation** compass uses **`presentation_yaw_deg`** separately from orbit **`yaw`** in state. @@ -66,6 +93,8 @@ See Epic 1 **Slice 2 — Locked isometric camera**: follow-center, zoom bands, o - **Click-through input (NEON-30):** `scripts/ground_pick.gd` unconditionally skips bodies in the `"occluder"` group during click-to-move ground-pick raycasts. When a cast hits an occluder the ray origin advances `OCCLUDER_PICK_THROUGH` meters past the hit point and continues, independent of whether `OcclusionPolicy` is currently fading that body. This keeps the `"occluder"` group as the sole tagging convention shared between the camera occlusion system and ground-pick input — no dedicated collision layer is added for this case. +- **Integration hardening (NEON-28):** [Consumer contract](#consumer-contract-neon-28) + `camera_state.gd` header; **E6.M2** adjacency notes ([E6_M2_ConsentAndRiskUxSignals.md](E6_M2_ConsentAndRiskUxSignals.md)); invalid / freed occluder bodies are **purged** from `_occluder_overrides` before each occlusion ray pass and before full restore (`occluder_override_key_is_valid`, tests in `isometric_follow_camera_test.gd`). + ## Jira backlog Parent epic: [NEON-1 — Epic 1 — Core Player Runtime](https://neon-sprawl.atlassian.net/browse/NEON-1). diff --git a/docs/decomposition/modules/E6_M2_ConsentAndRiskUxSignals.md b/docs/decomposition/modules/E6_M2_ConsentAndRiskUxSignals.md index db18d13..c048905 100644 --- a/docs/decomposition/modules/E6_M2_ConsentAndRiskUxSignals.md +++ b/docs/decomposition/modules/E6_M2_ConsentAndRiskUxSignals.md @@ -13,6 +13,15 @@ Entry warnings, in-zone risk UI, and clear PvP state communication using eligibility from [E6.M1](E6_M1_PvPEligibilityAndFlagState.md) and camera/readability context from [E1.M2](E1_M2_IsometricCameraController.md). +## E1.M2 camera adjacency (NEON-28) + +When implementing zone-risk HUD and related **client** presentation, treat [E1.M2](E1_M2_IsometricCameraController.md) as the framing source: + +- **May read** the follow rig’s **`camera_state`** snapshot (`client/scripts/camera_state.gd`) — field meanings and refresh cadence: [Consumer contract](E1_M2_IsometricCameraController.md#consumer-contract-neon-28). +- **Not duplicated in `CameraState` today:** occlusion on/off, bound zoom/occlusion resources, `presentation_yaw_deg`, `pitch_elevation_deg` — read **`IsometricFollowCamera`** exports (`client/scripts/isometric_follow_camera.gd`) when needed. +- **If `CameraState.yaw` becomes non-zero** (orbit enabled mid-project per E1.M2 rotation policy), screen-space layouts that assumed a fixed diagonal may need a compass / telegraph pass; keep **gameplay-adjacent** semantics **world-anchored** where possible. +- **Authority:** [Client vs server — E1.M2](client_server_authority.md#e1m2-isometriccameracontroller) — the server must **not** use client-reported camera pose for gameplay checks (targeting, LOS, zone rules). + ## Responsibilities - Show `RiskPrompt` on low-sec entry; drive `ZoneRiskState` / `PvPIndicatorState` in HUD. diff --git a/docs/decomposition/modules/documentation_and_implementation_alignment.md b/docs/decomposition/modules/documentation_and_implementation_alignment.md index dbb0ceb..4c27936 100644 --- a/docs/decomposition/modules/documentation_and_implementation_alignment.md +++ b/docs/decomposition/modules/documentation_and_implementation_alignment.md @@ -47,7 +47,7 @@ Rows appear when work starts; default for unlisted modules is **Planned** / not | Module | Register status | Snapshot | Plans / pointers | |--------|-----------------|----------|-------------------| | E1.M1 | Ready | Prototype milestone **Done** ([NEON-9](https://neon-sprawl.atlassian.net/browse/NEON-9)). Authoritative `PositionState` + **MoveCommand** over HTTP (JSON v1 snap); **in-memory** store by default, **Postgres** when configured ([NEON-5](../../plans/NEON-5-implementation-plan.md)); shared **NpgsqlDataSource** disposed on host shutdown ([NEON-15](../../plans/NEON-15-implementation-plan.md)); Godot sync + path-follow ([NEON-4](../../plans/NEON-4-implementation-plan.md), [NEON-8](../../plans/NEON-8-implementation-plan.md)); **InteractionRequest** + horizontal range ([NEON-6](../../plans/NEON-6-implementation-plan.md)). Follow-on: prediction/reconciliation, Slice 1 telemetry, Protobuf wire per [contracts.md](contracts.md). | [NEON-3](../../plans/NEON-3-implementation-plan.md), [NEON-4](../../plans/NEON-4-implementation-plan.md), [NEON-5](../../plans/NEON-5-implementation-plan.md), [NEON-6](../../plans/NEON-6-implementation-plan.md), [NEON-7](../../plans/NEON-7-implementation-plan.md), [NEON-8](../../plans/NEON-8-implementation-plan.md), [NEON-15](../../plans/NEON-15-implementation-plan.md); `server/NeonSprawl.Server/Game/PositionState/`, `Game/Interaction/`; [server README](../../../server/README.md) | -| E1.M2 | In progress | **Slice 2 prototype stories shipped:** fixed-yaw isometric follow + `CameraState` seam ([NEON-25](https://neon-sprawl.atlassian.net/browse/NEON-25)); discrete zoom bands via `ZoomBandConfig` resource ([NEON-26](https://neon-sprawl.atlassian.net/browse/NEON-26)); `OcclusionPolicy` RayCast-based material fade with `"occluder"` group tagging ([NEON-27](https://neon-sprawl.atlassian.net/browse/NEON-27)). Open: integration hardening + dependent contract notes ([NEON-28](https://neon-sprawl.atlassian.net/browse/NEON-28)). Client-local; no server use of camera pose. | [NEON-25](../../plans/NEON-25-implementation-plan.md), [NEON-26](../../plans/NEON-26-implementation-plan.md), [NEON-27](../../plans/NEON-27-implementation-plan.md); `client/scripts/isometric_follow_camera.gd`, `camera_state.gd`, `zoom_band_config.gd`, `occlusion_policy.gd`; [E1_M2_IsometricCameraController.md](E1_M2_IsometricCameraController.md) | +| E1.M2 | Ready | **Slice 2 prototype slice closed:** follow + `CameraState` ([NEON-25](https://neon-sprawl.atlassian.net/browse/NEON-25)); zoom bands ([NEON-26](https://neon-sprawl.atlassian.net/browse/NEON-26)); occlusion ([NEON-27](https://neon-sprawl.atlassian.net/browse/NEON-27)); occluder pick-through ([NEON-30](https://neon-sprawl.atlassian.net/browse/NEON-30)); contracts + hardening ([NEON-28](https://neon-sprawl.atlassian.net/browse/NEON-28)). Client-local; no server use of camera pose. | [NEON-25](../../plans/NEON-25-implementation-plan.md), [NEON-26](../../plans/NEON-26-implementation-plan.md), [NEON-27](../../plans/NEON-27-implementation-plan.md), [NEON-28](../../plans/NEON-28-implementation-plan.md), [NEON-30](../../plans/NEON-30-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) | --- diff --git a/docs/decomposition/modules/module_dependency_register.md b/docs/decomposition/modules/module_dependency_register.md index c52f230..dac8013 100644 --- a/docs/decomposition/modules/module_dependency_register.md +++ b/docs/decomposition/modules/module_dependency_register.md @@ -13,11 +13,11 @@ Fleshed-out scope, contracts, and integration notes live in **per-module documen | Module ID | Module Name | Depends On | Contract Needed | Phase Required | Status | |---|---|---|---|---|---| | E1.M1 | InputAndMovementRuntime | None | MoveCommand, PositionState, InteractionRequest | Prototype | Ready | -| E1.M2 | IsometricCameraController | E1.M1 | CameraState, ZoomBandConfig, OcclusionPolicy | Prototype | In progress | +| E1.M2 | IsometricCameraController | E1.M1 | CameraState, ZoomBandConfig, OcclusionPolicy | Prototype | Ready | | E1.M3 | InteractionAndTargetingLayer | E1.M1 | TargetState, InteractableDescriptor, SelectionEvent | Prototype | Planned | | E1.M4 | AbilityInputScaffold | E1.M3, E5.M1 | AbilityCastRequest, HotbarLoadout, CooldownSnapshot | Prototype | Planned | -**E1.M2 note:** Client follow rig + per-tick `CameraState` + **discrete zoom bands** (`ZoomBandConfig`, [NEON-26](https://neon-sprawl.atlassian.net/browse/NEON-26)) + **`OcclusionPolicy`** RayCast fade ([NEON-27](https://neon-sprawl.atlassian.net/browse/NEON-27)) are in repo ([NEON-25](https://neon-sprawl.atlassian.net/browse/NEON-25)–[NEON-27](https://neon-sprawl.atlassian.net/browse/NEON-27)). Integration hardening remains ([NEON-28](https://neon-sprawl.atlassian.net/browse/NEON-28)). +**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`** ([NEON-26](https://neon-sprawl.atlassian.net/browse/NEON-26)); **`OcclusionPolicy`** ([NEON-27](https://neon-sprawl.atlassian.net/browse/NEON-27)); pick-through **`"occluder"`** convention ([NEON-30](https://neon-sprawl.atlassian.net/browse/NEON-30)); consumer contract + occluder lifecycle hardening ([NEON-28](https://neon-sprawl.atlassian.net/browse/NEON-28)). See [E1_M2_IsometricCameraController.md](E1_M2_IsometricCameraController.md). ### Epic 2 — Skills and Progression Framework diff --git a/docs/plans/NEON-28-implementation-plan.md b/docs/plans/NEON-28-implementation-plan.md index 4fd3482..73bc2b0 100644 --- a/docs/plans/NEON-28-implementation-plan.md +++ b/docs/plans/NEON-28-implementation-plan.md @@ -39,15 +39,15 @@ Jira acceptance criteria (verbatim intent): -- [ ] **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**). +- [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 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). +- [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 @@ -109,3 +109,8 @@ Jira acceptance criteria (verbatim intent): - **`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, NEON-28 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. From f0ff5c9d770608ea1cf674aa5f71e6c701c69293 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Fri, 10 Apr 2026 22:20:51 -0400 Subject: [PATCH 04/12] NEON-28: fix occluder key check order for freed instances --- client/scripts/isometric_follow_camera.gd | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/client/scripts/isometric_follow_camera.gd b/client/scripts/isometric_follow_camera.gd index 3760af8..be9f128 100644 --- a/client/scripts/isometric_follow_camera.gd +++ b/client/scripts/isometric_follow_camera.gd @@ -227,8 +227,11 @@ static func occlusion_policy_is_valid(policy: Resource) -> bool: ## True if [param body] is a live [Node3D] suitable as an [member _occluder_overrides] key. ## Used before occlusion ray work and from tests (NEON-28). +## [code]is_instance_valid[/code] first: [code]is Node3D[/code] on a freed reference errors at runtime. static func occluder_override_key_is_valid(body: Variant) -> bool: - return body is Node3D and is_instance_valid(body) + if not is_instance_valid(body): + return false + return body is Node3D func _restore_occluder_overrides_list(overrides: Array) -> void: From 6340693b46c613d4161826aa3e1ff2945b5473aa Mon Sep 17 00:00:00 2001 From: VinPropane Date: Fri, 10 Apr 2026 22:23:50 -0400 Subject: [PATCH 05/12] NEON-28: run follow camera in physics after Player (fix jitter) --- client/README.md | 2 +- client/scripts/camera_state.gd | 5 +++-- client/scripts/isometric_follow_camera.gd | 6 ++++-- .../modules/E1_M2_IsometricCameraController.md | 6 +++--- docs/plans/NEON-28-implementation-plan.md | 2 ++ 5 files changed, 13 insertions(+), 8 deletions(-) diff --git a/client/README.md b/client/README.md index 6da0163..da926d8 100644 --- a/client/README.md +++ b/client/README.md @@ -13,7 +13,7 @@ Do not grow an all-in-one **`main.gd`**. The main scene root should **compose** ## Follow camera (NEON-25) -The main scene uses **`World/IsometricFollowCamera`** (`scripts/isometric_follow_camera.gd`): client-local **isometric follow** for **`Player`** (`NodePath` **`../Player`**), with damped eye motion (`follow_smoothness`, exponential lerp) and **teleport snap** when the eye is farther than **`snap_distance`** from the desired pose (e.g. after **`snap_to_server`**). Framing exports **`follow_distance`**, **`pitch_elevation_deg`**, and **`presentation_yaw_deg`** match the pre–NEON-25 static camera at spawn. **Orbit is off in the prototype:** **`allow_yaw`** is false, **`CameraState.yaw`** stays **0**, and no rotate input is read—enable **`allow_yaw`** later and drive **`_orbit_yaw_rad`** from input on the rig (see **`docs/decomposition/modules/E1_M2_IsometricCameraController.md`**). **`CameraState`** (`scripts/camera_state.gd`) is refreshed every frame on the rig. **`ground_pick`** uses the viewport camera; **`main.gd`** sets **`fallback_camera`** to **`World/IsometricFollowCamera/Camera3D`**. +The main scene uses **`World/IsometricFollowCamera`** (`scripts/isometric_follow_camera.gd`): client-local **isometric follow** for **`Player`** (`NodePath` **`../Player`**), with damped eye motion (`follow_smoothness`, exponential lerp) and **teleport snap** when the eye is farther than **`snap_distance`** from the desired pose (e.g. after **`snap_to_server`**). Framing exports **`follow_distance`**, **`pitch_elevation_deg`**, and **`presentation_yaw_deg`** match the pre–NEON-25 static camera at spawn. **Orbit is off in the prototype:** **`allow_yaw`** is false, **`CameraState.yaw`** stays **0**, and no rotate input is read—enable **`allow_yaw`** later and drive **`_orbit_yaw_rad`** from input on the rig (see **`docs/decomposition/modules/E1_M2_IsometricCameraController.md`**). **`CameraState`** (`scripts/camera_state.gd`) is refreshed every **`_physics_process`** tick on the rig (**`process_physics_priority`** after **`Player`**) so framing tracks **`move_and_slide`** without display-vs-physics jitter. **`ground_pick`** uses the viewport camera; **`main.gd`** sets **`fallback_camera`** to **`World/IsometricFollowCamera/Camera3D`**. ## Authoritative movement (NEON-4, NEON-8) diff --git a/client/scripts/camera_state.gd b/client/scripts/camera_state.gd index 2f2980e..a87f304 100644 --- a/client/scripts/camera_state.gd +++ b/client/scripts/camera_state.gd @@ -1,7 +1,8 @@ extends RefCounted -## Client-local **per-frame** snapshot from the follow rig (`isometric_follow_camera.gd`). -## The rig creates **one** instance and assigns fields every `_process` after pose is final. +## Client-local snapshot from the follow rig (`isometric_follow_camera.gd`). +## The rig creates **one** instance and assigns fields every **`_physics_process`** tick after +## the follow target has moved (rig uses `process_physics_priority` after `Player`). ## No `class_name` — see repo Godot headless / CI notes. ## ## **Consumer contract (NEON-28):** Risk UX / HUD may read this object from the rig’s diff --git a/client/scripts/isometric_follow_camera.gd b/client/scripts/isometric_follow_camera.gd index be9f128..3355431 100644 --- a/client/scripts/isometric_follow_camera.gd +++ b/client/scripts/isometric_follow_camera.gd @@ -1,6 +1,6 @@ extends Node3D -## NEON-25: client-local isometric follow; updates [CameraState] each `_process`. +## NEON-25: client-local isometric follow; updates [CameraState] each physics tick. ## NEON-26: discrete zoom bands via [member zoom_band_config]; wheel / `camera_zoom_*` actions. ## NEON-28: purge freed occluder keys from [member _occluder_overrides] before ray pass (see ## [method occluder_override_key_is_valid]). @@ -55,6 +55,8 @@ var _occluder_overrides: Dictionary = {} func _ready() -> void: + # After `Player` (default priority 0): read `global_position` post–`move_and_slide` this tick. + process_physics_priority = 1 _state = CameraStateScript.new() _init_zoom_band_index() if camera == null: @@ -80,7 +82,7 @@ func _ready() -> void: _warn_missing_follow_target_once() -func _process(delta: float) -> void: +func _physics_process(delta: float) -> void: if camera == null or _state == null: _restore_all_occluders() return diff --git a/docs/decomposition/modules/E1_M2_IsometricCameraController.md b/docs/decomposition/modules/E1_M2_IsometricCameraController.md index 3b43981..74d3f8e 100644 --- a/docs/decomposition/modules/E1_M2_IsometricCameraController.md +++ b/docs/decomposition/modules/E1_M2_IsometricCameraController.md @@ -49,7 +49,7 @@ Single place for dependents (e.g. **E6.M2**): what is on `CameraState` vs what s ### `CameraState` fields (`client/scripts/camera_state.gd`) -Refreshed **every `_process`** on `IsometricFollowCamera` after pose is final. Same object each frame (rig-owned). +Refreshed **every `_physics_process`** tick on `IsometricFollowCamera` after the follow target has moved (`process_physics_priority` after `Player`). Same object each tick (rig-owned). | Field | Meaning | |--------|---------| @@ -87,13 +87,13 @@ See Epic 1 **Slice 2 — Locked isometric camera**: follow-center, zoom bands, o - **Godot:** `client/scripts/isometric_follow_camera.gd` on **`World/IsometricFollowCamera`**; child **`Camera3D`** (current). **`scripts/camera_state.gd`** — `RefCounted` tick snapshot (`yaw` = orbit component, **0** while **`allow_yaw`** is false; **`distance`** = effective follow distance; **`zoom_band_index`**; **`focus_world`**, **`follow_target_path`**). - **Seam:** **`allow_yaw`** / **`max_yaw_deg`** on the rig; orbit would adjust **`_orbit_yaw_rad`** (no input bound yet). **Presentation** compass uses **`presentation_yaw_deg`** separately from orbit **`yaw`** in state. - **Zoom:** **`ZoomBandConfig`** resource (`client/scripts/zoom_band_config.gd`); default **`client/resources/isometric_zoom_bands.tres`** on main rig. Bands are **distance-only** (ascending near→far); **pitch / FOV** unchanged vs NEON-25. Input: **`camera_zoom_in`** / **`camera_zoom_out`** in `client/project.godot` (mouse wheel + `=` / `-` + keypad **+** / **−**). **`follow_distance`** remains **fallback** when config is null or has no bands; state **`zoom_band_index`** is **0** in that case. **`zoom_band_changed`** signal on rig; **TODO(E9.M1)** for throttled `camera_zoom_changed` telemetry. -- **Occlusion:** **`OcclusionPolicy`** resource (`client/scripts/occlusion_policy.gd`); default **`client/resources/isometric_occlusion_policy.tres`** on main rig. **Technique:** each `_process` frame, iterative `intersect_ray` from `_smoothed_eye` to player focus (up to `max_occluder_cast_depth` calls). Bodies tagged `"occluder"` that intersect the ray have their `MeshInstance3D` surfaces overridden with a transparent `StandardMaterial3D` copy (`fade_alpha = 0.25`). Materials restored instantly when the body clears the ray. **Null-material surfaces** (no mesh or override material set) receive a plain transparent `StandardMaterial3D`; other non-`StandardMaterial3D` surfaces are skipped with `push_warning`. **Prototype district:** `Obstacle` node in `main.tscn` tagged `"occluder"`. **Perf marker:** `occluder_count_log_threshold` export (default 0 = disabled) emits `push_warning` for stress-test reference. See [risks and telemetry](#risks-and-telemetry) for readability gate. +- **Occlusion:** **`OcclusionPolicy`** resource (`client/scripts/occlusion_policy.gd`); default **`client/resources/isometric_occlusion_policy.tres`** on main rig. **Technique:** each **`_physics_process`** tick, iterative `intersect_ray` from `_smoothed_eye` to player focus (up to `max_occluder_cast_depth` calls). Bodies tagged `"occluder"` that intersect the ray have their `MeshInstance3D` surfaces overridden with a transparent `StandardMaterial3D` copy (`fade_alpha = 0.25`). Materials restored instantly when the body clears the ray. **Null-material surfaces** (no mesh or override material set) receive a plain transparent `StandardMaterial3D`; other non-`StandardMaterial3D` surfaces are skipped with `push_warning`. **Prototype district:** `Obstacle` node in `main.tscn` tagged `"occluder"`. **Perf marker:** `occluder_count_log_threshold` export (default 0 = disabled) emits `push_warning` for stress-test reference. See [risks and telemetry](#risks-and-telemetry) for readability gate. > **Prototype demo readability gate:** before shipping the prototype demo, the PR for NEON-27 must include a before/after screenshot or clip demonstrating that the player is no longer fully hidden by the `Obstacle`. This serves as the first data point for the occlusion-hiding-telegraphs risk documented below. - **Click-through input (NEON-30):** `scripts/ground_pick.gd` unconditionally skips bodies in the `"occluder"` group during click-to-move ground-pick raycasts. When a cast hits an occluder the ray origin advances `OCCLUDER_PICK_THROUGH` meters past the hit point and continues, independent of whether `OcclusionPolicy` is currently fading that body. This keeps the `"occluder"` group as the sole tagging convention shared between the camera occlusion system and ground-pick input — no dedicated collision layer is added for this case. -- **Integration hardening (NEON-28):** [Consumer contract](#consumer-contract-neon-28) + `camera_state.gd` header; **E6.M2** adjacency notes ([E6_M2_ConsentAndRiskUxSignals.md](E6_M2_ConsentAndRiskUxSignals.md)); invalid / freed occluder bodies are **purged** from `_occluder_overrides` before each occlusion ray pass and before full restore (`occluder_override_key_is_valid`, tests in `isometric_follow_camera_test.gd`). +- **Integration hardening (NEON-28):** [Consumer contract](#consumer-contract-neon-28) + `camera_state.gd` header; **E6.M2** adjacency notes ([E6_M2_ConsentAndRiskUxSignals.md](E6_M2_ConsentAndRiskUxSignals.md)); invalid / freed occluder bodies are **purged** from `_occluder_overrides` before each occlusion ray pass and before full restore (`occluder_override_key_is_valid`, tests in `isometric_follow_camera_test.gd`). **Follow / state / occlusion** run in **`_physics_process`** with **`process_physics_priority = 1`** so the rig updates **after** `Player` (default **0**) and tracks **`move_and_slide`** without display-vs-physics jitter. ## Jira backlog diff --git a/docs/plans/NEON-28-implementation-plan.md b/docs/plans/NEON-28-implementation-plan.md index 73bc2b0..a5e6050 100644 --- a/docs/plans/NEON-28-implementation-plan.md +++ b/docs/plans/NEON-28-implementation-plan.md @@ -78,6 +78,7 @@ Jira acceptance criteria (verbatim intent): | **`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.” | +| **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). | ## Files to add @@ -114,3 +115,4 @@ Jira acceptance criteria (verbatim intent): - `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, NEON-28 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**). From 0792da702aaa6a0f0f88c6f1e8e58ae5499d3136 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Fri, 10 Apr 2026 22:27:27 -0400 Subject: [PATCH 06/12] chore: disable ASP.NET HttpLogging; update client project.godot --- client/project.godot | 7 +++---- server/NeonSprawl.Server/Program.cs | 17 ----------------- .../appsettings.Development.json | 3 +-- 3 files changed, 4 insertions(+), 23 deletions(-) diff --git a/client/project.godot b/client/project.godot index 054fc3c..d709293 100644 --- a/client/project.godot +++ b/client/project.godot @@ -46,14 +46,14 @@ interact={ } camera_zoom_in={ "deadzone": 0.5, -"events": [Object(InputEventMouseButton,"resource_local_to_scene":false,"resource_name":"","device":-1,"window_id":0,"alt_pressed":false,"shift_pressed":false,"ctrl_pressed":false,"meta_pressed":false,"button_mask":0,"position":Vector2(0, 0),"global_position":Vector2(0, 0),"button_index":4,"factor":1.0,"pressed":false,"canceled":false,"double_click":false,"script":null) +"events": [Object(InputEventMouseButton,"resource_local_to_scene":false,"resource_name":"","device":-1,"window_id":0,"alt_pressed":false,"shift_pressed":false,"ctrl_pressed":false,"meta_pressed":false,"button_mask":0,"position":Vector2(0, 0),"global_position":Vector2(0, 0),"factor":1.0,"button_index":4,"canceled":false,"pressed":false,"double_click":false,"script":null) , Object(InputEventKey,"resource_local_to_scene":false,"resource_name":"","device":-1,"window_id":0,"alt_pressed":false,"shift_pressed":false,"ctrl_pressed":false,"meta_pressed":false,"pressed":false,"keycode":61,"physical_keycode":61,"key_label":0,"unicode":0,"location":0,"echo":false,"script":null) , Object(InputEventKey,"resource_local_to_scene":false,"resource_name":"","device":-1,"window_id":0,"alt_pressed":false,"shift_pressed":false,"ctrl_pressed":false,"meta_pressed":false,"pressed":false,"keycode":4194437,"physical_keycode":4194437,"key_label":0,"unicode":0,"location":0,"echo":false,"script":null) ] } camera_zoom_out={ "deadzone": 0.5, -"events": [Object(InputEventMouseButton,"resource_local_to_scene":false,"resource_name":"","device":-1,"window_id":0,"alt_pressed":false,"shift_pressed":false,"ctrl_pressed":false,"meta_pressed":false,"button_mask":0,"position":Vector2(0, 0),"global_position":Vector2(0, 0),"button_index":5,"factor":1.0,"pressed":false,"canceled":false,"double_click":false,"script":null) +"events": [Object(InputEventMouseButton,"resource_local_to_scene":false,"resource_name":"","device":-1,"window_id":0,"alt_pressed":false,"shift_pressed":false,"ctrl_pressed":false,"meta_pressed":false,"button_mask":0,"position":Vector2(0, 0),"global_position":Vector2(0, 0),"factor":1.0,"button_index":5,"canceled":false,"pressed":false,"double_click":false,"script":null) , Object(InputEventKey,"resource_local_to_scene":false,"resource_name":"","device":-1,"window_id":0,"alt_pressed":false,"shift_pressed":false,"ctrl_pressed":false,"meta_pressed":false,"pressed":false,"keycode":45,"physical_keycode":45,"key_label":0,"unicode":0,"location":0,"echo":false,"script":null) , Object(InputEventKey,"resource_local_to_scene":false,"resource_name":"","device":-1,"window_id":0,"alt_pressed":false,"shift_pressed":false,"ctrl_pressed":false,"meta_pressed":false,"pressed":false,"keycode":4194435,"physical_keycode":4194435,"key_label":0,"unicode":0,"location":0,"echo":false,"script":null) ] @@ -61,9 +61,8 @@ camera_zoom_out={ [physics] -3d/physics_engine="Jolt Physics" common/physics_ticks_per_second=120 -common/physics_interpolation=false +3d/physics_engine="Jolt Physics" [rendering] diff --git a/server/NeonSprawl.Server/Program.cs b/server/NeonSprawl.Server/Program.cs index be4ae6a..5caf2d0 100644 --- a/server/NeonSprawl.Server/Program.cs +++ b/server/NeonSprawl.Server/Program.cs @@ -1,28 +1,11 @@ -using Microsoft.AspNetCore.HttpLogging; using NeonSprawl.Server.Game.Interaction; using NeonSprawl.Server.Game.PositionState; var builder = WebApplication.CreateBuilder(args); builder.Services.AddPositionStateStore(builder.Configuration); -if (builder.Environment.IsDevelopment()) -{ - builder.Services.AddHttpLogging(o => - { - o.LoggingFields = HttpLoggingFields.RequestMethod - | HttpLoggingFields.RequestPath - | HttpLoggingFields.ResponseStatusCode - | HttpLoggingFields.Duration; - }); -} - var app = builder.Build(); -if (app.Environment.IsDevelopment()) -{ - app.UseHttpLogging(); -} - app.MapGet("/", () => Results.Text( "Neon Sprawl game server — GET /health for JSON status.", "text/plain")); diff --git a/server/NeonSprawl.Server/appsettings.Development.json b/server/NeonSprawl.Server/appsettings.Development.json index 3e805ed..0c208ae 100644 --- a/server/NeonSprawl.Server/appsettings.Development.json +++ b/server/NeonSprawl.Server/appsettings.Development.json @@ -2,8 +2,7 @@ "Logging": { "LogLevel": { "Default": "Information", - "Microsoft.AspNetCore": "Warning", - "Microsoft.AspNetCore.HttpLogging.HttpLoggingMiddleware": "Information" + "Microsoft.AspNetCore": "Warning" } } } From e225f1c550f604daedb34f7728c0611297ce7a6e Mon Sep 17 00:00:00 2001 From: VinPropane Date: Fri, 10 Apr 2026 22:53:36 -0400 Subject: [PATCH 07/12] NEON-28: sync plan with review (physics tick, occluder handoff) --- docs/plans/NEON-28-implementation-plan.md | 6 ++- docs/reviews/2026-04-10-NEON-28.md | 50 +++++++++++++++++++++++ 2 files changed, 54 insertions(+), 2 deletions(-) create mode 100644 docs/reviews/2026-04-10-NEON-28.md diff --git a/docs/plans/NEON-28-implementation-plan.md b/docs/plans/NEON-28-implementation-plan.md index a5e6050..ebaaeba 100644 --- a/docs/plans/NEON-28-implementation-plan.md +++ b/docs/plans/NEON-28-implementation-plan.md @@ -52,7 +52,7 @@ Jira acceptance criteria (verbatim intent): ## 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 `_process`** on the rig. + - **`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 **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. @@ -104,7 +104,9 @@ Jira acceptance criteria (verbatim intent): | `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; free an occluder while faded and confirm no errors / stuck materials; spot-check Slice 2 behaviors (readable motion, occlusion active). +**Manual verification:** Main scene — zoom bands, occlusion fade, fixed yaw UX; spot-check Slice 2 behaviors (readable motion, occlusion active). + +**Freed occluder (end-to-end, editor):** Position player so `Obstacle` is between camera and player (fade active), then `queue_free` the occluder (remote inspector or one-shot script). Expect **no** recurring script errors and **no** stuck transparent materials on other meshes. **Smoke-tested on the story branch in-editor (2026-04-10)** before merge handoff — complements static `occluder_override_key_is_valid` tests, which do not execute the full purge path in CI. ## Open questions / risks diff --git a/docs/reviews/2026-04-10-NEON-28.md b/docs/reviews/2026-04-10-NEON-28.md new file mode 100644 index 0000000..4d6a79c --- /dev/null +++ b/docs/reviews/2026-04-10-NEON-28.md @@ -0,0 +1,50 @@ +# Code review — NEON-28 (Camera integration hardening) + +**Date:** 2026-04-10 +**Scope:** Branch `NEON-28-camera-integration-hardening` against `origin/main`. +**Base:** `origin/main` (merge-base `01cfd4ec4d8f31a15bbaa0429e46ef0d2ddea4c8`). + +## Verdict + +**Approve with nits** — The camera hardening and contract/doc updates line up well with NEON-28 and E1.M2 intent, and I did not find a correctness break in the reviewed camera path. ~~Clean up the remaining plan/scope mismatches before merge so the branch tells one consistent story.~~ **Plan Technical approach + manual occluder note addressed post-review; HttpLogging chore left on branch per author (suggestion 2 declined).** + +## Summary + +This branch closes the E1.M2 prototype slice in a coherent way: `CameraState` is documented as a consumer-facing snapshot, E1.M2/E6.M2/module-register docs now agree on what is and is not part of that contract, and the occluder hardening avoids touching freed keys before restore. Moving the rig update to `_physics_process` with `process_physics_priority = 1` is a sensible follow-up to the jitter concern and matches the updated module documentation. Overall risk is low in the camera code itself. ~~The main remaining concerns are documentation drift inside the NEON-28 plan and an extra server observability change that is not tied back to the story scope.~~ **Resolved / noted:** plan drift fixed; server logging change acknowledged as intentionally bundled. + +## Documentation checked + +| Document | Assessment | +|----------|------------| +| `docs/plans/NEON-28-implementation-plan.md` | **Matches** — Scope, decisions, shipped notes, acceptance checklist, and Technical approach (`_physics_process` + priority) align with the branch. | +| `docs/decomposition/modules/E1_M2_IsometricCameraController.md` | **Matches** — Consumer contract, E1.M2 Ready status, yaw semantics, occlusion hardening, and the `_physics_process` update all reflect the implementation. | +| `docs/decomposition/modules/E6_M2_ConsentAndRiskUxSignals.md` | **Matches** — Camera-adjacent consumer guidance and authority split are consistent with the code and E1.M2 doc. | +| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E1.M2 is marked Ready with appropriate script/scene pointers. | +| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E1.M2 tracking row reflects NEON-28 completion; no extra tracking-table update appears needed after this merge. | +| `docs/decomposition/epics/epic_01_core_player_runtime.md` | **Matches** — Slice 2 language now reflects “yaw fixed in UX” rather than “camera never rotates,” which is the right contract. | +| `docs/decomposition/modules/client_server_authority.md` | **Matches** — Camera remains client-local presentation only; no server gameplay authority was added. | +| `docs/decomposition/modules/contracts.md` | **Matches** — E1.M2’s Ready status is reasonable for this prototype slice given the documented contract and consumer guidance. | + +## Blocking issues + +_None identified in the implementation reviewed._ + +## Suggestions + +1. ~~`docs/plans/NEON-28-implementation-plan.md`: update the Technical approach bullet that still says `CameraState` is refreshed every `_process`. The same document now records the shipped behavior as `_physics_process` plus `process_physics_priority = 1`, so leaving both versions in the plan makes the acceptance source internally inconsistent.~~ **Done.** +2. ~~`server/NeonSprawl.Server/Program.cs` and `server/NeonSprawl.Server/appsettings.Development.json`: either split the ASP.NET HttpLogging removal into a separate chore or record the reason in the NEON-28 plan/PR. It is not part of the documented camera-hardening scope, and it reduces local request-path observability during the manual client/server checks described elsewhere in the repo.~~ **Intentionally not applied** (author keeps HttpLogging removal on this branch without plan/PR carve-out). + +## Nits + +- ~~`client/test/isometric_follow_camera_test.gd`: the new helper tests are worthwhile, but the actual purge/restore path is still effectively manual-only. If that editor verification has already been done, note it explicitly in the PR/story handoff so readers know the freed-occluder scenario was exercised end-to-end.~~ **Done.** — See `docs/plans/NEON-28-implementation-plan.md` §Tests manual verification (freed occluder, 2026-04-10). + +## Verification + +- Reviewed `git diff origin/main...HEAD` for the branch, with focused checks on `client/scripts/isometric_follow_camera.gd`, `client/scripts/camera_state.gd`, `client/test/isometric_follow_camera_test.gd`, and the updated E1.M2/E6.M2/module docs. +- `dotnet build NeonSprawl.sln` could not complete in this session because `server/NeonSprawl.Server/bin/Debug/net10.0/NeonSprawl.Server.dll` was locked by a running `dotnet.exe` process. +- Godot / GdUnit client tests were not run in this session. +- Before merge, I would still run: `dotnet build NeonSprawl.sln` with the dev server stopped, then the documented camera manual checks in `client/README.md` plus the NEON-28 plan step that frees an occluder while it is faded. + +--- + +*Review produced per [code review agent](.cursor/rules/code-review-agent.md).* From b702b14be427c16112fe7b70aa03addda3e88e63 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Fri, 10 Apr 2026 23:09:12 -0400 Subject: [PATCH 08/12] NEON-28: drop freed occluder keys without material restore --- client/scripts/isometric_follow_camera.gd | 22 +++++++++++++------ .../E1_M2_IsometricCameraController.md | 2 +- docs/plans/NEON-28-implementation-plan.md | 6 ++--- 3 files changed, 19 insertions(+), 11 deletions(-) diff --git a/client/scripts/isometric_follow_camera.gd b/client/scripts/isometric_follow_camera.gd index 3355431..49c3d2c 100644 --- a/client/scripts/isometric_follow_camera.gd +++ b/client/scripts/isometric_follow_camera.gd @@ -238,20 +238,28 @@ static func occluder_override_key_is_valid(body: Variant) -> bool: func _restore_occluder_overrides_list(overrides: Array) -> void: for entry in overrides: - var mi: MeshInstance3D = entry["mesh"] - if is_instance_valid(mi): - mi.set_surface_override_material(entry["surface"], entry["original"]) + var mi = entry.get("mesh") + if not is_instance_valid(mi) or not mi is MeshInstance3D: + continue + var s: int = int(entry.get("surface", -1)) + if s < 0: + continue + var original: Variant = entry.get("original") + if original != null: + if typeof(original) != TYPE_OBJECT or not is_instance_valid(original): + continue + (mi as MeshInstance3D).set_surface_override_material(s, original) -## Drop invalid keys from [member _occluder_overrides] and restore any surviving meshes. +## Drop invalid keys from [member _occluder_overrides] without touching meshes. +## When the occluder [Node3D] is freed, its [MeshInstance3D] children and materials are gone or +## tearing down — restoring overrides here triggers "freed instance" errors. Erasing the entry is +## enough; live bodies stay on the valid path ([method _restore_occluder]). func _purge_invalid_occluder_override_keys() -> void: var keys: Array = _occluder_overrides.keys() for body in keys: if occluder_override_key_is_valid(body): continue - var overrides: Variant = _occluder_overrides.get(body) - if overrides != null: - _restore_occluder_overrides_list(overrides) _occluder_overrides.erase(body) diff --git a/docs/decomposition/modules/E1_M2_IsometricCameraController.md b/docs/decomposition/modules/E1_M2_IsometricCameraController.md index 74d3f8e..c956c7f 100644 --- a/docs/decomposition/modules/E1_M2_IsometricCameraController.md +++ b/docs/decomposition/modules/E1_M2_IsometricCameraController.md @@ -93,7 +93,7 @@ See Epic 1 **Slice 2 — Locked isometric camera**: follow-center, zoom bands, o - **Click-through input (NEON-30):** `scripts/ground_pick.gd` unconditionally skips bodies in the `"occluder"` group during click-to-move ground-pick raycasts. When a cast hits an occluder the ray origin advances `OCCLUDER_PICK_THROUGH` meters past the hit point and continues, independent of whether `OcclusionPolicy` is currently fading that body. This keeps the `"occluder"` group as the sole tagging convention shared between the camera occlusion system and ground-pick input — no dedicated collision layer is added for this case. -- **Integration hardening (NEON-28):** [Consumer contract](#consumer-contract-neon-28) + `camera_state.gd` header; **E6.M2** adjacency notes ([E6_M2_ConsentAndRiskUxSignals.md](E6_M2_ConsentAndRiskUxSignals.md)); invalid / freed occluder bodies are **purged** from `_occluder_overrides` before each occlusion ray pass and before full restore (`occluder_override_key_is_valid`, tests in `isometric_follow_camera_test.gd`). **Follow / state / occlusion** run in **`_physics_process`** with **`process_physics_priority = 1`** so the rig updates **after** `Player` (default **0**) and tracks **`move_and_slide`** without display-vs-physics jitter. +- **Integration hardening (NEON-28):** [Consumer contract](#consumer-contract-neon-28) + `camera_state.gd` header; **E6.M2** adjacency notes ([E6_M2_ConsentAndRiskUxSignals.md](E6_M2_ConsentAndRiskUxSignals.md)); invalid / freed occluder bodies are **dropped** from `_occluder_overrides` before each occlusion ray pass and before full restore (`occluder_override_key_is_valid`, tests in `isometric_follow_camera_test.gd`) — **no** material restore on that path (freed subtree would error); live bodies still restore via `_restore_occluder` when they leave the ray. **Follow / state / occlusion** run in **`_physics_process`** with **`process_physics_priority = 1`** so the rig updates **after** `Player` (default **0**) and tracks **`move_and_slide`** without display-vs-physics jitter. ## Jira backlog diff --git a/docs/plans/NEON-28-implementation-plan.md b/docs/plans/NEON-28-implementation-plan.md index ebaaeba..51e9ea6 100644 --- a/docs/plans/NEON-28-implementation-plan.md +++ b/docs/plans/NEON-28-implementation-plan.md @@ -28,7 +28,7 @@ **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; purge invalid keys and restore surviving `MeshInstance3D` overrides. +- **`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 Jira)** @@ -63,7 +63,7 @@ Jira acceptance criteria (verbatim intent): - 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): drop dictionary keys whose body is **`not is_instance_valid`**; restore materials where `MeshInstance3D` is still valid; then erase entries. + - 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. @@ -76,7 +76,7 @@ Jira acceptance criteria (verbatim intent): | Topic | Choice | Rationale | |-------|--------|-----------| | **`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. | +| **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 Jira AC 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). | From bb39a64ebd279494fa3503452fcba0a53660fc06 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Fri, 10 Apr 2026 23:11:01 -0400 Subject: [PATCH 09/12] NEON-28: rebake nav after F9 obstacle removal (dev smoke) --- client/scripts/main.gd | 25 +++++++++++++++++++++++ docs/plans/NEON-28-implementation-plan.md | 2 +- 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/client/scripts/main.gd b/client/scripts/main.gd index 57bc70b..bfe9e7a 100644 --- a/client/scripts/main.gd +++ b/client/scripts/main.gd @@ -67,3 +67,28 @@ func _on_move_rejected(reason_code: String) -> void: _move_reject_label.text = "", CONNECT_ONE_SHOT ) + + +## Dev smoke (NEON-28 occluder purge): **F9** frees `Obstacle` while testing; remove when no longer needed. +## After removing nav source geometry, we **rebake** — otherwise `NavigationAgent3D` keeps a stale map +## and click-to-move can stop working. +func _unhandled_key_input(event: InputEvent) -> void: + if not event is InputEventKey: + return + var k := event as InputEventKey + if k.pressed and not k.echo and k.keycode == KEY_F9: + var obstacle := get_node_or_null("World/NavigationRegion3D/Obstacle") + if obstacle: + obstacle.queue_free() + call_deferred("_rebake_nav_mesh_after_obstacle_removed") + + +func _rebake_nav_mesh_after_obstacle_removed() -> void: + _nav_region.bake_navigation_mesh(false) + await get_tree().physics_frame + await get_tree().physics_frame + if not is_instance_valid(_player): + return + var na := _player.get_node_or_null("NavigationAgent3D") as NavigationAgent3D + if na != null: + na.set_target_position(_player.global_position) diff --git a/docs/plans/NEON-28-implementation-plan.md b/docs/plans/NEON-28-implementation-plan.md index 51e9ea6..7f34acf 100644 --- a/docs/plans/NEON-28-implementation-plan.md +++ b/docs/plans/NEON-28-implementation-plan.md @@ -106,7 +106,7 @@ Jira acceptance criteria (verbatim intent): **Manual verification:** Main scene — zoom bands, occlusion fade, fixed yaw UX; spot-check Slice 2 behaviors (readable motion, occlusion active). -**Freed occluder (end-to-end, editor):** Position player so `Obstacle` is between camera and player (fade active), then `queue_free` the occluder (remote inspector or one-shot script). Expect **no** recurring script errors and **no** stuck transparent materials on other meshes. **Smoke-tested on the story branch in-editor (2026-04-10)** before merge handoff — complements static `occluder_override_key_is_valid` tests, which do not execute the full purge path in CI. +**Freed occluder (end-to-end, editor):** Position player so `Obstacle` is between camera and player (fade active), then `queue_free` the occluder. Expect **no** recurring script errors and **no** stuck transparent materials on other meshes. **`Obstacle` lives under `NavigationRegion3D`** — after removing it, **rebake** the nav mesh (see **`main.gd` F9** dev path: `bake_navigation_mesh(false)` + refresh `NavigationAgent3D` target) or click-to-move can stop until the next full scene reload. Complements static `occluder_override_key_is_valid` tests, which do not execute the full purge path in CI. ## Open questions / risks From 1ab3fc1dcafb6347fd41ddf68f9afa5e3669acc8 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Fri, 10 Apr 2026 23:13:00 -0400 Subject: [PATCH 10/12] NEON-28: fix F9 nav rebake coroutine + resync agent goal --- client/scripts/main.gd | 15 ++++++++++----- client/scripts/player.gd | 8 ++++++++ docs/plans/NEON-28-implementation-plan.md | 2 +- 3 files changed, 19 insertions(+), 6 deletions(-) diff --git a/client/scripts/main.gd b/client/scripts/main.gd index bfe9e7a..2459f8f 100644 --- a/client/scripts/main.gd +++ b/client/scripts/main.gd @@ -72,6 +72,8 @@ func _on_move_rejected(reason_code: String) -> void: ## Dev smoke (NEON-28 occluder purge): **F9** frees `Obstacle` while testing; remove when no longer needed. ## After removing nav source geometry, we **rebake** — otherwise `NavigationAgent3D` keeps a stale map ## and click-to-move can stop working. +## Coroutine is started **directly** from input (not [method call_deferred]): deferred + [code]await[/code] +## often never resumes, so the rebake never finished and the nav map stayed stale. func _unhandled_key_input(event: InputEvent) -> void: if not event is InputEventKey: return @@ -80,15 +82,18 @@ func _unhandled_key_input(event: InputEvent) -> void: var obstacle := get_node_or_null("World/NavigationRegion3D/Obstacle") if obstacle: obstacle.queue_free() - call_deferred("_rebake_nav_mesh_after_obstacle_removed") + _rebake_nav_after_obstacle_removed_async() -func _rebake_nav_mesh_after_obstacle_removed() -> void: +func _rebake_nav_after_obstacle_removed_async() -> void: + await get_tree().process_frame + if not is_instance_valid(_nav_region): + return _nav_region.bake_navigation_mesh(false) + # Let NavigationServer3D apply the rebaked mesh before repathing. + await get_tree().physics_frame await get_tree().physics_frame await get_tree().physics_frame if not is_instance_valid(_player): return - var na := _player.get_node_or_null("NavigationAgent3D") as NavigationAgent3D - if na != null: - na.set_target_position(_player.global_position) + _player.sync_navigation_agent_after_map_rebuild() diff --git a/client/scripts/player.gd b/client/scripts/player.gd index 0341bf7..ed694ad 100644 --- a/client/scripts/player.gd +++ b/client/scripts/player.gd @@ -89,6 +89,14 @@ func clear_nav_goal() -> void: _nav_agent.set_target_position(global_position) +## Repath on a new nav map (e.g. after [NavigationRegion3D] rebake when geometry is removed). +func sync_navigation_agent_after_map_rebuild() -> void: + if _has_walk_goal: + _nav_agent.set_target_position(_auth_walk_goal) + else: + _nav_agent.set_target_position(global_position) + + func snap_to_server(world_pos: Vector3) -> void: global_position = world_pos velocity = Vector3.ZERO diff --git a/docs/plans/NEON-28-implementation-plan.md b/docs/plans/NEON-28-implementation-plan.md index 7f34acf..0662bda 100644 --- a/docs/plans/NEON-28-implementation-plan.md +++ b/docs/plans/NEON-28-implementation-plan.md @@ -106,7 +106,7 @@ Jira acceptance criteria (verbatim intent): **Manual verification:** Main scene — zoom bands, occlusion fade, fixed yaw UX; spot-check Slice 2 behaviors (readable motion, occlusion active). -**Freed occluder (end-to-end, editor):** Position player so `Obstacle` is between camera and player (fade active), then `queue_free` the occluder. Expect **no** recurring script errors and **no** stuck transparent materials on other meshes. **`Obstacle` lives under `NavigationRegion3D`** — after removing it, **rebake** the nav mesh (see **`main.gd` F9** dev path: `bake_navigation_mesh(false)` + refresh `NavigationAgent3D` target) or click-to-move can stop until the next full scene reload. Complements static `occluder_override_key_is_valid` tests, which do not execute the full purge path in CI. +**Freed occluder (end-to-end, editor):** Position player so `Obstacle` is between camera and player (fade active), then `queue_free` the occluder. Expect **no** recurring script errors and **no** stuck transparent materials on other meshes. **`Obstacle` lives under `NavigationRegion3D`** — after removing it, **rebake** the nav mesh (see **`main.gd` F9**: `await` a frame after `queue_free`, then `bake_navigation_mesh(false)`, then a few physics frames, then **`Player.sync_navigation_agent_after_map_rebuild()`**) or click-to-move can stop. Do **not** drive that sequence with **`call_deferred` + `await`** on the same method — the coroutine may never resume. Complements static `occluder_override_key_is_valid` tests, which do not execute the full purge path in CI. ## Open questions / risks From 398317d64f9a6f92bc35deb64480c13f1414ca51 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Fri, 10 Apr 2026 23:55:50 -0400 Subject: [PATCH 11/12] NEON-28: Dev obstacle smoke, nav reparent on hide, and docs - Input action dev_toggle_occluder_obstacle (Ctrl+Shift+K): toggle obstacle off nav source via reparent to World, rebake NavigationRegion3D, sync NavigationAgent3D; physics/process/collision hardening unchanged intent. - Occluder dictionary keys by instance id; related client fixes and README/project input note. - Plan and review: PARSED_GEOMETRY_BOTH / runtime geometry removal lesson for future mechanics. --- client/README.md | 2 + client/project.godot | 5 ++ client/scripts/ground_pick.gd | 4 + client/scripts/isometric_follow_camera.gd | 73 ++++++++++------ client/scripts/main.gd | 93 ++++++++++++++++----- client/scripts/player.gd | 11 ++- client/scripts/position_authority_client.gd | 37 ++++++++ docs/plans/NEON-28-implementation-plan.md | 8 +- docs/reviews/2026-04-10-NEON-28.md | 10 ++- 9 files changed, 194 insertions(+), 49 deletions(-) diff --git a/client/README.md b/client/README.md index da926d8..b371d21 100644 --- a/client/README.md +++ b/client/README.md @@ -118,6 +118,8 @@ On **Linux** (including GitHub Actions), the path must be **`gdUnit4`** with a c **Camera zoom:** Input actions **`camera_zoom_in`** / **`camera_zoom_out`** (mouse wheel, **`=`** / **`-`**, keypad **+** / **−**) are defined in **`project.godot`**. On layouts where **`+`** requires **Shift**, remap or add a binding under **Project → Project Settings → Input Map** if needed. +**Dev (NEON-28):** **`dev_toggle_occluder_obstacle`** (default **Ctrl+Shift+K**) toggles the prototype `Obstacle` visibility/collision in **`main.gd`**. **F9** / **F10** are used by the embedded debugger; use this action instead when the game is running in the editor. + **Git hook (recommended):** install the repo’s local **pre-push** GDScript lint hook from the repo root: ```bash diff --git a/client/project.godot b/client/project.godot index d709293..fdae86a 100644 --- a/client/project.godot +++ b/client/project.godot @@ -58,6 +58,11 @@ camera_zoom_out={ , Object(InputEventKey,"resource_local_to_scene":false,"resource_name":"","device":-1,"window_id":0,"alt_pressed":false,"shift_pressed":false,"ctrl_pressed":false,"meta_pressed":false,"pressed":false,"keycode":4194435,"physical_keycode":4194435,"key_label":0,"unicode":0,"location":0,"echo":false,"script":null) ] } +dev_toggle_occluder_obstacle={ +"deadzone": 0.5, +"events": [Object(InputEventKey,"resource_local_to_scene":false,"resource_name":"","device":-1,"window_id":0,"alt_pressed":false,"shift_pressed":true,"ctrl_pressed":true,"meta_pressed":false,"pressed":false,"keycode":75,"physical_keycode":75,"key_label":0,"unicode":107,"location":0,"echo":false,"script":null) +] +} [physics] diff --git a/client/scripts/ground_pick.gd b/client/scripts/ground_pick.gd index 7b2188b..3f48957 100644 --- a/client/scripts/ground_pick.gd +++ b/client/scripts/ground_pick.gd @@ -27,6 +27,10 @@ const WALL_NORMAL_DOT_CUTOFF: float = 0.09 var fallback_camera: Camera3D +func _ready() -> void: + set_process_input(true) + + func _input(event: InputEvent) -> void: if event is InputEventMouseButton: var mb := event as InputEventMouseButton diff --git a/client/scripts/isometric_follow_camera.gd b/client/scripts/isometric_follow_camera.gd index 49c3d2c..b792497 100644 --- a/client/scripts/isometric_follow_camera.gd +++ b/client/scripts/isometric_follow_camera.gd @@ -2,8 +2,9 @@ extends Node3D ## NEON-25: client-local isometric follow; updates [CameraState] each physics tick. ## NEON-26: discrete zoom bands via [member zoom_band_config]; wheel / `camera_zoom_*` actions. -## NEON-28: purge freed occluder keys from [member _occluder_overrides] before ray pass (see -## [method occluder_override_key_is_valid]). +## NEON-28: occluder overrides are keyed by [method Object.get_instance_id] only — never [Object] +## keys, or [method Dictionary.keys] / [method Dictionary.erase] can freeze after an occluder is +## [method Node.queue_free]'d (dev smoke: [InputMap] [code]dev_toggle_occluder_obstacle[/code] in [code]main.gd[/code]). ## Pitch / roll are fixed by presentation exports; **orbit yaw** in state stays **0** while ## [member allow_yaw] is false (no rotate input bound). Future orbit: read relative input here, ## add to `_orbit_yaw_rad`, clamp with [member max_yaw_deg]. @@ -48,7 +49,8 @@ var _smoothed_eye: Vector3 = Vector3.ZERO var _orbit_yaw_rad: float = 0.0 var _zoom_band_index: int = 0 var _warned_missing_follow_target: bool = false -## Keys: Node3D occluder bodies. Values: Array of {mesh, surface, original} restore entries. +## Keys: [code]int[/code] [method Object.get_instance_id] of the occluder body. Values: Array of +## {mesh, surface, original} restore entries. var _occluder_overrides: Dictionary = {} @onready var camera: Camera3D = $Camera3D @@ -251,16 +253,18 @@ func _restore_occluder_overrides_list(overrides: Array) -> void: (mi as MeshInstance3D).set_surface_override_material(s, original) -## Drop invalid keys from [member _occluder_overrides] without touching meshes. -## When the occluder [Node3D] is freed, its [MeshInstance3D] children and materials are gone or -## tearing down — restoring overrides here triggers "freed instance" errors. Erasing the entry is -## enough; live bodies stay on the valid path ([method _restore_occluder]). +## Drop stale entries (freed nodes or non-int legacy keys) without touching meshes. func _purge_invalid_occluder_override_keys() -> void: - var keys: Array = _occluder_overrides.keys() - for body in keys: - if occluder_override_key_is_valid(body): + var next: Dictionary = {} + for k in _occluder_overrides.keys(): + if typeof(k) != TYPE_INT: continue - _occluder_overrides.erase(body) + var occ_id: int = k as int + var node := instance_from_id(occ_id) + if not is_instance_valid(node) or not node is Node3D: + continue + next[occ_id] = _occluder_overrides[k] + _occluder_overrides = next ## Cast iterative rays from the smoothed eye to the player focus. @@ -302,14 +306,25 @@ func _update_occlusion(focus: Vector3) -> void: ) ) - var to_restore: Array = _occluder_overrides.keys() - for body in to_restore: - if body not in newly_blocking: - _restore_occluder(body) + var blocking_ids: Dictionary = {} + for body in newly_blocking: + if occluder_override_key_is_valid(body): + blocking_ids[(body as Node3D).get_instance_id()] = true + + for occ_id in _occluder_overrides.keys(): + if typeof(occ_id) != TYPE_INT: + continue + var id: int = occ_id as int + if blocking_ids.has(id): + continue + _restore_occluder_id(id) for body in newly_blocking: - if body not in _occluder_overrides: - _apply_occluder_fade(body) + if not occluder_override_key_is_valid(body): + continue + var bid: int = (body as Node3D).get_instance_id() + if not _occluder_overrides.has(bid): + _apply_occluder_fade(body as Node3D) ## Apply per-surface fade overrides to all [MeshInstance3D] children of [param body]. @@ -344,19 +359,27 @@ func _apply_occluder_fade(body: Node3D) -> void: faded.albedo_color.a = policy.fade_alpha mi.set_surface_override_material(s, faded) overrides.append({"mesh": mi, "surface": s, "original": saved}) - _occluder_overrides[body] = overrides + _occluder_overrides[body.get_instance_id()] = overrides -## Restore saved surface override materials on [param body] and remove it from tracking. -func _restore_occluder(body: Node3D) -> void: - if not _occluder_overrides.has(body): +## Restore saved surface override materials for occluder [param occ_id] and remove it from tracking. +func _restore_occluder_id(occ_id: int) -> void: + if not _occluder_overrides.has(occ_id): return - _restore_occluder_overrides_list(_occluder_overrides[body]) - _occluder_overrides.erase(body) + _restore_occluder_overrides_list(_occluder_overrides[occ_id]) + _occluder_overrides.erase(occ_id) ## Restore all currently faded occluders (called on policy disable or node exit). func _restore_all_occluders() -> void: _purge_invalid_occluder_override_keys() - for body in _occluder_overrides.keys(): - _restore_occluder(body) + for occ_id in _occluder_overrides.keys(): + if typeof(occ_id) == TYPE_INT: + _restore_occluder_id(occ_id as int) + + +## Public hook for [code]dev_toggle_occluder_obstacle[/code] smoke ([code]main.gd[/code]): restore materials and clear [member _occluder_overrides] **while bodies +## are still live**, then the caller may [method Node.queue_free] the occluder. Avoids any post-free +## occlusion pass touching a tearing-down [StaticBody3D] (the semi-transparent faded box case). +func restore_all_occluder_materials_now() -> void: + _restore_all_occluders() diff --git a/client/scripts/main.gd b/client/scripts/main.gd index 2459f8f..6a4b183 100644 --- a/client/scripts/main.gd +++ b/client/scripts/main.gd @@ -13,7 +13,13 @@ const MOVE_REJECT_MSG_SECONDS: float = 4.0 ## Bump on each rejection so older one-shot timers do not clear a newer message. var _move_reject_msg_token: int = 0 +## [InputMap] [code]dev_toggle_occluder_obstacle[/code] (default **Ctrl+Shift+K**): restored when the obstacle is shown again. +var _dev_saved_obstacle_layer: int = -1 +var _dev_saved_obstacle_mask: int = -1 +var _dev_saved_obstacle_process_mode: Node.ProcessMode = Node.PROCESS_MODE_INHERIT + @onready var _camera: Camera3D = $World/IsometricFollowCamera/Camera3D +@onready var _world: Node3D = $World @onready var _nav_region: NavigationRegion3D = $World/NavigationRegion3D @onready var _player: CharacterBody3D = $World/Player @onready var _ground_pick: Node3D = $GroundPick @@ -22,9 +28,14 @@ var _move_reject_msg_token: int = 0 @onready var _move_reject_label: Label = $UICanvas/MoveRejectLabel @onready var _floor: StaticBody3D = $World/NavigationRegion3D/Floor +## Cached `Obstacle` body (path breaks after reparent for nav bake; do not use a fixed [NodePath]). +var _dev_obstacle_smoke: Node3D + func _ready() -> void: + set_process_unhandled_key_input(true) await get_tree().process_frame + _dev_obstacle_smoke = get_node_or_null("World/NavigationRegion3D/Obstacle") as Node3D # Runtime load + .call avoids class_name / preload static resolution issues across Godot parses. load("res://scripts/random_floor_bumps.gd").call("spawn_short_random_bumps", _floor) # Default `bake_navigation_mesh()` bakes on a background thread; sync bake ensures the map @@ -69,31 +80,71 @@ func _on_move_rejected(reason_code: String) -> void: ) -## Dev smoke (NEON-28 occluder purge): **F9** frees `Obstacle` while testing; remove when no longer needed. -## After removing nav source geometry, we **rebake** — otherwise `NavigationAgent3D` keeps a stale map -## and click-to-move can stop working. -## Coroutine is started **directly** from input (not [method call_deferred]): deferred + [code]await[/code] -## often never resumes, so the rebake never finished and the nav map stayed stale. +## Dev smoke (NEON-28 occluder): input action [code]dev_toggle_occluder_obstacle[/code] (default **Ctrl+Shift+K**) +## toggles `Obstacle` **visibility + collision** — it does **not** call [method Node.queue_free]. +## Uses [member Node.process_mode] [code]PROCESS_MODE_DISABLED[/code] so the [StaticBody3D] is **removed +## from the physics simulation** (default [member CollisionObject3D.disable_mode] is [code]REMOVE[/code]). +## Layer/shape tweaks alone left a **Jolt ghost collider** (invisible wall) in practice. +## When hidden, reparents `Obstacle` under [member _world]: [NavigationMesh] source geometry scans +## [NavigationRegion3D] children only ([code]SOURCE_GEOMETRY_ROOT_NODE_CHILDREN[/code]), and the +## default [code]geometry_parsed_geometry_type[/code] is [code]PARSED_GEOMETRY_BOTH[/code], so a +## child [MeshInstance3D] still carved a hole even with collision disabled — rebake alone was not enough. +## After each toggle, rebakes (main thread) and syncs the player [NavigationAgent3D]. +## **F9** / **F10** are debugger shortcuts in the embedded game; use the Input Map action. +## For a true **freed-node** occluder test, reload the scene or remove the body in the editor. func _unhandled_key_input(event: InputEvent) -> void: - if not event is InputEventKey: + if not event.is_action_pressed("dev_toggle_occluder_obstacle"): return - var k := event as InputEventKey - if k.pressed and not k.echo and k.keycode == KEY_F9: - var obstacle := get_node_or_null("World/NavigationRegion3D/Obstacle") - if obstacle: - obstacle.queue_free() - _rebake_nav_after_obstacle_removed_async() + call_deferred("_dev_toggle_obstacle_smoke_deferred") -func _rebake_nav_after_obstacle_removed_async() -> void: - await get_tree().process_frame +func _dev_toggle_obstacle_smoke_deferred() -> void: + var obstacle := _dev_obstacle_smoke + if obstacle == null or not is_instance_valid(obstacle): + return + var rig: Node = get_node_or_null("World/IsometricFollowCamera") + if obstacle.visible: + if rig != null and rig.has_method("restore_all_occluder_materials_now"): + rig.call("restore_all_occluder_materials_now") + _dev_saved_obstacle_process_mode = obstacle.process_mode + var co := obstacle as CollisionObject3D + if co != null: + _dev_saved_obstacle_layer = co.collision_layer + _dev_saved_obstacle_mask = co.collision_mask + co.collision_layer = 0 + co.collision_mask = 0 + _dev_collision_shapes_set_disabled(obstacle, true) + obstacle.process_mode = Node.PROCESS_MODE_DISABLED + obstacle.visible = false + if obstacle.get_parent() == _nav_region: + obstacle.reparent(_world) + else: + if obstacle.get_parent() == _world: + obstacle.reparent(_nav_region) + obstacle.visible = true + obstacle.process_mode = _dev_saved_obstacle_process_mode + var co2 := obstacle as CollisionObject3D + if co2 != null and _dev_saved_obstacle_layer >= 0: + co2.collision_layer = _dev_saved_obstacle_layer + co2.collision_mask = _dev_saved_obstacle_mask + _dev_collision_shapes_set_disabled(obstacle, false) + call_deferred("_dev_rebake_nav_after_obstacle_toggle_deferred") + + +func _dev_rebake_nav_after_obstacle_toggle_deferred() -> void: + if not is_instance_valid(_nav_region): + return + if _nav_region.is_baking(): + await _nav_region.bake_finished if not is_instance_valid(_nav_region): return _nav_region.bake_navigation_mesh(false) - # Let NavigationServer3D apply the rebaked mesh before repathing. - await get_tree().physics_frame - await get_tree().physics_frame - await get_tree().physics_frame - if not is_instance_valid(_player): - return - _player.sync_navigation_agent_after_map_rebuild() + if is_instance_valid(_player): + _player.sync_navigation_agent_after_map_rebuild(_nav_region) + + +func _dev_collision_shapes_set_disabled(root: Node, disabled: bool) -> void: + if root is CollisionShape3D: + (root as CollisionShape3D).disabled = disabled + for c in root.get_children(): + _dev_collision_shapes_set_disabled(c, disabled) diff --git a/client/scripts/player.gd b/client/scripts/player.gd index ed694ad..2f7435f 100644 --- a/client/scripts/player.gd +++ b/client/scripts/player.gd @@ -90,7 +90,16 @@ func clear_nav_goal() -> void: ## Repath on a new nav map (e.g. after [NavigationRegion3D] rebake when geometry is removed). -func sync_navigation_agent_after_map_rebuild() -> void: +## Rebaking can leave [NavigationAgent3D] on a stale map RID; rebind to the world's map first. +func sync_navigation_agent_after_map_rebuild(nav_region: NavigationRegion3D) -> void: + var map_rid: RID = RID() + var w3d := get_world_3d() + if w3d != null: + map_rid = w3d.navigation_map + if map_rid == RID() and is_instance_valid(nav_region): + map_rid = NavigationServer3D.region_get_map(nav_region.get_region_rid()) + if map_rid != RID(): + _nav_agent.set_navigation_map(map_rid) if _has_walk_goal: _nav_agent.set_target_position(_auth_walk_goal) else: diff --git a/client/scripts/position_authority_client.gd b/client/scripts/position_authority_client.gd index 366f56a..1807311 100644 --- a/client/scripts/position_authority_client.gd +++ b/client/scripts/position_authority_client.gd @@ -18,6 +18,9 @@ var _http: Node var _busy: bool = false var _phase: Phase = Phase.BOOT_GET +## Latest click target while [_busy] (POST or VERIFY in flight); coalesced so clicks are not lost. +var _pending_move_target: Variant = null + func _create_http_request() -> Node: return HTTPRequest.new() @@ -26,6 +29,8 @@ func _create_http_request() -> Node: func _ready() -> void: _http = _create_http_request() add_child(_http) + if _http is HTTPRequest: + (_http as HTTPRequest).timeout = 30.0 @warning_ignore("unsafe_method_access") _http.request_completed.connect(_on_request_completed) @@ -40,7 +45,22 @@ func sync_from_server() -> void: func submit_move_target(world: Vector3) -> void: if _busy: + _pending_move_target = world return + _pending_move_target = null + _start_move_post(world) + + +func _try_flush_pending_move() -> void: + if _busy: + return + if _pending_move_target is Vector3: + var w: Vector3 = _pending_move_target as Vector3 + _pending_move_target = null + _start_move_post(w) + + +func _start_move_post(world: Vector3) -> void: _busy = true _phase = Phase.POST_MOVE var url := "%s/game/players/%s/move" % [_base_root(), _player_path_segment()] @@ -54,6 +74,7 @@ func submit_move_target(world: Vector3) -> void: if err != OK: push_warning("PositionAuthorityClient: POST failed to start (%s)" % err) _busy = false + _try_flush_pending_move() func _base_root() -> String: @@ -71,13 +92,18 @@ func _request_get() -> void: if err != OK: push_warning("PositionAuthorityClient: GET failed to start (%s)" % err) _busy = false + _try_flush_pending_move() func _on_request_completed( _result: int, response_code: int, _headers: PackedStringArray, body: PackedByteArray ) -> void: if _result != HTTPRequest.RESULT_SUCCESS: + push_warning( + "PositionAuthorityClient: HTTP failed (result=%s); releasing busy." % _result + ) _busy = false + _try_flush_pending_move() return var text := body.get_string_from_utf8() @@ -87,16 +113,19 @@ func _on_request_completed( _busy = false if response_code == 200: _emit_position_from_response(text, true) + _try_flush_pending_move() Phase.POST_MOVE: if response_code == 400: _emit_move_rejection_if_present(text) _busy = false + _try_flush_pending_move() return if response_code != 200: push_warning( "PositionAuthorityClient: move POST unexpected code %s" % response_code ) _busy = false + _try_flush_pending_move() return _phase = Phase.VERIFY_GET _request_get() @@ -104,6 +133,11 @@ func _on_request_completed( _busy = false if response_code == 200: _emit_position_from_response(text, false) + else: + push_warning( + "PositionAuthorityClient: VERIFY GET unexpected code %s" % response_code + ) + _try_flush_pending_move() func _emit_move_rejection_if_present(json_text: String) -> void: @@ -124,12 +158,15 @@ func _emit_move_rejection_if_present(json_text: String) -> void: func _emit_position_from_response(json_text: String, apply_as_snap: bool) -> void: var parsed: Variant = JSON.parse_string(json_text) if parsed == null: + push_warning("PositionAuthorityClient: position response JSON parse failed") return if not parsed is Dictionary: + push_warning("PositionAuthorityClient: position response is not a JSON object") return var data: Dictionary = parsed var pos_variant: Variant = data.get("position", null) if not pos_variant is Dictionary: + push_warning("PositionAuthorityClient: position response missing `position` object") return var pos: Dictionary = pos_variant var x: float = float(pos.get("x", 0.0)) diff --git a/docs/plans/NEON-28-implementation-plan.md b/docs/plans/NEON-28-implementation-plan.md index 0662bda..de55d50 100644 --- a/docs/plans/NEON-28-implementation-plan.md +++ b/docs/plans/NEON-28-implementation-plan.md @@ -79,6 +79,11 @@ Jira acceptance criteria (verbatim intent): | **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 Jira AC 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 NEON-28 dev smoke. ## Files to add @@ -106,7 +111,7 @@ Jira acceptance criteria (verbatim intent): **Manual verification:** Main scene — zoom bands, occlusion fade, fixed yaw UX; spot-check Slice 2 behaviors (readable motion, occlusion active). -**Freed occluder (end-to-end, editor):** Position player so `Obstacle` is between camera and player (fade active), then `queue_free` the occluder. Expect **no** recurring script errors and **no** stuck transparent materials on other meshes. **`Obstacle` lives under `NavigationRegion3D`** — after removing it, **rebake** the nav mesh (see **`main.gd` F9**: `await` a frame after `queue_free`, then `bake_navigation_mesh(false)`, then a few physics frames, then **`Player.sync_navigation_agent_after_map_rebuild()`**) or click-to-move can stop. Do **not** drive that sequence with **`call_deferred` + `await`** on the same method — the coroutine may never resume. Complements static `occluder_override_key_is_valid` tests, which do not execute the full purge path in CI. +**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 @@ -118,3 +123,4 @@ Jira acceptance criteria (verbatim intent): - `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, NEON-28 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**). diff --git a/docs/reviews/2026-04-10-NEON-28.md b/docs/reviews/2026-04-10-NEON-28.md index 4d6a79c..ae0f31a 100644 --- a/docs/reviews/2026-04-10-NEON-28.md +++ b/docs/reviews/2026-04-10-NEON-28.md @@ -16,7 +16,7 @@ This branch closes the E1.M2 prototype slice in a coherent way: `CameraState` is | Document | Assessment | |----------|------------| -| `docs/plans/NEON-28-implementation-plan.md` | **Matches** — Scope, decisions, shipped notes, acceptance checklist, and Technical approach (`_physics_process` + priority) align with the branch. | +| `docs/plans/NEON-28-implementation-plan.md` | **Matches** — Scope, decisions, shipped notes, acceptance checklist, and Technical approach (`_physics_process` + priority) align with the branch. **Supplement:** plan now records **runtime nav mesh** behavior (`PARSED_GEOMETRY_BOTH`, reparent + rebake for dev smoke); see plan **Decisions** and **Runtime navigation (lesson for future mechanics)**. | | `docs/decomposition/modules/E1_M2_IsometricCameraController.md` | **Matches** — Consumer contract, E1.M2 Ready status, yaw semantics, occlusion hardening, and the `_physics_process` update all reflect the implementation. | | `docs/decomposition/modules/E6_M2_ConsentAndRiskUxSignals.md` | **Matches** — Camera-adjacent consumer guidance and authority split are consistent with the code and E1.M2 doc. | | `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E1.M2 is marked Ready with appropriate script/scene pointers. | @@ -45,6 +45,14 @@ _None identified in the implementation reviewed._ - Godot / GdUnit client tests were not run in this session. - Before merge, I would still run: `dotnet build NeonSprawl.sln` with the dev server stopped, then the documented camera manual checks in `client/README.md` plus the NEON-28 plan step that frees an occluder while it is faded. +## Supplement — runtime navigation (2026-04-10) + +**Context:** Dev smoke for the prototype `Obstacle` must match **click-to-move**, not only physics and occlusion. + +**Lesson:** With Godot’s default **`NavigationMesh.geometry_parsed_geometry_type`** (**`PARSED_GEOMETRY_BOTH`**), **`MeshInstance3D` nodes under `NavigationRegion3D`** still contribute to bakes. **Disabling collision and rebaking** can leave the **nav hole** unchanged. **`main.gd`** fixes dev smoke by **reparenting** the obstacle **out of the region**, **rebaking**, and **`sync_navigation_agent_after_map_rebuild`**. + +**Product impact:** Any future mechanic that **removes or opens world geometry** during play (barricades, doors, destroyed cover) must account for **nav source geometry**, not just **collision** — see `docs/plans/NEON-28-implementation-plan.md` (**Decisions** table row and **Runtime navigation** subsection). + --- *Review produced per [code review agent](.cursor/rules/code-review-agent.md).* From d1bd20582a7f32769e2f232a10c5a9057d15f179 Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sat, 11 Apr 2026 00:01:25 -0400 Subject: [PATCH 12/12] chore: Fix gdlint/gdformat and harden pre-push on Windows - main.gd: class member order (prvvars before @onready); wrap doc lines to 100 cols. - isometric_follow_camera.gd: wrap doc lines to 100 cols. - position_authority_client.gd: gdformat. - pre-push: resolve .venv-gd/Scripts/gdlint.exe and gdformat.exe (Windows venv). - Add scripts/install-git-hooks.ps1; README notes hook install and why CI can still fail. --- client/README.md | 12 ++++++++++-- client/scripts/isometric_follow_camera.gd | 12 +++++++----- client/scripts/main.gd | 21 ++++++++++++--------- client/scripts/position_authority_client.gd | 4 +--- scripts/git-hooks/pre-push | 4 ++++ scripts/install-git-hooks.ps1 | 19 +++++++++++++++++++ 6 files changed, 53 insertions(+), 19 deletions(-) create mode 100644 scripts/install-git-hooks.ps1 diff --git a/client/README.md b/client/README.md index b371d21..cfb8079 100644 --- a/client/README.md +++ b/client/README.md @@ -120,12 +120,20 @@ On **Linux** (including GitHub Actions), the path must be **`gdUnit4`** with a c **Dev (NEON-28):** **`dev_toggle_occluder_obstacle`** (default **Ctrl+Shift+K**) toggles the prototype `Obstacle` visibility/collision in **`main.gd`**. **F9** / **F10** are used by the embedded debugger; use this action instead when the game is running in the editor. -**Git hook (recommended):** install the repo’s local **pre-push** GDScript lint hook from the repo root: +**Git hook (recommended):** install the repo’s local **pre-push** GDScript lint hook from the repo root (once per clone): ```bash ./scripts/install-git-hooks.sh ``` -It runs **`gdlint client/scripts client/test`** and **`gdformat --check client/scripts client/test`** before each push, preferring the repo-local **`.venv-gd/`** tools when present. +On **Windows** (PowerShell), same hook: + +```powershell +pwsh -File scripts/install-git-hooks.ps1 +``` + +It runs **`gdlint client/scripts client/test`** and **`gdformat --check client/scripts client/test`** before each push. It prefers **`.venv-gd/bin/`** (Unix) or **`.venv-gd/Scripts/`** (Windows venv) when present; without that venv or a global install, the hook **fails** and blocks the push — same as CI. + +**If CI fails gdlint but your push succeeded:** the hook was not installed, **`gdlint`/`gdformat` were missing** when the hook ran, or push bypassed hooks (`--no-verify`). **Reports:** GdUnit writes under **`reports/`** (gitignored); ignore locally generated HTML/XML when committing. diff --git a/client/scripts/isometric_follow_camera.gd b/client/scripts/isometric_follow_camera.gd index b792497..f05d90f 100644 --- a/client/scripts/isometric_follow_camera.gd +++ b/client/scripts/isometric_follow_camera.gd @@ -4,7 +4,8 @@ extends Node3D ## NEON-26: discrete zoom bands via [member zoom_band_config]; wheel / `camera_zoom_*` actions. ## NEON-28: occluder overrides are keyed by [method Object.get_instance_id] only — never [Object] ## keys, or [method Dictionary.keys] / [method Dictionary.erase] can freeze after an occluder is -## [method Node.queue_free]'d (dev smoke: [InputMap] [code]dev_toggle_occluder_obstacle[/code] in [code]main.gd[/code]). +## [method Node.queue_free]'d (dev smoke: action [code]dev_toggle_occluder_obstacle[/code] in +## [code]main.gd[/code]). ## Pitch / roll are fixed by presentation exports; **orbit yaw** in state stays **0** while ## [member allow_yaw] is false (no rotate input bound). Future orbit: read relative input here, ## add to `_orbit_yaw_rad`, clamp with [member max_yaw_deg]. @@ -231,7 +232,7 @@ static func occlusion_policy_is_valid(policy: Resource) -> bool: ## True if [param body] is a live [Node3D] suitable as an [member _occluder_overrides] key. ## Used before occlusion ray work and from tests (NEON-28). -## [code]is_instance_valid[/code] first: [code]is Node3D[/code] on a freed reference errors at runtime. +## [code]is_instance_valid[/code] first; [code]is Node3D[/code] on a freed ref errors at runtime. static func occluder_override_key_is_valid(body: Variant) -> bool: if not is_instance_valid(body): return false @@ -378,8 +379,9 @@ func _restore_all_occluders() -> void: _restore_occluder_id(occ_id as int) -## Public hook for [code]dev_toggle_occluder_obstacle[/code] smoke ([code]main.gd[/code]): restore materials and clear [member _occluder_overrides] **while bodies -## are still live**, then the caller may [method Node.queue_free] the occluder. Avoids any post-free -## occlusion pass touching a tearing-down [StaticBody3D] (the semi-transparent faded box case). +## Public hook for [code]dev_toggle_occluder_obstacle[/code] smoke; [code]main.gd[/code] dev toggle. +## Restore materials and clear [member _occluder_overrides] **while bodies are still live**, +## then the caller may [method Node.queue_free] the occluder. Avoids post-free occlusion touching a +## tearing-down [StaticBody3D] (semi-transparent faded box). func restore_all_occluder_materials_now() -> void: _restore_all_occluders() diff --git a/client/scripts/main.gd b/client/scripts/main.gd index 6a4b183..da00c39 100644 --- a/client/scripts/main.gd +++ b/client/scripts/main.gd @@ -13,10 +13,13 @@ const MOVE_REJECT_MSG_SECONDS: float = 4.0 ## Bump on each rejection so older one-shot timers do not clear a newer message. var _move_reject_msg_token: int = 0 -## [InputMap] [code]dev_toggle_occluder_obstacle[/code] (default **Ctrl+Shift+K**): restored when the obstacle is shown again. +## [InputMap] [code]dev_toggle_occluder_obstacle[/code] (default **Ctrl+Shift+K**): +## Restored when the obstacle is shown again. var _dev_saved_obstacle_layer: int = -1 var _dev_saved_obstacle_mask: int = -1 var _dev_saved_obstacle_process_mode: Node.ProcessMode = Node.PROCESS_MODE_INHERIT +## Cached `Obstacle` (path breaks after reparent for nav bake; no fixed [NodePath]). +var _dev_obstacle_smoke: Node3D @onready var _camera: Camera3D = $World/IsometricFollowCamera/Camera3D @onready var _world: Node3D = $World @@ -28,9 +31,6 @@ var _dev_saved_obstacle_process_mode: Node.ProcessMode = Node.PROCESS_MODE_INHER @onready var _move_reject_label: Label = $UICanvas/MoveRejectLabel @onready var _floor: StaticBody3D = $World/NavigationRegion3D/Floor -## Cached `Obstacle` body (path breaks after reparent for nav bake; do not use a fixed [NodePath]). -var _dev_obstacle_smoke: Node3D - func _ready() -> void: set_process_unhandled_key_input(true) @@ -80,15 +80,18 @@ func _on_move_rejected(reason_code: String) -> void: ) -## Dev smoke (NEON-28 occluder): input action [code]dev_toggle_occluder_obstacle[/code] (default **Ctrl+Shift+K**) -## toggles `Obstacle` **visibility + collision** — it does **not** call [method Node.queue_free]. -## Uses [member Node.process_mode] [code]PROCESS_MODE_DISABLED[/code] so the [StaticBody3D] is **removed -## from the physics simulation** (default [member CollisionObject3D.disable_mode] is [code]REMOVE[/code]). +## Dev smoke (NEON-28 occluder): action [code]dev_toggle_occluder_obstacle[/code] +## (default **Ctrl+Shift+K**). Toggles `Obstacle` **visibility + collision** — +## does not call [method Node.queue_free]. +## Uses [member Node.process_mode] [code]PROCESS_MODE_DISABLED[/code] so the [StaticBody3D] is +## **removed from the physics simulation** +## (default [member CollisionObject3D.disable_mode] is [code]REMOVE[/code]). ## Layer/shape tweaks alone left a **Jolt ghost collider** (invisible wall) in practice. ## When hidden, reparents `Obstacle` under [member _world]: [NavigationMesh] source geometry scans ## [NavigationRegion3D] children only ([code]SOURCE_GEOMETRY_ROOT_NODE_CHILDREN[/code]), and the ## default [code]geometry_parsed_geometry_type[/code] is [code]PARSED_GEOMETRY_BOTH[/code], so a -## child [MeshInstance3D] still carved a hole even with collision disabled — rebake alone was not enough. +## child [MeshInstance3D] still carved a hole even with collision disabled — rebake alone was not +## enough. ## After each toggle, rebakes (main thread) and syncs the player [NavigationAgent3D]. ## **F9** / **F10** are debugger shortcuts in the embedded game; use the Input Map action. ## For a true **freed-node** occluder test, reload the scene or remove the body in the editor. diff --git a/client/scripts/position_authority_client.gd b/client/scripts/position_authority_client.gd index 1807311..3166a57 100644 --- a/client/scripts/position_authority_client.gd +++ b/client/scripts/position_authority_client.gd @@ -99,9 +99,7 @@ func _on_request_completed( _result: int, response_code: int, _headers: PackedStringArray, body: PackedByteArray ) -> void: if _result != HTTPRequest.RESULT_SUCCESS: - push_warning( - "PositionAuthorityClient: HTTP failed (result=%s); releasing busy." % _result - ) + push_warning("PositionAuthorityClient: HTTP failed (result=%s); releasing busy." % _result) _busy = false _try_flush_pending_move() return diff --git a/scripts/git-hooks/pre-push b/scripts/git-hooks/pre-push index aab61d4..9bf2009 100755 --- a/scripts/git-hooks/pre-push +++ b/scripts/git-hooks/pre-push @@ -9,12 +9,16 @@ gdformat_bin="" if [[ -x "$repo_root/.venv-gd/bin/gdlint" ]]; then gdlint_bin="$repo_root/.venv-gd/bin/gdlint" +elif [[ -x "$repo_root/.venv-gd/Scripts/gdlint.exe" ]]; then + gdlint_bin="$repo_root/.venv-gd/Scripts/gdlint.exe" elif command -v gdlint >/dev/null 2>&1; then gdlint_bin="$(command -v gdlint)" fi if [[ -x "$repo_root/.venv-gd/bin/gdformat" ]]; then gdformat_bin="$repo_root/.venv-gd/bin/gdformat" +elif [[ -x "$repo_root/.venv-gd/Scripts/gdformat.exe" ]]; then + gdformat_bin="$repo_root/.venv-gd/Scripts/gdformat.exe" elif command -v gdformat >/dev/null 2>&1; then gdformat_bin="$(command -v gdformat)" fi diff --git a/scripts/install-git-hooks.ps1 b/scripts/install-git-hooks.ps1 new file mode 100644 index 0000000..d1d86e6 --- /dev/null +++ b/scripts/install-git-hooks.ps1 @@ -0,0 +1,19 @@ +# Installs repo pre-push hook (gdlint + gdformat). Same behavior as install-git-hooks.sh. +# Run from repo root: pwsh -File scripts/install-git-hooks.ps1 +$ErrorActionPreference = "Stop" +$repoRoot = (git rev-parse --show-toplevel).Trim() +$hookDir = Join-Path $repoRoot ".git/hooks" +$hookPath = Join-Path $hookDir "pre-push" +New-Item -ItemType Directory -Force -Path $hookDir | Out-Null +$unixRoot = $repoRoot -replace "\\", "/" +$lines = @( + "#!/bin/sh", + "set -e", + "repo_root=`"$unixRoot`"", + 'cd "$repo_root" || exit 1', + 'exec sh "$repo_root/scripts/git-hooks/pre-push" "$@"' +) +$content = ($lines -join "`n") + "`n" +$utf8 = New-Object System.Text.UTF8Encoding $false +[System.IO.File]::WriteAllText($hookPath, $content, $utf8) +Write-Host "Installed pre-push hook at $hookPath"