feat: NetworkAnimator and ClientNetworkAnimator #1191

pdeschain · 2021-09-16T02:30:39Z

ClientNetworkAnimator lives in Samples folder and is overriding NetworkAnimator to implement client-authority
NetworkAnimator lives in Components and is server-authoritative.
updated the test scene to use the two versions of the components
LINQ usage has been completely removed and instead serialization is relying on reused buffers, and client lists are also cached (with rebuilds for everything when necessary - when a new client connects for instance)
added capability for NetworkAnimator to be late-initialized by means of TryInitialize(Animator animator) method - it would allow to choose the Mecanim Animator component to sync later than on object spawn.

PR exposes new API of the NetworkAnimator class family:
public abstract bool WillCommitChanges {get;}
public bool TryInitialize(Animator animator)

- extracted the abstract base-class NetworkAnimator - ClientNetworkAnimator lives in Samples folder and is overriding NetworkAnimator to show client-authority - ServerNetworkAnimator lives in Components and is server-controlled. - updated the test scene to use two versions of the components - optimized unnecessary LINQ usage that caused GC pressure + caching the values to avoid invoking LINQ repeatedly - added capability for NetworkAnimator to be late-initialized by means of TryInitialize(Animator animator) method - it would allow to choose the Mecanim Animator component to sync later than on object spawn.

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

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

com.unity.netcode.gameobjects/Samples/ClientNetworkAnimator/Scripts/ClientNetworkAnimator.cs

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

SamuelBellomo · 2021-09-16T14:40:35Z

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

    /// </summary>
-    [AddComponentMenu("Netcode/" + nameof(NetworkAnimator))]
-    public class NetworkAnimator : NetworkBehaviour
+    public abstract class NetworkAnimator : NetworkBehaviour


Please add automated tests for NetworkAnimator

Sam and I had a conversation - he's fine with keeping the manual test for now, provided we add an automated soon.

…ity-Technologies/com.unity.netcode.gameobjects into pdeschain/networkanimator-refactoring

…n IsHost and dealing with ClientNetworkAnimator

…ameters are applied to the animator

…ity-Technologies/com.unity.netcode.gameobjects into pdeschain/networkanimator-refactoring

…code.gameobjects into pdeschain/networkanimator-refactoring # Conflicts: # com.unity.netcode.gameobjects/Components/NetworkAnimator.cs

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

LukeStampfli · 2021-09-17T20:28:52Z

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

+        /// This property tells us if the changes made to the Mecanim Animator will be synced to other peers or not.
+        /// If not - then whatever local changes are done to the Mecanim Animator - they'll get overriden.
+        /// </summary>
+        public virtual bool WillCommitChanges => IsServer;


Any reason why we use Will here compared to CanCommit of the NetworkTransform. I think it would be nice if we could have a consistent API here?

I don't mind changing it to CanCommit.

CanCommitToAnimator - to match the pattern from NetworkTransform

LukeStampfli · 2021-09-17T20:30:22Z

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

            }
        }

+        private ulong[] GetTargetClientIds(ulong originClientId)


I'm not sure whether I fully grasp why we are doing this here and need this sort of caching? Don't we just send the update to every client?

I feel like there has to be a way to do this in a more simple way. I mean sending RPCs. I saw a similar function for the server above as well. It's a lot of boilerplate just to send an RPC?

We're sending messages to every client except:

the origin client

and the server client (because we're sending this RPC from the server)

Prior version of this code relied on LINQ all the time to do this selection

I am not 100% sure if we need to filter things out like that, but I would think that it conserves bandwidth to not send data to the peers who already have it

Okay I think for now this is fine. I hope we can get back at a later point in time and figure out whether there is a way to provide a simple pattern for something like a EveryoneExceptSelf RPC.

@pdeschain your comment "We're sending"...must include that in the source please

And again, since NetworkAnimator should have just the server NA elements, this shouldn't be necessary here, only in CNA

Lastly, more like NetworkTransform, I'd prefer were your CNA just ignores what comes its way. After all, it should have its own local state, and it knows that it's its owner, so it can just skip that and avoid all this bookkeeping, no? Maybe I'm missing something.

…transform-teleport * sam/feature/client-network-transform: fixing issue with tests where we had tickrate set to 0. Removed singleton reference for this, since multiinstance tests don't like them :( delegate to allow server to override client changes work on setDelta feat: NetworkAnimator and ClientNetworkAnimator (#1191) # Conflicts: # com.unity.netcode.gameobjects/Components/NetworkTransform.cs

This reverts commit e47b73f.

Reverts the introduction of ClientNetworkAnimator and modifications to NetworkAnimator types refs: e47b73f

…hnologies/com.unity.multiplayer.mlapi into sam/feature/transform-teleport * 'sam/feature/transform-teleport' of github.com:Unity-Technologies/com.unity.multiplayer.mlapi: revert: ClientNetworkAnimator (#1191) (#1203)

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

mattwalsh-unity · 2021-09-29T01:05:21Z

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

-                }
            }

            private void SerializeBoolParameters<T>(BufferSerializer<T> serializer) where T : IReaderWriter


Again unfortunately not a way to combine these mostly replicated functions

mattwalsh-unity · 2021-09-29T01:46:13Z

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

-                            .Where(c => c.ClientId != NetworkManager.ServerClientId)
-                            .Select(c => c.ClientId)
-                            .ToArray()
+                        TargetClientIds = ServerToClientMessagingTargetClientIds


So the intention with the client / server separation was to have the library be purely server authoritative; but to enable users that might want a ClientNetworkTransform or ClientNetworkAnimator in this case we have those implementations in Samples, just as you have done. Thing is, this NetworkAnimator itself is has the client auth code in it (line 627), and then we are just activating that code with the ClientNetworkAnimator "CanCommitToAnimator" toggle. It would be more aligned with what we're trying to do if instead we could do something like override SendAllParamsAndState in the CNA sample. It probably seems like splitting hairs, but I'd like to go more that direction

mattwalsh-unity · 2021-09-29T01:50:50Z

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

            }
        }

+        private ulong[] GetTargetClientIds(ulong originClientId)


@pdeschain your comment "We're sending"...must include that in the source please

mattwalsh-unity · 2021-09-29T01:51:44Z

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

            }
        }

+        private ulong[] GetTargetClientIds(ulong originClientId)


And again, since NetworkAnimator should have just the server NA elements, this shouldn't be necessary here, only in CNA

mattwalsh-unity · 2021-09-29T01:58:44Z

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

            }
        }

+        private ulong[] GetTargetClientIds(ulong originClientId)


Lastly, more like NetworkTransform, I'd prefer were your CNA just ignores what comes its way. After all, it should have its own local state, and it knows that it's its owner, so it can just skip that and avoid all this bookkeeping, no? Maybe I'm missing something.

* Split NetworkAnimator into Client and Server auth versions - extracted the abstract base-class NetworkAnimator - ClientNetworkAnimator lives in Samples folder and is overriding NetworkAnimator to show client-authority - ServerNetworkAnimator lives in Components and is server-controlled. - updated the test scene to use two versions of the components - optimized unnecessary LINQ usage that caused GC pressure + caching the values to avoid invoking LINQ repeatedly - added capability for NetworkAnimator to be late-initialized by means of TryInitialize(Animator animator) method - it would allow to choose the Mecanim Animator component to sync later than on object spawn. * This is a blank commit * removed servernetworkanimator * optimization pass * # * fixed NetowrkAnimator applying animatorstate to itself twice when when IsHost and dealing with ClientNetworkAnimator * reverted a logic change in the order in which the synced animator parameters are applied to the animator * standards.py fix * Serializer api changed - updated the code accordingly * Jeff's comments * Luke's comments 1 * yamato standards

) * Split NetworkAnimator into Client and Server auth versions - extracted the abstract base-class NetworkAnimator - ClientNetworkAnimator lives in Samples folder and is overriding NetworkAnimator to show client-authority - ServerNetworkAnimator lives in Components and is server-controlled. - updated the test scene to use two versions of the components - optimized unnecessary LINQ usage that caused GC pressure + caching the values to avoid invoking LINQ repeatedly - added capability for NetworkAnimator to be late-initialized by means of TryInitialize(Animator animator) method - it would allow to choose the Mecanim Animator component to sync later than on object spawn. * This is a blank commit * removed servernetworkanimator * optimization pass * # * fixed NetowrkAnimator applying animatorstate to itself twice when when IsHost and dealing with ClientNetworkAnimator * reverted a logic change in the order in which the synced animator parameters are applied to the animator * standards.py fix * Serializer api changed - updated the code accordingly * Jeff's comments * Luke's comments 1 * yamato standards

…logies#1203) Reverts the introduction of ClientNetworkAnimator and modifications to NetworkAnimator types refs: e47b73f

pdeschain and others added 3 commits September 15, 2021 21:07

This is a blank commit

5921d87

Merge branch 'develop' into pdeschain/networkanimator-refactoring

57f1dcb

pdeschain requested review from 0xFA11 and SamuelBellomo September 16, 2021 02:53

Merge branch 'develop' into pdeschain/networkanimator-refactoring

20cbe96

pdeschain requested a review from mattwalsh-unity September 16, 2021 03:13

SamuelBellomo reviewed Sep 16, 2021

View reviewed changes

com.unity.netcode.gameobjects/Components/ServerNetworkAnimator.cs Outdated Show resolved Hide resolved

SamuelBellomo reviewed Sep 16, 2021

View reviewed changes

com.unity.netcode.gameobjects/Components/NetworkAnimator.cs Outdated Show resolved Hide resolved

SamuelBellomo reviewed Sep 16, 2021

View reviewed changes

com.unity.netcode.gameobjects/Samples/ClientNetworkAnimator/Scripts/ClientNetworkAnimator.cs Outdated Show resolved Hide resolved

SamuelBellomo reviewed Sep 16, 2021

View reviewed changes

com.unity.netcode.gameobjects/Components/NetworkAnimator.cs Show resolved Hide resolved

SamuelBellomo reviewed Sep 16, 2021

View reviewed changes

pdeschain and others added 8 commits September 16, 2021 12:18

removed servernetworkanimator

83f5f0b

Merge branch 'pdeschain/networkanimator-refactoring' of github.com:Un…

c565cfb

…ity-Technologies/com.unity.netcode.gameobjects into pdeschain/networkanimator-refactoring

Merge branch 'develop' into pdeschain/networkanimator-refactoring

3e25b2b

optimization pass

806d641

#

5ea30fa

fixed NetowrkAnimator applying animatorstate to itself twice when whe…

86199d9

…n IsHost and dealing with ClientNetworkAnimator

reverted a logic change in the order in which the synced animator par…

7b75474

…ameters are applied to the animator

Merge branch 'develop' into pdeschain/networkanimator-refactoring

1312028

pdeschain requested a review from SamuelBellomo September 17, 2021 01:59

pdeschain added 2 commits September 17, 2021 10:22

standards.py fix

4e4d8bb

Merge branch 'pdeschain/networkanimator-refactoring' of github.com:Un…

77959f4

…ity-Technologies/com.unity.netcode.gameobjects into pdeschain/networkanimator-refactoring

pdeschain changed the title ~~feat: ClientNetworkAnimator and ServerNetworkAnimator~~ feat: NetworkAnimator and ClientNetworkAnimator Sep 17, 2021

Merge branch 'develop' into pdeschain/networkanimator-refactoring

af99931

pdeschain requested review from LukeStampfli and NoelStephensUnity September 17, 2021 17:02

Merge branch 'develop' of github.com:Unity-Technologies/com.unity.net…

5c19bc9

…code.gameobjects into pdeschain/networkanimator-refactoring # Conflicts: # com.unity.netcode.gameobjects/Components/NetworkAnimator.cs

pdeschain requested a review from jeffreyrainy September 17, 2021 19:24

Serializer api changed - updated the code accordingly

d161bf6

jeffreyrainy reviewed Sep 17, 2021

View reviewed changes

com.unity.netcode.gameobjects/Components/NetworkAnimator.cs Outdated Show resolved Hide resolved

LukeStampfli reviewed Sep 17, 2021

View reviewed changes

pdeschain added 2 commits September 17, 2021 17:53

Jeff's comments

4db6641

Luke's comments 1

ba521c6

pdeschain requested review from jeffreyrainy and LukeStampfli September 17, 2021 22:00

yamato standards

4892b39

LukeStampfli approved these changes Sep 17, 2021

View reviewed changes

pdeschain merged commit e47b73f into develop Sep 17, 2021

pdeschain deleted the pdeschain/networkanimator-refactoring branch September 17, 2021 22:33

JesseOlmer added a commit that referenced this pull request Sep 20, 2021

Revert "feat: NetworkAnimator and ClientNetworkAnimator (#1191)"

122ebe6

This reverts commit e47b73f.

JesseOlmer pushed a commit that referenced this pull request Sep 20, 2021

revert: ClientNetworkAnimator (#1191) (#1203)

c73e232

Reverts the introduction of ClientNetworkAnimator and modifications to NetworkAnimator types refs: e47b73f

mattwalsh-unity reviewed Sep 29, 2021

View reviewed changes

com.unity.netcode.gameobjects/Components/NetworkAnimator.cs Show resolved Hide resolved

mattwalsh-unity reviewed Sep 29, 2021

View reviewed changes

mattwalsh-unity mentioned this pull request Sep 29, 2021

feature!: ClientNetworkAnimator refactor 2 #1232

Closed

feat: NetworkAnimator and ClientNetworkAnimator #1191

feat: NetworkAnimator and ClientNetworkAnimator #1191

Uh oh!

Conversation

pdeschain commented Sep 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pdeschain Sep 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pdeschain commented Sep 16, 2021 •

edited

Loading

pdeschain Sep 17, 2021 •

edited

Loading