diff --git a/docs/reviews/2026-04-07-NEON-25.md b/docs/reviews/2026-04-07-NEON-25.md new file mode 100644 index 0000000..6a5afd1 --- /dev/null +++ b/docs/reviews/2026-04-07-NEON-25.md @@ -0,0 +1,43 @@ +# Code review: NEON-25 (isometric follow camera) + +**Date:** 2026-04-07 +**Scope:** Branch `NEON-25-isometric-follow-camera` vs `main`; issue [NEON-25](https://neon-sprawl.atlassian.net/browse/NEON-25). Commits on branch include implementation plan, locked defaults, **NEON-29** backlog row in `E1_M2_IsometricCameraController.md`, and camera implementation + tests + docs. Manual testing reported OK by author. +**Base:** `main` (compare: `main...NEON-25-isometric-follow-camera`). + +## Verdict + +**Approve with nits** + +## Summary + +The change replaces a static `Camera3D` with an `IsometricFollowCamera` rig that follows the player using a documented spherical offset (`desired_eye_world`), exponential smoothing, teleport snap beyond `snap_distance`, and per-frame `look_at` toward a configurable focus. A `RefCounted` `CameraState` is owned by the rig and refreshed each `_process`, with orbit yaw held at zero while `allow_yaw` is false—matching prototype policy. `main.gd` stays a thin composer; `ground_pick` still receives the active `Camera3D`. Automated tests cover `CameraState` fields and the static eye-position helper; manual verification (movement, bumps, picking) is appropriate per plan. No server or authority surface is added. Risk is low: presentation-only, well-scoped, aligns with E1.M2 and the implementation plan. + +## Documentation checked + +| Document | Result | +|----------|--------| +| `docs/plans/NEON-25-implementation-plan.md` | **Matches** — locked defaults (`_process`, damped follow + snap, `NodePath` follow, per-frame `CameraState`), file list, tests, and README/module snapshot are reflected in the diff. | +| `docs/decomposition/modules/E1_M2_IsometricCameraController.md` | **Matches** — implementation snapshot documents rig scripts, yaw seam, single-band distance; Jira table includes NEON-29 (separate ticket; see nits). | +| `docs/decomposition/modules/client_server_authority.md` (E1.M2) | **Matches** — camera and `CameraState` remain client-local; no server use of camera pose. | +| `docs/decomposition/modules/module_dependency_register.md` | **Partially matches** — E1.M2 row still **Planned** with full contract set; NEON-25 delivers a slice (follow + `CameraState` seam), not zoom/occlusion. Updating register or module **Status** after merge is optional hygiene, not required for this PR’s correctness. | +| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **N/A** — no E1.M2-specific row found; no conflict. | + +## Blocking issues + +None. + +## Suggestions + +1. **Null follow target:** If `follow_target_path` breaks or resolves empty, `_process` returns without updating `_smoothed_eye` or syncing state—acceptable for the current scene, but a one-line comment or dev warning could save a future scene refactor. +2. **Register / status (optional):** After merge, consider whether `module_dependency_register.md` or the E1.M2 summary **Status** should reflect partial delivery (e.g. follow baseline landed; zoom/occlusion still open)—only if the team tracks that table at story granularity. + +## Nits + +- **Nit:** `camera_state.gd` header references `[member presentation_yaw_deg]`; that property lives on `isometric_follow_camera.gd`, not on `CameraState`—minor doc confusion for Godot help tooltips. +- **Nit:** Same branch carries **NEON-29** (prototype district) backlog registration; harmless, but reviewers merging a “NEON-25 only” PR should expect that small doc delta. +- **Nit:** Plan wording “set state yaw to `0` when `allow_yaw` is false” is satisfied via `_orbit_yaw_rad = 0` and `_state.yaw = _orbit_yaw_rad`; presentation yaw remains in exports—consistent with module intent but worth remembering for readers who expect a single “total yaw” on state. + +## Verification + +- **Automated:** From `client/`, run GdUnit headless per `client/README.md` (Godot 4.6 import + `GdUnitCmdTool` on `res://test`). +- **Manual:** (Author confirmed OK.) Re-run: server + F5, click-move floor and NEON-7 bumps, ground pick from moving camera, idle observation; optional inspector tweak of `follow_smoothness` / `snap_distance` if feel regresses on different hardware.