don
/
neon-sprawl
mirror of https://github.com/ViPro-Technologies/neon-sprawl
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity
neon-sprawl/docs/reviews/2026-04-07-NEON-25.md

4.1 KiB
Raw Blame History

Code review: NEON-25 (isometric follow camera)

Date: 2026-04-07
Scope: Branch NEON-25-isometric-follow-camera vs main; issue 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.