51 lines
5.6 KiB
Markdown
51 lines
5.6 KiB
Markdown
# Code review — NEO-22 WASD locomotion (wall scrape / floor seam)
|
||
|
||
**Date:** 2026-04-17
|
||
**Scope:** Branch `NEO-22-retire-click-to-move-wasd-locomotion`; working-tree diff for `client/scripts/player.gd` only (unstaged vs `HEAD`). No PR URL supplied.
|
||
**Base:** `origin/main` @ merge-base `aee3ca80d8451952c62fff7e8e72899e0d62bf68` (inferred).
|
||
|
||
## Verdict
|
||
|
||
**Approve** (blocking CI line-count issue resolved; remaining bullets are non-blocking suggestions.)
|
||
|
||
## Summary
|
||
|
||
The WASD tick implements floor coyote time, wall-scrape detection (including slide normals when `is_on_floor` / `is_on_wall` disagree with Jolt), wish-aligned velocity cleanup when a floor continuation probe says walkable floor exists ahead, micro XZ projection onto the wish line to damp seam lip slip, median floor-ray feet Y sampling, and post–`move_and_slide` feet Y smoothing / one-frame absorption (in `player_locomotion_wasd.gd` + callbacks on `player.gd`). Intent matches NEO-22 acceptance language (stable WASD on stepped geometry; NEO-14-class seam mitigation). **Repository CI:** `player.gd` is split so `gdlint` `max-file-lines` should pass. Secondary risks are behavioral (global coyote zeros vertical velocity for several ticks after *any* floor loss) and maintainability (hot-path branching split across two files).
|
||
|
||
## Documentation checked
|
||
|
||
| Path | Assessment |
|
||
|------|------------|
|
||
| `docs/plans/NEO-22-implementation-plan.md` | **Partially matches** — WASD seam stability and `player.gd` locomotion work align with goal and acceptance bullets. This diff does **not** address stream/authority wiring, README updates, click removal verification, or GdUnit coverage listed in the plan; treat those as story-level gaps before calling NEO-22 done. Cleanup bullet (“narrow NEO-11/NEO-14 blocks once WASD stable”) is **not** exercised here (additions are NEO-14-flavored mitigations for WASD, not deletion of legacy nav blocks). |
|
||
| `docs/plans/NEO-14-implementation-plan.md` | **N/A** for literal checklist (not re-read end-to-end); comments in code cite NEO-14 seam behavior — **matches** spirit of prior seam/jitter work applied to WASD. |
|
||
| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — E1.M1 remains the owning module; no new contract surfaces in this diff. |
|
||
| `docs/decomposition/modules/E1_M1_InputAndMovementRuntime.md` | **Partially matches** — implementation snapshot already describes WASD + move-stream; **Purpose** still reads “click-to-move or path-follow baseline,” which is legacy wording until NEO-22 doc pass lands. |
|
||
| `docs/decomposition/modules/client_server_authority.md` | **N/A** — no server or wire changes; client-side `CharacterBody3D` nudges remain in local prediction territory. |
|
||
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **N/A** — no register status row change required for this slice alone. |
|
||
|
||
## Blocking issues
|
||
|
||
1. ~~**`gdlint` `max-file-lines` will fail after merge.** Repo `gdlintrc` sets `max-file-lines: 1600` with an explicit note that `player.gd` is a large controller. `origin/main`’s `client/scripts/player.gd` is **1483** lines; the working tree is **~1781** lines, i.e. **over the 1600 limit**. CI (`gdlint` on `client/scripts/`) should fail until lines are trimmed, config is temporarily raised with a tracked follow-up to split the script, or locomotion helpers move to another file under the NEO-22 refactor plan.~~ Done. WASD locomotion tick moved to `client/scripts/player_locomotion_wasd.gd`; `player.gd` is back under the 1600 line cap (`gdlint` / `gdformat` clean).
|
||
|
||
## Suggestions
|
||
|
||
1. **Coyote vs ledge fall:** Floor coyote refreshes on `is_on_floor()` or `scrape_probe_hold`, otherwise decrements. While grounded-by-coyote is true, `velocity.y` is forced to `0.0` before `move_and_slide()`. That means **several physics ticks with no gravity** after leaving walkable support even on an open ledge (no wall scrape), which may feel like a short hover. If coyote is only meant to paper over Jolt floor flicker during wall scrape, consider narrowing when `velocity.y` is zeroed (e.g. require scrape vertical contact or probe hold), and keep normal air gravity otherwise.
|
||
|
||
2. **Hot-path calls:** Scrape vertical contact checks can scan all slide collisions and are invoked multiple times in one WASD tick. Caching the result once per tick would reduce work and keep behavior deterministic if slide ordering ever changes.
|
||
|
||
3. **NEO-22 test AC:** The plan asks for GdUnit coverage when wish/scrape logic is non-trivial. Floor-ray feet Y, micro-slip projection, and wish alignment helpers in `player_locomotion_wasd.gd` are good candidates for small pure tests with a stubbed `PhysicsDirectSpaceState3D` or extracted math-only helpers.
|
||
|
||
4. **Readability:** The post–`move_and_slide` block repeats the same scrape-contact predicates; folding into a single boolean or early section may reduce mistake surface when tuning.
|
||
|
||
## Nits
|
||
|
||
- **Nit:** In floor-ray median sampling, `var y_vals: Array = []` could use a typed array (`Array[float]`) for consistency with typed GDScript elsewhere.
|
||
|
||
- **Nit:** `wish_probe` duplicates direction already in `_locomotion_wish_world_xz`; low cost, but if refactors continue, a single source avoids drift.
|
||
|
||
## Verification
|
||
|
||
- Install tooling per `gdscript-style` / CI: `pip install "gdtoolkit==4.5.0"`, then `gdlint client/scripts client/test` and `gdformat --check client/scripts client/test`.
|
||
- Godot manual: WASD along stepped props and column-like seams from NEO-14 QA; confirm no regression on flat idle (NEO-14 idle path unchanged in diff but worth a smoke check).
|
||
- Walk off an open ledge with no wall: confirm fall start timing matches design (see coyote suggestion).
|