Skip to content

Commit

Permalink
fix: NetworkAnimator Synchronizing transition twice [MTT-3564] (Unity…
Browse files Browse the repository at this point in the history
…-Technologies#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.
  • Loading branch information
NoelStephensUnity authored Aug 17, 2022
1 parent 045cac3 commit 2d9f786
Show file tree
Hide file tree
Showing 7 changed files with 210 additions and 32 deletions.
1 change: 1 addition & 0 deletions com.unity.netcode.gameobjects/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
39 changes: 29 additions & 10 deletions com.unity.netcode.gameobjects/Components/NetworkAnimator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -427,6 +428,7 @@ internal void ServerSynchronizeNewPlayer(ulong playerId)

var animMsg = new AnimationMessage
{
Transition = m_Animator.IsInTransition(layer),
StateHash = stateHash,
NormalizedTime = normalizedTime,
Layer = layer,
Expand All @@ -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)
Expand Down Expand Up @@ -482,6 +487,7 @@ internal void CheckForAnimatorChanges()

var animMsg = new AnimationMessage
{
Transition = m_Animator.IsInTransition(layer),
StateHash = stateHash,
NormalizedTime = normalizedTime,
Layer = layer,
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -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>
Expand All @@ -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)
Expand All @@ -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);
}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -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)}]");
}
}
}
}

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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}
Expand Down
Loading

0 comments on commit 2d9f786

Please sign in to comment.