fix: network animator override (#1735)

* fix: using animator override controller breaks parameters
* animator tests
* addred trigger reset test
* added hash version of tests
* perf improvement idea from Noel
* changelog

Author: mattwalsh-unity

com.unity.netcode.gameobjects/CHANGELOG.md

@@ -9,11 +9,11 @@ Additional documentation and release notes are available at [Multiplayer Documen
 ## [Unreleased]
 
 ### Added
+- First set of tests for NetworkAnimator - parameter syncing, trigger set / reset, override network animator (#7135)
 
 ### Changed
 
 ### Fixed
-
 - Fixed an issue where sometimes the first client to connect to the server could see messages from the server as coming from itself. (#1683)
 - Fixed an issue where clients seemed to be able to send messages to ClientId 1, but these messages would actually still go to the server (id 0) instead of that client. (#1683)
 - Improved clarity of error messaging when a client attempts to send a message to a destination other than the server, which isn't allowed. (#1683)
@@ -27,6 +27,8 @@ Additional documentation and release notes are available at [Multiplayer Documen
 - Fixed NetworkList to properly call INetworkSerializable's NetworkSerialize() method (#1682)
 - Fixed The NetworkConfig's checksum hash includes the NetworkTick so that clients with a different tickrate than the server are identified and not allowed to connect (#1728)
 - Fixed OwnedObjects not being properly modified when using ChangeOwnership (#1731)
+- Improved performance in NetworkAnimator (#1735)
+- Fixed display over "always sync" network animator parameters even when an animator controller override is in use (#1735)
 
 ## [1.0.0-pre.5] - 2022-01-26
 

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

@@ -62,12 +62,12 @@ public Animator Animator
             }
         }
 
-        /* 
-         * AutoSend is the ability to select which parameters linked to this animator 
+        /*
+         * AutoSend is the ability to select which parameters linked to this animator
          * get replicated on a regular basis regardless of a state change. The thinking
-         * behind this is that many of the parameters people use are usually booleans 
-         * which result in a state change and thus would cause a full sync of state. 
-         * Thus if you really care about a parameter syncing then you need to be explict
+         * behind this is that many of the parameters people use are usually booleans
+         * which result in a state change and thus would cause a full sync of state.
+         * Thus if you really care about a parameter syncing then you need to be explicit
          * by selecting it in the inspector when an NetworkAnimator is selected.
          */
         public void SetParameterAutoSend(int index, bool value)
@@ -87,6 +87,8 @@ public bool GetParameterAutoSend(int index)
             return (m_ParameterSendBits & (uint)(1 << index)) != 0;
         }
 
+        private bool m_SendMessagesAllowed = false;
+
         // Animators only support up to 32 params
         public static int K_MaxAnimationParams = 32;
 
@@ -103,11 +105,11 @@ private unsafe struct AnimatorParamCache
             public fixed byte Value[4]; // this is a max size of 4 bytes
         }
 
-        // 128bytes per Animator 
+        // 128bytes per Animator
         private FastBufferWriter m_ParameterWriter = new FastBufferWriter(K_MaxAnimationParams * sizeof(float), Allocator.Persistent);
         private NativeArray<AnimatorParamCache> m_CachedAnimatorParameters;
 
-        // We cache these values because UnsafeUtility.EnumToInt use direct IL that allows a nonboxing conversion
+        // We cache these values because UnsafeUtility.EnumToInt uses direct IL that allows a non-boxing conversion
         private struct AnimationParamEnumWrapper
         {
             public static readonly int AnimatorControllerParameterInt;
@@ -133,14 +135,6 @@ internal void ResetParameterOptions()
             m_ParameterSendBits = 0;
         }
 
-        private bool sendMessagesAllowed
-        {
-            get
-            {
-                return IsServer && NetworkObject.IsSpawned;
-            }
-        }
-
         public override void OnDestroy()
         {
             if (m_CachedAnimatorParameters.IsCreated)
@@ -153,6 +147,10 @@ public override void OnDestroy()
 
         public override void OnNetworkSpawn()
         {
+            if (IsServer)
+            {
+                m_SendMessagesAllowed = true;
+            }
             var parameters = m_Animator.parameters;
             m_CachedAnimatorParameters = new NativeArray<AnimatorParamCache>(parameters.Length, Allocator.Persistent);
 
@@ -199,9 +197,14 @@ public override void OnNetworkSpawn()
             }
         }
 
+        public override void OnNetworkDespawn()
+        {
+            m_SendMessagesAllowed = false;
+        }
+
         private void FixedUpdate()
         {
-            if (!sendMessagesAllowed)
+            if (!m_SendMessagesAllowed)
             {
                 return;
             }
@@ -233,7 +236,7 @@ private void FixedUpdate()
         private void CheckAndSend()
         {
             var networkTime = NetworkManager.ServerTime.Time;
-            if (sendMessagesAllowed && m_SendRate != 0 && m_NextSendTime < networkTime)
+            if (m_SendMessagesAllowed && m_SendRate != 0 && m_NextSendTime < networkTime)
             {
                 m_NextSendTime = networkTime + m_SendRate;
 
@@ -290,8 +293,8 @@ private bool CheckAnimStateChanged(out int stateHash, out float normalizedTime)
 
         /* $AS TODO: Right now we are not checking for changed values this is because
         the read side of this function doesn't have similar logic which would cause
-        an overflow read because it doesn't know if the value is there or not. So 
-        there needs to be logic to track which indexes changed in order for there 
+        an overflow read because it doesn't know if the value is there or not. So
+        there needs to be logic to track which indexes changed in order for there
         to be proper value change checking. Will revist in 1.1.0.
         */
         private unsafe bool WriteParameters(FastBufferWriter writer, bool autoSend)
@@ -438,11 +441,24 @@ private void SendAnimTriggerClientRpc(AnimationTriggerMessage animSnapshot, Clie
             }
         }
 
+        /// <summary>
+        /// Sets the trigger for the associated animation
+        ///  Note, triggers are special vs other kinds of parameters.  For all the other parameters we watch for changes
+        ///  in FixedUpdate and users can just set them normally off of Animator. But because triggers are transitory
+        ///  and likely to come and go between FixedUpdate calls, we require users to set them here to guarantee us to
+        ///  catch it...then we forward it to the Animator component
+        /// </summary>
+        /// <param name="triggerName">The string name of the trigger to activate</param>
         public void SetTrigger(string triggerName)
         {
             SetTrigger(Animator.StringToHash(triggerName));
         }
 
+        /// <summary>
+        /// Sets the trigger for the associated animation.  See note for SetTrigger(string)
+        /// </summary>
+        /// <param name="hash">The hash for the trigger to activate</param>
+        /// <param name="reset">If true, resets the trigger</param>
         public void SetTrigger(int hash, bool reset = false)
         {
             var animMsg = new AnimationTriggerMessage();
@@ -455,11 +471,19 @@ public void SetTrigger(int hash, bool reset = false)
             }
         }
 
+        /// <summary>
+        /// Resets the trigger for the associated animation.  See note for SetTrigger(string)
+        /// </summary>
+        /// <param name="triggerName">The string name of the trigger to reset</param>
         public void ResetTrigger(string triggerName)
         {
             ResetTrigger(Animator.StringToHash(triggerName));
         }
 
+        /// <summary>
+        /// Resets the trigger for the associated animation.  See note for SetTrigger(string)
+        /// </summary>
+        /// <param name="hash">The hash for the trigger to reset</param>
         public void ResetTrigger(int hash)
         {
             SetTrigger(hash, true);

com.unity.netcode.gameobjects/Editor/NetworkAnimatorEditor.cs

@@ -66,7 +66,17 @@ private void DrawControls()
                 return;
             }
 
-            var controller = m_AnimSync.Animator.runtimeAnimatorController as AnimatorController;
+            AnimatorController controller;
+            var overrideController = m_AnimSync.Animator.runtimeAnimatorController as AnimatorOverrideController;
+            if (overrideController != null)
+            {
+                controller = overrideController.runtimeAnimatorController as AnimatorController;
+            }
+            else
+            {
+                controller = m_AnimSync.Animator.runtimeAnimatorController as AnimatorController;
+            }
+
             if (controller != null)
             {
                 var showWarning = false;

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

@@ -370,10 +370,7 @@ internal void InternalOnNetworkSpawn()
             InitializeVariables();
         }
 
-        internal void InternalOnNetworkDespawn()
-        {
-
-        }
+        internal void InternalOnNetworkDespawn() { }
 
         /// <summary>
         /// Gets called when the local client gains ownership of this object