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
|
||
|
||
**Request changes**
|
||
|
||
## Summary
|
||
|
||
The diff extends `_physics_process_locomotion_wasd` with 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. Intent matches NEO-22 acceptance language (stable WASD on stepped geometry; NEO-14-class seam mitigation). The main merge risk is **repository CI**: `player.gd` grows past the configured `gdlint` file-length ceiling. Secondary risks are behavioral (global coyote zeros vertical velocity for several ticks after *any* floor loss) and maintainability (more hot-path branching in an already large controller).
|
||
|
||
## 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.
|
||
|
||
## Suggestions
|
||
|
||
1. **Coyote vs ledge fall:** `_locomotion_floor_coyote` refreshes on `is_on_floor()` or `scrape_probe_hold`, otherwise decrements. While `locomotion_grounded_y` 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:** `_locomotion_has_scrape_vertical_contact()` can scan all slide collisions and is invoked multiple times in one `_physics_process_locomotion_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. `_locomotion_floor_ray_feet_y`, `_locomotion_floor_ray_feet_y_median`, `_locomotion_project_global_xz_on_wish_line_if_micro_slip`, and/or `_locomotion_align_velocity_xz_to_wish` 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 `wish_probe_active and is_on_floor() and _locomotion_has_scrape_vertical_contact()`; folding into a single boolean or early section may reduce mistake surface when tuning.
|
||
|
||
## Nits
|
||
|
||
- **Nit:** In `_locomotion_floor_ray_feet_y_median`, `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/player.gd` and `gdformat --check client/scripts/player.gd` (expect **max-file-lines** failure until resolved).
|
||
- 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).
|