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