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>
This commit is contained in:
parent
30d3d114b0
commit
36255af0f6
19 changed files with 5390 additions and 393 deletions
|
|
@ -93,6 +93,21 @@ internal sealed class LiveEntityNetworkUpdateController
|
|||
private ulong _remoteArmPositionAuthorityVersion;
|
||||
private AcDream.Core.World.WorldEntity? _remoteArmExpectedEntity;
|
||||
|
||||
/// <summary>
|
||||
/// B4 fix (C4 route 5 round-2 architecture review): the same #315
|
||||
/// cached-delegate discipline for the adopted-body missile arm's
|
||||
/// teleport-hook currency check
|
||||
/// (<see cref="RemoteArmCallbacks.IsCurrentProjectilePositionOwner"/>).
|
||||
/// The missile dispatch block stamps these scratch fields immediately
|
||||
/// before calling <see cref="RunRemoteTeleportHook"/> instead of
|
||||
/// allocating a fresh <c>Func<bool></c> closure every accepted
|
||||
/// missile packet — the same defect class the remote arm's own #315
|
||||
/// collapse already closed, flagged independently by both round-2
|
||||
/// reviews for this arm.
|
||||
/// </summary>
|
||||
private LiveEntityRecord? _projectileArmPositionRecord;
|
||||
private ulong _projectileArmPositionAuthorityVersion;
|
||||
|
||||
/// <summary>
|
||||
/// #315: the two per-packet delegates cached ONCE (constructed here,
|
||||
/// reused for every accepted remote Position) rather than allocated
|
||||
|
|
@ -113,10 +128,22 @@ internal sealed class LiveEntityNetworkUpdateController
|
|||
internal readonly Func<bool> IsCurrentPositionOwner;
|
||||
internal readonly Func<bool> RunTeleportHook;
|
||||
|
||||
/// <summary>
|
||||
/// B4 fix: the projectile (missile) arm's currency check, cached
|
||||
/// the same way as the two remote-arm delegates above rather than
|
||||
/// allocated fresh per accepted missile packet. Bound to
|
||||
/// <see cref="IsCurrentProjectileArmPositionOwner"/>, which reads
|
||||
/// the <c>_projectileArmPosition*</c> scratch fields the missile
|
||||
/// dispatch block stamps immediately before use.
|
||||
/// </summary>
|
||||
internal readonly Func<bool> IsCurrentProjectilePositionOwner;
|
||||
|
||||
internal RemoteArmCallbacks(LiveEntityNetworkUpdateController owner)
|
||||
{
|
||||
IsCurrentPositionOwner = owner.IsCurrentRemoteArmPositionOwner;
|
||||
RunTeleportHook = owner.RunCachedRemoteTeleportHook;
|
||||
IsCurrentProjectilePositionOwner =
|
||||
owner.IsCurrentProjectileArmPositionOwner;
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -1478,6 +1505,25 @@ internal sealed class LiveEntityNetworkUpdateController
|
|||
&& (_remoteArmExpectedEntity is null
|
||||
|| ReferenceEquals(record.WorldEntity, _remoteArmExpectedEntity));
|
||||
|
||||
/// <summary>
|
||||
/// B4 fix: the cached backing method for
|
||||
/// <see cref="RemoteArmCallbacks.IsCurrentProjectilePositionOwner"/>.
|
||||
/// Reads the <c>_projectileArmPosition*</c> scratch fields the missile
|
||||
/// dispatch block in <c>OnPosition</c> stamps immediately before calling
|
||||
/// <see cref="RunRemoteTeleportHook"/> for the adopted-body case. No
|
||||
/// expected-entity check here — unlike the remote arm, the missile
|
||||
/// dispatch block never captured one; parity with the remote arm's
|
||||
/// extra guard is not required because the caller already re-validates
|
||||
/// <c>ReferenceEquals(positionRecord.WorldEntity, entity)</c> earlier in
|
||||
/// the SAME accepted-Position dispatch for the non-missile tail, and
|
||||
/// the missile tail's own record is the SAME instance stamped here —
|
||||
/// there is no second entity to disagree with.
|
||||
/// </summary>
|
||||
private bool IsCurrentProjectileArmPositionOwner() =>
|
||||
_projectileArmPositionRecord is { } record
|
||||
&& _liveEntities.IsCurrentPositionAuthority(
|
||||
record, _projectileArmPositionAuthorityVersion);
|
||||
|
||||
/// <summary>
|
||||
/// #315: the cached backing method for
|
||||
/// <see cref="RemoteArmCallbacks.RunTeleportHook"/>. Only ever invoked on
|
||||
|
|
@ -2102,27 +2148,113 @@ internal sealed class LiveEntityNetworkUpdateController
|
|||
: new System.Numerics.Quaternion(p.RotationX, p.RotationY, p.RotationZ, p.RotationW);
|
||||
_movementTruthDiagnostics.OnServerEcho(update, worldPos);
|
||||
|
||||
// Missiles reconcile the same predicted PhysicsBody in place. The
|
||||
// timestamp gate above already rejected stale corrections; returning
|
||||
// here prevents the generic remote locomotion path from allocating a
|
||||
// second body or interpolation owner for the projectile.
|
||||
if (_projectileController?.ApplyAuthoritativePosition(
|
||||
acceptedPositionRecord,
|
||||
acceptedPositionAuthorityVersion,
|
||||
acceptedPositionVelocityAuthorityVersion,
|
||||
worldPos,
|
||||
new System.Numerics.Vector3(
|
||||
p.PositionX,
|
||||
p.PositionY,
|
||||
p.PositionZ),
|
||||
rot,
|
||||
acceptedSpawn.Physics?.Velocity
|
||||
?? System.Numerics.Vector3.Zero,
|
||||
p.LandblockId,
|
||||
_physicsScriptGameTime,
|
||||
_origin.CenterX,
|
||||
_origin.CenterY) == true)
|
||||
// C4 route 5 (D-P1/D-P6, REVISED after the review round — A2/R1,
|
||||
// A9): classify ONCE, kind-aware. This single call now decides both
|
||||
// the remote route (unchanged for a non-missile packet — see the
|
||||
// reuse below) AND whether this packet is a missile packet,
|
||||
// replacing the former ApplyAuthoritativePosition short-circuit.
|
||||
// The null-classification arm's test is the SAME conjunctive
|
||||
// predicate the classifier itself applies
|
||||
// (RuntimeEntityObjectLifetime.ClassifyRemoteAcceptedPosition,
|
||||
// D-P1) — Missile bit AND a bound RuntimeProjectile whose Body is
|
||||
// the canonical PhysicsBody, never the bit alone, so an unbindable
|
||||
// or not-yet-bound missile takes the ordinary remote tail exactly
|
||||
// as it did before this route (the deleted method's TryGetCurrent
|
||||
// fall-through) — and explicitly fenced off the local player (A9):
|
||||
// update.Guid == _playerServerGuid always takes the null branch
|
||||
// below, and ACE never sets Missile on a player, but the fence
|
||||
// makes that structurally true rather than incidentally true.
|
||||
RuntimeAuthoritativePositionRoute? earlyRemoteRoute =
|
||||
update.Guid != _playerServerGuid
|
||||
? ClassifyRemoteAcceptedPosition(
|
||||
update,
|
||||
acceptedPositionCanonical,
|
||||
timestampDisposition,
|
||||
timestamps,
|
||||
worldPos)
|
||||
: null;
|
||||
bool isMissilePacket = earlyRemoteRoute is { } classifiedRoute
|
||||
? classifiedRoute.OperationKind
|
||||
is RuntimeSetPositionOperationKind.ProjectileAuthoritative
|
||||
: update.Guid != _playerServerGuid
|
||||
&& (acceptedPositionCanonical.FinalPhysicsState
|
||||
& AcDream.Core.Physics.PhysicsStateFlags.Missile) != 0
|
||||
&& acceptedPositionCanonical.Projectile is { } boundProjectile
|
||||
&& ReferenceEquals(
|
||||
acceptedPositionCanonical.PhysicsBody,
|
||||
boundProjectile.Body);
|
||||
if (isMissilePacket)
|
||||
{
|
||||
// The projectile arm over the canonical Runtime placement owner
|
||||
// (RuntimeRemotePlacementDriveController.ApplyAcceptedProjectilePosition)
|
||||
// — never the generic remote locomotion path below, which would
|
||||
// allocate a second body or interpolation owner for the
|
||||
// projectile. A null classification (login-window shape) or a
|
||||
// RejectedAuthority/RejectedData disposition is swallowed:
|
||||
// nothing to route, the shared authority gate above already
|
||||
// rejected an invalid payload.
|
||||
if (earlyRemoteRoute is { } route)
|
||||
{
|
||||
// A3/R2 fix: retail's teleport_hook @0x00514ED0 runs five
|
||||
// manager-guarded actions BEFORE the placement, in addition
|
||||
// to the collision force-end the Runtime seam performs on
|
||||
// its own (action 6). For a bare arrow/bolt (no RemoteMotion
|
||||
// adopted) all five are structurally absent no-ops through
|
||||
// retail's own per-manager guards — matching what the
|
||||
// Runtime seam already does unaided. For the ADOPTED-BODY
|
||||
// case (TryBind's shared-body branch: an ordinary remote
|
||||
// whose Missile bit was set by a later State packet, so it
|
||||
// still carries a live RemoteMotion with a populated Interp
|
||||
// queue and possibly an armed ConstrainTo leash) those five
|
||||
// actions are LIVE and must run — using the SAME ordered
|
||||
// hook seam and per-packet currency check the remote
|
||||
// teleport arm already uses (RunRemoteTeleportHook, #315
|
||||
// pattern), so retail's per-manager guards decide for
|
||||
// themselves rather than being re-derived here.
|
||||
if (route.Disposition
|
||||
is RuntimeAuthoritativePositionDisposition.SetPosition
|
||||
&& acceptedPositionCanonical.RemoteMotion is RemoteMotion adoptedRemote)
|
||||
{
|
||||
// B4 fix (round-2 review): cache the currency-check
|
||||
// delegate the same way the remote arm's #315 collapse
|
||||
// already does, instead of allocating a fresh closure
|
||||
// every accepted missile packet. Scratch fields stamped
|
||||
// immediately before use; nothing reads them between
|
||||
// calls, so last-packet staleness is harmless.
|
||||
_projectileArmPositionRecord = acceptedPositionRecord;
|
||||
_projectileArmPositionAuthorityVersion =
|
||||
acceptedPositionAuthorityVersion;
|
||||
RunRemoteTeleportHook(
|
||||
acceptedPositionCanonical,
|
||||
adoptedRemote,
|
||||
_remoteArmCallbacks.IsCurrentProjectilePositionOwner);
|
||||
}
|
||||
|
||||
RuntimeRemotePlacementExecutionStatus? placementStatus =
|
||||
_remotePlacementDrive.ApplyAcceptedProjectilePosition(
|
||||
acceptedPositionCanonical,
|
||||
route);
|
||||
// A1/R5 fix: mirror Runtime's OWN presentation gate
|
||||
// (ApplyAcceptedProjectilePosition/SyncProjectilePresentation
|
||||
// — every outcome except Deferred/RejectedByPlacement) rather
|
||||
// than acknowledging unconditionally. Deferred already
|
||||
// snapped the body to the PARKED result and withdrew the
|
||||
// entity; RejectedByPlacement leaves the body exactly where
|
||||
// it was. Acknowledging either would move the render entity
|
||||
// to (or through) a pose/cell Runtime explicitly declined to
|
||||
// publish — the concrete Interpolate/RejectedByPlacement
|
||||
// wrong-cell scenario the review found.
|
||||
if (placementStatus is not null
|
||||
and not RuntimeRemotePlacementExecutionStatus.Deferred
|
||||
and not RuntimeRemotePlacementExecutionStatus.RejectedByPlacement)
|
||||
{
|
||||
_projectileController?.SyncPresentationFromResolvedBody(
|
||||
acceptedPositionRecord,
|
||||
_physicsScriptGameTime);
|
||||
}
|
||||
}
|
||||
return;
|
||||
}
|
||||
|
||||
if (!_liveEntities.TryGetRecord(
|
||||
update.Guid,
|
||||
|
|
@ -2136,26 +2268,18 @@ internal sealed class LiveEntityNetworkUpdateController
|
|||
return;
|
||||
}
|
||||
|
||||
// C4 route 4a: classify BEFORE the generic write below so a remote
|
||||
// whose accepted Position resolves to NoPositionOperation (retail's
|
||||
// airborne no-op — writes nothing at all) or Interpolate (retail's
|
||||
// near InterpolateTo queue — no direct body write here) never
|
||||
// receives it. The local player never reaches this generic-remote
|
||||
// code path at all. C4 route 4b-2 routes the >=96 m far snap and C4
|
||||
// route 4b-3 routes the teleport/cell-less classification through
|
||||
// the canonical Runtime placement owner (ApplyRemoteContactRouting);
|
||||
// a rejected authority or payload, and "no classification at all",
|
||||
// take the stated UnroutedCatchUp policy
|
||||
// (RuntimeRemoteFarSnapPosition.ResolveArm).
|
||||
RuntimeAuthoritativePositionRoute? earlyRemoteRoute =
|
||||
update.Guid != _playerServerGuid
|
||||
? ClassifyRemoteAcceptedPosition(
|
||||
update,
|
||||
acceptedPositionCanonical,
|
||||
timestampDisposition,
|
||||
timestamps,
|
||||
worldPos)
|
||||
: null;
|
||||
// C4 route 4a/5: `earlyRemoteRoute` was already classified above
|
||||
// (D-P6) — this is the SAME value, reused so a remote whose accepted
|
||||
// Position resolves to NoPositionOperation (retail's airborne
|
||||
// no-op — writes nothing at all) or Interpolate (retail's near
|
||||
// InterpolateTo queue — no direct body write here) never receives
|
||||
// the generic write below. The local player never reaches this
|
||||
// generic-remote code path at all. C4 route 4b-2 routes the >=96 m
|
||||
// far snap and C4 route 4b-3 routes the teleport/cell-less
|
||||
// classification through the canonical Runtime placement owner
|
||||
// (ApplyRemoteContactRouting); a rejected authority or payload, and
|
||||
// "no classification at all", take the stated UnroutedCatchUp
|
||||
// policy (RuntimeRemoteFarSnapPosition.ResolveArm).
|
||||
|
||||
TryApplyGenericRemoteRenderPose(
|
||||
entity,
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue