feat: snapshot. Fully integrated despawn, mtt-1092, mtt-1056 #1062

jeffreyrainy · 2021-08-17T23:55:56Z

This completes the despawn with snapshot feature, leaving it disable by default.
Also makes all the API internal as opposed to public.
Fixes issues with despawning.

The SnapshotSystem.cs file is still prototypical, but the other files are fully up for review.

…ssage queue

…hot-system-spawn3

…bles

… variables

…Technologies/com.unity.multiplayer.mlapi into experimental/snapshot-prep2-spawn

…apshot-system-spawn3

…hot-system-spawn3

…receive

…hot-system-spawn3

…function. This will allow reusing it from NetworkShow. But as-is, no impact

…<NetworkObject>) instead of calling a deeper-nested function. Make the code more uniform and prepares for incoming snapshot changes

…hot-prep3-spawn

…n' into experimental/snapshot-system-spawn3

…ide(List<NetworkObject>). Same reasons as for NetworkShow

…n' into experimental/snapshot-system-spawn3

NoelStephensUnity

Looks good to me.
:)

LukeStampfli · 2021-08-19T09:06:37Z

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

+        {
+            SnapshotDespawnCommand command;
+            command.NetworkObjectId = NetworkObjectId;
+            command.TickWritten = default; // will be reset in Despawn


Not sure if I understand this comment. Will this value later be set correctly by the SnapshotSystem or?

Yes, tickWritten is meant to be set by SnapshotSystem, not by the calling code. But for some obscure C# design reason, apparently that struct can't have either default constructor nor static initializer.

I'll change the comment to // will be set internally by SnapshotSystem as the comment is from before it was moved.

ok, so when we call this function we know we want a Despawn command but we don't know yet what tick it is associated with because we haven't written it yet and are not sure which tick it will be written on?

I guess I always thought the associated tick would be the server tick on which it occurred not when it was written, which I think we would know at this point.

The way it is handled, "on which it occurred" is "when it was written". I just didn't want NetworkObject to do it because NetworkObject doesn't need to care about time.

internal void Spawn(SnapshotSpawnCommand command) { command.TickWritten = (ushort)m_CurrentTick;

this is called when the spawn/despawn arrives in Snapshot. Not when the command is sent. So, yeah, the original comment was very misleading.

// will be set internally by SnapshotSystem is the new comment. Please let me know if it is still unclear

LukeStampfli · 2021-08-19T09:08:48Z

com.unity.netcode.gameobjects/Tests/Editor/SnapshotRttTests.cs

@@ -9,7 +9,7 @@ public class SnapshotRttTests
        [Test]
        public void TestBasicRtt()
        {
-            var snapshot = new SnapshotSystem();
+            var snapshot = new SnapshotSystem(NetworkManager.Singleton);


So this is an editor test. And I don't see us creating a NetworkManager in this test or in SetUp/Teardown. So where does this NetworkManger.Singleton come from. Pretty sure it is null. But then why is this test not failing?

nice catch. As it turns out snapshot is only used to snapshot.GetConnectionRtt. It doesn't run or anything. The rest is really unit testing the ConnectionRtt. As such, it doesn't care to have a null NetworkManager.

I think it would be clearer if we did:

var snapshot = new SnapshotSystem(default);

And then, it would still compile when the Singleton gets removed in the future.

LukeStampfli · 2021-08-19T09:12:29Z

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

+                if (count > 20)
+                {
+                    // timeout waiting for object to reach the expect visibility
+                    Debug.Assert(false);


Suggested change

Debug.Assert(false);

Assert.Fail("timeout waiting for object to reach the expect visibility");

I think we should use NUnit asserts here and not unity logger.

LukeStampfli · 2021-08-19T09:16:22Z

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

@@ -10,21 +10,31 @@ namespace Unity.Netcode
    // Might include tick in a future milestone, to address past variable value


I can understand that spawn/despawn have to be part of the SnapshotSystem. (even though I'd prefer them to be separate 😃). But could we consider maybe moving them into a partial class for better code separation and visibility. (What I mean is make SnapshotSystem partial then create SnapshotSystem.Spawning.cs and add a partial SnapshotSystem class to it and move all spawning functions over.

MTT-1099
:-) that's the best I can offer at the moment. Acknowledging it, and remembering to come back to it.

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

LukeStampfli · 2021-08-19T09:22:39Z

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

@@ -437,7 +555,7 @@ private INetworkVariable FindNetworkVar(VariableKey key)

    internal class ClientData
    {
-        internal struct SentSpawn
+        internal struct SentSpawn // also despawns


I don't understand what this code comment means.

SentSpawnOrDespawn would be the proper class name. The comment is a bit terse. Maybe I could comment // this struct also stores Despawns, not just Spawns

LukeStampfli · 2021-08-19T09:25:45Z

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

+        {
+            if (NumSpawns >= m_MaxSpawns)
+            {
+                Array.Resize(ref Spawns, 2 * m_MaxSpawns);


Why are we not just using a list for this? Looks like we keep track of size and resize which is exactly what a list is for?

That might be me coming from C++. Are C# list contiguous in memory ? If so, do they grow in an amortized linear fashion ? If both are yes, then sure, List would work then, too.
What I wanted to avoid was triggering memory alloc at every AddSpawn. I'll learn on C# Lists first, then we can decide.

Might not be too early to use NativeList

I'm willing to add a bug/task to review it and then prioritize, but I'd rather have this go in as a first step, to keep things moving.

Lists in c# are contiguous in memory. They grow by doubling the size of their underlying buffer (and memory copy the old content). Same goes for NativeList.

👍 thanks, I really got confused by their naming.
MTT-1101 will address this.

LukeStampfli · 2021-08-19T09:26:53Z

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

+            s.SequenceNumber = clientData.SequenceNumber;
+            clientData.SentSpawns.Add(s);
+
+            writer.WriteUInt64(despawn.NetworkObjectId);


Suggested change

writer.WriteUInt64(despawn.NetworkObjectId);

writer.WriteUInt64Packed(despawn.NetworkObjectId);

LukeStampfli · 2021-08-19T09:27:03Z

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

+        {
+            SnapshotDespawnCommand command;
+
+            command.NetworkObjectId = reader.ReadUInt64();


Suggested change

command.NetworkObjectId = reader.ReadUInt64();

command.NetworkObjectId = reader.ReadUInt64Packed();

LukeStampfli · 2021-08-19T09:28:10Z

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

+            clientData.SentSpawns.Add(s);
+
+            writer.WriteUInt64(despawn.NetworkObjectId);
+            writer.WriteUInt16(despawn.TickWritten);


Ticks are now ulongs in the new tick system. I think we might need to adjust this here?

LukeStampfli · 2021-08-19T09:29:08Z

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

+            {
+                if (command.TargetClientIds == default)
+                {
+                    command.TargetClientIds = GetClientList();


This allocates each spawn/despawn. Don't think we can fix this here quickly. Just pointing out that we are again adding a significant amount of allocations.

Yes. This is unfortunate, but hopefully bearable for "small scale coop". The plan is to replace this with a bit mask, but I'm not seeing this making it in time for the next release.

LukeStampfli

The overall logic itself looks functional to me and should allow us to do spawns/despawns over snapshots. Added a few suggestions to improve code quality and clarity.

mattwalsh-unity · 2021-08-19T14:52:12Z

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

@@ -234,6 +234,11 @@ public void NetworkShow(ulong clientId)
                throw new VisibilityChangeException("The object is already visible");
            }

+            if (NetworkManager.NetworkConfig.UseSnapshotSpawn)


Interesting, when I look at this code I'm expecting to see an 'else' where the old way of spawning happens. So SnapshotSpawn doesn't include a call to SendSpawnCallForObject? Could it?

I've put in Slack a link to the lucidchart diagram about this "[Thursday PR]". NetworkObject.Show calls directly into Snapshot. It then calls into SendSpawnCallForObject in SpawnManager. The old way, in SpawnManager is then disabled, there. That's where the missing else is

That design came around because the other flow, SpawnInternal, can be put in Snapshot at once (one command for all connections). If I had put the Snapshot hook in SendSpawnCallForObject then SnapshotSystem would get the same spawn multiple times. Similarly if I had refactored the old ways to be done for all connections at once, I would have touched code I didn't need to and risk breaking things.

mattwalsh-unity · 2021-08-19T14:59:06Z

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

+            command.ObjectPosition = transform.position;
+            command.ObjectRotation = transform.rotation;
+            command.ObjectScale = transform.localScale;
+            command.TickWritten = default; // will be reset in Spawn


Same as above, I thought we record tick it happened not tick it was written out

we do record the tick it was written, just deeper in the stack:

SnapshotSystem.Spawn() at com.unity.netcode.gameobjects/Runtime/Core/SnapshotSystem.cs:line 849 NetworkObject.SnapshotSpawn() at com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs:line 451 NetworkObject.SpawnInternal() at com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs:line 493

internal void Spawn(SnapshotSpawnCommand command) { command.TickWritten = m_CurrentTick;

mattwalsh-unity · 2021-08-19T15:03:59Z

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

+            {
+                Array.Resize(ref Spawns, 2 * m_MaxSpawns);
+                m_MaxSpawns = m_MaxSpawns * 2;
+                Debug.Log($"[JEFF] spawn size is now {m_MaxSpawns}");


[MATT] Did you mean to leave this in? :)

Actually, yes. But if it is an issue, I'm willing to remove it. The release will not be cut just yet, and even if it were, this is not used, by default. Even it it were, this log would only occur every time the buffer doubles, so, at most 4-5 times ?

Otherwise, with the number of merges we do, I found I was wasting tons of time adding/removing these. Your call.

mattwalsh-unity · 2021-08-19T15:15:49Z

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

+            {
+                Array.Resize(ref Despawns, 2 * m_MaxDespawns);
+                m_MaxDespawns = m_MaxDespawns * 2;
+                Debug.Log($"[JEFF] despawn size is now {m_MaxDespawns}");


Comment leftover?

Same as above.

…hot-mtt1092-mtt1056

…rged to develop

…ndation supports it. Only merge after #1062 (#1064)

…nsform * develop: (21 commits) test: adding more details to multiprocess readme (#1050) refactor!: convert NetworkTransform.NetworkState to `struct` (#1061) fix: networkmanager destroy on app quit (#1011) feat: snapshot, using unreliable packets, now that the underlying foundation supports it. Only merge after #1062 (#1064) feat: snapshot. Fully integrated despawn, mtt-1092, mtt-1056 (#1062) fix: eliminate bad use-after-free(destroy) pattern (#1068) chore: cleanup meta files for empty dirs (#1067) chore: minor MLAPI to Netcode rename (#1065) feat: report network behaviour name to the profiler (#1058) fix: player movement (#1063) test: Add unit tests for NetworkTime properties (#1053) chore: remove authority & netvar perms from NetworkTransform (#1059) feat: networktransform pos/rot/sca thresholds on state sync (#1055) feat: expose network behaviour type name internally (#1057) chore: remove all the old profiling code (#1048) fix: if-guard `NetworkManager.__rpc_name_table` access (#1056) fix: Disabling fixedupdate portion of SpawnRpcDespawn test because it's failing for known reasons that will be fixed in the IMessage refactor. (#1049) feat: Implement metrics for the new network profiler (#960) chore!: change package name & asmdefs (#1026) feat: per axis networktransform state sync (+bitwise state comp) (#1042) ... # Conflicts: # com.unity.netcode.gameobjects/Prototyping/NetworkTransform.cs

…echnologies#1062) * feat: snapshot. Attempt at applying snapshot spawn branch over new message queue * feat: snapshot. Reusing spawn entries. * feat: snapshot fix, prefab handling * feat: snapshot. reduce log spam * snapshot: merge preparation. Removing old acks, removing unused variables * feat: snapshot. merge preparation. Removing old acks, removing unused variables * feat: snapshot. more reasonable sentinel checks around snapshot send/receive * feat: snapshot. small clenaup * feat: snapshot. Moving configuration items to NetworkConfig * feat: snapshot. Adjusting to code standard * refactor: moved code from SpawnInternal into separate SendToSnapshot function. This will allow reusing it from NetworkShow. But as-is, no impact * refactor: calling networkShow(NetworkObject) code in networkshow(List<NetworkObject>) instead of calling a deeper-nested function. Make the code more uniform and prepares for incoming snapshot changes * style: whitespace * refactor: also using NetworkHide(NetworkObject) to implement NetworkHide(List<NetworkObject>). Same reasons as for NetworkShow * feat: snapshot. Safer access to Connection RTT structures * feat: snapshot. placeholder comment for code to come later * feat: snapshot. Snapshot fully working for spawn test. Keeps reference to proper network manager. Allows sending spawn to individual instances * feat: snapshot. despawn going via snapshot. snapshot message size limitation. snapshot message packing in a LRU fashion * feat: snapshot. Incrementing the local span and despawn store (amortized linear) when game code demands too much spawn/despawns * feat: snapshot. Using different list to keep track of spawn despawn times. Probably not needed, might be revert in the future, but eases debugging. Also debug prints, obviously temporary * feat: snapshot. extra logging for debugging * fix: snapshot. MTT-1056 despawn loss * feat: snapshot. Proper test for length used in snapshot message. Removed redundant condition for target client list * fix: snapshot. NetworkHide in snapshot mode was incorrectly hiding the object on all machines * feat: snapshot. Keeping snapshot disabled for this release * style: coding standards fix * test: making the networkshow/hide test more resistant to timing difference on the test machine, waiting half a second instead of a tenth. * test: removing Sleep() code in NetworkShowHide that attempted to deal with timing. Used explicit timeout instead. Less flaky * feat: snapshot. First pass at PR code review * feat: snapshot. Adjusting to int ticks * feat: snapshot. Reducing the amount of logging, as this is getting merged to develop Co-authored-by: M. Fatih MAR <mfatihmar@gmail.com>

…ndation supports it. Only merge after Unity-Technologies#1062 (Unity-Technologies#1064)

jeffreyrainy and others added 30 commits July 27, 2021 13:31

feat: snapshot. Attempt at applying snapshot spawn branch over new me…

94313c7

…ssage queue

feat: snapshot. Reusing spawn entries.

daa7477

Merge branch 'develop' into experimental/snapshot-system-spawn3

9cb59e4

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

62e87d5

…hot-system-spawn3

feat: snapshot fix, prefab handling

64bb7ec

feat: snapshot. reduce log spam

847adcd

Merge branch 'develop' into experimental/snapshot-system-spawn3

bc49244

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

4963d9c

…hot-system-spawn3

snapshot: merge preparation. Removing old acks, removing unused varia…

10ed439

…bles

feat: snapshot. merge preparation. Removing old acks, removing unused…

9674acc

… variables

Merge branch 'experimental/snapshot-prep2-spawn' of github.com:Unity-…

bc4805d

…Technologies/com.unity.multiplayer.mlapi into experimental/snapshot-prep2-spawn

Merge branch 'develop' into experimental/snapshot-prep2-spawn

5c9a703

Merge branch 'experimental/snapshot-prep2-spawn' into experimental/sn…

411ef16

…apshot-system-spawn3

Merge branch 'experimental/snapshot-prep2-spawn' into experimental/sn…

8201cd2

…apshot-system-spawn3

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

7a597b3

…hot-system-spawn3

feat: snapshot. more reasonable sentinel checks around snapshot send/…

d2318bc

…receive

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

9f81229

…hot-system-spawn3

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

d0b6617

…hot-system-spawn3

feat: snapshot. small clenaup

43c26f6

feat: snapshot. Moving configuration items to NetworkConfig

4e32df9

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

4222365

…hot-system-spawn3

feat: snapshot. Adjusting to code standard

2bf493d

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

16d3db1

…hot-system-spawn3

refactor: moved code from SpawnInternal into separate SendToSnapshot …

d421c64

…function. This will allow reusing it from NetworkShow. But as-is, no impact

refactor: calling networkShow(NetworkObject) code in networkshow(List…

d891f1e

…<NetworkObject>) instead of calling a deeper-nested function. Make the code more uniform and prepares for incoming snapshot changes

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

7c55e4e

…hot-prep3-spawn

style: whitespace

140a9e7

Merge remote-tracking branch 'origin/experimental/snapshot-prep3-spaw…

775eb83

…n' into experimental/snapshot-system-spawn3

refactor: also using NetworkHide(NetworkObject) to implement NetworkH…

8494c4d

…ide(List<NetworkObject>). Same reasons as for NetworkShow

Merge remote-tracking branch 'origin/experimental/snapshot-prep3-spaw…

767d9eb

…n' into experimental/snapshot-system-spawn3

NoelStephensUnity approved these changes Aug 18, 2021

View reviewed changes

LukeStampfli reviewed Aug 19, 2021

View reviewed changes

com.unity.netcode.gameobjects/Runtime/Core/SnapshotSystem.cs Show resolved Hide resolved

LukeStampfli reviewed Aug 19, 2021

View reviewed changes

com.unity.netcode.gameobjects/Runtime/Core/SnapshotSystem.cs Show resolved Hide resolved

LukeStampfli reviewed Aug 19, 2021

View reviewed changes

mattwalsh-unity reviewed Aug 19, 2021

View reviewed changes

jeffreyrainy added 4 commits August 19, 2021 12:13

feat: snapshot. First pass at PR code review

cf6b615

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

4c48ced

…hot-mtt1092-mtt1056

feat: snapshot. Adjusting to int ticks

5015050

feat: snapshot. Reducing the amount of logging, as this is getting me…

dc9dfbc

…rged to develop

jeffreyrainy enabled auto-merge (squash) August 19, 2021 18:58

LukeStampfli approved these changes Aug 19, 2021

View reviewed changes

jeffreyrainy merged commit 428d6e4 into develop Aug 19, 2021

jeffreyrainy deleted the experimental/snapshot-mtt1092-mtt1056 branch August 19, 2021 19:35

jeffreyrainy added a commit that referenced this pull request Aug 19, 2021

feat: snapshot, using unreliable packets, now that the underlying fou…

46b5552

…ndation supports it. Only merge after #1062 (#1064)

mollstam pushed a commit to Keepsake-Games/com.unity.netcode.gameobjects that referenced this pull request Feb 13, 2023

feat: snapshot, using unreliable packets, now that the underlying fou…

1f68978

…ndation supports it. Only merge after Unity-Technologies#1062 (Unity-Technologies#1064)

	Debug.Assert(false);
	Assert.Fail("timeout waiting for object to reach the expect visibility");

		@@ -10,21 +10,31 @@ namespace Unity.Netcode
		// Might include tick in a future milestone, to address past variable value

	writer.WriteUInt64(despawn.NetworkObjectId);
	writer.WriteUInt64Packed(despawn.NetworkObjectId);

	command.NetworkObjectId = reader.ReadUInt64();
	command.NetworkObjectId = reader.ReadUInt64Packed();

feat: snapshot. Fully integrated despawn, mtt-1092, mtt-1056 #1062

feat: snapshot. Fully integrated despawn, mtt-1092, mtt-1056 #1062

Uh oh!

Conversation

jeffreyrainy commented Aug 17, 2021

Uh oh!

NoelStephensUnity left a comment

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeffreyrainy Aug 19, 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!

LukeStampfli left a comment

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!

Uh oh!

jeffreyrainy Aug 19, 2021 •

edited

Loading