Experimental/singleton removal merge by mattwalsh-unity · Pull Request #495 · Unity-Technologies/com.unity.netcode.gameobjects

mattwalsh-unity · 2021-02-24T18:45:56Z

This is a draft PR so we can have a place to discuss the singleton removal

…codegen but expect I'll need a bit of help to figure out what to do in NetworkBehaviourILPP

unity-cla-assistant · 2021-02-24T18:46:00Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

0xFA11 · 2021-02-24T20:49:01Z

com.unity.multiplayer.mlapi/Tests/Editor/BitSerializerTests.cs

+// TODO: put this back!!!
+/*namespace MLAPI.EditorTests


this is definitely not acceptable :)
I did this because I can no longer use var inReader = PooledBitReader.Get(inStream) API

I've fixed this is the new equivalent file (NetworkSerializerTests), although my solution requires a NetworkManager to be present in the test scene.

0xFA11 · 2021-02-24T20:49:55Z

com.unity.multiplayer.mlapi/Runtime/Core/NetworkingManager.cs

            NetworkConfig.NetManager = this;

-            rpcQueueContainer = new RpcQueueContainer(this, false, LoopbackEnabled);
+            rpcQueueContainer = new RpcQueueContainer(this, false);
            //Note: Since frame history is not being used, this is set to 0
            //To test frame history, increase the number to (n) where n > 0
            rpcQueueContainer.Initialize(0);


this function and OnSingletonReady event should be gone, probably.

yes, removed it.

com.unity.multiplayer.mlapi/Editor/NetworkedAnimatorEditor.cs

jeffreyrainy · 2021-02-24T22:12:27Z

com.unity.multiplayer.mlapi/Runtime/Core/NetworkedBehaviour.cs

+            List<INetworkedVar> networkedVarList, Stream stream, ulong clientId, NetworkedBehaviour logInstance)
        {
-            using (PooledBitReader reader = PooledBitReader.Get(stream))
+            using (var reader = networkingManager.PooledBitReaders.GetReader(stream))


I get the singleton-side change. But, out of curiosity, what's the rationale for using var instead of the type ?

It's kind of like the 'auto' specifier in C++.

wackoisgod · 2021-02-25T06:39:30Z

com.unity.multiplayer.mlapi/Editor/NetworkingManagerEditor.cs

+    /// </summary>
+    public static NetworkingManager GetAnyNetworkingManager()
+    {
+        return FindObjectOfType<NetworkingManager>();


While I get this is just editor code FindObjectOfType is one of the worst functions ever! Also assume we have more than one manager in a scene that has different settings does that matter ?

We had similar discussions over at the RFC-side in case you'd like to contribute :)

wackoisgod · 2021-02-25T06:41:59Z

com.unity.multiplayer.mlapi/Editor/TrackedObjectEditor.cs

-            if(NetworkingManager.Singleton != null && NetworkingManager.Singleton.IsServer)
+
+            //FIXME: Singleton Conversion, get all of them and be sure to get the server!
+            NetworkingManager manager = NetworkingManagerEditor.GetNetworkingManager(true);


So I think generally in the non-singleton case I would expect the editors to show only useful data if the actual object being selected is a NetworkManager... I just don't think that picking the first thing you see if actually useful.

Also FindObjectOfType doesn't guarantee order so you could call it more than once and get different objects which doesn't seem like the desired behavior either.

This is probably the #1 unresolved issue in this addition

I called out something similar on the RFC-side, inlining my suggestions here too:

Rather than finding NetworkManager with GameObject.FindWithTag, it might be useful to have a static NetworkManager[] array globally where we could quickly iterate over and find the one we're looking for.

Similar to what I said above, we could store NetworkManager instances in a static NetworkManager[] container to avoid Object.FindObjectOfType call.

(response to Matt's Dictionary idea)
oh, static array or hashmap, the main idea being avoiding gameobject hierarchy traversal with Object.FindObjectOfType because seems like it could be unnecessary with a faster centralized static container lookup.

Yeah, I think one way or another we'll want a central table of NMs

wackoisgod · 2021-02-25T06:43:16Z

com.unity.multiplayer.mlapi/Runtime/Core/NetworkedObject.cs

+        /// The NetworkingManager that owns this Object. 
        /// </summary>
-        public NetworkingManager NetworkManager => NetworkingManager.Singleton;
+        public NetworkingManager NetManager { get; internal set; }


Why did we change the name to NetManager ?

yeah, we shouldn't change that like this — we're going to have a renaming RFC soon where we will specifically discuss naming types.

these are all normalized to NetworkManager now.

wackoisgod · 2021-02-25T06:44:43Z

com.unity.multiplayer.mlapi/Runtime/Core/NetworkedObject.cs

+                return;
+            }
+
+            NetworkingManager networkingManager = networkedObjects[0].NetManager;


Is it guaranteed that every NetworkObject will have the same NetManager?

@TwoTenPvP this (and the non-static NetworkShow) are not in use. Are these worth keeping around?

If so, we could make this an instance method off of NetworkedManager and if you pass in a NetworkedObject that doesn't belong to this NM, just skip it or throw an exception.

Or, it could be a static method off of NM. It really doesn't seem to have much to to with NetworkedObject. @wackoisgod not every NetworkObject has to have the same NM; but as written it would just show the pile of objects passed in regardless of NM.

It was very difficult for me to envision a scenario where you would hand this method a heterogenous list of NetworkObjects, belonging to multiple different NetworkManagers. It seemed like something that could be handled by specifying that as a method precondition in the method's comments, which I've added in my latest changes.

That said, I was anticipating these methods would go away in favor of matt's new visibility work.

wackoisgod · 2021-02-25T06:48:14Z

So I am concerned about that var name change from NetworkManager to NetManager on things as that is a breaking change

com.unity.multiplayer.mlapi/Editor/MLAPIProfiler.cs

mattwalsh-unity · 2021-02-26T01:44:39Z

com.unity.multiplayer.mlapi/Editor/MLAPIProfiler.cs

-					container.ticks[i] = ProfilerTick.FromStream(stream);
-				}
+                {
+                    container.ticks[i] = ProfilerTick.FromStream(manager, stream);


Same story here, in the FromStream lambda, you can first read the NM id, then de-ref and not need to pass this in.

Also was able to break this dep same was as above

mattwalsh-unity · 2021-02-26T02:04:08Z

com.unity.multiplayer.mlapi/Runtime/Configuration/NetworkConfig.cs

            using (BitStream stream = new BitStream(binary))
            {
-                using (PooledBitReader reader = PooledBitReader.Get(stream))
+                using (PooledBitReader reader = NetManager.PooledBitReaders.GetReader(stream))


How come this has to hang off of NetManager?

Fixed to be a static again

mattwalsh-unity · 2021-02-26T02:15:55Z

com.unity.multiplayer.mlapi/Runtime/Core/NetworkedObject.cs

        public static void NetworkHide(List<NetworkedObject> networkedObjects, ulong clientId)
        {
-            if (!NetworkingManager.Singleton.IsServer)
+            if( networkedObjects.Count == 0 )


Same story as NetworkShow, above

mattwalsh-unity · 2021-02-26T02:21:37Z

com.unity.multiplayer.mlapi/Runtime/LagCompensation/LagCompensationManager.cs

+            if( simulatedObjects.Count > 0 )
            {
-                throw new NotServerException("Only the server can perform lag compensation");
+                if( !simulatedObjects[0].NetObject.NetManager.IsServer )


Oh boy, I guess you could however unlikely, have a collection of simulatedObjects were some are from one NM that is set up as a server and some are as a client. I haven't worked with LagCompensationManager though.

Is this telling us that this should be an instance method? There should be an LCM per NM?

mattwalsh-unity · 2021-02-26T02:22:09Z

com.unity.multiplayer.mlapi/Runtime/LagCompensation/LagCompensationManager.cs

+        public static void Simulate(NetworkingManager networkingManager, ulong clientId, Action action)
        {
-            if (!NetworkingManager.Singleton.IsServer)
+            if (!networkingManager.IsServer)


Yeah, again, maybe this is telling us this should be an instance method.

mattwalsh-unity · 2021-02-26T02:25:43Z

com.unity.multiplayer.mlapi/Runtime/Messaging/InternalMessageSender.cs

 namespace MLAPI.Messaging
 {
-    internal static class InternalMessageSender
+    internal class InternalMessageSender


We could leave this class static (and stateless) and just pass in NM in each send. What does the room think? That's more like what the UnnamedMessageSender does

0xFA11 · 2021-02-26T13:22:07Z

com.unity.multiplayer.mlapi/Runtime/Core/NetworkingManager.cs

-        /// <summary>
-        /// The singleton instance of the NetworkingManager
-        /// </summary>
-        public static NetworkingManager Singleton { get; private set; }


I think we should get rid of this singleton boss but this PR looks like a mess to me.
(I know this is not a very constructive feedback, sorry).

There are lots of things going on here:

NetManager/NetworkManager, NetObject and other naming inconsistencies

Assumptions like:

Controversial changes like pinning PooledBitReader, InternalMessageSender etc to NetworkManager

using (PooledBitReader reader = NetManager.PooledBitReaders.GetReader(stream))

other fundamental behavioral changes which could be counted as high risks

Overall, I believe we need to redo/rework singleton removal again and think about design changes more carefully.

However, if I should consider this PR only as a draft that's never going to be merged into develop, then I'm fine :)

This is a draft PR so we can have a place to discuss the singleton removal

I wanted to double-check the statement above.

mattwalsh-unity · 2021-02-26T23:02:11Z

com.unity.multiplayer.mlapi/Runtime/Messaging/Buffering/BufferManager.cs

            foreach (KeyValuePair<ulong, Queue<BufferedMessage>> pair in bufferQueues)
            {
-                while (pair.Value.Count > 0 && Time.realtimeSinceStartup - pair.Value.Peek().bufferTime >= NetworkingManager.Singleton.NetworkConfig.MessageBufferTimeout)
+                while (pair.Value.Count > 0 && Time.realtimeSinceStartup - pair.Value.Peek().bufferTime >= messageBufferTimeout)


Do we think we want to have per-NM messageBufferTimeouts or could we simplify and have one shared static one?

mattwalsh-unity · 2021-02-26T23:11:10Z

com.unity.multiplayer.mlapi/Runtime/Messaging/CustomMessageManager.cs

        public static event UnnamedMessageDelegate OnUnnamedMessage;

-        internal static void InvokeUnnamedMessage(ulong clientId, Stream stream)
+        internal static void InvokeUnnamedMessage(NetworkingManager networkingManager, ulong clientId, Stream stream)


When I look at this function I really wonder what if we mad this an instance method off of NetworkingManager? It's weird to have a static method that takes an instance and then does most of its work with that instance.

com.unity.multiplayer.mlapi/Runtime/Messaging/RpcQueue/QueueHistoryFrame.cs

mattwalsh-unity · 2021-02-27T19:07:47Z

com.unity.multiplayer.mlapi/Runtime/Messaging/RpcQueue/RpcQueueProcessor.cs

        // Batcher object used to manage the RPC batching on the send side
        private readonly RpcBatcher m_RpcBatcher = new RpcBatcher();
        private const int k_BatchThreshold = 512;
+        private NetworkingManager m_NetManager;


So it appears we have an NM instance in this class so that:

we can get an NM-specific RPC Queue Container, which I have some doubts on is the right abstraction

we can call NM:InvokeRPC (and, in turn, InvokeRPC is only in NM because it needs to find the right spawn manager, and maybe there's a cleaner way to do that)

we can call NM:MessageSender.Send()

com.unity.multiplayer.mlapi/Runtime/NetworkedVar/Collections/NetworkedDictionary.cs

mattwalsh-unity · 2021-02-27T19:20:44Z

com.unity.multiplayer.mlapi/Runtime/Profiling/ProfilerTickData.cs

        /// <param name="stream">The stream containing the ProfilerTick data</param>
        /// <returns>The ProfilerTick with data read from the stream</returns>
-		public static ProfilerTick FromStream(Stream stream)
+		public static ProfilerTick FromStream(NetworkingManager manager, Stream stream)


NM is being passed in so that ProfilerTickData can get an NM so that it can get a PooledBitReader, which - not to belabor the point - should be detached from NM

mattwalsh-unity · 2021-02-27T19:22:16Z

com.unity.multiplayer.mlapi/Runtime/SceneManagement/SceneSwitchProgress.cs

        /// </summary>
        public event OnClientLoadedSceneDelegate OnClientLoadedScene;
+
+        private NetworkingManager networkingManager;


inconsistent, is netManager everywhere else

mattwalsh-unity · 2021-02-27T19:29:33Z

com.unity.multiplayer.mlapi/Runtime/Transports/UNET/UnetTransport.cs

+
+            NetworkEventType eventType = relayTransport.ReceiveFromHost(hostId, out int connectionId, out int channelId, messageBuffer, messageBuffer.Length, out int receivedSize, out byte error);
+
+            if(eventType != NetworkEventType.Nothing )


Did you mean to leave this in?

mattwalsh-unity

Mostly want to have a discussion on where we can break the NetworkManager dependencies

com.unity.multiplayer.mlapi/Runtime/Messaging/RpcQueue/RpcQueueContainer.cs

mattwalsh-unity · 2021-03-05T16:48:37Z

com.unity.multiplayer.mlapi/Runtime/Serialization/BitReader.cs

                ulong networkId = ReadUInt64Packed();

-                if (networkingManager.SpawnManager.SpawnedObjects.ContainsKey(networkId))
+                var netManager = SpawnManager.SpawnedObjectsByNetworkingManager[networkId];


After talking to @TwoTenPvP, I've come to see this is probably not correct code. Instead, I intend to remove this table and instead pass the NM into ReadObjectPacked.

wackoisgod · 2021-03-07T00:35:07Z

com.unity.multiplayer.mlapi/Editor/NetworkingManagerEditor.cs

+    {
+        foreach (var manager in FindObjectsOfType<NetworkingManager>())
+        {
+            if (manager.IsServer == isServer)


So I am confused by the IsServer check because from what I can tell its always false? https://github.com/Unity-Technologies/com.unity.multiplayer.mlapi/pull/495/files#diff-641f8f5b259017aefa68498910442cb599d894ed91c96e4811d0a79b5f0151feR24

no, that's for the NetworkedObjectEditor.cs in the Editor, not at Runtime I believe, no?

dwoodruffsf · 2021-03-07T23:11:45Z

Just a heads up that I have updated this branch to fix all merge conflicts with develop. The branch is mergeable (at least as of this comment), but for some reasons, neither my latest changes, nor the merge status, is showing properly in this PR (maybe because this is Matt's PR, and it still represents the merge at the last commit that Matt made it?).

dwoodruffsf · 2021-03-08T17:45:23Z

Follow on note: I have finished an integration test against the BossRoom sample (which turned up a ton of problems!) I have fixed all of them, and I now think this PR could probably be taken (or rather a new one, from the latest commit on this branch). A couple of interesting things did change:

Added ActiveScene, FindObjectsOfTypeInScene, and FindGameObjectsWithTagInScene methods to NetworkManager. These are worth looking at; they encapsulate the weirdness of what it means to say a NetworkManager is "associated" with a scene, and have usability considerations, particularly for devs that are setting DontDestroyOnLoad on their NetworkManagers.
Made NetworkWriterPool symmetric with NetworkReaderPool; now they both live on NetworkManager, even though NetworkWriterPool doesn't really need NetworkManager for anything except logging. This is basically the opposite direction that Matt was going where he was trying to break the dependency between NetworkManager and the serialization layer. What would you guys think about just removing the Read/Write(GameObject) magic? This seems like a low-value feature to me, and it's causing lots of problems with this refactor--without it we could easily make the NetworkReader/Writer pools static, without hitting the concerns I expressed to Matt in a side thread.

TwoTenPvP · 2021-04-29T12:31:18Z

@mattwalsh-unity Can this be closed?

JesseOlmer · 2021-05-19T16:08:26Z

@mattwalsh-unity can we close this?

dwoodruffsf and others added 5 commits February 23, 2021 09:39

first pass on singleton removal work. I have gotten things as far as …

bc9cef3

…codegen but expect I'll need a bit of help to figure out what to do in NetworkBehaviourILPP

setting up example of server and client connecting in process

d4579ba

Fix: Allowing singleton to work with uNET

e6bb49e

singleton fixups

8085b11

merge branch 'develop' into 'experimental/singleton-removal-merge'

6c41534

mattwalsh-unity requested review from 0xFA11, JesseOlmer and TwoTenPvP and removed request for TwoTenPvP February 24, 2021 18:46

mattwalsh-unity marked this pull request as draft February 24, 2021 18:48

mattwalsh-unity requested review from NoelStephensUnity, TwoTenPvP and jeffreyrainy February 24, 2021 18:48

0xFA11 reviewed Feb 24, 2021

View reviewed changes

jeffreyrainy reviewed Feb 24, 2021

View reviewed changes

com.unity.multiplayer.mlapi/Editor/NetworkedAnimatorEditor.cs Outdated Show resolved Hide resolved

jeffreyrainy reviewed Feb 24, 2021

View reviewed changes

com.unity.multiplayer.mlapi/Editor/NetworkedAnimatorEditor.cs Outdated Show resolved Hide resolved

jeffreyrainy reviewed Feb 24, 2021

View reviewed changes

mattwalsh-unity requested a review from dwoodruffsf February 25, 2021 03:07

wackoisgod reviewed Feb 25, 2021

View reviewed changes

mattwalsh-unity commented Feb 26, 2021

View reviewed changes

com.unity.multiplayer.mlapi/Editor/MLAPIProfiler.cs Outdated Show resolved Hide resolved

mattwalsh-unity commented Feb 26, 2021

View reviewed changes

restore netowrked animator headers, etc.

35f3d20

mattwalsh-unity commented Feb 26, 2021

View reviewed changes

0xFA11 reviewed Feb 26, 2021

View reviewed changes

mattwalsh-unity commented Feb 26, 2021

View reviewed changes

com.unity.multiplayer.mlapi/Runtime/Messaging/RpcQueue/QueueHistoryFrame.cs Outdated Show resolved Hide resolved

mattwalsh-unity commented Feb 27, 2021

View reviewed changes

com.unity.multiplayer.mlapi/Runtime/NetworkedVar/Collections/NetworkedDictionary.cs Outdated Show resolved Hide resolved

mattwalsh-unity commented Feb 27, 2021

View reviewed changes

com.unity.multiplayer.mlapi/Runtime/Messaging/RpcQueue/RpcQueueContainer.cs Outdated Show resolved Hide resolved

mattwalsh-unity added 3 commits February 27, 2021 14:53

linked networked manager to transport

d961ec1

hack checkpoint

aa43072

one more netmanager dependency removal

1537ab6

mattwalsh-unity commented Mar 5, 2021

View reviewed changes

more misc fixups

5498470

wackoisgod reviewed Mar 7, 2021

View reviewed changes

mattwalsh-unity closed this May 19, 2021

0xFA11 deleted the experimental/singleton-removal-merge branch May 19, 2021 17:04


		NetworkEventType eventType = relayTransport.ReceiveFromHost(hostId, out int connectionId, out int channelId, messageBuffer, messageBuffer.Length, out int receivedSize, out byte error);

		if(eventType != NetworkEventType.Nothing )

Conversation

mattwalsh-unity commented Feb 24, 2021

Uh oh!

unity-cla-assistant commented Feb 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

mattwalsh-unity Feb 26, 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!

wackoisgod commented Feb 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattwalsh-unity Feb 26, 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!

0xFA11 Feb 26, 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

unity-cla-assistant commented Feb 24, 2021 •

edited

Loading

mattwalsh-unity Feb 26, 2021 •

edited

Loading

wackoisgod commented Feb 25, 2021 •

edited

Loading

mattwalsh-unity Feb 26, 2021 •

edited

Loading

0xFA11 Feb 26, 2021 •

edited

Loading