From f412da8f443126ae4964cc8c0f2bb9e36d636c6e Mon Sep 17 00:00:00 2001 From: VinPropane Date: Sun, 5 Apr 2026 15:04:39 -0400 Subject: [PATCH] docs: Mark NS-23 code review blocking items and suggestions done --- docs/reviews/2026-04-05-NS-23.md | 40 ++++++++++++++++++-------------- 1 file changed, 22 insertions(+), 18 deletions(-) diff --git a/docs/reviews/2026-04-05-NS-23.md b/docs/reviews/2026-04-05-NS-23.md index 9378aa5..de4d4f1 100644 --- a/docs/reviews/2026-04-05-NS-23.md +++ b/docs/reviews/2026-04-05-NS-23.md @@ -4,48 +4,52 @@ **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**. +**Follow-up:** Design and docs were aligned after the initial review; blocking items and suggestions below are **done** unless marked otherwise. ## 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. +**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 team’s stated tradeoff for prototype bumps but **undermines** “walk around **`Obstacle`**” as the default single-click experience. +**`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 **NS-24** (Tech Debt). +**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](https://neon-sprawl.atlassian.net/browse/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. | +| Document | Result (at closure) | +|----------|---------------------| +| `docs/plans/NS-23-implementation-plan.md` | **Matches** after **Locked — prototype movement tradeoff**, updated AC, tests, and **NS-24** 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** + **NS-24** link. | +| `server/README.md` | **Matches** — client nav presentation; detours not guaranteed on one click. | ## 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. +**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 -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. +**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. ~~**NS-24:**~~ **Done.** Out of NS-23 scope in plan; **[NS-24](https://neon-sprawl.atlassian.net/browse/NS-24)** linked from **`NS-23-implementation-plan.md`** and **`client/README.md`**. ## 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. +- **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; confirm bump flow acceptable; confirm gray **Obstacle** often requires **multiple clicks** / partial path—aligned with updated docs. +- Godot **F5** with server: boot snap; click floor; bump flow; gray **Obstacle** may need **multiple clicks** per 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.