8.9 KiB

Raw Blame History

Code review — NEO-14 idle jitter follow-up (Godot client)

Date: 2026-04-10 Scope: Branch NEO-14-idle-jitter-followup vs origin/main @ 2aff663. Issue NEO-14 (re-opened). No PR URL supplied; working-tree diff reviewed. Base: origin/main @ 2aff663 (post-NEO-20 merge)

Verdict

Request changes — two CI-blocking lint issues must be fixed before merge.

Summary

This branch re-opens NEO-14 to eliminate residual idle x/z drift on flat ground that became worse after the NEO-15/26/27/30 follow-camera work. The fix is localized entirely to player.gd: a stable-flat-support fast-path that skips the corrective move_and_slide() / rim-escape loop, an explicit x/z anchor to clamp away residual horizontal creep while the player is classified as stable, a correction-budget cap (IDLE_MANUAL_CORRECTION_MAX_TICKS) to prevent edge contacts from running the loop indefinitely, and a capsule-feet reference point for both goal-arrival and descend-bypass checks. _snap_capsule_upright() gains an early-out when the basis is already identity. Debug tracing is gated behind @export var debug_idle_trace. Thirteen new unit tests cover the new static helpers and state invariants. Server contracts are untouched; risk is client-only.

Documentation checked

Document	Result
`docs/plans/NEO-14-implementation-plan.md`	Matches — Resolution section updated, Decisions table present, Files to modify and Tests sections reflect what shipped. AC boxes not yet checked (branch not merged).
`docs/reviews/2026-04-05-NEO-14.md`	Matches — existing review is the prior-story approval; this is a follow-up branch against the same issue. No bullets to strike through in the old file from this diff.
`docs/decomposition/modules/E1_M1_InputAndMovementRuntime.md`	Matches — change is client movement-loop polish; server authority (`PositionState`, `MoveCommand`) and all contracts unchanged. Module status ("Ready — prototype") does not need updating for this fix.
`docs/decomposition/modules/module_dependency_register.md`	N/A — no new modules or dependencies introduced.
`docs/decomposition/modules/client_server_authority.md`	N/A — no authority boundary changes.
`client/README.md`	Matches — README updated in both the tradeoff note (descend-bypass now uses feet height) and the Idle stability paragraph (flat-idle hardening, x/z anchor, identity-basis short-circuit). Manual verification step 6 updated to check x/z specifically on flat floor then on bumps.

Blocking issues

~~@export var debug_idle_trace declaration order violates gdlint class-definitions-order (CI will fail).~~ Done. Moved @export var debug_idle_trace to the const/export block in client/scripts/player.gd so it now precedes all non-export vars.

@export vars must appear before public and private vars. Currently debug_idle_trace is on line 56, sandwiched between private vars that start on line 49:
```
# current (wrong order)
var _idle_anchor_active: bool = false
var _idle_anchor_xz: Vector2 = Vector2.ZERO
var _idle_manual_correction_ticks: int = 0
@export var debug_idle_trace: bool = false   # ← must move up
var _debug_last_idle_xz: Vector2 = Vector2.INF
...
```
Move @export var debug_idle_trace to immediately after the last const block (before any var declarations), or at minimum before the first var _… line, so it precedes all non-export vars per gdlint order: const → @export vars → public vars → private vars.

~~vertical_arrival_error function signature is 109 characters — exceeds the 100-char gdlint max-line-length limit (CI will fail).~~ Done. Wrapped the signature across multiple lines in client/scripts/player.gd to satisfy max-line-length.

# 109 chars — too long
static func vertical_arrival_error(goal_y: float, body_origin_y: float, capsule_half_height: float) -> float:

Wrap the parameter list:

static func vertical_arrival_error(
    goal_y: float, body_origin_y: float, capsule_half_height: float
) -> float:

Suggestions

~~set_notify_transform(true) is always enabled regardless of debug_idle_trace.~~ Done. client/scripts/player.gd now only registers transform-change notifications in _ready() when debug_idle_trace is enabled. _notification(NOTIFICATION_TRANSFORM_CHANGED) is invoked by the engine on every transform write (physics, snap, anchor hold) in both debug and release builds. The callback itself bails quickly when debug_idle_trace is false, so functional correctness is fine, but the notification dispatch fires unconditionally at 120 TPS. Consider gating the registration:
```
if debug_idle_trace:
    set_notify_transform(true)
```
Or enable it only in debug builds (OS.is_debug_build()). At two bumps and one player this overhead is negligible, but the pattern will scale poorly if the per-node notification cost is revisited later.
~~idle_support_is_stable: the slide_normals parameter is silently ignored via @warning_ignore + no-op self-assignment.~~ Done. Renamed the parameter to _slide_normals and replaced the no-op assignment with a brief reserved-for-future comment in client/scripts/player.gd. The intent (reserve for future extra-contact analysis) is reasonable, but the current pattern is easy to misread:
```
static func idle_support_is_stable(
    on_floor: bool, floor_normal: Vector3, slide_normals: Array[Vector3], loose_ticks: int
) -> bool:
    @warning_ignore("unused_parameter")
    slide_normals = slide_normals   # ← does nothing
    ...
```
Either drop the parameter until it is used (callers pass _current_slide_normals() which allocates a new array each call), or add a ## doc comment explicitly noting "reserved — not yet evaluated". If the parameter is intentionally kept for API-stability reasons, a @warning_ignore on the declaration rather than a no-op assignment is cleaner in GDScript.
~~set_authoritative_nav_goal clears _idle_anchor_active but has no test coverage for that invariant.~~ Done. client/test/player_test.gd now seeds _idle_anchor_active before set_authoritative_nav_goal() and asserts the flag is cleared. The new test test_snap_to_server_clears_idle_anchor covers snap_to_server, and test_clear_nav_goal_clears_velocity_and_nav_target checks _idle_anchor_active. The same assertion for set_authoritative_nav_goal is absent; a one-liner would close the gap.
~~_floor_angle_loose_ticks is never decremented in the "budgeted" anchor path.~~ Done. Added the missing decrement to the budgeted idle-anchor branch in client/scripts/player.gd. When _idle_manual_correction_ticks >= IDLE_MANUAL_CORRECTION_MAX_TICKS the code holds the anchor and returns early — skipping the decrement that only lives in the corrective branch. In practice the player is already anchored so the loose angle has no visible effect, but arrival with bumps could hold FLOOR_MAX_ANGLE_MOVING_DEG longer than intended if the budget is hit quickly. Consider adding a decrement in the budgeted path:
```
if _floor_angle_loose_ticks > 0:
    _floor_angle_loose_ticks -= 1
```

Nits

_debug_trace_frame increments every _physics_process tick unconditionally (including during walking). This is fine for a debug tool; just be aware it will wrap around to int overflow after very long sessions (non-issue at 120 TPS unless someone leaves it running for ~200 days).
The @warning_ignore("unused_parameter") annotation placement inside the function body (rather than on the parameter/declaration) is valid GDScript but unusual. See suggestion 2.
_maybe_idle_bump_proximity_escape: when test_move blocks an escape step the function returns false immediately, stopping any further bump scan. This was the pre-existing behavior and is unchanged; just noting it persists in the new bool-return form.

Verification

Per NEO-14-implementation-plan.md and updated client/README.md step 6:

Fix the two blocking lint issues and run gdlint client/scripts client/test + gdformat --check client/scripts client/test locally to confirm clean.
Run headless GdUnit: godot --headless --script res://addons/gdUnit4/bin/GdUnitCmdTool.gd --add res://test/ — expect 52+ tests, 0 errors, 0 failures.
Run main.tscn with server: stand idle 10+ s at spawn on flat ground; confirm no visible vibration and no x/z drift.
Walk to a destination, stop, idle again — same stability check.
Idle on the random green bumps for 10+ s — capsule should not drift radially or oscillate.
Click-move including stepped bumps (NEO-10 props); confirm walking, arrival, and snap_to_server on cold boot are unaffected.
NEO-20 occluder click-through sanity (click behind an occluder body — ground pick still registers).

8.9 KiB Raw Blame History