39 lines
3.4 KiB
Markdown
39 lines
3.4 KiB
Markdown
# NEO-20 review
|
|
|
|
**Date:** 2026-04-09
|
|
**Scope:** Branch `NEO-20-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 NEO-20 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 NEO-17. 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/NEO-20-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/NEO-20-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/NEO-20-implementation-plan.md`.
|
|
|
|
## Nits
|
|
|
|
- ~~Nit: Consider adding a short NEO-20-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 (NEO-20 — 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 NEO-20 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.
|