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-09-NEON-30.md

3.4 KiB
Raw Blame History

NEON-30 review

Date: 2026-04-09 Scope: Branch NEON-30-click-through-input reviewed against origin/main...HEAD Base: origin/main

Verdict

Approve with nits

Summary

This branch keeps the fix narrowly scoped to client/scripts/ground_pick.gd: occluder-tagged bodies are now skipped before the existing walkable-surface logic runs, which matches the intended NEON-30 behavior and avoids introducing a new collision-layer contract. The supporting unit tests and module/README updates are directionally correct and consistent with the existing "occluder" group convention established in NEON-27. I did not find a correctness issue in the shipped code path from the diff alone. I could not run the Godot/GdUnit verification commands in this environment because godot is not installed locally, so the full click-through behavior still needs author-side verification.

Documentation checked

  • docs/plans/NEON-30-implementation-plan.md — partially matches. The implementation and recorded decisions line up with the diff, but the acceptance-criteria checklist is still entirely unchecked even though the branch is presented as implemented; per the plan/doc workflow, that checklist should be refreshed before merge.
  • docs/decomposition/modules/E1_M2_IsometricCameraController.md — matches. The new click-through input note accurately records the shared "occluder" group contract and the absence of a new collision-layer convention.
  • docs/decomposition/modules/module_dependency_register.md — matches / no update needed. E1.M2 remains In progress; this story does not appear to change the module readiness state.
  • .cursor/rules/testing-expectations.md — matches. The branch updates client/test/ground_pick_test.gd alongside the production GDScript change and still relies on manual verification for the full pick flow, which is acceptable for this scope.

Blocking issues

None.

Suggestions

  1. Refresh the acceptance-criteria checklist in docs/plans/NEON-30-implementation-plan.md before merge. The current file still shows every box unchecked, which leaves the canonical story artifact out of sync with the implemented branch and weakens the plan-as-acceptance-criteria workflow this repo is using. Done. All three AC boxes checked in docs/plans/NEON-30-implementation-plan.md.

Nits

  • Nit: Consider adding a short NEON-30-specific manual check bullet to client/README.md near the existing movement verification steps so the expected "click behind the obstacle while opaque/faded" regression check is visible where contributors already look for client runtime checks. Done. Added ### Manual check (NEON-30 — occluder click-through) section in client/README.md covering opaque/faded occluder clicks and regression cases.

Verification

  • Install Godot 4.6 on the author machine, then from client/ run godot --headless --path . -s res://addons/gdUnit4/bin/GdUnitCmdTool.gd --ignoreHeadlessMode -a res://test.
  • Run the manual NEON-30 scenario from client/scenes/main.tscn: click the ground behind World/NavigationRegion3D/Obstacle while it is opaque and while it is faded by the occlusion system; confirm the player still receives a valid move target behind it.
  • Re-check the existing non-occluder cases called out in client/README.md, especially normal floor clicks and the far pad / pedestal rejection behavior, to confirm the added occluder skip did not regress earlier ground-pick rules.