fix: ClientNetworkTransform ownership change with half precision synchronization issues [MTT-7271] (#2712)

* update

Adding OnOwnershipChanged(ulong previous, ulong current)

* fix

This resolves the issue with client authoritative network transforms and the random "noise" that would occur when transitioning ownership from a remote client back to the host-server.
- Latent messages from the client would still be received and processed after ownership changed.
- Ownership changed messages would proceed the NetworkTransform initialization state update message. Now ownership changed messages precede the NetworkTransform initialization state update message.
- Clients could sometimes have the same network tick value even when the tick event had triggered, which for NetworkDeltaPosition would cause dropped state updates.

Author: NoelStephensUnity

com.unity.netcode.gameobjects/Components/NetworkTransform.cs

@@ -499,6 +499,10 @@ public Quaternion GetRotation()
             /// <remarks>
             /// When there is no change in an updated state's position then there are no values to return.
             /// Checking for <see cref="HasPositionChange"/> is one way to detect this.
+            /// When used with half precision it returns the half precision delta position state update 
+            /// which will not be the full position.
+            /// To get a NettworkTransform's full position, use <see cref="GetSpaceRelativePosition(bool)"/> and
+            /// pass true as the parameter.
             /// </remarks>
             /// <returns><see cref="Vector3"/></returns>
             public Vector3 GetPosition()
@@ -1110,7 +1114,16 @@ public Vector3 GetSpaceRelativePosition(bool getCurrentState = false)
             }
             else
             {
-                return m_CurrentPosition;
+                // When half float precision is enabled, get the NetworkDeltaPosition's full position
+                if (UseHalfFloatPrecision)
+                {
+                    return m_HalfPositionState.GetFullPosition();
+                }
+                else
+                {
+                    // Otherwise, just get the current position
+                    return m_CurrentPosition;
+                }
             }
         }
 
@@ -1393,6 +1406,8 @@ protected virtual void OnAuthorityPushTransformState(ref NetworkTransformState n
         {
         }
 
+        // Tracks the last tick a state update was sent (see further below)
+        private int m_LastTick;
         /// <summary>
         /// Authoritative side only
         /// If there are any transform delta states, this method will synchronize the
@@ -1411,11 +1426,27 @@ private void TryCommitTransform(ref Transform transformToCommit, bool synchroniz
             if (ApplyTransformToNetworkStateWithInfo(ref m_LocalAuthoritativeNetworkState, ref transformToCommit, synchronize))
             {
                 m_LocalAuthoritativeNetworkState.LastSerializedSize = m_OldState.LastSerializedSize;
-                OnAuthorityPushTransformState(ref m_LocalAuthoritativeNetworkState);
 
+                // Make sure our network tick is incremented
+                if (m_LastTick == m_LocalAuthoritativeNetworkState.NetworkTick && !m_LocalAuthoritativeNetworkState.IsTeleportingNextFrame)
+                {
+                    // When running in authority and a remote client is the owner, the client can hit a perfect window of time where
+                    // it is still on the previous network tick (as a count) but still have had the tick event triggered.
+                    // (This is cheaper than calculating the exact tick each time and only can occur on clients)
+                    if (!IsServer)
+                    {
+                        m_LocalAuthoritativeNetworkState.NetworkTick = m_LocalAuthoritativeNetworkState.NetworkTick + 1;
+                    }
+                    else
+                    {
+                        NetworkLog.LogError($"[NT TICK DUPLICATE] Server already sent an update on tick {m_LastTick} and is attempting to send again on the same network tick!");
+                    }
+                }
+                m_LastTick = m_LocalAuthoritativeNetworkState.NetworkTick;
                 // Update the state
                 UpdateTransformState();
 
+                OnAuthorityPushTransformState(ref m_LocalAuthoritativeNetworkState);
                 m_LocalAuthoritativeNetworkState.IsTeleportingNextFrame = false;
             }
         }
@@ -2209,6 +2240,7 @@ private void ApplyUpdatedState(NetworkTransformState newState)
                 m_HalfPositionState.HalfVector3.Axis = m_LocalAuthoritativeNetworkState.NetworkDeltaPosition.HalfVector3.Axis;
                 // and update our target position
                 m_TargetPosition = m_HalfPositionState.ToVector3(newState.NetworkTick);
+                m_LocalAuthoritativeNetworkState.NetworkDeltaPosition.CurrentBasePosition = m_HalfPositionState.CurrentBasePosition;
                 m_LocalAuthoritativeNetworkState.CurrentPosition = m_TargetPosition;
             }
 
@@ -2455,6 +2487,9 @@ private void NetworkTickSystem_Tick()
                 // Update any changes to the transform
                 var transformSource = transform;
                 OnUpdateAuthoritativeState(ref transformSource);
+
+                m_CurrentPosition = GetSpaceRelativePosition();
+                m_TargetPosition = GetSpaceRelativePosition();
             }
             else
             {
@@ -2466,9 +2501,6 @@ private void NetworkTickSystem_Tick()
             }
         }
 
-
-
-
         /// <inheritdoc/>
         public override void OnNetworkSpawn()
         {
@@ -2510,24 +2542,25 @@ public override void OnDestroy()
         }
 
         /// <inheritdoc/>
-        public override void OnGainedOwnership()
+        public override void OnLostOwnership()
         {
-            // Only initialize if we gained ownership
-            if (OwnerClientId == NetworkManager.LocalClientId)
-            {
-                Initialize();
-            }
+            base.OnLostOwnership();
         }
 
         /// <inheritdoc/>
-        public override void OnLostOwnership()
+        public override void OnGainedOwnership()
         {
-            // Only initialize if we are not authority and lost
-            // ownership
-            if (OwnerClientId != NetworkManager.LocalClientId)
+            base.OnGainedOwnership();
+        }
+
+        protected override void OnOwnershipChanged(ulong previous, ulong current)
+        {
+            // If we were the previous owner or the newly assigned owner then reinitialize
+            if (current == NetworkManager.LocalClientId || previous == NetworkManager.LocalClientId)
             {
                 Initialize();
             }
+            base.OnOwnershipChanged(previous, current);
         }
 
         /// <summary>
@@ -2552,6 +2585,7 @@ protected virtual void OnInitialize(ref NetworkVariable<NetworkTransformState> r
 
         }
 
+
         /// <summary>
         /// Initializes NetworkTransform when spawned and ownership changes.
         /// </summary>
@@ -2572,7 +2606,8 @@ protected void Initialize()
                 {
                     m_HalfPositionState = new NetworkDeltaPosition(currentPosition, NetworkManager.NetworkTickSystem.ServerTime.Tick, math.bool3(SyncPositionX, SyncPositionY, SyncPositionZ));
                 }
-
+                m_CurrentPosition = currentPosition;
+                m_TargetPosition = currentPosition;
                 // Authority only updates once per network tick
                 NetworkManager.NetworkTickSystem.Tick -= NetworkTickSystem_Tick;
                 NetworkManager.NetworkTickSystem.Tick += NetworkTickSystem_Tick;
@@ -2835,6 +2870,13 @@ public bool IsServerAuthoritative()
         /// <param name="messagePayload">serialzied <see cref="NetworkTransformState"/></param>
         private void TransformStateUpdate(ulong senderId, FastBufferReader messagePayload)
         {
+            if (!OnIsServerAuthoritative() && IsServer && OwnerClientId == NetworkManager.ServerClientId)
+            {
+                // Ownership must have changed, ignore any additional pending messages that might have
+                // come from a previous owner client.
+                return;
+            }
+
             // Forward owner authoritative messages before doing anything else
             if (IsServer && !OnIsServerAuthoritative())
             {
@@ -2856,6 +2898,7 @@ private void TransformStateUpdate(ulong senderId, FastBufferReader messagePayloa
         /// <param name="messagePayload">the owner state message payload</param>
         private unsafe void ForwardStateUpdateMessage(FastBufferReader messagePayload)
         {
+            var serverAuthoritative = OnIsServerAuthoritative();
             var currentPosition = messagePayload.Position;
             var messageSize = messagePayload.Length - currentPosition;
             var writer = new FastBufferWriter(messageSize, Allocator.Temp);
@@ -2867,7 +2910,7 @@ private unsafe void ForwardStateUpdateMessage(FastBufferReader messagePayload)
                 for (int i = 0; i < clientCount; i++)
                 {
                     var clientId = NetworkManager.ConnectionManager.ConnectedClientsList[i].ClientId;
-                    if (!OnIsServerAuthoritative() && (NetworkManager.ServerClientId == clientId || clientId == OwnerClientId))
+                    if (NetworkManager.ServerClientId == clientId || (!serverAuthoritative && clientId == OwnerClientId))
                     {
                         continue;
                     }

com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs

@@ -532,6 +532,23 @@ internal void InternalOnGainedOwnership()
             OnGainedOwnership();
         }
 
+        /// <summary>
+        /// Invoked on all clients, override this method to be notified of any
+        /// ownership changes (even if the instance was niether the previous or
+        /// newly assigned current owner). 
+        /// </summary>
+        /// <param name="previous">the previous owner</param>
+        /// <param name="current">the current owner</param>
+        protected virtual void OnOwnershipChanged(ulong previous, ulong current)
+        {
+
+        }
+
+        internal void InternalOnOwnershipChanged(ulong previous, ulong current)
+        {
+            OnOwnershipChanged(previous, current);
+        }
+
         /// <summary>
         /// Gets called when we loose ownership of this object
         /// </summary>

com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs

@@ -840,6 +840,21 @@ internal void InvokeBehaviourOnGainedOwnership()
             }
         }
 
+        internal void InvokeOwnershipChanged(ulong previous, ulong next)
+        {
+            for (int i = 0; i < ChildNetworkBehaviours.Count; i++)
+            {
+                if (ChildNetworkBehaviours[i].gameObject.activeInHierarchy)
+                {
+                    ChildNetworkBehaviours[i].InternalOnOwnershipChanged(previous, next);
+                }
+                else
+                {
+                    Debug.LogWarning($"{ChildNetworkBehaviours[i].gameObject.name} is disabled! Netcode for GameObjects does not support disabled NetworkBehaviours! The {ChildNetworkBehaviours[i].GetType().Name} component was skipped during ownership assignment!");
+                }
+            }
+        }
+
         internal void InvokeBehaviourOnNetworkObjectParentChanged(NetworkObject parentNetworkObject)
         {
             for (int i = 0; i < ChildNetworkBehaviours.Count; i++)

com.unity.netcode.gameobjects/Runtime/Messaging/Messages/ChangeOwnershipMessage.cs

@@ -60,6 +60,8 @@ public void Handle(ref NetworkContext context)
                 }
             }
 
+            networkObject.InvokeOwnershipChanged(originalOwner, OwnerClientId);
+
             networkManager.NetworkMetrics.TrackOwnershipChangeReceived(context.SenderId, networkObject, context.MessageSize);
         }
     }

com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs

@@ -257,6 +257,7 @@ internal void ChangeOwnership(NetworkObject networkObject, ulong clientId)
                 throw new SpawnStateException("Object is not spawned");
             }
 
+            var previous = networkObject.OwnerClientId;
             // Assign the new owner
             networkObject.OwnerClientId = clientId;
 
@@ -286,6 +287,12 @@ internal void ChangeOwnership(NetworkObject networkObject, ulong clientId)
                     NetworkManager.NetworkMetrics.TrackOwnershipChangeSent(client.Key, networkObject, size);
                 }
             }
+
+            // After we have sent the change ownership message to all client observers, invoke the ownership changed notification.
+            /// !!Important!!
+            /// This gets called specifically *after* sending the ownership message so any additional messages that need to proceed an ownership
+            /// change can be sent from NetworkBehaviours that override the <see cref="NetworkBehaviour.OnOwnershipChanged"></see>
+            networkObject.InvokeOwnershipChanged(previous, clientId);
         }
 
         internal bool HasPrefab(NetworkObject.SceneObject sceneObject)

com.unity.netcode.gameobjects/Tests/Runtime/NetworkTransform/NetworkTransformTests.cs

@@ -588,6 +588,7 @@ public void ParentedNetworkTransformTest([Values] Precision precision, [Values]
             success = WaitForConditionOrTimeOutWithTimeTravel(AllChildObjectInstancesHaveChild);
             Assert.True(success, "Timed out waiting for all instances to have parented a child!");
 
+            TimeTravelToNextTick();
             // This validates each child instance has preserved their local space values
             AllChildrenLocalTransformValuesMatch(false);
 
@@ -689,7 +690,7 @@ protected override void OnNewClientCreated(NetworkManager networkManager)
 
         private Precision m_Precision = Precision.Full;
         private float m_CurrentHalfPrecision = 0.0f;
-        private const float k_HalfPrecisionPosScale = 0.03f;
+        private const float k_HalfPrecisionPosScale = 0.041f;
         private const float k_HalfPrecisionRot = 0.725f;
 
         protected override float GetDeltaVarianceThreshold()