Skip to content

fix: server doesn't apply triggers to itself; add host and server Animation tests (#1746) #1751

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Feb 25, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 27 additions & 0 deletions com.unity.netcode.gameobjects/Components/NetworkAnimator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,12 @@ private unsafe void ReadParameters(FastBufferReader reader)
}
}

/// <summary>
/// Internally-called RPC client receiving function to update some animation parameters on a client when
/// the server wants to update them
/// </summary>
/// <param name="animSnapshot">the payload containing the parameters to apply</param>
/// <param name="clientRpcParams">unused</param>
[ClientRpc]
private unsafe void SendAnimStateClientRpc(AnimationMessage animSnapshot, ClientRpcParams clientRpcParams = default)
{
Expand All @@ -320,6 +326,12 @@ private unsafe void SendAnimStateClientRpc(AnimationMessage animSnapshot, Client
}
}

/// <summary>
/// Internally-called RPC client receiving function to update a trigger when the server wants to forward
/// a trigger for a client to play / reset
/// </summary>
/// <param name="animSnapshot">the payload containing the trigger data to apply</param>
/// <param name="clientRpcParams">unused</param>
[ClientRpc]
private void SendAnimTriggerClientRpc(AnimationTriggerMessage animSnapshot, ClientRpcParams clientRpcParams = default)
{
Expand Down Expand Up @@ -359,8 +371,23 @@ public void SetTrigger(int hash, bool reset = false)

if (IsServer)
{
// trigger the animation locally on the server...
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the missing server animation update bug fix

if (reset)
{
m_Animator.ResetTrigger(hash);
}
else
{
m_Animator.SetTrigger(hash);
}

// ...then tell all the clients to do the same
SendAnimTriggerClientRpc(animMsg);
}
else
{
Debug.LogWarning("Trying to call NetworkAnimator.SetTrigger on a client...ignoring");
}
}

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,21 @@ public class NetworkAnimatorTests : BaseMultiInstanceTest
private Animator m_PlayerOnServerAnimator;
private Animator m_PlayerOnClientAnimator;


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so I'm not sure how changes in this test file actually tests against the fix proposed here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before, we just called Setup and set things up in host mode. Host mode passed the old code with the bug, because the bug only shows up in server mode. The new test code now supports host and server mode and would expose the bug (if it wasn't fixed)

[UnitySetUp]
public override IEnumerator Setup()
{
yield return StartSomeClientsAndServerWithPlayers(useHost: true, nbClients: NbClients,
// this is treacherous...normally BaseMultiInstance calls StartSomeClientsAndServerWithPlayers for you
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NoelStephensUnity this was what I was trying to convey in our slack thread

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a way to handle this a little more gracefully in the NetcodeIntegraitonTest, and was why I included an example of how to do this in the documentation (because the way it is currently can be problematic to figure out the best way to achieve the same kind of results).

// in its version of Setup. In this case, I want to alternatively test Server and Host mode via parameters
// in each test. However I cannot inject the host / server mode into the corresponding Setup calls, hence
// I wrote and manually call an Init function. However, if I don't write this null version of setup
// I will get 2 calls to StartSomeClientsAndServerWithPlayers, which wrecks everything.
yield return null;
}

public IEnumerator Init(bool hostMode)
{
yield return StartSomeClientsAndServerWithPlayers(useHost: hostMode, nbClients: NbClients,
updatePlayerPrefab: playerPrefab =>
{
// ideally, we would build up the AnimatorController entirely in code and not need an asset,
Expand Down Expand Up @@ -63,8 +74,10 @@ private bool HasClip(Animator animator, string clipName)
}

[UnityTest]
public IEnumerator AnimationTriggerReset([Values(true, false)] bool asHash)
public IEnumerator AnimationTriggerReset([Values(true, false)] bool asHash, [Values(true, false)] bool hostMode)
{
yield return Init(hostMode);

// We have "UnboundTrigger" purposely not bound to any animations so we can test resetting.
// If we used a trigger that was bound to a transition, then the trigger would reset as soon as the
// transition happens. This way it will stay stuck on
Expand Down Expand Up @@ -114,9 +127,12 @@ public IEnumerator AnimationTriggerReset([Values(true, false)] bool asHash)
Assert.False(s_GloabalTimeOutHelper.TimedOut, "Timed out on client reset check");
}


[UnityTest]
public IEnumerator AnimationStateSyncTest()
public IEnumerator AnimationStateSyncTest([Values(true, false)] bool hostMode)
{
yield return Init(hostMode);

// check that we have started in the default state
Assert.True(m_PlayerOnServerAnimator.GetCurrentAnimatorStateInfo(0).IsName("DefaultState"));
Assert.True(m_PlayerOnClientAnimator.GetCurrentAnimatorStateInfo(0).IsName("DefaultState"));
Expand All @@ -137,8 +153,10 @@ public IEnumerator AnimationStateSyncTest()
}

[UnityTest]
public IEnumerator AnimationStateSyncTriggerTest([Values(true, false)] bool asHash)
public IEnumerator AnimationStateSyncTriggerTest([Values(true, false)] bool asHash, [Values(true, false)] bool hostMode)
{
yield return Init(hostMode);

string triggerString = "TestTrigger";
int triggerHash = Animator.StringToHash(triggerString);

Expand Down Expand Up @@ -172,8 +190,9 @@ public IEnumerator AnimationStateSyncTriggerTest([Values(true, false)] bool asHa
}

[UnityTest]
public IEnumerator AnimationStateSyncTestWithOverride()
public IEnumerator AnimationStateSyncTestWithOverride([Values(true, false)] bool hostMode)
{
yield return Init(hostMode);
// set up the animation override controller
var overrideController = Resources.Load("TestAnimatorOverrideController") as AnimatorOverrideController;
m_PlayerOnServer.GetComponent<Animator>().runtimeAnimatorController = overrideController;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,19 +147,19 @@ AnimatorController:
m_DefaultFloat: 0
m_DefaultInt: 0
m_DefaultBool: 0
m_Controller: {fileID: 0}
m_Controller: {fileID: 9100000}
- m_Name: TestTrigger
m_Type: 9
m_DefaultFloat: 0
m_DefaultInt: 0
m_DefaultBool: 0
m_Controller: {fileID: 0}
m_Controller: {fileID: 9100000}
- m_Name: UnboundTrigger
m_Type: 9
m_DefaultFloat: 0
m_DefaultInt: 0
m_DefaultBool: 0
m_Controller: {fileID: 0}
m_Controller: {fileID: 9100000}
m_AnimatorLayers:
- serializedVersion: 5
m_Name: Base Layer
Expand Down