56 lines
4.2 KiB
Markdown
56 lines
4.2 KiB
Markdown
# Code review — NEO-14 idle / rest stability (Godot client)
|
||
|
||
**Date:** 2026-04-05
|
||
**Scope:** Branch `NEO-14-idle-vibration-fix` vs merge-base `3283f6d04c8724f6536ca591a2a4c8ca06ea7b61` (tracked as `origin/main` at review time). Issue **NEO-14**; no PR URL supplied.
|
||
**Base:** `origin/main` @ `3283f6d04c8724f6536ca591a2a4c8ca06ea7b61`
|
||
|
||
**Follow-up:** Review **suggestions** below are **done** (strikethrough + **Done.**); **nits** remain optional.
|
||
|
||
## Verdict
|
||
|
||
**Approve with nits**
|
||
|
||
## Summary
|
||
|
||
This branch delivers **NEO-14** by hardening the prototype **`CharacterBody3D`** idle path (`player.gd`): dual **`floor_max_angle`** behavior, rim/straddle detection, idle nudges, bump proximity escape, **`floor_block_on_wall`**, **`NavigationAgent3D.avoidance_enabled = false`**, and **physics interpolation off** at project + player node level. It fixes **compound internal-edge** issues by spawning each QA bump as its **own `StaticBody3D`** (`random_floor_bumps.gd`), adds **Jolt** in **`project.godot`**, and reduces **z-fighting** via capsule **mesh Y lift** (visual-only vs collision shape). **`client/README.md`** and **`server/README.md`** (bump height copy) are updated. **Server contracts** (`MoveCommand` / `PositionState`) are not changed; risk is localized to client feel and QA geometry. Manual QA by the author reports **no remaining jitter** in exercised poses.
|
||
|
||
## Documentation checked
|
||
|
||
| Document | Result |
|
||
|----------|--------|
|
||
| `docs/plans/NEO-14-implementation-plan.md` | **Matches** — **Resolution**, checked AC, **Shipped note**, updated **Open questions** (`015ca13`). |
|
||
| `docs/plans/NEO-11-implementation-plan.md` | **Matches** at intent level (client nav visual, server authority unchanged); no contract drift observed in the diff. |
|
||
| `docs/decomposition/modules/E1_M1_InputAndMovementRuntime.md` | **Matches.** Prototype client locomotion and reconciliation surface remain consistent; changes are polish under “movement-loop” / QA, not authority or wire shape. |
|
||
| `docs/decomposition/modules/module_dependency_register.md` | **N/A** for code behavior; cited as the hub for module IDs. No register **Status** change required for this client-only jitter fix. |
|
||
| `docs/decomposition/modules/client_server_authority.md` | **N/A** (no change to who owns `PositionState` or move validation). |
|
||
| `client/README.md` | **Matches** NEO-14 call to document the pattern and manual steps (idle 10+ s, bumps). |
|
||
|
||
## Blocking issues
|
||
|
||
*(none)*
|
||
|
||
## Suggestions
|
||
|
||
**All done.**
|
||
|
||
1. ~~**Close the loop in `docs/plans/NEO-14-implementation-plan.md`:**~~ **Done.** Check off acceptance criteria; add **Resolution** (shipped mitigations) and **Shipped note** under technical approach — `015ca13`.
|
||
2. ~~**DRY bump collision constants** (`BUMP_COLLISION_*` aligned across `player.gd` / `random_floor_bumps.gd`):~~ **Done.** `client/scripts/random_floor_bump_collision_constants.gd` + preload from both scripts; README references — `015ca13`, doc sync `87d8ee0` (file later renamed off **`ns19_*`** identifiers).
|
||
3. ~~**Future character yaw** (`_snap_capsule_upright()` full identity basis):~~ **Done.** `TODO(NEO-14 follow-on)` on `_snap_capsule_upright()` in `player.gd`; plan **Open questions** — `015ca13`.
|
||
|
||
## Nits
|
||
|
||
- **`get_tree().get_nodes_in_group(...)`** on **`random_floor_bump_mesh`** each idle tick is fine at **two** bumps; if the group grows, cache references after spawn or guard cost.
|
||
- ~~**Commit history:**~~ **Done.** Branch history squashed to a **single** **`NEO-14:`** commit before merge; no stray **`fix:`** / **`docs:`** subjects on the slice.
|
||
|
||
## Verification
|
||
|
||
Per **`NEO-14-implementation-plan.md`** and **`client/README.md`**:
|
||
|
||
1. Run **`main.tscn`** with server as in NEO-7/NEO-11 README steps.
|
||
2. **Idle 10+ s** at spawn and after a move (no click goal); confirm **no visible vibration** on slab and on **random green bumps** (new positions each run).
|
||
3. **Walking**, **arrival**, **`snap_to_server`** on cold boot; **NEO-10** reject UX (pedestal / far pad) unchanged.
|
||
4. **Obstacle / descend** flows per NEO-11 tradeoff (multi-click around tall obstacle if needed).
|
||
5. Spot-check **bump rim**, **cylinder side**, and **floor–bump** contact (prior jitter loci).
|
||
|
||
**Automated:** None required for NEO-14 (no Godot harness in repo).
|