fix: NetworkAnimator Synchronizing transition twice [MTT-3564] (#2084)

* fix

This fixes the issue where transitions were being both triggered and the change in animation state (i.e. transition state) was being synchronized and applied to clients already transitioning which would cause any StateMachineBehaviours to have their OnStateEnter method invoked twice.

* fix

Making adjustments to account for changes to the Transition state update vs trigger update fix.

* test

Adding the additional StateMachineBehaviour (CheckStateEnter) to validate this fix.
Adjusting two tests for this fix.
Includes the animation related assets needed for the test.

* style

adding additional comments for some changes that were not obvious to the fix.

* update

removing code used to debug

* style

MTT-3564
updating changelog.

* style

correcting grammar in comment

* style

slight adjustment to the comment

* style

* update

removing more debug related code.

* style

minor integration test variable rename.

Author: NoelStephensUnity

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -20,6 +20,7 @@ Additional documentation and release notes are available at [Multiplayer Documen
 - Fixed issue when attempting to spawn a parent `GameObject`, with `NetworkObject` component attached, that has one or more child `GameObject`s, that are inactive in the hierarchy, with `NetworkBehaviour` components it will no longer attempt to spawn the associated `NetworkBehaviour`(s) or invoke ownership changed notifications but will log a warning message. (#2096)
 - Fixed an issue where destroying a NetworkBehaviour would not deregister it from the parent NetworkObject, leading to exceptions when the parent was later destroyed. (#2091)
 - Fixed issue where `NetworkObject.NetworkHide` was despawning and destroying, as opposed to only despawning, in-scene placed `NetworkObject`s. (#2086)
+- Fixed `NetworkAnimator` synchronizing transitions twice due to it detecting the change in animation state once a transition is started by a trigger. (#2084)
 - Fixed issue where `NetworkAnimator` would not synchronize a looping animation for late joining clients if it was at the very end of its loop. (#2076)
 - Fixed issue where `NetworkAnimator` was not removing its subscription from `OnClientConnectedCallback` when despawned during the shutdown sequence. (#2074)
 - Fixed IsServer and IsClient being set to false before object despawn during the shutdown sequence. (#2074)

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

@@ -163,6 +163,7 @@ public class NetworkAnimator : NetworkBehaviour
         internal struct AnimationMessage : INetworkSerializable
         {
             // state hash per layer.  if non-zero, then Play() this animation, skipping transitions
+            internal bool Transition;
             internal int StateHash;
             internal float NormalizedTime;
             internal int Layer;
@@ -427,6 +428,7 @@ internal void ServerSynchronizeNewPlayer(ulong playerId)
 
                 var animMsg = new AnimationMessage
                 {
+                    Transition = m_Animator.IsInTransition(layer),
                     StateHash = stateHash,
                     NormalizedTime = normalizedTime,
                     Layer = layer,
@@ -442,6 +444,9 @@ private void OnClientConnectedCallback(ulong playerId)
             m_NetworkAnimatorStateChangeHandler.SynchronizeClient(playerId);
         }
 
+        /// <summary>
+        /// Checks for changes in both Animator parameters and state.
+        /// </summary>
         internal void CheckForAnimatorChanges()
         {
             if (!IsOwner && !IsServerAuthoritative() || IsServerAuthoritative() && !IsServer)
@@ -482,6 +487,7 @@ internal void CheckForAnimatorChanges()
 
                 var animMsg = new AnimationMessage
                 {
+                    Transition = m_Animator.IsInTransition(layer),
                     StateHash = stateHash,
                     NormalizedTime = normalizedTime,
                     Layer = layer,
@@ -744,7 +750,13 @@ internal unsafe void UpdateParameters(ParametersUpdateMessage parametersUpdate)
         /// </summary>
         private unsafe void UpdateAnimationState(AnimationMessage animationState)
         {
-            if (animationState.StateHash != 0)
+            if (animationState.StateHash == 0)
+            {
+                return;
+            }
+
+            var currentState = m_Animator.GetCurrentAnimatorStateInfo(animationState.Layer);
+            if (currentState.fullPathHash != animationState.StateHash || m_Animator.IsInTransition(animationState.Layer) != animationState.Transition)
             {
                 m_Animator.Play(animationState.StateHash, animationState.Layer, animationState.NormalizedTime);
             }
@@ -830,6 +842,10 @@ private unsafe void SendAnimStateServerRpc(AnimationMessage animSnapshot, Server
         [ClientRpc]
         private unsafe void SendAnimStateClientRpc(AnimationMessage animSnapshot, ClientRpcParams clientRpcParams = default)
         {
+            if (IsServer)
+            {
+                return;
+            }
             var isServerAuthoritative = IsServerAuthoritative();
             if (!isServerAuthoritative && !IsOwner || isServerAuthoritative)
             {
@@ -879,11 +895,7 @@ private void SendAnimTriggerServerRpc(AnimationTriggerMessage animationTriggerMe
         [ClientRpc]
         internal void SendAnimTriggerClientRpc(AnimationTriggerMessage animationTriggerMessage, ClientRpcParams clientRpcParams = default)
         {
-            var isServerAuthoritative = IsServerAuthoritative();
-            if (!isServerAuthoritative && !IsOwner || isServerAuthoritative)
-            {
-                m_Animator.SetBool(animationTriggerMessage.Hash, animationTriggerMessage.IsTriggerSet);
-            }
+            m_Animator.SetBool(animationTriggerMessage.Hash, animationTriggerMessage.IsTriggerSet);
         }
 
         /// <summary>
@@ -900,8 +912,13 @@ public void SetTrigger(string triggerName)
         /// <param name="setTrigger">sets (true) or resets (false) the trigger. The default is to set it (true).</param>
         public void SetTrigger(int hash, bool setTrigger = true)
         {
-            var isServerAuthoritative = IsServerAuthoritative();
-            if (IsOwner && !isServerAuthoritative || IsServer && isServerAuthoritative)
+            // MTT-3564:
+            // After fixing the issue with trigger controlled Transitions being synchronized twice,
+            // it exposed additional issues with this logic.  Now, either the owner or the server can
+            // update triggers. Since server-side RPCs are immediately invoked, for a host a trigger
+            // will happen when SendAnimTriggerClientRpc is called.  For a client owner, we call the
+            // SendAnimTriggerServerRpc and then trigger locally when running in owner authority mode.
+            if (IsOwner || IsServer)
             {
                 var animTriggerMessage = new AnimationTriggerMessage() { Hash = hash, IsTriggerSet = setTrigger };
                 if (IsServer)
@@ -911,9 +928,11 @@ public void SetTrigger(int hash, bool setTrigger = true)
                 else
                 {
                     SendAnimTriggerServerRpc(animTriggerMessage);
+                    if (!IsServerAuthoritative())
+                    {
+                        m_Animator.SetTrigger(hash);
+                    }
                 }
-                //  trigger the animation locally on the server...
-                m_Animator.SetBool(hash, setTrigger);
             }
         }
 

testproject/Assets/Tests/Manual/NetworkAnimatorTests/CheckStateEnterCount.cs

@@ -0,0 +1,101 @@
+using System.Collections.Generic;
+using UnityEngine;
+using Unity.Netcode;
+
+namespace TestProject.RuntimeTests
+{
+    public class CheckStateEnterCount : StateMachineBehaviour
+    {
+        public static Dictionary<ulong, Dictionary<int, List<AnimatorStateInfo>>> OnStateEnterCounter = new Dictionary<ulong, Dictionary<int, List<AnimatorStateInfo>>>();
+        public static bool IsIntegrationTest;
+        public static bool IsManualTestEnabled = true;
+
+        public static void ResetTest(bool isIntegrationTest = true)
+        {
+            IsIntegrationTest = isIntegrationTest;
+            IsManualTestEnabled = !isIntegrationTest;
+            OnStateEnterCounter.Clear();
+        }
+
+        public static bool AllStatesEnteredMatch(List<ulong> clientIdsToCheck)
+        {
+            if (clientIdsToCheck.Contains(NetworkManager.ServerClientId))
+            {
+                clientIdsToCheck.Remove(NetworkManager.ServerClientId);
+            }
+
+            if (!OnStateEnterCounter.ContainsKey(NetworkManager.ServerClientId))
+            {
+                Debug.Log($"Server has not entered into any states! OnStateEntered Entry Count ({OnStateEnterCounter.Count})");
+                return false;
+            }
+
+            var serverStates = OnStateEnterCounter[NetworkManager.ServerClientId];
+
+            foreach (var layerEntries in serverStates)
+            {
+                var layerIndex = layerEntries.Key;
+                var layerStates = layerEntries.Value;
+                if (layerStates.Count > 1)
+                {
+                    Debug.Log($"Server layer ({layerIndex}) state was entered ({layerStates.Count}) times!");
+                    return false;
+                }
+
+                foreach (var clientId in clientIdsToCheck)
+                {
+                    if (!OnStateEnterCounter.ContainsKey(clientId))
+                    {
+                        Debug.Log($"Client-{clientId} never entered into any state for layer index ({layerIndex})!");
+                        return false;
+                    }
+                    var clientStates = OnStateEnterCounter[clientId];
+                    if (!clientStates.ContainsKey(layerIndex))
+                    {
+                        Debug.Log($"Client-{clientId} never layer ({layerIndex}) state!");
+                        return false;
+                    }
+                    var clientLayerStateEntries = clientStates[layerIndex];
+                    if (clientLayerStateEntries.Count > 1)
+                    {
+                        Debug.Log($"Client-{clientId} layer ({layerIndex}) state was entered ({layerStates.Count}) times!");
+                        return false;
+                    }
+                    // We should have only entered into the state once on the server
+                    // and all connected clients
+                    var serverAnimStateInfo = layerStates[0];
+                    var clientAnimStateInfo = clientLayerStateEntries[0];
+                    // We just need to make sure we are looking at the same state
+                    if (clientAnimStateInfo.fullPathHash != serverAnimStateInfo.fullPathHash)
+                    {
+                        Debug.Log($"Client-{clientId} full path hash ({clientAnimStateInfo.fullPathHash}) for layer ({layerIndex}) was not the same as the Server full path hash ({serverAnimStateInfo.fullPathHash})!");
+                        return false;
+                    }
+                }
+            }
+            return true;
+        }
+
+        public override void OnStateEnter(Animator animator, AnimatorStateInfo stateInfo, int layerIndex)
+        {
+            if (IsIntegrationTest)
+            {
+                var networkObject = animator.GetComponent<NetworkObject>();
+                var localClientId = networkObject.NetworkManager.IsServer ? NetworkManager.ServerClientId : networkObject.NetworkManager.LocalClientId;
+                if (!OnStateEnterCounter.ContainsKey(localClientId))
+                {
+                    OnStateEnterCounter.Add(localClientId, new Dictionary<int, List<AnimatorStateInfo>>());
+                }
+                if (!OnStateEnterCounter[localClientId].ContainsKey(layerIndex))
+                {
+                    OnStateEnterCounter[localClientId].Add(layerIndex, new List<AnimatorStateInfo>());
+                }
+                OnStateEnterCounter[localClientId][layerIndex].Add(stateInfo);
+            }
+            else if (IsManualTestEnabled)
+            {
+                Debug.Log($"[{layerIndex}][{stateInfo.shortNameHash}][{stateInfo.normalizedTime}][{animator.IsInTransition(layerIndex)}]");
+            }
+        }
+    }
+}

testproject/Assets/Tests/Manual/NetworkAnimatorTests/CheckStateEnterCount.cs.meta

@@ -337,6 +337,18 @@ AnimatorState:
   m_MirrorParameter: 
   m_CycleOffsetParameter: 
   m_TimeParameter: 
+--- !u!114 &-268161786856038416
+MonoBehaviour:
+  m_ObjectHideFlags: 1
+  m_CorrespondingSourceObject: {fileID: 0}
+  m_PrefabInstance: {fileID: 0}
+  m_PrefabAsset: {fileID: 0}
+  m_GameObject: {fileID: 0}
+  m_Enabled: 1
+  m_EditorHideFlags: 0
+  m_Script: {fileID: 11500000, guid: beb3702bd4c8d274e9dffe0c6467eafb, type: 3}
+  m_Name: 
+  m_EditorClassIdentifier: 
 --- !u!1107 &-108110249979471251
 AnimatorStateMachine:
   serializedVersion: 6
@@ -453,6 +465,18 @@ AnimatorController:
     m_IKPass: 0
     m_SyncedLayerAffectsTiming: 0
     m_Controller: {fileID: 9100000}
+--- !u!114 &252787195921886346
+MonoBehaviour:
+  m_ObjectHideFlags: 1
+  m_CorrespondingSourceObject: {fileID: 0}
+  m_PrefabInstance: {fileID: 0}
+  m_PrefabAsset: {fileID: 0}
+  m_GameObject: {fileID: 0}
+  m_Enabled: 1
+  m_EditorHideFlags: 0
+  m_Script: {fileID: 11500000, guid: beb3702bd4c8d274e9dffe0c6467eafb, type: 3}
+  m_Name: 
+  m_EditorClassIdentifier: 
 --- !u!1101 &1138737138882309440
 AnimatorStateTransition:
   m_ObjectHideFlags: 1
@@ -561,6 +585,7 @@ AnimatorState:
   - {fileID: 1138737138882309440}
   m_StateMachineBehaviours:
   - {fileID: 5559889042091692034}
+  - {fileID: -268161786856038416}
   m_Position: {x: 50, y: 50, z: 0}
   m_IKOnFeet: 0
   m_WriteDefaultValues: 1
@@ -625,6 +650,7 @@ AnimatorState:
   - {fileID: 1830534497079063084}
   m_StateMachineBehaviours:
   - {fileID: 3735381422868909868}
+  - {fileID: 252787195921886346}
   m_Position: {x: 50, y: 50, z: 0}
   m_IKOnFeet: 0
   m_WriteDefaultValues: 0

testproject/Assets/Tests/Manual/NetworkAnimatorTests/CubeAnimatorController.controller

@@ -578,7 +578,7 @@ GameObject:
   - component: {fileID: 3604323723684300772}
   - component: {fileID: 6047057957339659638}
   - component: {fileID: 8104648428997222210}
-  - component: {fileID: 5157980021080854601}
+  - component: {fileID: 1245349943772079751}
   m_Layer: 0
   m_Name: NetworkAnimatedCube-Server
   m_TagString: Untagged
@@ -635,7 +635,7 @@ MonoBehaviour:
   m_Name: 
   m_EditorClassIdentifier: 
   TestIterations: 20
---- !u!114 &5157980021080854601
+--- !u!114 &1245349943772079751
 MonoBehaviour:
   m_ObjectHideFlags: 0
   m_CorrespondingSourceObject: {fileID: 0}