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

6.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. Supplement: plan now records runtime nav mesh behavior (PARSED_GEOMETRY_BOTH, reparent + rebake for dev smoke); see plan Decisions and Runtime navigation (lesson for future mechanics).
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.

Supplement — runtime navigation (2026-04-10)

Context: Dev smoke for the prototype Obstacle must match click-to-move, not only physics and occlusion.

Lesson: With Godot’s default NavigationMesh.geometry_parsed_geometry_type (PARSED_GEOMETRY_BOTH), MeshInstance3D nodes under NavigationRegion3D still contribute to bakes. Disabling collision and rebaking can leave the nav hole unchanged. main.gd fixes dev smoke by reparenting the obstacle out of the region, rebaking, and sync_navigation_agent_after_map_rebuild.

Product impact: Any future mechanic that removes or opens world geometry during play (barricades, doors, destroyed cover) must account for nav source geometry, not just collision — see docs/plans/NEON-28-implementation-plan.md (Decisions table row and Runtime navigation subsection).

Review produced per code review agent.