56 lines
3.9 KiB
Markdown
56 lines
3.9 KiB
Markdown
# Code review: NEO-11 client path-follow (`NEO-11-client-path-follow`)
|
||
|
||
**Date:** 2026-04-05
|
||
**Scope:** Branch `NEO-11-client-path-follow` vs `origin/main` (full NEO-11 slice: client nav, wiring, docs, small server remark).
|
||
**Base:** `origin/main`
|
||
|
||
**Follow-up:** Design and docs were aligned after the initial review; blocking items and suggestions below are **done** unless marked otherwise.
|
||
|
||
## Verdict
|
||
|
||
**Approve** — original **Request changes** (plan/README vs obstacle wording) and review **suggestions** 1–3 are **resolved** in follow-up commits. Optional **nit** (rename `DESCEND_GOAL_Y_MARGIN`) remains open.
|
||
|
||
## Summary
|
||
|
||
The branch adds **`NavigationRegion3D`** / **`NavigationAgent3D`**, sync nav bake in **`main.gd`**, boot snap vs post-move nav goal via **`PositionAuthorityClient`**’s second signal argument, and horizontal path-follow in **`player.gd`**. **`ground_pick.gd`** gains stepped walkable picking for steep surfaces. Server change is documentation-only on **`MoveCommandValidation`** (vertical step remark).
|
||
|
||
**`player.gd`** intentionally skips nav waypoints when **`_auth_walk_goal.y < global_position.y - DESCEND_GOAL_Y_MARGIN`**, so for typical picks (floor **Y** vs mid-capsule origin) the client often **bee-lines in xz** toward the goal. That matches the **locked prototype tradeoff** (smooth bumps; multi-click around obstacles when needed).
|
||
|
||
**Idle optimization:** skipping **`move_and_slide()`** when there is no walk goal, velocity is zero, and **`is_on_floor()`** is a reasonable jitter mitigation; residual vibration is tracked in **[NEO-14](https://linear.app/neon-sprawl/issue/NEO-14)** (Tech Debt).
|
||
|
||
## Documentation checked
|
||
|
||
| Document | Result (at closure) |
|
||
|----------|---------------------|
|
||
| `docs/plans/NEO-11-implementation-plan.md` | **Matches** after **Locked — prototype movement tradeoff**, updated AC, tests, and **NEO-14** link. |
|
||
| `docs/decomposition/modules/E1_M1_InputAndMovementRuntime.md` | **Matches** — snapshot notes single-click obstacle detours not guaranteed. |
|
||
| `docs/decomposition/modules/client_server_authority.md` | **Matches** — server owns **`PositionState`**; client presentation path valid. |
|
||
| `docs/decomposition/modules/module_dependency_register.md` / `documentation_and_implementation_alignment.md` | **N/A** deep diff. |
|
||
| `client/README.md` | **Matches** — tradeoff, manual steps, **Known issue** + **NEO-14** link. |
|
||
| `server/README.md` | **Matches** — client nav presentation; detours not guaranteed on one click. |
|
||
|
||
## Blocking issues
|
||
|
||
**All resolved.**
|
||
|
||
1. ~~**Plan and README vs behavior:**~~ **Done.** Plan and **`client/README.md`** now state the **locked tradeoff** (descend bypass → xz bee-line; navmesh detours not guaranteed on one click; multi-click OK). AC checklist and manual QA updated accordingly.
|
||
|
||
## Suggestions
|
||
|
||
**All done.**
|
||
|
||
1. ~~**`DESCEND_GOAL_Y_MARGIN`:**~~ **Done.** Comment added in `client/scripts/player.gd` (surface pick Y vs mid-capsule; bumps vs obstacles).
|
||
2. ~~**`_skip_move_when_idle_on_floor`:**~~ **Done.** Doc comment above the helper (`is_on_floor()` / last `move_and_slide()`; static floors).
|
||
3. ~~**NEO-14:**~~ **Done.** Out of NEO-11 scope in plan; **[NEO-14](https://linear.app/neon-sprawl/issue/NEO-14)** linked from **`NEO-11-implementation-plan.md`** and **`client/README.md`**.
|
||
|
||
## Nits
|
||
|
||
- **Nit (optional):** Consider renaming **`DESCEND_GOAL_Y_MARGIN`** to something that signals “auth pick Y vs body origin” to reduce future “why do we always bee-line?” confusion. **Not done** — purely optional.
|
||
|
||
## Verification
|
||
|
||
- `cd server/NeonSprawl.Server && dotnet test` (no logic change expected, sanity).
|
||
- Godot **F5** with server: boot snap; click floor; bump flow; gray **Obstacle** may need **multiple clicks** per docs.
|
||
- Confirm NEO-10 reject targets still show rejection UX (`reasonCode` / label).
|
||
- `gdlint` / `gdformat` on touched GDScript if CI does not already run on the branch.
|