feat(retail): Phase B.6 — server-driven auto-walk done right
Closes #63, #69, #74, #75. Replaces the chain of Commit-B workarounds that compensated for ACE's MoveToChain getting cancelled by a leaked user-MoveToState packet during inbound auto-walk. The fix is architectural — auto-walk drives the body directly from the server-supplied path data, no player-input synthesis, no spurious wire-packet transitions, no grace-period band-aid. Architectural change (closes #75): PlayerMovementController.ApplyAutoWalkOverlay → DriveServerAutoWalk. - Steps Yaw toward target at retail-faithful turn rates. - Computes desired forward velocity from path runRate. - Calls _motion.DoMotion(WalkForward, speed) directly for the motion-interpreter state (drives animation cycle). - Sets _body.set_local_velocity directly when grounded. - Returns true to gate the user-input motion + velocity section in Update so user-input flow doesn't overwrite auto-walk velocity or motion state. Mirrors retail's MovementManager::PerformMovement case 6 (decomp 0x00524440) which never touches the user-input pipeline during server-controlled auto-walk. Wire-layer guard at GameWindow.cs:6419 retained as a SEMANTIC statement (`if (result.MotionStateChanged && !IsServerAutoWalking)`): user-MoveToState packets are for user-driven motion intent. During server-controlled auto-walk, the motion-state transitions caused by the animation override (RunForward / WalkForward / TurnLeft / TurnRight cycles) must not leak as user-cancellation packets. This is NOT the deleted 500ms grace-period band-aid; it's the wire-layer expressing the user-vs-server motion split. Animation plumbed for auto-walk phases (closes #69): - Moving forward → WalkForward (speed=1.0) / RunForward (speed=runRate) - Turn-first phase → TurnLeft / TurnRight (sign of yawStep) - Aligned-but-pre-step / arrival → no override (idle) Driven via _autoWalkMovingForwardThisFrame + _autoWalkTurnDirectionThisFrame fields set in DriveServerAutoWalk and read in the MovementResult construction at the bottom of Update. UpdatePlayerAnimation picks up the localAnimCmd as the highest-priority animation source. Walk/run threshold = 1.0m, retail-observed. ACE's wire-default of 15.0f is too generous; ACE's own physics layer uses 1.0f at MovementParameters.cs:50 (with the 15.0f line commented out) and Creature.cs:312 notes "default 15 distance seems too far". The formula matches retail's MovementParameters::get_command at decomp 0x0052aa00: running = (initialDist - distance_to_object) >= threshold, evaluated ONCE at chain start and held for the rest of the auto-walk (matches retail "runs all the way / walks all the way" behaviour). Wire-supplied threshold is ignored. Pickup gate (IsPickupableTarget) now uses BF_STUCK (acclient.h:6435, bit 0x4) to discriminate immovable scenery from real pickup items that share a Misc ItemType. Sign (pwd=0x14 with BF_STUCK) → blocked; spell component (pwd=0x10, no BF_STUCK) → allowed. ACE's PutItemInContainer (Player_Inventory.cs:831-836) responds with WeenieError.Stuck (0x29) on stuck items so the gate prevents wasted wire packets + a UX dead-end. R-key dispatch by target type. UseCurrentSelection's top-level IsUseableTarget gate was wrong (blocked USEABLE_NO=1 items that ARE pickupable). Reordered: 1. Creature → SendUse 2. Pickupable → SendPickUp 3. Useable → SendUse 4. Otherwise → "cannot be used" toast Each handler keeps its own gate. Matches retail's per-action server-side validation. AP cadence revert (closes #74). With the MoveToChain race fixed, the per-frame "send while moving" cadence is no longer load-bearing. Reverted to retail's two-branch ShouldSendPositionEvent gate (acclient_2013_pseudo_c.txt:700233-700285): Interval NOT elapsed (< 1 sec): send if cell or contact-plane changed. Interval elapsed (>= 1 sec): send if cell or position frame changed. Adds _lastSentContactPlane field + ApproxPlaneEqual helper + PlayerMovementController.ContactPlane public accessor. Extended NotePositionSent(Vector3, uint, Plane, float) — both outbound sites (MoveToState + AP) pass _playerController.ContactPlane. Effective rates: 0 Hz idle, ~1 Hz smooth motion, per-event on cell/plane changes, 0 Hz airborne. CLAUDE.md updated with no-workarounds rule (commit `da126f9` on the worktree branch). Saved as feedback memory at memory/feedback_no_workarounds.md. Tests: build green; Core.Net 294/294; Core 1073/1081 (baseline, 8 pre-existing Physics failures unchanged). Visual-verified end-to-end on 2026-05-16 for far/near Use + PickUp on NPCs, doors, items, spell components, signs (correctly blocked), corpses, turn-first animation, run/walk thresholds, idle quiet, smooth- motion 1Hz. Spec: docs/superpowers/specs/2026-05-16-phase-b6-suppress-movetostate-during-inbound-autowalk-design.md Plan: docs/superpowers/plans/2026-05-16-phase-b6-suppress-movetostate-during-inbound-autowalk.md Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Erik
parent
b5da17db76
commit
d640ed74e1
6 changed files with 1317 additions and 200 deletions
|
|
@ -0,0 +1,156 @@
|
|||
# Phase B.6 — Suppress outbound `MoveToState` during inbound MoveToObject auto-walk
|
||||
|
||||
**Date:** 2026-05-16
|
||||
**Closes:** [#63](../../ISSUES.md) (server-initiated auto-walk not honored — the half about ACE's `MoveToChain` callback never firing), [#74](../../ISSUES.md) (AP cadence chattier than retail)
|
||||
**Milestone:** M1 — Walkable + clickable world
|
||||
|
||||
## Goal
|
||||
|
||||
Make ACE's server-side `MoveToChain` callback fire the Use action on arrival, eliminating the client-side retry workaround that today's `feat(retail): Commit B` (`b5da17d`) restored as an issue-#63 mitigation.
|
||||
|
||||
## Background
|
||||
|
||||
When the player double-clicks an NPC across the room in our current build:
|
||||
|
||||
1. We send `Use(guid)` wire packet immediately at click.
|
||||
2. ACE: player not in `WithinUseRadius` → server-side `CreateMoveToChain(item, (success) => TryUseItem(item, success))` ([`Player_Use.cs:205`](../../../references/ACE/Source/ACE.Server/WorldObjects/Player_Use.cs)).
|
||||
3. ACE broadcasts `UpdateMotion` with `MovementType=6` (MoveToObject) targeting our player.
|
||||
4. Our client honors it via `BeginServerAutoWalk(...)` at [`GameWindow.cs:3389-3414`](../../../src/AcDream.App/Rendering/GameWindow.cs).
|
||||
5. The auto-walk overlay at [`PlayerMovementController.ApplyAutoWalkOverlay`](../../../src/AcDream.App/Input/PlayerMovementController.cs) synthesizes `input.Forward = true, input.Run = true` (line 611-619) so the rest of `Update` runs as if the user pressed W.
|
||||
6. **Bug:** the rest of `Update` computes `MotionStateChanged = true` and the outbound wire layer at [`GameWindow.cs:6410-6445`](../../../src/AcDream.App/Rendering/GameWindow.cs) builds and sends a `MoveToState` packet with `forwardCommand=RunForward, holdKey=Run`.
|
||||
7. ACE reads that as **user took manual control** and cancels its own `MoveToChain` — the `TryUseItem` callback never fires.
|
||||
8. We've worked around this by sending `Use` a second time on local-arrival (`_pendingPostArrivalAction` in [`GameWindow.SendUse` far-range path](../../../src/AcDream.App/Rendering/GameWindow.cs)). ACE accepts the second Use because we're now in range. **This is what we're retiring.**
|
||||
|
||||
The retail client and holtburger's Rust client don't have this bug: neither synthesizes player-input MoveToState during inbound MoveToObject. Their auto-walk is body-level only; ACE's chain runs uninterrupted; the callback fires on arrival.
|
||||
|
||||
### Retail anchor
|
||||
|
||||
- ACE `Player_Use.cs:205` — `CreateMoveToChain(item, (success) => TryUseItem(item, success))`.
|
||||
- ACE `Player_Move.cs:150` — chain polls and fires `callback(true)` when within use radius.
|
||||
- Holtburger [`simulation.rs:178-206`](../../../references/holtburger/crates/holtburger-core/src/client/simulation.rs) — `MoveToObject` handler returns `Ok(Vec::new())`, no outbound packets in response. No retry mechanism anywhere in its codebase.
|
||||
|
||||
## Design
|
||||
|
||||
Single-flag suppression of the outbound `MoveToState` send while a server-initiated auto-walk is active. The body keeps walking locally (synth still drives the animation cycle + per-frame position update), `AutonomousPosition` keeps flowing (so ACE's `WithinUseRadius` poll sees us approach), but the misleading "user is RunForward" packet is silenced.
|
||||
|
||||
### Wire-level changes
|
||||
|
||||
**File: [`src/AcDream.App/Input/PlayerMovementController.cs`](../../../src/AcDream.App/Input/PlayerMovementController.cs)**
|
||||
|
||||
No new field needed — [`IsServerAutoWalking`](../../../src/AcDream.App/Input/PlayerMovementController.cs:273) already exists, added by B.6 slice 2 (2026-05-14) for this purpose. Backs onto private `_autoWalkActive` (line 254). Set true by `BeginServerAutoWalk`, false by `EndServerAutoWalk`. User-input cancellation at line 478-482 already clears the flag, so manual takeover transitions correctly to "user is in control, send MoveToState normally."
|
||||
|
||||
**File: [`src/AcDream.App/Rendering/GameWindow.cs`](../../../src/AcDream.App/Rendering/GameWindow.cs)**
|
||||
|
||||
Guard the `MoveToState.Build` block at lines 6410-6445 with a second condition:
|
||||
|
||||
```csharp
|
||||
if (result.MotionStateChanged && !_playerController.IsServerAutoWalking)
|
||||
{
|
||||
// ... existing MoveToState build + SendGameAction ...
|
||||
}
|
||||
```
|
||||
|
||||
No other path needs adjustment; the AutonomousPosition path remains unchanged (heartbeat-driven, gated on `Contact && OnWalkable`).
|
||||
|
||||
Single-clause add; `IsServerAutoWalking` already documented at line 267-273 as the predicate for "the next Update synthesizes Forward+Run."
|
||||
|
||||
### Workaround retirement
|
||||
|
||||
After the wire fix is verified, the following Commit B safeguards are deleted:
|
||||
|
||||
1. **Far-range Use retry.** In [`GameWindow.SendUse`](../../../src/AcDream.App/Rendering/GameWindow.cs) (~line 9201-9204), remove the `_pendingPostArrivalAction = (guid, false);` assignment from the far-range path and rewrite the comment block to say "ACE auto-fires Use via MoveToChain callback; client sends Use exactly once."
|
||||
|
||||
2. **Far-range PickUp retry.** Same shape in [`GameWindow.SendPickUp`](../../../src/AcDream.App/Rendering/GameWindow.cs) (~line 9265).
|
||||
|
||||
3. **Log strings.** Drop `(queued for arrival re-send pending #63)` from the same paths.
|
||||
|
||||
4. **AP cadence revert.** Replace the per-frame `positionChanged` gate in [`PlayerMovementController.cs:1261-1275`](../../../src/AcDream.App/Input/PlayerMovementController.cs:1261) with retail's narrow gate per `acclient_2013_pseudo_c.txt:700233-700285`:
|
||||
|
||||
```csharp
|
||||
// During sub-interval: send only on cell-or-contact-plane change.
|
||||
// After interval elapses: send only if position/frame changed.
|
||||
// Retail-faithful per acclient_2013_pseudo_c.txt:700233 ShouldSendPositionEvent.
|
||||
bool intervalElapsed = !_lastSentInitialized
|
||||
|| (_simTimeSeconds - _lastSentTime) >= HeartbeatInterval;
|
||||
|
||||
bool cellChanged = _lastSentCellId != CellId;
|
||||
bool planeChanged = !_lastSentContactPlane.Equals(_body.ContactPlane);
|
||||
bool frameChanged = !ApproxPositionEqual(_lastSentPos, _body.Position);
|
||||
|
||||
bool sendThisFrame = intervalElapsed
|
||||
? (cellChanged || frameChanged)
|
||||
: (cellChanged || planeChanged);
|
||||
|
||||
HeartbeatDue = _body.InContact && _body.OnWalkable && sendThisFrame;
|
||||
```
|
||||
|
||||
`_lastSentContactPlane` requires adding a `System.Numerics.Plane _lastSentContactPlane;` field next to the other `_lastSent*` fields. `System.Numerics.Plane` is a struct with `IEquatable<Plane>` so direct `!_lastSentContactPlane.Equals(_body.ContactPlane)` works. Extend `NotePositionSent` to accept a `Plane` and stamp it alongside position/cell/time. The outbound layer at `GameWindow.cs:6410-6445` (MoveToState) and `~line 6310` (AutonomousPosition) both gain a `contactPlane: _body.ContactPlane` argument.
|
||||
|
||||
### Issues closed
|
||||
|
||||
- **#63** — server-initiated auto-walk not honored. (Both halves: B.6 slice 2 shipped the inbound handling on 2026-05-14; this commit fixes the MoveToChain callback-cancellation gap.)
|
||||
- **#74** — AP cadence chattier than retail. (Direct revert to retail's narrow gate enabled by the workaround retirement.)
|
||||
|
||||
## Out of scope (file as new issues)
|
||||
|
||||
- **Retail status messages** ("Approaching {target}" / "Using the {target}" on screen). Client-side UX polish; not wire-critical. Mentioned by user during brainstorm 2026-05-16 as nice-to-have to match retail feel. File as `#75 — Retail-faithful pending-action status messages on screen`.
|
||||
|
||||
## Testing plan
|
||||
|
||||
### Unit tests
|
||||
|
||||
No new unit tests required. The behavioral change is wire-level integration; existing Core.Net 294 baseline must hold. Existing 1073 Core tests baseline must hold (8 pre-existing physics failures unchanged).
|
||||
|
||||
### Visual verification (user-driven)
|
||||
|
||||
1. **Far-range Use NPC.** Double-click a Pathwarden / Royal Guard ~8-15 m away. Expected log shape:
|
||||
```
|
||||
[B.4b] use guid=0x... seq=X
|
||||
[autowalk-mt] mt=0x06 isMoveTo=True ...
|
||||
[autowalk-begin] dest=...
|
||||
[autowalk-end] reason=arrived
|
||||
```
|
||||
**Expected: NO `[B.4b] use-deferred` line.** Dialogue fires from ACE's callback. One Use packet on the wire, not two.
|
||||
|
||||
2. **Far-range PickUp item.** F-key a ground item ~5-10 m away. Same shape — one PickUp, no retry.
|
||||
|
||||
3. **Close-range Use NPC behind player.** Within 3 m of an NPC facing away, press R. Body turns 180°, then dialogue fires. The close-range deferred path is unchanged by this work; should still produce:
|
||||
```
|
||||
[B.4b] use deferred (close-range, turn-first)
|
||||
[autowalk-end] reason=arrived
|
||||
[B.4b] use-deferred
|
||||
```
|
||||
(`use-deferred` is correct here — close-range deferral is retail-faithful turn-first, not a workaround.)
|
||||
|
||||
4. **Open inn door from across the room.** Walks, opens. ONE `[B.4b] use` line, no retry, no double-open.
|
||||
|
||||
5. **User takes manual control mid-auto-walk.** Click far NPC → during the walk, press W. Auto-walk cancels via `user-input`, normal MoveToState resumes firing. The action does NOT fire on arrival (user cancelled).
|
||||
|
||||
6. **Rapid click between two targets.** Click NPC, before arrival click a second NPC. ACE re-broadcasts MoveToObject with the new target. Our overlay re-targets (existing `BeginServerAutoWalk` overwrite). Action fires on arrival at the SECOND target. No stale `_pendingPostArrivalAction` from the first click (close-range path keeps its handling; far-range no longer queues anything).
|
||||
|
||||
### Pre-conditions
|
||||
|
||||
- ACE running on `127.0.0.1:9000`.
|
||||
- `+Acdream` character at Holtburg (test character).
|
||||
- `ACDREAM_PROBE_AUTOWALK=1` for the trace lines.
|
||||
- Build green: `dotnet build src/AcDream.App/AcDream.App.csproj -c Debug`.
|
||||
|
||||
## Acceptance criteria
|
||||
|
||||
- All six visual-verify scenarios pass.
|
||||
- No `use-deferred` log line for FAR-range Use (only for close-range turn-first defer, which is correct).
|
||||
- Core.Net tests pass (294/294).
|
||||
- Core tests baseline holds (1073/1081).
|
||||
- Issues #63 and #74 close with this commit's SHA.
|
||||
|
||||
## Risk + rollback
|
||||
|
||||
**Risk:** If ACE's `MoveToChain` somehow STILL doesn't fire the callback (e.g., a separate ACE bug we haven't identified), Use action breaks for far-range targets. The user would observe "walked to NPC, no dialogue."
|
||||
|
||||
**Rollback:** trivial git revert. The fix is one guard clause + workaround deletions. Reverting restores the working-but-chatty Commit B state at `b5da17d`.
|
||||
|
||||
**Detection during visual verify:** if scenario 1 fails (no dialogue after arrival), revert before merging.
|
||||
|
||||
## Plan handoff
|
||||
|
||||
This design is the input for `superpowers:writing-plans`, which produces the task-by-task plan with code blocks ready to paste.
|
||||
Loading…
Add table
Add a link
Reference in a new issue