refactor!: remove network dictionary & set, use native container in List, add tests #1149

mattwalsh-unity · 2021-09-08T00:43:20Z

This started with a fix observed by @ShadauxCat to avoid boxing in the constructor. I had wanted to (but deferred) simplify the operations that these containers do to be more aligned with what native containers support (mostly, removing the Set operations). It then dawned on me that having them derive from ISet<>, IDictionary<>, etc. wasn't providing any value other than forcing the classes to implement things that in a netcode context aren't always sensible. And when I removed that, it was only a small amount of work to swap in the native containers.

com.unity.netcode.gameobjects/Runtime/NetworkVariable/Collections/NetworkDictionary.cs

mattwalsh-unity · 2021-09-08T00:51:41Z

com.unity.netcode.gameobjects/Runtime/NetworkVariable/Collections/NetworkDictionary.cs

@@ -127,33 +109,6 @@ public override void ReadDelta(Stream stream, bool keepDirtyDelta)
                            }
                        }
                        break;
-                    case NetworkDictionaryEvent<TKey, TValue>.EventType.RemovePair:


I can't see a good reason for RemovePair. This was a holdover from implementing IDictionary. We didn't actually do anything special like deal with if you remove an existing key with a mismatched value, and to me implementing this is unnecessarily complicated, hence out it goes.

com.unity.netcode.gameobjects/Runtime/NetworkVariable/Collections/NetworkDictionary.cs

mattwalsh-unity · 2021-09-08T00:59:27Z

com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariableTests.cs

@@ -79,6 +81,12 @@ public void Awake()
            TheDictionary.OnDictionaryChanged += DictionaryChanged;
        }

+        public void OnDestroy()


Horrible, again, I could use some help on how to avoid needing this to avoid leaks (or at least leak warnings!)

since NetworkSet is still a class, we could use its dtor to dispose underlying data structure there.
but if/when we turn it into a struct, we'll need to call Dispose() on it explicitly.
it's generally a good practice to use MonoBehaviour's Awake/OnDestroy pairs for alloc/free.
I think these look good to me, except I'd alloc TheSet in the Awake method instead of relying on ctor. (future-proofing)

I don't think we can use the dtor here because the Native Collection will complain, this is because dtors only happen when the GC happens which is not always when the object is specifically no longer referenced. So this is the proper location to Dispose the collection.

There is no Awake, NetworkVariableBase is not a monobehaviour. That is why (and this is the part I truly I don't like) this 'OnDestroy()' has to be called in the OnDestroy() method of NetworkVariableTests.

Maybe instead I need to NetworkBehaviour scan and clean up its NetworkVariable instances...

oh, that makes sense.

on the second comment though, whatever creates/registers/initializes NetVar should clean it — in this case, sounds like NetBhv might be a good candidate for that since it registers NetVars and could clean them up in its OnDestroy().
I don't remember the code very well though, if we recreate (therefore down the line realloc) them on connect/spawn, we might pair spawn/despawn, if not, we might dealloc on OnDestroy().

com.unity.netcode.gameobjects/Runtime/NetworkVariable/Collections/NetworkSet.cs

Cosmin-B · 2021-09-08T12:07:45Z

Hey @mattwalsh-unity after looking through these changes and after speaking to some people internally, I came to the conclusion that I cannot find use-cases where we would need to replicate a NetworkedSet or a NetworkDictionary? Could perhaps help me understand what is the value that we are putting out there to the users having a NetworkedSet or NetworkDictionary?

In 99% of my previous projects, I never needed a replicated set or map, and in the cases that I did needed I found other ways to go around with just a simple NetworkedList or a fixedList.

com.unity.netcode.gameobjects/Runtime/NetworkVariable/Collections/NetworkSet.cs

ThusSpokeNomad · 2021-09-08T14:29:05Z

testproject/Packages/packages-lock.json

-      "version": "2.0.9",
+      "version": "2.0.8",


not needed :P

com.unity.netcode.gameobjects/Runtime/NetworkVariable/Collections/NetworkSet.cs

VALERE91Unity · 2021-09-08T15:34:06Z

Hey @mattwalsh-unity after looking through these changes and after speaking to some people internally, I came to the conclusion that I cannot find use-cases where we would need to replicate a NetworkedSet or a NetworkDictionary? Could perhaps help me understand what is the value that we are putting out there to the users having a NetworkedSet or NetworkDictionary?

In 99% of my previous projects, I never needed a replicated set or map, and in the cases that I did needed I found other ways to go around with just a simple NetworkedList or a fixedList.

I agree with you Replicated Containers in general are messy and often a case of "Hey I want to shoot myself please give me that loaded gun without safety". I don't see a real use case for map and set (even dynamic size lists are often more buggy than useful) that cannot be better implemented as a List (often fixed-size) of buckets (example inventory).

becksebenius-unity · 2021-09-08T16:01:19Z

Hey @mattwalsh-unity after looking through these changes and after speaking to some people internally, I came to the conclusion that I cannot find use-cases where we would need to replicate a NetworkedSet or a NetworkDictionary? Could perhaps help me understand what is the value that we are putting out there to the users having a NetworkedSet or NetworkDictionary?
In 99% of my previous projects, I never needed a replicated set or map, and in the cases that I did needed I found other ways to go around with just a simple NetworkedList or a fixedList.

I agree with you Replicated Containers in general are messy and often a case of "Hey I want to shoot myself please give me that loaded gun without safety". I don't see a real use case for map and set (even dynamic size lists are often more buggy than useful) that cannot be better implemented as a List (often fixed-size) of buckets (example inventory).

I somewhat disagree with this. Users use dictionaries and hashsets in their single player code all the time and it's dead simple. To replicate that kind of state without the prebuilt options, it's a huge PITA. I agree that we shouldn't provide unsafe tools for our users, but I think we shouldn't rule out the use cases for replicating dictionaries and sets without adequate research if our goal is to make multiplayer easy for our users.

ThusSpokeNomad · 2021-09-08T16:16:26Z

Hey @mattwalsh-unity after looking through these changes and after speaking to some people internally, I came to the conclusion that I cannot find use-cases where we would need to replicate a NetworkedSet or a NetworkDictionary? Could perhaps help me understand what is the value that we are putting out there to the users having a NetworkedSet or NetworkDictionary?
In 99% of my previous projects, I never needed a replicated set or map, and in the cases that I did needed I found other ways to go around with just a simple NetworkedList or a fixedList.

I agree with you Replicated Containers in general are messy and often a case of "Hey I want to shoot myself please give me that loaded gun without safety". I don't see a real use case for map and set (even dynamic size lists are often more buggy than useful) that cannot be better implemented as a List (often fixed-size) of buckets (example inventory).

I somewhat disagree with this. Users use dictionaries and hashsets in their single player code all the time and it's dead simple. To replicate that kind of state without the prebuilt options, it's a huge PITA. I agree that we shouldn't provide unsafe tools for our users, but I think we shouldn't rule out the use cases for replicating dictionaries and sets without adequate research if our goal is to make multiplayer easy for our users.

"to make multiplayer easy" we still need to have constraints/limits on NetworkDictionary compared to System.Collections.Generic.Dictionary.
either way, SP -> MP transition wouldn't be just "replace Dictionary with NetworkDictionary" because in the networking scenario, there will be more limits, nuances and behavioral differences in between.

I'm leaning not towards any direction, just wanted to reply to that argument :)

andrews-unity · 2021-09-08T16:25:53Z

Hey @mattwalsh-unity after looking through these changes and after speaking to some people internally, I came to the conclusion that I cannot find use-cases where we would need to replicate a NetworkedSet or a NetworkDictionary? Could perhaps help me understand what is the value that we are putting out there to the users having a NetworkedSet or NetworkDictionary?
In 99% of my previous projects, I never needed a replicated set or map, and in the cases that I did needed I found other ways to go around with just a simple NetworkedList or a fixedList.

I agree with you Replicated Containers in general are messy and often a case of "Hey I want to shoot myself please give me that loaded gun without safety". I don't see a real use case for map and set (even dynamic size lists are often more buggy than useful) that cannot be better implemented as a List (often fixed-size) of buckets (example inventory).

I somewhat disagree with this. Users use dictionaries and hashsets in their single player code all the time and it's dead simple. To replicate that kind of state without the prebuilt options, it's a huge PITA. I agree that we shouldn't provide unsafe tools for our users, but I think we shouldn't rule out the use cases for replicating dictionaries and sets without adequate research if our goal is to make multiplayer easy for our users.

Sadly I will disagree with you on this... I don't know if any engine that supports replication of dictionaries out of the box, and I can't recall a time where I wanted or needed to use one to replicate. To me personally promoting the replication of these kinds of things is just bad practice and what we have there today is just not the right solution.

becksebenius-unity · 2021-09-08T16:58:49Z

Hey @mattwalsh-unity after looking through these changes and after speaking to some people internally, I came to the conclusion that I cannot find use-cases where we would need to replicate a NetworkedSet or a NetworkDictionary? Could perhaps help me understand what is the value that we are putting out there to the users having a NetworkedSet or NetworkDictionary?
In 99% of my previous projects, I never needed a replicated set or map, and in the cases that I did needed I found other ways to go around with just a simple NetworkedList or a fixedList.

I agree with you Replicated Containers in general are messy and often a case of "Hey I want to shoot myself please give me that loaded gun without safety". I don't see a real use case for map and set (even dynamic size lists are often more buggy than useful) that cannot be better implemented as a List (often fixed-size) of buckets (example inventory).

I somewhat disagree with this. Users use dictionaries and hashsets in their single player code all the time and it's dead simple. To replicate that kind of state without the prebuilt options, it's a huge PITA. I agree that we shouldn't provide unsafe tools for our users, but I think we shouldn't rule out the use cases for replicating dictionaries and sets without adequate research if our goal is to make multiplayer easy for our users.

Sadly I will disagree with you on this... I don't know if any engine that supports replication of dictionaries out of the box, and I can't recall a time where I wanted or needed to use one to replicate. To me personally promoting the replication of these kinds of things is just bad practice and what we have there today is just not the right solution.

(Responding to both Fatih and Andrew's replies, github conversations suck)

I can't say that I've had a use case for this either and I'm not so much advocating we keep this specific implementation or even necessarily any implementation if we find that it's not needed. My point is that I think our default stance should be "Provide multiplayer analogs for common single player tools" and then research where we can make exceptions, instead of the other way around. So far the justification for removing these seems to be "no one internally has a use case" which seems like a pretty weak case to me.

That said if we feel these implementations shouldn't exist today because we're trying to deliver 1.0 with the smallest stable foundation possible, I have no argument with that. My argument is with justifying its removal from project scope with limited internal discussion.

andrews-unity · 2021-09-08T17:10:22Z

"Provide multiplayer analogs for common single player tools"

I generally agree with you but my main concern is "Provide multiplayer analogs for common single player tools" the issue I have with this is that we allow you to pretty much write any C# you want and that is a rather large API surface to support. I think we should be good about making sure that things that interact with unity things do work as good as their single player parts but I can't think that you would suggest that we just support all C# because that is the API surface for single player games.

We don't have the luxury of DOTS or Unreal etc... which have placed restrictions on what you can and cannot do in their eco system and I do think we should be able to look at those same considerations while still wanting folks to have their single player and multiplayer games be similar.

mattwalsh-unity · 2021-09-14T01:56:51Z

com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs

@@ -824,5 +824,13 @@ protected NetworkObject GetNetworkObject(ulong networkId)
        {
            return NetworkManager.SpawnManager.SpawnedObjects.TryGetValue(networkId, out NetworkObject networkObject) ? networkObject : null;
        }
+
+//        public void OnDestroy()


This is the last issue I need to resolve; I really hoped this would work as it would make cleanup automatic.

mattwalsh-unity · 2021-09-14T21:30:21Z

com.unity.netcode.gameobjects/Runtime/Core/NetworkBehaviour.cs

+            //  registered here and therefore won't be cleaned up.
+            //
+            // we should study to understand the initialization patterns
+            if (!m_VarInit)


Again, there is something weird on why - in tests anyway - we instantiate a NB band then ditch it before we ever call InitializeVariables. But regardless, we are now saying that Netvars need a chance to be disposed, and this ensures we don't skip any. There are certainly better ways to register the netvars than the scan in InitializeVariables which we can look at later which might eliminate the need for this

mattwalsh-unity · 2021-09-14T21:54:09Z

com.unity.netcode.gameobjects/Runtime/NetworkVariable/Collections/NetworkList.cs

+            }
+        }
+
+        public NetworkList(NetworkVariableReadPermission readPerm, NativeList<T> values) : base(readPerm)


Matt to check if I can pass in INativeList (or read-only collection,etc.) just for the constructor

IEnumerable<T> it is :)

mattwalsh-unity · 2021-09-14T21:57:48Z

com.unity.netcode.gameobjects/Runtime/NetworkVariable/NetworkVariableBase.cs

@@ -118,5 +118,9 @@ public virtual bool CanClientWrite(ulong clientId)
        /// <param name="stream">The stream to read the delta from</param>
        /// <param name="keepDirtyDelta">Whether or not the delta should be kept as dirty or consumed</param>
        public abstract void ReadDelta(Stream stream, bool keepDirtyDelta);
+
+        public virtual void Dispose()


Prefer IDisposable

mattwalsh-unity · 2021-09-14T21:59:04Z

com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariableTests.cs

 using UnityEngine;
 using UnityEngine.TestTools;
 using NUnit.Framework;
 using Unity.Collections;

+// TODO: guard write operations


Clean up comment

mattwalsh-unity · 2021-09-14T22:03:50Z

com.unity.netcode.gameobjects/Tests/Runtime/NetworkVariableTests.cs

-                () =>
+            Assert.IsTrue(m_Player1OnServer.TheList.Count == 3);
+
+            int now = 0;


change to 'index'

…Unity-Technologies/com.unity.multiplayer.mlapi into refactor/simplify_collection_interfaces

…nsform * develop: feat: NetworkBehaviour.IsSpawned (#1190) feat: added tip to the network manager inspector that directs to install tools (MTT-1211) (#1182) refactor!: remove network dictionary & set, use native container in List, add tests (#1149) fix: Fixed remote disconnects not properly cleaning up (#1184) test: base changes to PR-1114 (#1165) test: verify do not destroy networkobjects on networkmanager shutdown (#1183) chore: removal of EnableNetworkVariable in NetworkConfig. It's always True now (#1179) fix: Fix DontDestroyWithOwner not returning ownership (#1181) test: Giving Android some more room as the connection tests are timing sensitive (#1178) fix: unitytransport connectionmode buttons (#1176) test: added min frames to multi-instance helper (#1170) chore: Add mobile tests to nightly trigger (#1161) feat: snapshot spawn pre-requisite (#1166) feat: Unity Transport + Relay (#887) feat: client scene synchronization mode (#1171) # Conflicts: # testproject/Assets/Scenes/SampleScene.unity

…am/feature/client-network-transform * sam/feature/interpolation-for-network-transform: (22 commits) fixing line issue more formatting fixing formatting issue removing not submitted LiteNetLib from ZooSam feat: Fast buffer reader and fast buffer writer (#1082) restricting public api bumping exec order feat: NetworkBehaviour.IsSpawned (#1190) feat: added tip to the network manager inspector that directs to install tools (MTT-1211) (#1182) refactor!: remove network dictionary & set, use native container in List, add tests (#1149) fix: Fixed remote disconnects not properly cleaning up (#1184) test: base changes to PR-1114 (#1165) test: verify do not destroy networkobjects on networkmanager shutdown (#1183) chore: removal of EnableNetworkVariable in NetworkConfig. It's always True now (#1179) fix: Fix DontDestroyWithOwner not returning ownership (#1181) test: Giving Android some more room as the connection tests are timing sensitive (#1178) fix: unitytransport connectionmode buttons (#1176) test: added min frames to multi-instance helper (#1170) chore: Add mobile tests to nightly trigger (#1161) feat: snapshot spawn pre-requisite (#1166) ... # Conflicts: # com.unity.netcode.gameobjects/Components/NetworkTransform.cs # testproject/Assets/Scenes/SampleScene.unity

JadenH · 2021-11-08T00:02:51Z

I'm working to upgrade my project from 0.1.0 and ran into all my NetworkList and NetworkDictionaries no longer supported due to their removal. Is there a good alternative or best practice for replacing these? Seems like support was dropped without any communication in the docs. https://docs-multiplayer.unity3d.com/docs/migration/upgrade-guide

It sounds like they were removed due to the amount of work required to support them, is that right?

ThusSpokeNomad · 2021-11-08T15:52:11Z

hey @JadenH,
we kept NetworkList<T> and removed other network collections (set, dictionary).
you should be able to stick with it in 1.0 release.

JulianHeuser · 2022-06-09T16:11:57Z

Is there any alternative to network dictionaries? And I don't want to be told to just use network list. I get that it's "bad practice" but it's really unwarranted to remove a feature like this when the alternative is a deprecated system, and the documentation for the new system is pitiful at best. The link two comments earlier isn't even a year old and it's already broken.

…ist, add tests (Unity-Technologies#1149) * refactor!: clean up network dictionary, use native containers * added dirty list as native container * remove set & dictionary * list cleanup * more tests * rounded out xtors in NetworkList * improved ienumerable, removed client writability Co-authored-by: Andrew Spiering <andrews@unity3d.com>

mattwalsh-unity requested review from ShadauxCat, ThusSpokeNomad and andrews-unity September 8, 2021 00:43