feat: snapshot. Milestone 1b. Testproject "manual test" "scene transitioning" working with snapshot. Disabled by default. #862

jeffreyrainy · 2021-05-27T18:40:05Z

This updates develop with the current Snapshot-System branch. No impact as everything is disabled by default, but keeps the branches close to each other.

…t every frame, having an empty NativeContainer. New channel and messagetype.

…ure around writing and sending the snapshot

…ot to send and the received one

…anch

…hot-system

… received

…updated on all machines

…e network manager

…hot-system

…to develop

…intainability

…hot-system

…to experimental/snapshot-system-m1

… of snapshot table

… of passing its value around

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

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

LukeStampfli · 2021-06-17T18:15:31Z

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

+            {
+                if (Entries[i].Fresh && Entries[i].Key.TickWritten > 0)
+                {
+                    var nv = FindNetworkVar(Entries[i].Key);


This will fail if a single NetworkObject has changed between the client and the server.

Example:

Client sends variable for NetworkObject with Id 5 (PlayerPrefab)

Server Despawns NetworkObjectId with Id 5

Server spawns (NetworkOtherPrefab), Id 5 gets assigned to that new object.

This function tries to read the snapshot, searches for a variable which does not exist on "NetworkOtherPrefab"

Hmm, I was prepared to write a very similar comment, until this popped out.

Ok, good point, seems like I made assumptions about uniqueness of Ids which don't hold.

How did that not manifest in existing mlapi ? It seems the whole serialization makes the same assumptions, no ?

Right now this is not handled in MLAPI. There is an id recycling delay which in theory should prevent the above situation in many cases. But that's just one of the cases. Another problem would be if the object just does not exist anymore due to being despawned/destroyed.

I don't think this PR should/can address all those issues. Just wanted to point them out.

LukeStampfli · 2021-06-17T18:20:05Z

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

@@ -20,16 +20,16 @@ internal struct VariableKey
        public ulong NetworkObjectId; // the NetworkObjectId of the owning GameObject
        public ushort BehaviourIndex; // the index of the behaviour in this GameObject
        public ushort VariableIndex; // the index of the variable in this NetworkBehaviour
+        public ushort TickWritten; // the network tick at which this variable was set


This VariableKey should implement IEquatable<VariableKey> and override the hashcode to avoid boxing and generally for better performance.

Nice trick!

Can you elaborate where the boxing occurs, currently, and how making the VariableKey equatable would help ? Are you suggesting the Find function to rely on this equatable interface ?

I thought that we were using this key in hashsets/dictionary as a key. After looking more closely at the code this seems to not be the case.

The IEquatable interface prevents boxing when doing equality comparisons between two variables of the same type. So variableKey1.Equals(variableKey2) allocates if VariableKey is a struct type. Usually it's standard practice to have structs which act as a key implement the interface. Here it is not necessary yet but not implementing the interface could burn us in the future if someone uses this in a dictionary/hashset.

Rider can auto generate the interface just implement Equatable<VariableKey> and Rider Intelisense will give you then option to generate equality members.

This is a pretty old branch. Considering that:

This branch uses (!m_TickIndex || (Entries[i].Key.TickWritten == key.TickWritten))) (One snapshot cares about tick, the other doesn't)

The snapshot-system-spawn branch has a plain comparison.

Those are not actually used as key.
I'll simply throw a comment for now, and eventually add the IEquatable interface in the spawn branch.

Cosmin-B · 2021-06-17T18:34:58Z

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

        public ushort Position; // the offset in our Buffer
-        public ushort Length; // the length of the data in Buffer
+        public ushort Length; // the Length of the data in Buffer
        public bool Fresh; // indicates entries that were just received

        public const int NotFound = -1;


Should this live inside the actual struct?

I don't get your question. Maybe the comment can be improved. This is the length of a specific entry. So, yes, each Entry requires its own length.

Ups, I meant the NotFound constant variable.

It could be in Snapshot too. But since it is a constant and doesn't contribute to object size, what difference does it make? I kind-of likeEntry.NotFound as it expresses clearly what it means. How do others feel ?

…hot-system-m1b

mattwalsh-unity · 2021-07-02T00:28:41Z

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

@@ -74,7 +89,8 @@ public int Find(VariableKey key)
            {
                if (Entries[i].Key.NetworkObjectId == key.NetworkObjectId &&
                    Entries[i].Key.BehaviourIndex == key.BehaviourIndex &&
-                    Entries[i].Key.VariableIndex == key.VariableIndex)
+                    Entries[i].Key.VariableIndex == key.VariableIndex &&


Seems like it would be pretty straightforward to establish a dictionary and then de-ref the key.

On m_tickIndex, I had to stop to reason about this code. I think I would name it m_TickAsIndex else my brain really wants it to mean "the index of a tick"

Ok, so this logic is, if I find a matching entry AND (either I'm not using tick as an index OR the tick written matches the one I'm looking at).

I'm assuming the tick matching part is so that we don't have to clear out old entries? I get that. But I'm not sure why if this isn't true that it's still ok for it to be a hit if m_tickIndex is false.

In this (old) branch, we still had different Snapshots per connection. I was swayed, in our discussions, by Luke's very good and valid arguments. As such, the latest branch (spawn) doesn't need this anymore.

But to clarify this code, in order for a Key to match, the NetworkObjectId, the BehaviourIndex and the VariableIndex have to match. Also, you need one of two things to hold:

either this Snapshot doesn't care about ticks (!m_TickIndex) or

the ticks actually match (Entries[i].Key.TickWritten == key.TickWritten)

This was so that per-connection snapshots would only maintain the latest value of each variable, but the main snapshot would maintain multiple.

I could comment a bit to improve, but since the next branch will drop this, it would be a bit moot.

mattwalsh-unity · 2021-07-02T00:33:54Z

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

@@ -86,16 +102,12 @@ public int Find(VariableKey key)
        /// <summary>
        /// Adds an entry in the table for a new key
        /// </summary>
-        // todo: change the params to take a Key instead
-        public int AddEntry(ulong networkObjectId, int behaviourIndex, int variableIndex)
+        public int AddEntry(VariableKey k)


I believe this can be declared 'in'

mattwalsh-unity · 2021-07-02T00:42:58Z

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

        public ushort Position; // the offset in our Buffer
-        public ushort Length; // the length of the data in Buffer
+        public ushort Length; // the Length of the data in Buffer
        public bool Fresh; // indicates entries that were just received


Nit, I might re-name this 'Unprocessed' (assuming that's what it means)

Yes, but same comment as much of above. This got removed already from the spawn branch.

mattwalsh-unity · 2021-07-02T00:46:32Z

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

+        /// Must match WriteEntry
+        /// </summary>
+        /// <param name="reader">The readed to read the entry from</param>
+        internal Entry ReadEntry(NetworkReader reader)


Can be 'in' modified, no?

Why? NetworkReader is a reference type. I thought in was meant to improve the performance by not copying value types. If it was an in parameter, it would only prevent assigning reader a new NetworkReader. Since the code doesn't do it, since it wouldn't affect the caller if it did, and since there's no performance improvements in doing it, I fail to see the point.

mattwalsh-unity · 2021-07-02T00:47:24Z

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

        {
            // todo --M1--
            // this will change once we start reusing the snapshot buffer memory
            // todo: deal with free space
            // todo: deal with full buffer

-            entry.Position = (ushort)FreeMemoryPosition;
+            int pos;


Super nit, put right ahead of first usage on line 175 instead

mattwalsh-unity · 2021-07-02T00:47:57Z

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

+
+            if (!ret)
+            {
+                //todo: error handling


Maybe throw an assert in here for the time being? This is a 'game over' situation, yes?

Would you clarify what you would like to see? Something like:

namespace MLAPI.Exceptions { public class SnapshotMemoryException : Exception

?

I read throw as in exception. The alternative is indeed just an assert. But since Assert are development build only, I feel we need an alternative way to report and handle errors

mattwalsh-unity · 2021-07-02T00:54:09Z

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

+
+            for (var i = 0; i < LastEntry; i++)
+            {
+                if (Entries[i].Fresh && Entries[i].Key.TickWritten > 0)


Do we have to do a linear search from 0 until we find Fresh ones? Like could we just have an index into the first Fresh one rather than scan down to it?

For some reason I figured Entries would be a circular buffer, but maybe instead when we get a new entry we just find the first un-fresh slot and stick it there.

Actually yeah, I never see where LastEntry decreases except when clear sets it to zero. I'm consufed

Yes, good points. Maybe I was expecting code to prematurely go into develop and make in in stages.

Once we have acknowledgements, variables will be removed from the main snapshots. When the last variable is removed LastEntry will decrease back. This PR was only meant for the M1b milestone and getting the scale-demo to pass.

mattwalsh-unity · 2021-07-02T01:39:37Z

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

        private NetworkManager m_NetworkManager = NetworkManager.Singleton;
+        private Snapshot m_Snapshot = new Snapshot(NetworkManager.Singleton, false);


perhaps rename to m_ServerSnapshot?

Good point. But the spawn version is now using a single snapshot, so the naming might not be super important. :-)

Also, when it doesn't come with extra cost, I try to abstract away the difference between server and client. Maybe one day we'll end up with hybrid combination: partial meshes, connection between hosts, etc... Then, the less "server"- and "client"-specific code we have, the most flexible it should be.

mattwalsh-unity · 2021-07-02T01:47:15Z

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

+
+                if (!m_ClientReceivedSnapshot.ContainsKey(clientId))
+                {
+                    m_ClientReceivedSnapshot[clientId] = new Snapshot(m_NetworkManager, false);


Shouldn't there be a corresponding removal from m_ClientReceivedSnapshot when a client disconnects? (I know it wouldn't happen here, but seems like we need to listen for client disconnects or something to sweep these out)

This is another part that goes away in the spawn branch. Since there's a single snapshot, we don't allocate per client.

mattwalsh-unity

Clarifications / renames / a few concerns

LukeStampfli

I don't think there is anything critical anymore preventing us from merging this. All Code looks good or has a comment explaining the WIP nature of it.

…hot-system-m1b

…nsform * develop: feat: snapshot. Adding RTT computation API. (#963) feat: snapshot. Milestone 1b. Testproject "manual test" "scene transitioning" working with snapshot. Disabled by default. (#862) test: multiprocess tests part 6: fixing issues runnings all tests together (#957) docs: Perf tests part 5. Adding documentation and instructions (#952) test: Perf tests part 4. Adding example of performance test with spawning x network objects at once (#925) test: Correctly teardown OnNetworkSpawn/Despawn tests.

jeffreyrainy added 30 commits April 28, 2021 13:23

chore: MTT-635 Add a Snapshot class to the NetworkManager. First part

b26e577

chore: MTT-635 Some part of Snapshot system. Sending an empty snapsho…

4a59e2e

…t every frame, having an empty NativeContainer. New channel and messagetype.

chore: MTT-635 Dropping native arrays for now, adding a bit of struct…

4c0e113

…ure around writing and sending the snapshot

chore: MTT-635 Added EntryBlock. Used separate objects for the snapsh…

4cd3034

…ot to send and the received one

chore: MTT-635 minor, constant for size, improved comment

97c8200

MTT-635 minor, adjusting code to minimize diffs, for a first commit

2527505

feat: MTT-635 code review comments

600279f

feat: snapshot system, in-progress stuff before moving to a common br…

068e1f9

…anch

feat: snapshot, incremental progress

4c29875

feat: snapshot. Server vars now replicate on client's

4147f24

Merge remote-tracking branch 'origin/develop' into experimental/snaps…

0e979e7

…hot-system

feat: snapshot. Removed small memory allocation for list of variables…

adc1d4b

… received

feat: snapshot. Format and old comments removal

5f800b0

feat: snapshot. sending the snapshot from client to server too

a431032

feat: snapshot. Storing ticks so 1 server and 2 client have all vars …

5e83049

…updated on all machines

style: 'if' instead of 'switch' for NetworkUpdateStage.EarlyUpdate

aabefbf

feat: snapshot. Removed a memory allocation, used a member var for th…

6a97b72

…e network manager

Merge remote-tracking branch 'origin/develop' into experimental/snaps…

8568fdf

…hot-system

feat: snapshot. Disabling snapshot for the M1 branch, to allow merge …

7807636

…to develop

style: Fix for coding style

1638fa3

style: Fix for coding style

082bd75

refactor: snapshot. Moved matching 'read' and 'write' together for ma…

1312f9c

…intainability

refactor: snapshot. Moved matching 'read' and 'write' together for ma…

79a020e

…intainability

Merge remote-tracking branch 'origin/develop' into experimental/snaps…

98fba2b

…hot-system

Merge remote-tracking branch 'origin/experimental/snapshot-system' in…

6799dcd

…to experimental/snapshot-system-m1

feat: snapshot. Adding tick to snapshot frame. Improving DebugDisplay…

41934c8

… of snapshot table

feat: snapshot. Review comment, using the behaviour member id instead…

4969fec

… of passing its value around

feat: snapshot. Review comment, using the behaviour member id instead…

bd466d8

… of passing its value around

style: Adding comments/documentation

8a206ec

style: Adding comments/documentation

ce3f2d2

LukeStampfli reviewed Jun 17, 2021

View reviewed changes

com.unity.multiplayer.mlapi/Runtime/Core/SnapshotSystem.cs Show resolved Hide resolved

Cosmin-B suggested changes Jun 17, 2021

View reviewed changes

com.unity.multiplayer.mlapi/Runtime/Core/IndexAllocator.cs Outdated Show resolved Hide resolved

LukeStampfli reviewed Jun 17, 2021

View reviewed changes

Cosmin-B reviewed Jun 17, 2021

View reviewed changes

jeffreyrainy added 2 commits June 18, 2021 11:25

feat: snapshot. Applying code review comments

51698a2

feat: snapshot. Checking variable presence, before trying to read it.

2276459

jeffreyrainy requested a review from Cosmin-B June 28, 2021 12:58

jeffreyrainy added 2 commits June 28, 2021 09:47

Merge remote-tracking branch 'origin/develop' into experimental/snaps…

c2fb29a

…hot-system-m1b

Merge remote-tracking branch 'origin/develop' into experimental/snaps…

01ff555

…hot-system-m1b

mattwalsh-unity reviewed Jul 2, 2021

View reviewed changes

mattwalsh-unity suggested changes Jul 2, 2021

View reviewed changes

LukeStampfli approved these changes Jul 5, 2021

View reviewed changes

Cosmin-B approved these changes Jul 5, 2021

View reviewed changes

jeffreyrainy added 2 commits July 5, 2021 17:15

feat: snapshot. Some review comments

aecefeb

Merge remote-tracking branch 'origin/develop' into experimental/snaps…

ecae08e

…hot-system-m1b

mattwalsh-unity approved these changes Jul 14, 2021

View reviewed changes

Merge remote-tracking branch 'origin/develop' into experimental/snaps…

1cac0d9

…hot-system-m1b

jeffreyrainy enabled auto-merge (squash) July 15, 2021 02:54

jeffreyrainy merged commit bdd7d71 into develop Jul 15, 2021

jeffreyrainy deleted the experimental/snapshot-system-m1b branch July 15, 2021 03:09

		private NetworkManager m_NetworkManager = NetworkManager.Singleton;
		private Snapshot m_Snapshot = new Snapshot(NetworkManager.Singleton, false);

feat: snapshot. Milestone 1b. Testproject "manual test" "scene transitioning" working with snapshot. Disabled by default. #862

feat: snapshot. Milestone 1b. Testproject "manual test" "scene transitioning" working with snapshot. Disabled by default. #862

Uh oh!

Conversation

jeffreyrainy commented May 27, 2021

Uh oh!

Uh oh!

Uh oh!

LukeStampfli Jun 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!

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!

jeffreyrainy Jun 18, 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!

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!

jeffreyrainy Jul 5, 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!

jeffreyrainy Jul 5, 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!

mattwalsh-unity Jul 2, 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!

LukeStampfli Jun 17, 2021 •

edited

Loading

jeffreyrainy Jun 18, 2021 •

edited

Loading

jeffreyrainy Jul 5, 2021 •

edited

Loading

jeffreyrainy Jul 5, 2021 •

edited

Loading

mattwalsh-unity Jul 2, 2021 •

edited

Loading

mattwalsh-unity Jul 2, 2021 •

edited

Loading