Skip to content

fix: NetworkAnimator not detecting and synchronizing cross fade initiated transitions#2481

Merged
NoelStephensUnity merged 14 commits intodevelopfrom
fix/networkanimator-notsending-animationstates
Apr 5, 2023
Merged

fix: NetworkAnimator not detecting and synchronizing cross fade initiated transitions#2481
NoelStephensUnity merged 14 commits intodevelopfrom
fix/networkanimator-notsending-animationstates

Conversation

@NoelStephensUnity
Copy link
Copy Markdown
Member

@NoelStephensUnity NoelStephensUnity commented Mar 30, 2023

This PR resolves the issue where NetworkAnimator was not properly detecting and synchronizing cross fade initiated transitions and not properly synchronizing changes to animation states.
It also reduces the cost of each animation state update by packing eligible properties.

Changelog

  • Fixed: Issue where NetworkAnimator was not properly detecting and synchronizing cross fade initiated transitions.
  • Fixed: Issue where NetworkAnimator was not properly synchronizing animation state updates.

Testing and Documentation

  • Includes integration test.
  • Includes updates to manual test.
  • No documentation changes or additions were necessary.

For client authoritative, the server now actually sends any pending animation updates to any non-authoritative clients.
Trigger not updating properly on server when server authoritative but was the owner sending the trigger update.
Improve detection of the type of animation state change.
This fixes the issue where cross fades were not being handled by NetworkAnimator.
This is a first pass fix and the serialization could use an overhaul to reduce bandwidth costs.
Making sure we don't toggle between transition and crossfade.
Packing values for AnimationState
Updating the manual test used to get cross fading to AnimationStates without transitions working.
Minor updates and cleanup
Added additional comments.
Adding some cross fade tests to the NetworkAnimatorTests.
Also fixed an issue with the coroutineRunner being destroyed before the IntegrationTestSceneHandler was.
This is the final test for cross fade initiated transitions and whether it is detected and synchronized or not.
updating the changelog
@NoelStephensUnity NoelStephensUnity marked this pull request as ready for review March 30, 2023 17:24
@NoelStephensUnity NoelStephensUnity requested a review from a team as a code owner March 30, 2023 17:24
writeSize += FastBufferWriter.GetWriteSize(Layer);
writeSize += FastBufferWriter.GetWriteSize(Weight);
BytePacker.WriteValuePacked(writer, Transition);
BytePacker.WriteValuePacked(writer, CrossFade);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just a note that packing on a bool or byte value is just a passthrough to writer.WriteValueSafe(value). Not anything that needs changing, just wanted to note this in case you were expecting something different.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Actually, I should make that a single byte with flags... thanks for the call out. 👍

NoelStephensUnity and others added 3 commits April 3, 2023 14:47
Making the serialization of the two bools one byte instead of two bytes.
removing redundant if check for IsServerAuthoritative
{
if (serializer.IsWriter)
var isWriter = serializer.IsWriter;
if (isWriter)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just curious but why make this change?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good catch! I originally was going to be checking that at other locations but ended up serializing in a way that did not need that. There is no reason for that adjustment and will be committing an update that removes it. 👍

m_StateFlags |= k_IsTransition;
}

if (!writer.TryBeginWrite(writeSize))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seemed to explicitly want to check it could write the data to error here. Where will it error now for the same condition?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The primary reason for the switch was due to the added byte packing to reduce bandwidth costs per update (which the size depends upon how well the property is packed)... the remaining serialized properties didn't make sense, processing wise, to do that same pre-calculation so I just ran with safe serialization for everything so that issue is no longer of concern.

removing extra local variable being used to determine if the serilization was in the write or read context.
@NoelStephensUnity NoelStephensUnity merged commit 4ebfb5f into develop Apr 5, 2023
@NoelStephensUnity NoelStephensUnity deleted the fix/networkanimator-notsending-animationstates branch April 5, 2023 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants