acdream/docs/research/2026-05-06-locomotion-cycle-transitions/findings-static.md
Erik 8fa04af4c7 fix(motion): #39 candidate — un-gate UP velocity-cycle for player remotes (forward only)
Adds a player-remote velocity-fallback path to ApplyServerControlledVelocityCycle
so that when retail (the actor) toggles Shift while holding W and acdream is
the observer, the visible leg cycle switches Run↔Walk within ~200–500 ms even
though no fresh UM arrives. Static analysis (ACE GameActionMoveToState +
MovementData.cs auto-upgrade + acdream's prior diag traces) suggests retail
does NOT broadcast a fresh MoveToState on HoldKey-only changes — acdream's
UMs handle direction-key changes and our local +Acdream's transitions, but
retail-driven actors leave the cycle stuck.

Changes (all in src/AcDream.App/Rendering/GameWindow.cs):
- New RemoteMotion.LastUMTime field, stamped in OnLiveMotionUpdated
- ApplyServerControlledVelocityCycle: removed inner IsPlayerGuid gate;
  routes player remotes to new ApplyPlayerLocomotionRefinement
- ApplyPlayerLocomotionRefinement (forward-direction only):
  - 500 ms UM grace window (UMs win when fresh)
  - Forward-direction-only (low byte 0x05 / 0x07)
  - Hysteresis: Run → Walk demote at < 4.5 m/s; Walk → Run promote > 5.5 m/s
  - Skip SetCycle when neither motion ID nor speedMod changed meaningfully
  - [UPCYCLE_PLAYER] diag gated on ACDREAM_REMOTE_VEL_DIAG=1
- Outer call site in OnLivePositionUpdated un-gated (!IsPlayerGuid removed);
  per-remote routing now lives inside the function

Scope: case #1 (Run↔Walk forward) only. Cases #2–#7 (backward, sidestep
speed-buckets, direction-flips) remain deferred — PlanFromVelocity is
forward-only and its NPC-tuned thresholds (RunThreshold=1.25) do not
separate player Walk (~2.5 m/s) from player Run (~9 m/s); a TTD trace
of retail's per-direction algorithm should ground the wider fix.

ISSUES.md #39 updated with progress; investigation-prompt.md and a new
findings-static.md committed under
docs/research/2026-05-06-locomotion-cycle-transitions/ (the prompt was
authored on a parallel branch in commit 7a38da3 and is brought into this
worktree here so the next session can find it without branch-hopping).

Build clean. The 8 pre-existing test failures on this branch
(BSPStepUpTests.C3_Path6_AirborneMoverHitsSteepSlope, MotionInterpreter
WalkBackward GetMaxSpeed, etc.) are unrelated to this change — verified
by running them with the diff stashed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-05-06 06:34:20 +02:00

167 lines
7 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters

This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.

# Locomotion-cycle transitions — static-analysis findings
**Date:** 2026-05-06 (follow-up to `investigation-prompt.md`)
**Status:** static analysis complete; candidate fix shipped for case #1; cases #2#7 deferred until TTD validation.
This is what code reading + ACE cross-reference + named-retail grep
established before any TTD/cdb trace was run. It scopes the candidate
fix and identifies which questions still need the trace.
---
## What's confirmed by static analysis
### 1. ACE broadcasts unconditionally on inbound MoveToState
`references/ACE/Source/ACE.Server/Network/GameAction/Actions/GameActionMoveToState.cs:36`
calls `session.Player.BroadcastMovement(moveToState)` for every received
MoveToState, no diff-check. So **whether ACE broadcasts a UM is determined
purely by whether the client sent a MoveToState**.
### 2. ACE auto-upgrades `WalkForward + HoldKey.Run → RunForward` on broadcast
`references/ACE/Source/ACE.Server/Network/Motion/MovementData.cs:110-113`
shows the conversion: when client sends `WalkForward + HoldKey.Run`, the
broadcast `interpState.ForwardCommand` is upgraded to `RunForward`. So
the broadcast UM byte differs between Run and Walk even though the
client's wire byte is the same `WalkForward`.
### 3. Therefore the bug is one of these two
Either:
- **A.** Retail (the actor) does NOT send a fresh MoveToState when only
HoldKey changes (Shift toggle while a direction key is held). ACE has
nothing to broadcast → no UM arrives at the observer. UPs continue
with new velocity (run-pace ↔ walk-pace), but observer's UM-driven
cycle never updates.
- **B.** Retail DOES send a fresh MoveToState. ACE broadcasts a UM. Our
parser receives it but fails to apply the cycle change for some
reason.
The diagnostic data captured in 2026-05-03's investigation
(`[FWD_WIRE]`, `[UM_RAW]`, `[SETCYCLE]`) is more consistent with **A**:
no `[FWD_WIRE]` line was logged for Shift toggles, suggesting no UM
arrived. But that data was for a different scenario, so a fresh trace
is needed.
**This question is the primary gap the TTD/cdb trace must close.**
A 2-minute cdb session with a breakpoint counter on
`acclient!CommandInterpreter::SendMovementEvent` answers it
definitively.
### 4. `ApplyServerControlledVelocityCycle` was gated against player remotes at three sites
In `src/AcDream.App/Rendering/GameWindow.cs` before this change:
- **Inner gate**, function entry (~line 3326):
`if (IsPlayerGuid(serverGuid)) return;`
- **Outer gate**, call site in `OnLivePositionUpdated` (~line 3683):
`if (!IsPlayerGuid(update.Guid) && rmState.HasServerVelocity ...)`
- **TickAnimations gate** (~line 6325):
`if (!IsPlayerGuid(serverGuid) && rm.HasServerVelocity)`
this one is for the "stale velocity → reset to zero" path, used by
NPCs whose MoveTo has expired. Leave as-is for now; player remotes
don't go through this branch (no MoveTo path).
### 5. `ServerControlledLocomotion.PlanFromVelocity` thresholds are wrong for player speeds
In `src/AcDream.Core/Physics/ServerControlledLocomotion.cs:30-33`:
```csharp
public const float StopSpeed = 0.20f;
public const float RunThreshold = 1.25f;
```
For player speeds (Walk ≈ 2.5 m/s, Run ≈ 9 m/s — both observed in
prior `[VEL_DIAG]` traces), `RunThreshold = 1.25` is below both
buckets. Both speeds resolve to `RunForward`. So even if we just
un-gated the existing function for player remotes, the function would
*always* return `RunForward` and the bug would persist.
The function works correctly for NPCs (typical NPC speed range
16 m/s falls on the right side of the 1.25 threshold for the
Idle/Walk/Run distinction).
**Implication:** the player path must use different thresholds. The
candidate fix introduces hysteresis tuned for player speeds (4.5 demote,
5.5 promote) and routes player remotes through a dedicated
`ApplyPlayerLocomotionRefinement` helper instead of `PlanFromVelocity`.
### 6. `PlanFromVelocity` returns Forward-only motions
It returns `Ready`, `WalkForward`, or `RunForward` — never sidestep,
never backward. Even with thresholds fixed, calling it for a sidestepping
player remote would clobber `SideStepLeft`/`Right` with `RunForward`.
**Implication:** the player path must scope itself to forward direction
only; sidestep/backward HoldKey toggles are deferred until TTD confirms
retail's per-direction algorithm.
---
## What the candidate fix does (and doesn't)
**Does:**
- Adds `RemoteMotion.LastUMTime` (timestamp of most recent UM for a remote).
- Stamps `LastUMTime` at the top of `OnLiveMotionUpdated`.
- Removes the `IsPlayerGuid` early return inside
`ApplyServerControlledVelocityCycle`.
- Removes the `!IsPlayerGuid` gate at the outer call site in
`OnLivePositionUpdated` (so player remotes get the function called).
- Routes player remotes through new `ApplyPlayerLocomotionRefinement`:
- 500 ms UM grace window (UMs are authoritative).
- Forward-direction-only refinement (low byte 0x05 / 0x07).
- Hysteresis: Run → Walk demote at < 4.5 m/s; Walk Run promote at
> 5.5 m/s.
- SetCycle skipped if neither motion ID nor speedMod changed
meaningfully.
- Diagnostic `[UPCYCLE_PLAYER]` line gated on `ACDREAM_REMOTE_VEL_DIAG=1`.
**Does NOT (deferred to TTD-validated follow-up):**
- Backward (case #2) HoldKey toggle.
- Sidestep speed-bucket refinement (cases #4, #5).
- Direction-flip transitions (cases #3, #6, #7) these are believed
to work today via UM-driven SetCycle, but the prompt's acceptance
criteria explicitly call for visual confirmation of all 7 cases.
- Tuning the grace window or the hysteresis thresholds against retail's
exact behavior. 500 ms / 4.5 / 5.5 are defensible defaults but TTD
may show retail uses different values.
---
## Acceptance for the candidate fix (case #1 only)
When acdream observes a retail-driven character:
- Hold W (run) for 2 s, then press Shift, then release Shift, then
release W. The visible leg cycle should switch Run Walk Run
Idle within ~200500 ms of each transition.
- All other working cases (acdream-on-acdream, retail-on-acdream,
Idle Run, Idle Walk) must still work no regressions.
If the candidate fix produces visible RunWalk transitions on retail
actors, ship it (close part of #39, file the remaining 6 cases as a
follow-up issue) and run TTD on cases #2#7 next.
If it doesn't switch the cycle (or thrashes / regresses something),
the next investigation step is the cdb breakpoint count on
`CommandInterpreter::SendMovementEvent` to settle question A vs. B
above before any further fix attempt.
---
## Files touched by the candidate fix
- `src/AcDream.App/Rendering/GameWindow.cs`
- Added `RemoteMotion.LastUMTime` field
- Stamped `LastUMTime` at top of `OnLiveMotionUpdated`
- Removed inner `IsPlayerGuid` gate in `ApplyServerControlledVelocityCycle`
- Routed player remotes to new `ApplyPlayerLocomotionRefinement`
- Added constants `UmGraceSeconds`, `PlayerRunPromoteSpeed`,
`PlayerRunDemoteSpeed`
- Removed `!IsPlayerGuid` gate at outer call site in `OnLivePositionUpdated`
No changes to `ServerControlledLocomotion.cs`, `MotionInterpreter.cs`,
or `AnimationSequencer.cs`.