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-10-NEON-28.md

5.2 KiB
Raw Blame History

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.