From f47a5b55482070cfd16dbcc7b25d57266b178626 Mon Sep 17 00:00:00 2001 From: mriise Date: Fri, 8 May 2026 12:30:08 -0700 Subject: [PATCH 1/2] fix: Network Object ownership state machine current ownership handling required that we re-trigger the on-interact start when claiming ownership which would cause a double sfx play (and other bugs). This adds a simple state machine for pending requests so we can gate on state transition a bit more cleanly and still account for this case. --- .../Interactions/BasisPlayerInteract.cs | 20 --- .../BasisNetworkBehaviour.cs | 166 ++++++++++++++---- .../Networking/BasisNetworkOwnership.cs | 28 +-- .../Object Sync/BasisObjectSyncNetworking.cs | 122 ++++++------- .../com.basis.shim/Shims/BasisNetworkShim.cs | 2 +- 5 files changed, 199 insertions(+), 139 deletions(-) diff --git a/Basis/Packages/com.basis.framework/Interactions/BasisPlayerInteract.cs b/Basis/Packages/com.basis.framework/Interactions/BasisPlayerInteract.cs index 5b24785c70..5c6add199c 100644 --- a/Basis/Packages/com.basis.framework/Interactions/BasisPlayerInteract.cs +++ b/Basis/Packages/com.basis.framework/Interactions/BasisPlayerInteract.cs @@ -717,26 +717,6 @@ public static void DrawAll() } } - public bool ForceSetInteracting(BasisInteractableObject interactableObject, BasisInput input) - { - if (input.TryGetRole(out BasisBoneTrackedRole role) && interactableObject.Inputs.ChangeStateByRole(role, BasisInteractInputState.Hovering)) - { - for (int i = 0; i < InteractInputs.Length; i++) - { - if (InteractInputs[i].IsInput(input)) - { - BasisDebug.Log("Stole ownership, starting interact", BasisDebug.LogTag.Networking); - interactableObject.OnInteractStart(input); - InteractInputs[i].lastTarget = interactableObject; - } - } - - return true; - } - - return false; - } - public static bool IsDesktopCenterEye(BasisInput input) { return BasisDeviceManagement.IsUserInDesktop() && diff --git a/Basis/Packages/com.basis.framework/Networking/BasisNetworkBehaviour/BasisNetworkBehaviour.cs b/Basis/Packages/com.basis.framework/Networking/BasisNetworkBehaviour/BasisNetworkBehaviour.cs index 6854efcfcf..b23663f79a 100644 --- a/Basis/Packages/com.basis.framework/Networking/BasisNetworkBehaviour/BasisNetworkBehaviour.cs +++ b/Basis/Packages/com.basis.framework/Networking/BasisNetworkBehaviour/BasisNetworkBehaviour.cs @@ -5,9 +5,35 @@ using System; using System.Collections; using System.Text; +using System.Threading; using System.Threading.Tasks; using UnityEngine; using static BasisNetworkCommon; + +public enum NetworkOwnershipState +{ + /// + /// Ownership state is unknown/uninitialized or server-owned. + /// + Unknown, + /// + /// Owned by a remote source. + /// + Remote, + /// + /// Ownership request sent, awaiting confirmation. + /// + PendingClaim, + /// + /// Owned locally. + /// + Local, + /// + /// Ownership release requested, awaiting confirmation. + /// + PendingRelease, +} + namespace Basis { public abstract class BasisNetworkBehaviour : BasisNetworkContentBase @@ -19,16 +45,25 @@ public ushort NetworkID get => networkID; private set => networkID = value; } + public NetworkOwnershipState OwnershipState = NetworkOwnershipState.Unknown; /// - /// only set true when the server approves our ownership + /// True after server has confirmed ownership. + /// Equivalent to OwnershipState == Local. /// - public bool IsOwnedLocallyOnServer = false; + public bool IsOwnedLocallyOnServer => OwnershipState == NetworkOwnershipState.Local; /// - /// this is instantly set when we request ownership. + /// True from claim ownership start (PendingClaim) and + /// while the server agrees (Local). /// - public bool IsOwnedLocallyOnClient = false; + public bool IsOwnedLocallyOnClient => + OwnershipState == NetworkOwnershipState.Local + || OwnershipState == NetworkOwnershipState.PendingClaim; + public ushort CurrentOwnerId; public BasisNetworkPlayer currentOwnedPlayer; + private Task _pendingClaim; + private Task _pendingRelease; + private readonly CancellationTokenSource _destroyCts = new(); /// /// the reason its start instead of awake is to make sure progation occurs to everything no matter the net connect @@ -48,6 +83,7 @@ public virtual void Start() } public virtual void OnDestroy() { + _destroyCts.Cancel(); if (HasNetworkID) { BasisNetworkGenericMessages.UnregisterHandler(NetworkID); @@ -127,26 +163,36 @@ private void LowLevelOwnershipReleased(string uniqueEntityID) } private void LowLevelOwnershipTransfer(string uniqueEntityID, ushort NetIdNewOwner, bool isOwner) { - - if (uniqueEntityID == clientIdentifier) + if (uniqueEntityID != clientIdentifier) return; + Transition(isOwner ? NetworkOwnershipState.Local : NetworkOwnershipState.Remote); + CurrentOwnerId = NetIdNewOwner; + if (BasisNetworkPlayers.GetPlayerById(CurrentOwnerId, out currentOwnedPlayer)) { - IsOwnedLocallyOnServer = isOwner; - IsOwnedLocallyOnClient = isOwner; - CurrentOwnerId = NetIdNewOwner; - if (BasisNetworkPlayers.GetPlayerById(CurrentOwnerId, out currentOwnedPlayer)) - { - OnOwnershipTransfer(currentOwnedPlayer); - } - else - { - BasisUnInitalizedPlayer UnInitalizedPlayer = new BasisUnInitalizedPlayer(CurrentOwnerId); - BasisDebug.LogError($"No Owner for Id {CurrentOwnerId} Creating Fake {nameof(BasisUnInitalizedPlayer)} this should only occur rarely"); - UnInitalizedPlayer.Initialize(); - OnOwnershipTransfer(UnInitalizedPlayer); - } + OnOwnershipTransfer(currentOwnedPlayer); } + else + { + BasisUnInitalizedPlayer UnInitalizedPlayer = new BasisUnInitalizedPlayer(CurrentOwnerId); + BasisDebug.LogError($"No Owner for Id {CurrentOwnerId} Creating Fake {nameof(BasisUnInitalizedPlayer)} this should only occur rarely"); + UnInitalizedPlayer.Initialize(); + OnOwnershipTransfer(UnInitalizedPlayer); + } + } + + private void Transition(NetworkOwnershipState newState) + { + if (OwnershipState == newState) return; + BasisDebug.Log($"[ownership] {clientIdentifier}: {OwnershipState} -> {newState}", BasisDebug.LogTag.Networking); + OwnershipState = newState; + OnOwnershipStateChanged(); } /// + /// Called whenever changes (including optimistic + /// transitions and rollbacks on failed requests). Subclasses override to react + /// without depending on the server-driven . + /// + protected virtual void OnOwnershipStateChanged() { } + /// /// this is used for sending Network Messages /// very much a data sync that can be used more like a traditional sync method /// @@ -276,32 +322,78 @@ private static string SiblingIndexIfNeeded(Transform t) } public async void TakeOwnership() { - //no need to use await ownership will get back here from lower level. - await TakeOwnershipAsync(); + await ClaimOwnership(); } - /// - /// actively takes ownership from another player - /// - /// - /// - public async Task TakeOwnershipAsync(int Timout = 5000) + + public Task ClaimOwnership(int timeoutMs = 5000) { - IsOwnedLocallyOnClient = true; + if (OwnershipState == NetworkOwnershipState.Local) + return Task.FromResult(new BasisOwnershipResult(true, BasisNetworkPlayer.LocalPlayer.playerId)); + if (_pendingClaim != null) + return _pendingClaim; + return _pendingClaim = InternalClaim(timeoutMs); + } + + private async Task InternalClaim(int timeoutMs) + { + var prev = OwnershipState; + Transition(NetworkOwnershipState.PendingClaim); CurrentOwnerId = BasisNetworkPlayer.LocalPlayer.playerId; currentOwnedPlayer = BasisNetworkPlayer.LocalPlayer; - BasisOwnershipResult Result = await BasisNetworkOwnership.TakeOwnershipAsync(clientIdentifier, BasisNetworkConnection.LocalPlayerPeer.RemoteId, Timout); - return Result; + try + { + var result = await BasisNetworkOwnership.TakeOwnershipAsync( + clientIdentifier, + BasisNetworkConnection.LocalPlayerPeer.RemoteId, + timeoutMs, + _destroyCts.Token); + // On failure, the server-driven OnOwnershipTransfer never fires, so revert + // the optimistic transition. Caller is responsible for any retry. + if (!result.Success && OwnershipState == NetworkOwnershipState.PendingClaim) + { + Transition(prev); + } + return result; + } + finally { _pendingClaim = null; } + } + + public Task ReleaseOwnership(int timeoutMs = 5000) + { + if (OwnershipState == NetworkOwnershipState.Unknown || OwnershipState == NetworkOwnershipState.Remote) + return Task.FromResult(new BasisOwnershipResult(true, CurrentOwnerId)); + if (_pendingRelease != null) + return _pendingRelease; + return _pendingRelease = InternalRelease(timeoutMs); + } + + private async Task InternalRelease(int timeoutMs) + { + var prev = OwnershipState; + Transition(NetworkOwnershipState.PendingRelease); + try + { + var result = await BasisNetworkOwnership.RemoveOwnershipAsync( + clientIdentifier, + timeoutMs, + _destroyCts.Token); + if (!result.Success && OwnershipState == NetworkOwnershipState.PendingRelease) + { + Transition(prev); + } + return result; + } + finally { _pendingRelease = null; } } + /// - /// requests who is the owner + /// Polls ownership state from the server. Triggers OnOwnershipTransfer. /// - /// - /// - public async Task RequestWhoIsOwnershipAsync(int Timout = 5000) + public async Task PollOwnership(int timeoutMs = 5000) { - BasisOwnershipResult Result = await BasisNetworkOwnership.RequestCurrentOwnershipAsync(clientIdentifier, Timout); - return Result; + return await BasisNetworkOwnership.RequestCurrentOwnershipAsync(clientIdentifier, timeoutMs, _destroyCts.Token); } + public virtual void OnNetworkReady() { diff --git a/Basis/Packages/com.basis.framework/Networking/BasisNetworkOwnership.cs b/Basis/Packages/com.basis.framework/Networking/BasisNetworkOwnership.cs index 7fe276db38..e06ac8419c 100644 --- a/Basis/Packages/com.basis.framework/Networking/BasisNetworkOwnership.cs +++ b/Basis/Packages/com.basis.framework/Networking/BasisNetworkOwnership.cs @@ -10,10 +10,10 @@ public static partial class BasisNetworkOwnership { - public static async Task RemoveOwnershipAsync(string UniqueNetworkId, int timeoutMs = 5000) + public static async Task RemoveOwnershipAsync(string UniqueNetworkId, int timeoutMs = 5000, CancellationToken externalToken = default) { var tcs = new TaskCompletionSource(); - using var cancellationTokenSource = new CancellationTokenSource(); + using var cts = CancellationTokenSource.CreateLinkedTokenSource(externalToken); void OnOwnershipTransferred(string ownershipID, ushort playerID, bool isLocalOwner) { @@ -24,7 +24,7 @@ void OnOwnershipTransferred(string ownershipID, ushort playerID, bool isLocalOwn } } - cancellationTokenSource.Token.Register(() => + cts.Token.Register(() => { BasisNetworkPlayer.OnOwnershipTransfer -= OnOwnershipTransferred; tcs.TrySetResult(BasisOwnershipResult.Failed); @@ -56,17 +56,17 @@ void OnOwnershipTransferred(string ownershipID, ushort playerID, bool isLocalOwn return BasisOwnershipResult.Failed; } - cancellationTokenSource.CancelAfter(timeoutMs); + cts.CancelAfter(timeoutMs); return await tcs.Task; } - public static async Task TakeOwnershipAsync(string UniqueNetworkId, int NewOwner, int timeoutMs = 5000) + public static async Task TakeOwnershipAsync(string UniqueNetworkId, int NewOwner, int timeoutMs = 5000, CancellationToken externalToken = default) { - return await TakeOwnershipAsync(UniqueNetworkId, (ushort)NewOwner, timeoutMs); + return await TakeOwnershipAsync(UniqueNetworkId, (ushort)NewOwner, timeoutMs, externalToken); } - public static async Task TakeOwnershipAsync(string UniqueNetworkId, ushort NewOwner, int timeoutMs = 5000) + public static async Task TakeOwnershipAsync(string UniqueNetworkId, ushort NewOwner, int timeoutMs = 5000, CancellationToken externalToken = default) { var tcs = new TaskCompletionSource(); - using var cancellationTokenSource = new CancellationTokenSource(); + using var cts = CancellationTokenSource.CreateLinkedTokenSource(externalToken); void OnOwnershipTransferred(string ownershipID, ushort playerID, bool isLocalOwner) { @@ -77,7 +77,7 @@ void OnOwnershipTransferred(string ownershipID, ushort playerID, bool isLocalOwn } } - cancellationTokenSource.Token.Register(() => + cts.Token.Register(() => { BasisNetworkPlayer.OnOwnershipTransfer -= OnOwnershipTransferred; tcs.TrySetResult(BasisOwnershipResult.Failed); @@ -109,7 +109,7 @@ void OnOwnershipTransferred(string ownershipID, ushort playerID, bool isLocalOwn return BasisOwnershipResult.Failed; } - cancellationTokenSource.CancelAfter(timeoutMs); + cts.CancelAfter(timeoutMs); return await tcs.Task; } /// @@ -127,7 +127,7 @@ public static bool IsOwnerLocalValidation(string OwnershipId) } return false; } - public static async Task RequestCurrentOwnershipAsync(string UniqueNetworkId, int timeoutMs = 5000) + public static async Task RequestCurrentOwnershipAsync(string UniqueNetworkId, int timeoutMs = 5000, CancellationToken externalToken = default) { if (BasisNetworkPlayers.OwnershipPairing.TryGetValue(UniqueNetworkId, out ushort Unique)) { @@ -141,7 +141,7 @@ public static async Task RequestCurrentOwnershipAsync(stri } var tcs = new TaskCompletionSource(); - using var cancellationTokenSource = new CancellationTokenSource(); + using var cts = CancellationTokenSource.CreateLinkedTokenSource(externalToken); void OnOwnershipTransferred(string ownershipID, ushort playerID, bool isLocalOwner) { @@ -152,7 +152,7 @@ void OnOwnershipTransferred(string ownershipID, ushort playerID, bool isLocalOwn } } - cancellationTokenSource.Token.Register(() => + cts.Token.Register(() => { BasisNetworkPlayer.OnOwnershipTransfer -= OnOwnershipTransferred; tcs.TrySetResult(BasisOwnershipResult.Failed); @@ -184,7 +184,7 @@ void OnOwnershipTransferred(string ownershipID, ushort playerID, bool isLocalOwn return BasisOwnershipResult.Failed; } - cancellationTokenSource.CancelAfter(timeoutMs); + cts.CancelAfter(timeoutMs); return await tcs.Task; } } diff --git a/Basis/Packages/com.basis.framework/Networking/Object Sync/BasisObjectSyncNetworking.cs b/Basis/Packages/com.basis.framework/Networking/Object Sync/BasisObjectSyncNetworking.cs index bba8f81adb..bd5a456228 100644 --- a/Basis/Packages/com.basis.framework/Networking/Object Sync/BasisObjectSyncNetworking.cs +++ b/Basis/Packages/com.basis.framework/Networking/Object Sync/BasisObjectSyncNetworking.cs @@ -3,7 +3,6 @@ using Basis.Scripts.Device_Management.Devices; using Basis.Scripts.Networking; using Basis.Scripts.Networking.Compression; -using Basis.Scripts.Networking.NetworkedAvatar; using Basis.Network.Core; using UnityEngine; public class BasisObjectSyncNetworking : BasisNetworkBehaviour @@ -14,7 +13,8 @@ public class BasisObjectSyncNetworking : BasisNetworkBehaviour BasisPositionRotationScale LocalLastData = new BasisPositionRotationScale(); [SerializeField] public BasisTranslationUpdate BTU = new BasisTranslationUpdate(); - public BasisInput pendingStealRequest = null; + [SerializeField] + private bool ReleaseOwnershipOnDrop = false; public float CatchupLerp = 5; public byte[] buffer = new byte[BasisPositionRotationScale.Size]; public Transform SelfTransform; @@ -30,6 +30,7 @@ public void Awake() BasisPickupInteractable.CanHoverInjected.Add(CanHover); BasisPickupInteractable.CanInteractInjected.Add(CanInteract); BasisPickupInteractable.OnInteractStartEvent.AddListener(OnInteractStartEvent); + BasisPickupInteractable.OnInteractEndEvent.AddListener(OnInteractEndEvent); } if (BasisPickupInteractable.RigidRef != null) { @@ -47,6 +48,7 @@ public void OnDisable() BasisPickupInteractable.CanHoverInjected.Remove(CanHover); BasisPickupInteractable.CanInteractInjected.Remove(CanInteract); BasisPickupInteractable.OnInteractStartEvent.RemoveListener(OnInteractStartEvent); + BasisPickupInteractable.OnInteractEndEvent.RemoveListener(OnInteractEndEvent); } } public override void OnDestroy() @@ -57,7 +59,7 @@ public override void OnDestroy() } public override void OnNetworkReady() { - ControlState(); + ApplyState(); } private bool CanHover(BasisInput input) @@ -71,31 +73,23 @@ private bool CanHover(BasisInput input) } private bool CanInteract(BasisInput input) { - // Allow interact if we arent connected or if we own it locally - if (IsOwnedLocallyOnClient) - { - return true; - } - // Allow if stealing is enabled and no other input has a steal in progress - // NOTE: pendingStealRequest is only set in OnInteractStartEvent to avoid - // side effects when this is called speculatively (e.g. via IsInfluencable) - return CanNetworkSteal && (pendingStealRequest == null || pendingStealRequest == input); + return IsOwnedLocallyOnClient || CanNetworkSteal; } private void OnInteractStartEvent(BasisInput input) { - if (!IsOwnedLocallyOnClient) + // Remote grabbed pickup sets kinematic during lerp, so preserve locally expected kinematic state + if (BasisPickupInteractable != null && BasisPickupInteractable.KinematicWhileInteracting) { - pendingStealRequest = input; + BasisPickupInteractable._previousKinematicValue = false; } - CanInteractAsync(); // ControlState handles the ownership transfer logic here + _ = ClaimOwnership(); } - private async void CanInteractAsync() + private void OnInteractEndEvent(BasisInput input) { - var result = await TakeOwnershipAsync(5000); // 5 second timeout - if (result.Success == false) - { - pendingStealRequest = null; - } + if (!ReleaseOwnershipOnDrop) return; + // Only release if no other hand is still interacting with the pickup. + if (BasisPickupInteractable != null && BasisPickupInteractable.Inputs.AnyInteracting()) return; + _ = ReleaseOwnership(); } public void SetIsKinematicOnPickup(bool state) { @@ -104,11 +98,12 @@ public void SetIsKinematicOnPickup(bool state) BasisPickupInteractable.RigidRef.isKinematic = state; } } - public override void OnOwnershipTransfer(BasisNetworkPlayer NetIdNewOwner) - { - ControlState(); - } - public void ControlState() + protected override void OnOwnershipStateChanged() => ApplyState(); + /// + /// Pure state-driven physics + driver-set assignment. Local press already ran + /// OnInteractStart end-to-end with CanNetworkSteal=true, so no replay path. + /// + public void ApplyState() { #if UNITY_SERVER if (SelfTransform == null) @@ -121,49 +116,42 @@ public void ControlState() } } #endif - //lets always just update the last data so going from here we have some reference of last. - if (IsOwnedLocallyOnClient) - { - BasisObjectSyncDriver.AddLocalOwner(this); - BasisObjectSyncDriver.RemoveRemoteOwner(this); - if (pendingStealRequest != null) - { - // Set non-kinematic before ForceSetInteracting so that OnInteractStart - // saves the correct _previousKinematicValue (false) for restore on drop - SetIsKinematicOnPickup(false); - BasisPlayerInteract.Instance.ForceSetInteracting(BasisPickupInteractable, pendingStealRequest); - pendingStealRequest = null; - // ForceSetInteracting -> OnInteractStart re-applies isKinematic = true - // when KinematicWhileInteracting is enabled, so don't override after - } - else if (BasisPickupInteractable != null - && BasisPickupInteractable.KinematicWhileInteracting - && BasisPickupInteractable.RequiresUpdateLoop) - { - // Currently held with KinematicWhileInteracting - preserve kinematic state - } - else - { - SetIsKinematicOnPickup(false); - } - } - else + switch (OwnershipState) { - // Initialize BTU from current transform so the lerp job doesn't - // snap the object back to origin before the first sync arrives - SelfTransform.GetLocalPositionAndRotation(out UnityEngine.Vector3 currentPos, out UnityEngine.Quaternion currentRot); - BTU.TargetPosition = currentPos; - BTU.TargetRotation = currentRot; - BTU.TargetScales = SelfTransform.localScale; - BTU.LerpMultipliers = CatchupLerp; + case NetworkOwnershipState.PendingClaim: + case NetworkOwnershipState.Local: + BasisObjectSyncDriver.AddLocalOwner(this); + BasisObjectSyncDriver.RemoveRemoteOwner(this); + if (BasisPickupInteractable != null + && BasisPickupInteractable.KinematicWhileInteracting + && BasisPickupInteractable.RequiresUpdateLoop) + { + // Currently held with KinematicWhileInteracting - preserve kinematic state + } + else + { + SetIsKinematicOnPickup(false); + } + break; + case NetworkOwnershipState.Remote: + case NetworkOwnershipState.Unknown: + case NetworkOwnershipState.PendingRelease: + // Initialize BTU from current transform so the lerp job doesn't + // snap the object back to origin before the first sync arrives + SelfTransform.GetLocalPositionAndRotation(out UnityEngine.Vector3 currentPos, out UnityEngine.Quaternion currentRot); + BTU.TargetPosition = currentPos; + BTU.TargetRotation = currentRot; + BTU.TargetScales = SelfTransform.localScale; + BTU.LerpMultipliers = CatchupLerp; - BasisObjectSyncDriver.RemoveLocalOwner(this); - BasisObjectSyncDriver.AddRemoteOwner(this); - if (BasisPickupInteractable != null) - { - BasisPickupInteractable.Drop(); - } - SetIsKinematicOnPickup(true); + BasisObjectSyncDriver.RemoveLocalOwner(this); + BasisObjectSyncDriver.AddRemoteOwner(this); + if (BasisPickupInteractable != null) + { + BasisPickupInteractable.Drop(); + } + SetIsKinematicOnPickup(true); + break; } } public override void OnNetworkMessage(ushort PlayerID, byte[] buffer, DeliveryMethod DeliveryMethod) diff --git a/Basis/Packages/com.basis.shim/Shims/BasisNetworkShim.cs b/Basis/Packages/com.basis.shim/Shims/BasisNetworkShim.cs index 004eff59af..ae12a5fdbe 100644 --- a/Basis/Packages/com.basis.shim/Shims/BasisNetworkShim.cs +++ b/Basis/Packages/com.basis.shim/Shims/BasisNetworkShim.cs @@ -50,7 +50,7 @@ public override void OnPlayerJoined(BasisNetworkPlayer player) public void RequestOwnershipIfNone() { - RequestWhoIsOwnershipAsync(); + PollOwnership(); } } } From 51655070a514568930648d6134688c3e596add68 Mon Sep 17 00:00:00 2001 From: mriise Date: Fri, 8 May 2026 12:42:35 -0700 Subject: [PATCH 2/2] chore: wording --- .../Networking/Object Sync/BasisObjectSyncNetworking.cs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/Basis/Packages/com.basis.framework/Networking/Object Sync/BasisObjectSyncNetworking.cs b/Basis/Packages/com.basis.framework/Networking/Object Sync/BasisObjectSyncNetworking.cs index bd5a456228..7aa1cdc076 100644 --- a/Basis/Packages/com.basis.framework/Networking/Object Sync/BasisObjectSyncNetworking.cs +++ b/Basis/Packages/com.basis.framework/Networking/Object Sync/BasisObjectSyncNetworking.cs @@ -99,10 +99,7 @@ public void SetIsKinematicOnPickup(bool state) } } protected override void OnOwnershipStateChanged() => ApplyState(); - /// - /// Pure state-driven physics + driver-set assignment. Local press already ran - /// OnInteractStart end-to-end with CanNetworkSteal=true, so no replay path. - /// + public void ApplyState() { #if UNITY_SERVER