acdream/docs/research/2026-08-04-c4-route-5-retail-review-round3.md
Erik 36255af0f6 fix(physics): C4 route 5 — projectile authoritative placement (#276 partial)
Ports retail's missile Position handling into the canonical Runtime
placement owner instead of the deleted ApplyAuthoritativePosition
short-circuit. The Create/residence-window halves of the projectile
pipeline (RuntimeProjectile binding, TryBind's adopted-body branch,
the collision/shadow registration) were already canonical from prior
slices; this closes the remaining gap — how an ACCEPTED Position for
an in-flight missile is classified, placed, and presented.

Byte-decode (Step 1 hard gate, before any code was written):
CPhysicsObj::MoveOrTeleport @0x00516330-0x00516438 disassembled from
the PDB-paired binary (Capstone, x86 32-bit thiscall). `ret 0x10`
establishes four stack args; [esp+0x7c] (arg5, the velocity pointer)
is never referenced in any of the three branches (teleport/near/far).
The retail reviewer independently reproduced this by searching the
whole function body for the `24 7c` mod/rm+disp8 encoding a
`[esp+0x7c]` read would require and found zero occurrences. This
retired a fabricated `?? Vector3.Zero` fallback in the deleted method
— retail's PositionPack::UnPack initializes an absent velocity to
zero and MoveOrTeleport never installs it; the projectile's Vector
channel (RuntimeProjectilePhysicsUpdater.ApplyAuthoritativeVector)
remains the sole velocity authority for a missile. D-P5 in the
contract; the Runtime seam commits no velocity from the Position
packet at all.

The unbound-missile fix: RuntimeEntityObjectLifetime's
ClassifyRemoteAcceptedPosition now derives ProjectileAuthoritative
from a CONJUNCTIVE predicate — the Missile bit AND a bound
RuntimeProjectile whose Body is the canonical PhysicsBody — never the
bit alone. Retail places every non-player CPhysicsObj unconditionally
(there is no missile-specific placement gate in MoveOrTeleport or its
callers), so an unbindable or not-yet-bound missile taking the
ordinary remote tail is retail-faithful, not a fallback: the earlier
bit-only discriminator would have silently frozen it instead.

AP-141 records this as a deliberate, recorded divergence, not
fidelity. Retail mechanically WOULD arm a missile's ConstrainTo leash
on any nonzero MoveOrTeleport return: HandleReceivedPosition
@0x00453FD0's only kind test is player-vs-not, ConstrainTo
@0x00454272 has no kind test of its own, and CPhysicsObj::ConstrainTo
@0x00510520 creates a PositionManager on demand via
MakePositionManager @0x00510523 if one doesn't exist. acdream
deliberately does not construct that EntityPhysicsHost/
PositionManager/InterpolationManager chain for a ballistic body — the
route-5b split the C4 route 5 contract rejected — so a live missile
never shows an armed leash and never catches up via the near/
UnroutedCatchUp policy. This divergence is safe specifically because
ACE never sends UpdatePosition for a missile
(references/ACE/Source/ACE.Server/WorldObjects/WorldObject_Tick.cs:
333-334, SendUpdatePosition() commented out inside the
PhysicsState.Missile branch at :265) — every half of this row is
deterministic-test-gated only, never exercised against a real server.

AP-141 also records the surviving ConstrainTo re-anchor divergence
under clause (b): for the adopted-body case (TryBind's shared-body
branch — an ordinary remote whose Missile bit is set by a later
State packet, so it still carries a live RemoteMotion), acdream now
ports retail's teleport-branch and far-branch StopInterpolating
action (Interp.Clear()), but never re-arms or re-anchors the
inherited ConstrainTo leash the way retail's HandleReceivedPosition
@0x00454254/@0x00454272 does on every nonzero return. The risk
column's earlier wording — that a stale leash "would drag the body
toward a stale anchor" — was wrong and is retracted in this same
commit: ConstraintManager.ConstraintPos is write-only in both retail
and the port (never read by AdjustOffset), and
ConstraintManager::adjust_offset @0x00556180 only tapers or zeroes an
already-composed per-tick offset while InContact — a leash brakes
motion the interp/sticky chain already produced, it cannot pull
anything toward the anchor. The real residual is one tick of un-reset
brake accumulator, contact-gated, and it cannot move an airborne
far-snapped missile at all (the clamp branch does not run while
airborne).

NO CONNECTED GATE EXISTS for this route, by design: ACE never sends a
missile UpdatePosition (see above), so retail's own server never
exercises this code path in play. Every proof obligation here is
test-gated only — Runtime and App-level fixtures constructing the
packet directly — never a live client/server capture.

Three review rounds closed 8 MAJOR findings before this landed:
round 1 (A1 App discarded the seam's status; A2/R1 silent swallow on
an unbound missile; A3/R2 the adopted-body teleport_hook never
wired; A4/A5 zero Runtime/App test coverage); round 2 (a
ParentCellId regression introduced by round 1's own R6 finding,
which the retail reviewer retracted the following round as factually
wrong — the fix here is the REVERT to record.FullCellId, not the
relocation round 1 shipped; B2 the far-branch StopInterpolating skip
never extended to the adopted-body case; residual App/Runtime store-
path coverage; a per-packet closure contradicting the file's own
#315 cached-delegate pattern). Round 3 closed on coverage alone (no
defect): the Advance() retry arm's projectile branch — added at
round 2, semantically reordered at round 2's B5 fix (skip prediction
invalidation on a re-parked Contention, since it writes nothing) —
had never been executed by any test; two new tests drive it directly
and are sabotage-verified against both the reordering and the
retry-arm's own SyncProjectilePresentation call site. The one
recorded defect this campaign produced (the ParentCellId regression)
was caused by complying with a review finding that its own author
later retracted — the standing lesson recorded for future rounds is
that review findings are evidence to re-verify against the code, not
commands to obey unconditionally.

Complete Release suite: 11,063 passed / 4 skipped / 0 failed
(baseline 11,036 at 30d3d114, +27 new tests across this campaign).

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2026-08-04 21:03:41 +02:00

15 KiB
Raw Blame History

C4 route 5 — projectile authoritative placement: retail-conformance review, ROUND 3 (narrow delta)

Reviewer lens: retail fidelity only. Delta against round 2 (FAIL: B1, B2), which the architecture reviewer corroborated independently as its B2 and B1.

Subject: the uncommitted working tree, HEAD 30d3d114.


VERDICT: PASS, with one MUST-FIX-IN-COMMIT documentation correction (C1)

Both round-2 MAJORs are closed, and closed well. B1's revert is right and its regression test is self-verifying rather than merely sabotage-checked. B2's fix is placed in the correct layer, in the correct order, with the correct guard.

The one required edit is a single sentence in AP-141's risk column that asserts a consequence neither retail nor acdream has — and I wrote the mistake it was copied from. Round 2's B2 claimed a stale leash would "drag the freshly-placed missile back". That is wrong: acdream's ConstraintManager port brakes, it never pulls. I retract it in §C1 and state what the residual actually is. Because register rows land in the same commit as the behaviour they describe, this is a must-fix now, not a follow-up.


A. The four items you asked me to verify

A1 — B1's revert: is the reasoning now right, not just the value?

Yes. PASS on both.

ProjectileController.SyncPresentationFromResolvedBody now writes entity.ParentCellId = record.FullCellId. I re-derived each of the four claims in the rewritten doc block rather than accepting them:

claim in the comment verified
on a committed outcome the two sources agree (CommitCanonical writes both) ✓ — the choice is a genuine no-op there
StoreAcceptedDestinationPose composes body.Position from accepted.PositionX + worldOffset(accepted.LandblockId) — the wire cell's frame ✓ read the method; it writes only Position/Orientation, offset from accepted.LandblockId
record.FullCellId is that same wire cell, stamped by the merge before classification RuntimeEntityRecord.RefreshDerivedStateSetFullCell(position.LandblockId, …) at :232-237
the body's own CellPosition.ObjCellId is the source cell it left ✓ untouched by the store fallback
cross-check (1): the sibling remote arm pairs wire position with wire cell TryApplyGenericRemoteRenderPoseentity.ParentCellId = landblockId
cross-check (2): Runtime's SyncProjectilePresentation publishes the shadow at record.FullCellId in the same packet ✓ — reading the body's cell here would disagree with the shadow for the same body in the same call
cross-check (3): retail's store_position @0x00515CE2 writes the whole Position including objcell_id, so after a retail store the object's cell IS the destination

The comment also correctly names the body's stale post-store cell as the acdream-side residual (AP-138, shared with the remote arms) rather than truth to project. The reasoning is sound and matches the code beside it.

A2 — the indoor-staging test: does the discrimination argument hold, and is it legitimate?

Yes to both. This is the strongest test in the change.

I verified the mechanism at PhysicsBody.SyncCellPositionDelta (PhysicsBody.cs:286-306), which runs whenever Position is written:

if ((cell & 0xFFFFu) is not (>= 1u and <= 0x40u))
{
    CellPosition = new Position(cell, new CellFrame(local, ));   // indoor: id PINNED
    return;
}
uint adjusted = cell;
if (LandDefs.AdjustToOutside(ref adjusted, ref local))            // outdoor: id RE-DERIVED
    CellPosition = new Position(adjusted, );

So the implementer's account is exactly right: an outdoor source cell (index 1..0x40) has its cell id re-derived — including the 192 m landblock wrap — by AdjustToOutside as a side effect of the store's position write, so body.CellPosition.ObjCellId converges toward the destination landblock on its own and the two expressions stop discriminating. An indoor source cell takes the early-return branch: the delta is carried into the local frame and the id stays pinned. That is the only staging in which body.CellPosition.ObjCellId and record.FullCellId provably differ across the store path.

Legitimate, not contrived. IndoorSourceCell = SourceLandblock | 0x0100u is the canonical first EnvCell in AC's cell-id encoding — outdoor landcells occupy 0x00010x0040, EnvCells start at 0x0100 — so this is an ordinary dungeon cell, not a magic number chosen to break an assertion. A missile in a dungeon is an ordinary scenario, and the branch it exercises is the one production takes indoors.

Better than the sabotage run the implementer also did: the test asserts the divergence occurred before asserting the outcome —

Assert.Equal(IndoorSourceCell, body.CellPosition.ObjCellId);   // the divergence is real
Assert.Equal(DestinationCell, fixture.Entity.ParentCellId);    // and ParentCellId ignored it

— so if staging ever stops discriminating (a future change makes the store write the cell, say), the test fails loudly instead of silently going vacuous. That is a self-verifying discriminator, which is the right answer to "a green suite is not evidence".

A3 — B2's split: is "queue cleared, leash still armed" retail's far-branch behaviour?

Two of the three halves are retail. The third is not, and it is now recorded — so the outcome is acceptable, but the plain answer to your question is: no, retail does not leave the leash as-is.

Retail's far path, in order:

005163c1  position_manager = this_1->position_manager;
005163c9  if (position_manager != 0)
005163cb      PositionManager::StopInterpolating(position_manager);
005163d9  CPhysicsObj::SetPositionSimple(this_1, arg2, 1);
005163e8  return 1;
  ↓ back in SmartBox::HandleReceivedPosition
00454254  if (MoveOrTeleport(...) != 0) {
00454258      GetMaxConstraintDistance / GetStartConstraintDistance
00454272      ConstrainTo(arg2, &arg2->m_position, start, max);   ← re-anchor
          }
half retail acdream far arm verdict
clear the interpolation queue yes, whenever position_manager != 0 @0x005163CB route.StopInterpolating && record.RemoteMotion is RemoteMotion → adopted.Interp.Clear() correct
UnConstrain no — the far branch never calls teleport_hook not run correct; the test's "proving the far branch really does run only StopInterpolating, not the full hook" is right
re-anchor the leash yes&arg2->m_position is the object's just-updated position, so retail re-anchors on every nonzero return not run divergent

The fix's placement is right in every other respect: it lives in the Runtime seam (not App), runs strictly before TryExecuteAcceptedRemotePosition (mirroring @0x005163CB before @0x005163D9), uses the same route.StopInterpolating gate and the same Interp.Clear() mapping the sibling ApplyAcceptedRemoteFarSnap uses, and the storing partition still runs afterwards so the 4b-2 "cleared-then-frozen" hazard cannot reappear. record.RemoteMotion is RemoteMotion is the right analogue of retail's position_manager != 0 here, because Interp is a non-null field of RemoteMotion — the same predicate the remote far arm relies on.

What the missing re-anchor actually costs — correcting round 2. See §C1. It is one tick of brake-taper state, contact-gated, and cannot move the body.

A4 — AP-141: does the row describe the shipped code exactly?

Almost. The divergence description is now accurate; one sentence of the risk column is not (C1).

Verified accurate:

  • the bare-missile far skip as faithful by consequence — retail's own position_manager != 0 guard @0x005163C9 skips it for a never-interpolated object ✓;
  • the adopted-body far clear as ported, matching the shipped route.StopInterpolating && record.RemoteMotion is RemoteMotion arm ✓;
  • clause (b) extended to say acdream never arms or re-anchors on any disposition, with @0x00454272 cited ✓ — this is the honest recording of the A3 residual;
  • the "NARROWED … the far-branch clause was factually wrong for the adopted-body case" preamble, which is the right way to retire a superseded claim rather than quietly rewriting it ✓;
  • clauses (a) and (c) unchanged and still accurate ✓.

A5 — #317

Discharges R8. PASS. Filed OPEN with the byte-decode citation (MoveOrTeleport @0x00516330-@0x00516438, every branch, velocity argument never read), the correct scope note (the 4a call left in production deliberately), a real root cause, and an acceptance criterion that names the right next step — auditing the whole accepted-Position velocity chain rather than just the one function. My round-2 judgment stands: deferring the audit was always fine; what was missing was the tracking, and it now exists.

Nit only: the body cites the call site as "~line 2444" in one paragraph and "~line 2420" in another. It names the symbol in both, which is what process rule 6 says to trust, so this is cosmetic.


B. Round-2 findings — delta status

ID status
B1 (ParentCellId) CLOSED — reverted, reasoning verified, self-verifying indoor regression test
B2 (far-branch adopted body) CLOSED for the StopInterpolating half; the ConstrainTo re-anchor half is now a recorded divergence in AP-141 clause (b) rather than an unrecorded one. Acceptable.
B3 (double report_collision_end) still present (App hook action 6 + Runtime seam's LeaveWorld). Benign — the second call finds an empty table. Not re-raised.
B4 (per-packet closure) CLOSED_remoteArmCallbacks.IsCurrentProjectilePositionOwner + _projectileArmPosition* scratch fields, the same cached shape #315 introduced for the remote arm. Comment now describes what the code does.
B5 (R8 tracking) CLOSED — #317
B6 (retry-arm store gap) unchanged; parity with the remote arm, pre-existing. Not re-raised.

C. Findings this round

C1 — MINOR, must fix in this commit — AP-141's risk column asserts a consequence neither retail nor acdream has, and I am the source of the error

Where: docs/architecture/retail-divergence-register.md, AP-141, risk column, final clause:

"…but is never re-anchored at the new position by either — if it survives un-cleared some other way, it would drag the body toward a stale anchor."

This is wrong, and it is my round-2 wording. I wrote that a leash anchored at the pre-snap position would "drag the freshly-placed missile back". I inferred it from retail's re-anchor existing, without reading acdream's port of what a leash does. The in-tree port says otherwise, explicitly:

  • ConstraintManager.ConstraintPos"+0x0c retail constraint_pos — the leash anchor. Stored by ConstrainTo, never read by AdjustOffset (retail + ACE — write-only in this class)."
  • ConstraintManager.AdjustOffset (retail ConstraintManager::adjust_offset @0x00556180) only brakes: while _host.InContact it tapers the already-composed per-tick offset between ConstraintDistanceStart and ConstraintDistanceMax, or zeroes it past max — then unconditionally overwrites ConstraintPosOffset with that tick's step length.

A leash therefore damps motion the interp/sticky chain already produced; it has no mechanism to move anything toward the anchor. The dragging half of round-2 B2 was the interpolation queue, which does move the body toward waypoints — and that half is now fixed.

What the missing re-anchor actually costs. Retail's ConstrainTo(arg2, &arg2->m_position, …) sets ConstraintPos to the object's just-written position and re-initialises ConstraintPosOffset = Distance(anchor, host.Position) = 0, i.e. it resets the brake accumulator at every accepted Position. acdream leaves ConstraintPosOffset at the previous tick's step length. The observable difference is confined to the single tick after the packet, only when the object is InContact, and only if that step length already exceeded ConstraintDistanceStart — and a far-snapped missile is airborne, where the clamp branch does not run at all.

Correct replacement for the sentence (substance, not wording): an adopted-body missile's inherited leash is never re-anchored, so its brake accumulator (ConstraintPosOffset) is not reset to zero at each accepted Position as retail's @0x00454272 re-anchor does; the anchor itself is write-only in both retail and the port, so a stale leash brakes rather than pulls and cannot move the body.

Keep the rest of the clause — "never re-anchored at the new position by either" is the accurate divergence and should stay.

C2 — MINOR (nit) — two comments the fix touched are slightly off

  1. ProjectileController.SyncPresentationFromResolvedBody's doc block uses <paramref name="record"/> twice; the parameter is named expectedRecord (record is a local from TryGetCurrent). The paramref will not resolve.
  2. MissileAdoptedBody_FarCommit_ClearsInterpQueueButLeavesConstraintArmed's doc says the leash "must stay armed (proving the far branch really does run only StopInterpolating, not the full hook)" — true and well-argued about the hook, but silent on @0x00454272, which is the half retail does run. One clause ("…armed but, unlike retail, not re-anchored — AP-141") keeps a future reader from reading the far branch's leash handling as fully faithful.

D. Re-verified retail, no change

Spot-checked because the fix touched the surrounding code:

  • MoveOrTeleport @0x00516330 far branch: position_manager != 0 guard @0x005163C9, StopInterpolating @0x005163CB, SetPositionSimple(…, 1) @0x005163D9, return 1 @0x005163E8 — the ported order is correct.
  • HandleReceivedPosition @0x00454254/@0x00454272 — the single post-operation arming site, no kind test, anchor is the object's own just-updated position.
  • teleport_hook @0x00514ED0 — six actions, five per-manager guarded; the far branch does not call it.
  • store_position @0x00515CE2 — writes objcell_id with the frame (basis of B1's revert).
  • ACE never sends a missile UpdatePosition (WorldObject_Tick.cs:333-334 inside the :265 Missile branch), so every residual here remains test-reachable only. Stated for calibration, not as a reason to ship anything.
  • AP-131 / AP-135 untouched; #276 not closed; docs/ISSUES.md gains only #317.

E. Summary

# Sev Where Action
C1 MINOR, must-fix-in-commit AP-141 risk column, final clause replace the "drag the body toward a stale anchor" claim — the anchor is write-only; a leash brakes, never pulls. Retracts my own round-2 wording.
C2 nit SyncPresentationFromResolvedBody paramref; far-adopted test doc one-line each

No code changes required. With C1 corrected, route 5 is retail-conformant on every path I have examined across three rounds.