52 lines
5.2 KiB
Markdown
52 lines
5.2 KiB
Markdown
# Code review: NS-23 client path-follow (`NS-23-client-path-follow`)
|
||
|
||
**Date:** 2026-04-05
|
||
**Scope:** Branch `NS-23-client-path-follow` vs `origin/main` (full NS-23 slice: client nav, wiring, docs, small server remark).
|
||
**Base:** `origin/main`
|
||
|
||
**Resolution (same day):** Design updated — **automatic obstacle navigation is not expected**; docs/plan/README aligned (`NS-23` plan **Locked — prototype movement tradeoff**, `client/README`, `server/README`, E1.M1 snapshot). Prior **Request changes** on documentation mismatch is **addressed**.
|
||
|
||
## Verdict
|
||
|
||
**Request changes** — implementation is coherent for the **accepted product tradeoff** (smooth bumps, obstacle pathing often requires multiple clicks), but **`docs/plans/NS-23-implementation-plan.md` and `client/README.md` still read as if obstacle avoidance always works**, which conflicts with shipped behavior and with the plan’s own AC wording.
|
||
|
||
## 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 team’s stated tradeoff for prototype bumps but **undermines** “walk around **`Obstacle`**” as the default single-click experience.
|
||
|
||
**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 **NS-24** (Tech Debt).
|
||
|
||
## Documentation checked
|
||
|
||
| Document | Result |
|
||
|----------|--------|
|
||
| `docs/plans/NS-23-implementation-plan.md` | **Partially matches.** Structure and locked authority table match the wiring. **Conflicts:** AC checklist marks “continuous motion … around static obstacles” as done; with the descend bypass, that is **not** generally true for single clicks past the gray box. **Open questions** says “None” but the obstacle/bump tradeoff is material—should be recorded here (or in a “Locked tradeoff” subsection). |
|
||
| `docs/decomposition/modules/E1_M1_InputAndMovementRuntime.md` | **Matches** snapshot line for NS-23 nav path-follow + server authority unchanged. After merge, consider a **one-line** note that client nav is best-effort and may bee-line when the descend heuristic fires, if you want the module doc to warn downstream readers. |
|
||
| `docs/decomposition/modules/client_server_authority.md` | **Matches** — server still owns **`PositionState`**; client presentation path remains valid. |
|
||
| `docs/decomposition/modules/module_dependency_register.md` / `documentation_and_implementation_alignment.md` | **N/A** deep diff; no register update strictly required for this review beyond optional E1.M1 nuance above. |
|
||
| `client/README.md` | **Partially matches / conflicts.** Manual step 4 still says the capsule **walks around `Obstacle`** toward the target; that is **misleading** with current **`player.gd`**. |
|
||
| `server/README.md` | **Matches** — NS-23 presentation vs straight-line validation is stated correctly. |
|
||
|
||
## Blocking issues
|
||
|
||
1. **Plan and README vs behavior:** Update **`docs/plans/NS-23-implementation-plan.md`** (and **`client/README.md`** manual check) to state the **locked tradeoff**: descend bypass (goal **Y** below body) causes **xz bee-line** toward the verified target; **navmesh detours around tall obstacles are not guaranteed** on a single click—players may need **segmented clicks** to wrap geometry. Adjust the AC checklist wording or unchecked items so the document is not self-contradictory.
|
||
|
||
## Suggestions
|
||
|
||
1. **`DESCEND_GOAL_Y_MARGIN`:** Add a one-line comment that this compares **authoritative target Y** (often **surface** from pick) to **CharacterBody3D** origin (**mid-capsule**), so the branch triggers for most floor targets—intentional for bumps, consequential for obstacles.
|
||
2. **`_skip_move_when_idle_on_floor`:** Document that **`is_on_floor()`** is updated by **`move_and_slide()`**; skipping slides when idle relies on the last frame’s floor state (acceptable for a static prototype floor).
|
||
3. **NS-24:** Keep **idle/rest vibration** out of NS-23 closure; link **NS-24** from the plan or README “Known issues” if you add a short note.
|
||
|
||
## Nits
|
||
|
||
- **Nit:** 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.
|
||
|
||
## Verification
|
||
|
||
- `cd server/NeonSprawl.Server && dotnet test` (no logic change expected, sanity).
|
||
- Godot **F5** with server: boot snap; click floor; confirm bump flow acceptable; confirm gray **Obstacle** often requires **multiple clicks** / partial path—aligned with updated docs.
|
||
- Confirm NS-19 reject targets still show rejection UX (`reasonCode` / label).
|
||
- `gdlint` / `gdformat` on touched GDScript if CI does not already run on the branch.
|