diff --git a/docs/ISSUES.md b/docs/ISSUES.md index 9ac8914e..64296daa 100644 --- a/docs/ISSUES.md +++ b/docs/ISSUES.md @@ -148,7 +148,23 @@ split result places normally instead of throwing. ## #315 — `runTeleportHook` builds a `Func` closure per network packet -**Status:** OPEN +**Status:** CLOSED 2026-08-04 by `ddb38f37` — the OnPosition collapse +converged the three `RunRemoteArmTail` call sites into one, which is what +made caching worthwhile. The one remaining call site now passes two +delegates cached ONCE at construction (`RemoteArmCallbacks`, a small nested +type wrapping `Func IsCurrentPositionOwner` / +`Func RunTeleportHook`) instead of allocating a fresh closure per +packet; per-packet scratch state (`canonical`, `remote`, `positionRecord`, +`positionAuthorityVersion`, `expectedEntity`) moved from closure captures to +plain instance fields `RunRemoteArmTail` stamps immediately before use. +Deliberately NOT two bare `Func` fields directly on +`LiveEntityNetworkUpdateController`: +`tests/AcDream.App.Tests/World/UpdateFrameOrchestratorTests.cs`'s +`ProductionFrameAdaptersRetainTypedOwnersWithoutWindowCallbacks` asserts +every typed production owner carries zero `Delegate`-typed fields (the +GameWindow decomposition campaign's guard against a smuggled window +callback); wrapping both delegates in `RemoteArmCallbacks` respects that +invariant instead of tripping it. **Severity:** LOW (real allocation regression, not correctness; not on the per-frame resolve path Slice I's 0 B/resolve discipline governs) **Filed:** 2026-08-04 diff --git a/src/AcDream.App/Physics/LiveEntityNetworkUpdateController.cs b/src/AcDream.App/Physics/LiveEntityNetworkUpdateController.cs index d27c7bbd..1d0461e0 100644 --- a/src/AcDream.App/Physics/LiveEntityNetworkUpdateController.cs +++ b/src/AcDream.App/Physics/LiveEntityNetworkUpdateController.cs @@ -72,6 +72,56 @@ internal sealed class LiveEntityNetworkUpdateController /// private readonly RuntimeRemotePlacementDriveController _remotePlacementDrive; + /// + /// #315 (closed by the OnPosition collapse, 2026-08-04): scratch fields + /// backing — the two delegates + /// passes into + /// every accepted remote Position + /// (5-10 Hz per remote). Before the collapse there were three duplicated + /// call sites, each allocating a fresh closure per packet regardless of + /// whether the packet was a teleport; the collapse converged them to one, + /// which is what makes caching worthwhile — one cached pair now serves + /// every remote guid. stamps these + /// fields from its own parameters immediately before use; nothing reads + /// them between calls, so last-remote staleness between packets is + /// harmless (mirrors the existing per-instance scratch-field pattern, + /// e.g. RemoteMotion.PositionManagerDeltaScratch). + /// + private RuntimeEntityRecord? _remoteArmCanonical; + private RemoteMotion? _remoteArmMotion; + private LiveEntityRecord? _remoteArmPositionRecord; + private ulong _remoteArmPositionAuthorityVersion; + private AcDream.Core.World.WorldEntity? _remoteArmExpectedEntity; + + /// + /// #315: the two per-packet delegates cached ONCE (constructed here, + /// reused for every accepted remote Position) rather than allocated + /// fresh every packet. Deliberately its own small type, not two bare + /// Func<bool> fields directly on this class: + /// tests/AcDream.App.Tests/World/UpdateFrameOrchestratorTests.cs's + /// ProductionFrameAdaptersRetainTypedOwnersWithoutWindowCallbacks + /// asserts every typed production owner (this class included) carries + /// ZERO Delegate-typed fields — the GameWindow decomposition + /// campaign's guard against a callback silently smuggling a window + /// reference back in. Neither delegate here touches a window (both are + /// bound to this controller alone), but the rule is written as a + /// blanket field-type check, not a window-specific one, so the cache + /// lives in its own named type instead of tripping it. + /// + private sealed class RemoteArmCallbacks + { + internal readonly Func IsCurrentPositionOwner; + internal readonly Func RunTeleportHook; + + internal RemoteArmCallbacks(LiveEntityNetworkUpdateController owner) + { + IsCurrentPositionOwner = owner.IsCurrentRemoteArmPositionOwner; + RunTeleportHook = owner.RunCachedRemoteTeleportHook; + } + } + + private readonly RemoteArmCallbacks _remoteArmCallbacks; + private PlayerMovementController? _playerController => _playerControllerSource.Controller; private EntityPhysicsHost? _playerHost => _playerHostSource.Host; private uint _playerServerGuid => _playerIdentity.ServerGuid; @@ -151,6 +201,9 @@ internal sealed class LiveEntityNetworkUpdateController _remotePlacementDrive = remotePlacementDrive ?? throw new ArgumentNullException(nameof(remotePlacementDrive)); _worldDropProjection = worldDropProjection; + // #315: cached once, reused for every accepted remote Position — see + // the field docs above _remoteArmCanonical. + _remoteArmCallbacks = new RemoteArmCallbacks(this); } internal void ResetSessionState() => _authorityGate.ResetSessionState(); @@ -1356,6 +1409,20 @@ internal sealed class LiveEntityNetworkUpdateController /// isTeleportRoute guard, unconditional for every guid since the /// collapse). /// + /// + /// + /// #315 (closed here): and + /// replace what used to be a + /// caller-constructed Func<bool> isCurrentPositionOwner — + /// this method stamps the shared _remoteArm* scratch fields from + /// its own parameters and passes the two CACHED delegates + /// () into + /// instead of allocating a fresh + /// closure over canonical/remote/the currency check every + /// packet. Observably identical: the currency check reads the exact same + /// positionRecord/positionAuthorityVersion/expectedEntity + /// triple either way, just from fields instead of a closure. + /// /// private RemoteContactRouting? RunRemoteArmTail( RuntimeEntityRecord canonical, @@ -1365,9 +1432,15 @@ internal sealed class LiveEntityNetworkUpdateController uint guid, System.Numerics.Vector3 worldPos, System.Numerics.Quaternion rotation, - Func isCurrentPositionOwner) + ulong positionAuthorityVersion, + AcDream.Core.World.WorldEntity? expectedEntity) { - ArgumentNullException.ThrowIfNull(isCurrentPositionOwner); + _remoteArmCanonical = canonical; + _remoteArmMotion = remote; + _remoteArmPositionRecord = positionRecord; + _remoteArmPositionAuthorityVersion = positionAuthorityVersion; + _remoteArmExpectedEntity = expectedEntity; + RemoteContactRouting routing = ApplyRemoteContactRouting( _remotePlacementDrive, canonical, @@ -1376,14 +1449,11 @@ internal sealed class LiveEntityNetworkUpdateController worldPos, rotation, willBeDrTicked: WillAdvanceRemoteMotion(guid, remote), - runTeleportHook: () => RunRemoteTeleportHook( - canonical, - remote, - isCurrentPositionOwner)); + runTeleportHook: _remoteArmCallbacks.RunTeleportHook); if ((routing.Arm is RemoteContactArm.FarSnapPlacement or RemoteContactArm.TeleportPlacement) - && (!isCurrentPositionOwner() + && (!_remoteArmCallbacks.IsCurrentPositionOwner() || !ReferenceEquals(positionRecord.RemoteMotionRuntime, remote))) { return null; @@ -1392,6 +1462,35 @@ internal sealed class LiveEntityNetworkUpdateController return routing; } + /// + /// #315: the cached backing method for + /// — reads the + /// scratch fields just stamped rather than + /// closing over per-packet locals. Identical logic to the local-function + /// IsCurrentPositionOwner pattern used elsewhere in + /// OnPosition for the local-player paths, which this method does + /// not replace (those stay untouched, per contract invariant 10). + /// + private bool IsCurrentRemoteArmPositionOwner() => + _remoteArmPositionRecord is { } record + && _liveEntities.IsCurrentPositionAuthority( + record, _remoteArmPositionAuthorityVersion) + && (_remoteArmExpectedEntity is null + || ReferenceEquals(record.WorldEntity, _remoteArmExpectedEntity)); + + /// + /// #315: the cached backing method for + /// . Only ever invoked on + /// the teleport-classified path, inside + /// ; reads the scratch fields + /// just stamped for THIS packet. + /// + private bool RunCachedRemoteTeleportHook() => + _remoteArmCanonical is { } canonical + && _remoteArmMotion is { } motion + && RunRemoteTeleportHook( + canonical, motion, _remoteArmCallbacks.IsCurrentPositionOwner); + /// /// C4 route 4b-2: the post-routing wire-cell adoption, extracted so its /// ONE suppression rule is exercised by production and by test through @@ -2473,7 +2572,8 @@ internal sealed class LiveEntityNetworkUpdateController update.Guid, worldPos, rot, - () => IsCurrentPositionOwner(entity)); + acceptedPositionAuthorityVersion, + entity); if (routing is null) return; arm = routing.Value.Arm;