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-17-NEO-22.md

5.8 KiB
Raw Blame History

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. Done. client/test/player_locomotion_wasd_test.gd covers the extracted static helpers; full run_wasd + move_and_slide remains manual / integration territory.

  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).