diff --git a/docs/research/2026-08-05-317-velocity-chain-audit.md b/docs/research/2026-08-05-317-velocity-chain-audit.md new file mode 100644 index 00000000..ac9c08a6 --- /dev/null +++ b/docs/research/2026-08-05-317-velocity-chain-audit.md @@ -0,0 +1,186 @@ +# #317 — accepted-Position velocity chain: retail audit (2026-08-06) + +Report-only per `CLAUDE.md`'s investigation rule. **No code changed, no fix +applied, no fix approved.** Written at HEAD `3aab05b0` (branch +`claude/acdream-physics-divergence-5aa784`). + +--- + +## Verdict: **NO RETAIL BASIS.** Velocity is a separate wire channel in retail, and acdream's Position path crosses it. + +Retail installs a remote body's velocity in exactly one function, gated on a +timestamp channel that an accepted Position never advances. acdream commits the +Position packet's velocity on *every* accepted Position. The correct retail +mechanism **already exists in acdream** on the VectorUpdate path, so the +Position-path call is both non-retail and redundant with a correct sibling. + +Severity stays **LOW** — no observable defect is known, and §5 lists what I +could *not* establish. This is not a "rip it out tonight" finding. + +--- + +## 1. Retail's velocity chain — verified by symbol + +### 1.1 The sole installer + +`SmartBox::DoVectorUpdate` @0x004521C0 (pseudo-C 91208). Body: + +``` +004521db wrap-safe compare of incoming stamp (edi) vs arg2->update_times[3] +004521e5 if (eax_4 > 0x7fff) c = edi < esi ; wrapped +004521e7 else c = esi < edi ; unwrapped +004521f5 if () +004521f7 arg2->update_times[3] = edi ; stamp VECTOR_TS +00452204 if (arg2 != this->player): +0045221e CPhysicsObj::set_velocity(arg2, arg3, 1) +0045222c CPhysicsObj::set_omega(arg2, arg4, 1) +00452204 else if (cmdinterp->UsePositionFromServer() != 0): +0045221e CPhysicsObj::set_velocity(arg2, arg3, 1) +0045222c CPhysicsObj::set_omega(arg2, arg4, 1) +``` + +`update_times[3]` is **VECTOR_TS** — a different channel from Position's +`update_times[0]` (POSITION_TS, stamped at @0x00454079/@0x00454084 in +`HandleReceivedPosition`). + +> **Binary Ninja artifact, noted for successors.** The gate renders as +> `if (-((eax_4 - eax_4)) != 0)` @0x004521F5 — always false — the same dropped +> flag-test artifact the C5b review hit in `HandleReceivedPosition`'s Gate A. +> The real predicate is the `c` computed at @0x004521E5–@0x004521E7. The +> neighbouring `SmartBox::HandlePlayerTeleport` @0x00452150 carries the +> identical artifact @0x00452186. **Do not read a comparison in this function +> family from the pseudo-C**; disassemble the PDB-paired binary. + +### 1.2 Its only two callers — neither is the Position path + +Exhaustive grep of `DoVectorUpdate(` across `acclient_2013_pseudo_c.txt`: + +| call site | enclosing function | +|---|---| +| @0x004534E6 (92225) | `SmartBox::HandleVectorUpdate` @0x00453480 — the dedicated VectorUpdate wire handler | +| @0x00454EE9 (93916) | `SmartBox::HandleCreateObject` @0x00454C80 — initial PhysicsDesc | + +**`SmartBox::HandleReceivedPosition` @0x00453FD0 is not among them.** + +### 1.3 The Position path's only `set_velocity` zeroes the LOCAL player + +@0x004541B4 (93029), inside `HandleReceivedPosition`'s teleport arm — +`set_velocity(player_2, &var_54, 1)` with a zero vector, after `TeleportPlayer`. +It is a local-player teleport reset, not a remote velocity install. This +confirms C4 route 5's finding rather than inheriting it. + +### 1.4 Retail *receives* the wire velocity and discards it on this path + +`PositionPack::UnPack` @0x00516740 (284585) parses origin, the flag-gated +quaternion components (bits 8 / 0x10 / 0x20), a **velocity** (written +@0x005167E9), and a `placement_id` (@0x00516828/@0x00516830). +`SmartBox::UnpackPositionEvent` @0x004542C0 unpacks into a stack `PositionPack` +(`var_68` @0x004542E2), resolves the object, and proceeds to the +`update_times[8]` compare. The parsed velocity is never routed to +`DoVectorUpdate` or `set_velocity`. + +**So retail's behaviour is deliberate, not accidental**: the byte is on the +wire, retail decodes it, and the Position path does not install it. Only a +VectorUpdate (or a CreateObject's PhysicsDesc) moves a remote's velocity. + +--- + +## 2. acdream's side + +**Call site:** `src/AcDream.App/Physics/LiveEntityNetworkUpdateController.cs:2459` +— on every accepted Position that passes the position/velocity authority +checks, commits `acceptedSpawn.Physics?.Velocity ?? Vector3.Zero` through +`_liveEntities.TryCommitAuthoritativeVelocity(...)`. + +**Method:** `LiveEntityRuntime.TryCommitAuthoritativeVelocity` +(`src/AcDream.App/World/LiveEntityRuntime.cs:2824`) → `TryCommitAuthoritativeVectorCore` +with `angularVelocity: null` → `_physics.TryCommitAuthoritativeVector(...)`. + +**The sibling that IS retail-correct:** `TryCommitAuthoritativeVector` +(`:2843`), whose own doc comment reads *"Commits the paired retail VectorUpdate +writes (`set_velocity`, then `set_omega`) to one canonical body."* That is +`DoVectorUpdate`'s exact shape, on the correct channel. + +**The wire does carry it.** `UpdatePosition` parses +`PositionFlags.HasVelocity = 0x01` → 3×f32 +(`src/AcDream.Core.Net/Messages/UpdatePosition.cs:47,132-136`), mirroring +retail's `PositionPack`. So acdream is not inventing data — it is installing +data retail decodes and drops. + +**The divergence, stated exactly:** acdream lets the POSITION channel write +`body.Velocity`; retail lets only the VECTOR channel (and CreateObject) do so. +A stale or unrelated velocity riding a Position packet is therefore installed +by acdream and ignored by retail. Note also that acdream's call passes +`?? Vector3.Zero` — a Position packet *without* `HasVelocity` actively zeroes +the body's velocity, which retail never does on this path. + +--- + +## 3. Issue text: accurate at HEAD + +`docs/ISSUES.md:340` is correct in every checked particular — the route-5 +disassembly finding, the corrected in-place comment (still present and accurate +at `LiveEntityNetworkUpdateController.cs:2435-2451`), and the "call left +unaudited" status. The `~line 2420` / `~line 2444` citations have drifted to +`:2435` and `:2459`. Nothing stale in substance. + +--- + +## 4. Recommended action — **NOT APPROVED** + +Two defensible outcomes; §5 decides which. + +**(a) Remove the call.** It is a channel crossing retail does not perform, and +the retail-correct mechanism already exists on the VectorUpdate path. This is +the honest fix *if* §5 shows nothing depends on it. + +**(b) Keep it and file a register row** as a deliberate acdream adaptation — +the same class as **AP-135**, which files acdream-only per-packet bookkeeping +writes on the airborne no-op branch. If ACE's Position cadence is the only +practical velocity source for remotes in our stack, a row is the honest +outcome and removal would be the regression. + +**Do not fold either into another slice.** This call sits on the hottest inbound +path and its removal is a behaviour change; it deserves its own gate. + +--- + +## 5. What I could NOT establish (flagged, not guessed) + +1. **What consumes `body.Velocity` for a remote, and whether removal regresses + anything.** This is the decisive input and it is unmeasured. Specifically + unresolved: whether **AP-80**'s velocity-derived animation cycle reads this + field for remotes. The call site's own comment says the Position-*delta* + velocity computed further down is "animation diagnostics, never substituted + into physics" — which implies this call is the one that does write physics, + but I did not trace the readers. +2. **Whether ACE actually sets `HasVelocity` on remote Position updates**, and + how often. If it never does, acdream's `?? Vector3.Zero` is the live + behaviour and the analysis above changes character — it becomes "acdream + zeroes remote velocity on every Position," which is a different and possibly + more consequential divergence than "acdream installs a stale one." A + WireMCP loopback capture against the local ACE answers this cheaply. +3. **Whether acdream's VectorUpdate path is wired and reached in production** + at all. If it is dormant, removing the Position-path call would leave remote + velocity with no writer, and (b) becomes correct by default. + +Each is a bounded follow-up. None requires the retail binary again — the retail +half of this audit is settled. + +--- + +## 6. Retail anchors cited + +| symbol | address | pseudo-C | +|---|---|---| +| `SmartBox::DoVectorUpdate` | 0x004521C0 | 91208 | +| — its `set_velocity` / `set_omega` | 0x0045221E / 0x0045222C | 91233 / 91234 | +| — its VECTOR_TS gate + stamp | 0x004521E5 / 0x004521F7 | — | +| `SmartBox::HandleVectorUpdate` | 0x00453480 (call @0x004534E6) | 92225 | +| `SmartBox::HandleCreateObject` | 0x00454C80 (call @0x00454EE9) | 93916 | +| `SmartBox::HandleReceivedPosition` | 0x00453FD0 | 92896 | +| — its only `set_velocity` (zeroes LOCAL player) | 0x004541B4 | 93029 | +| `SmartBox::UnpackPositionEvent` | 0x004542C0 | 93055 | +| `PositionPack::UnPack` | 0x00516740 | 284585 | +| — its velocity write | 0x005167E9 | — | +| BN dropped-flag artifacts | 0x004521F5, 0x00452186 | — |