NEON-28: sync plan with review (physics tick, occluder handoff)
parent
0792da702a
commit
e225f1c550
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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).*
|
||||
Loading…
Reference in New Issue