refactor!: remove network variable settings, network behaviour cleanup #1097

mattwalsh-unity · 2021-08-26T18:52:19Z

No description provided.

mattwalsh-unity · 2021-08-26T18:53:05Z

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

@@ -425,7 +425,7 @@ internal void InitializeVariables()
                        sortedFields[i].SetValue(this, instance);
                    }

-                    instance.NetworkBehaviour = this;
+                    instance.Initialize(this);


Don't want NetworkBehaviour to be exposed, want tighter control over this

mattwalsh-unity · 2021-08-26T18:54:15Z

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

@@ -624,15 +624,15 @@ private bool CouldHaveDirtyNetworkVariables()
            return false;
        }

-        internal static void HandleNetworkVariableDeltas(List<NetworkVariableBase> networkVariableList, Stream stream, ulong clientId, NetworkBehaviour logInstance, NetworkManager networkManager)
+        internal void HandleNetworkVariableDeltas(Stream stream, ulong clientId, NetworkBehaviour logInstance)


Besides just generally making more sense as an instance method, if and when we remove NetworkBehaviour dep from NetworkVariable, this will make it much easier for reasons that were clear to me as I experimented doing so

Yes, this appears to have been jammed in for logging messages so I can clean up further

mattwalsh-unity · 2021-08-26T18:54:40Z

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

@@ -717,20 +717,20 @@ internal static void HandleNetworkVariableDeltas(List<NetworkVariableBase> netwo
            }
        }

-        internal static void WriteNetworkVariableData(List<NetworkVariableBase> networkVariableList, Stream stream, ulong clientId, NetworkManager networkManager)
+        internal void WriteNetworkVariableData(Stream stream, ulong clientId)


Same story here, no good reason to make this static

mattwalsh-unity · 2021-08-26T18:55:04Z

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

                            writer.WritePadBits();
                        }
                    }
                }
            }
        }

-        internal static void SetNetworkVariableData(List<NetworkVariableBase> networkVariableList, Stream stream, NetworkManager networkManager)
+        internal void SetNetworkVariableData(Stream stream)


Same story here, cleaner as instance method

mattwalsh-unity · 2021-08-26T18:56:41Z

com.unity.netcode.gameobjects/Runtime/Messaging/InternalMessageHandler.cs

@@ -222,7 +222,7 @@ public void HandleNetworkVariableDelta(ulong clientId, Stream stream)
                    }
                    else
                    {
-                        NetworkBehaviour.HandleNetworkVariableDeltas(instance.NetworkVariableFields, stream, clientId, instance, NetworkManager);
+                        instance.HandleNetworkVariableDeltas(stream, clientId, instance);


It is unclear to me if this second 'instance' parameter (which is the log instance) makes sense. We only call this HandleNetworkVariableDeltas method once, which is here. Do we sometimes want a different instance for this log behaviour than this?

mattwalsh-unity · 2021-08-26T18:58:36Z

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

-                 m_DirtyEvents.Add(dictionaryEvent);
-             }
-
+             m_DirtyEvents.Add(dictionaryEvent);


It just doesn't seem to be the right abstraction that the dictionary should know how many clients are connected. I would prefer the decision to be made 'upstream' even if it means a slight performance cost when no clients are connected, which seems like a performance-not-important moment anyway

mattwalsh-unity · 2021-08-26T18:59:11Z

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

-        /// <summary>
-        /// The temporary accessor to enable struct element access until [MTT-1020] complete
-        /// </summary>
-        public ref T ValueRef


Forgot to remove this in my prev PR

mattwalsh-unity · 2021-08-26T19:00:29Z

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

-        // demolish me
-        // or better setter?
-        internal protected NetworkBehaviour NetworkBehaviour { get; internal set; }
+        private protected void EnsureInitialized()


Avoids being replicated in all the container classes. I suppose we could have a new subclass just for containers and put this in it, but didn't seem worth it

Again I think we can nuke this from @MFatihMAR 's observation

mattwalsh-unity · 2021-08-26T19:03:55Z

com.unity.netcode.gameobjects/Runtime/Spawning/NetworkSpawnManager.cs

@@ -291,6 +291,12 @@ internal NetworkObject CreateLocalNetworkObject(bool isSceneObject, uint globalO
        // Ran on both server and client
        internal void SpawnNetworkObjectLocally(NetworkObject networkObject, ulong networkId, bool sceneObject, bool playerObject, ulong? ownerClientId, Stream dataStream, bool readNetworkVariable, bool destroyWithScene)
        {
+            networkObject.IsSceneObject = sceneObject;


This initialization should happen up here before we call SetNetworkVariableData. Technically, yes, since we still have a NetworkBehaviour ref in each NetworkVariable the old code works because we don't access the NB until after these initializations. However it's not good practice, and if and when we break the NB dependency this initialization has to happen up here

Matt will reconsider if we want to be this daring to move this up here

mattwalsh-unity · 2021-08-26T19:26:36Z

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

@@ -475,14 +465,6 @@ public int LastModifiedTick
                return NetworkTickSystem.NoTick;
            }
        }
-
-        private void EnsureInitialized()


We think we can just nuke this

…nsform * develop: (26 commits) fix: client connected InvokeOnClientConnectedCallback with scene management disabled (#1123) fix: removed `public` class `NetcodeObserver` (MTT-1157) (#1122) feat: add NetworkMessageSent/Received metrics (#1112) feat: snapshot. MTU sizing option for Snapshot. MTT-1087 (#1111) Add metrics for transport bytes sent and received (#1104) fix: Missing end profiling sample (#1118) chore: support standalone mode for netcode runtimetests (#1115) feat: Change MetricNames for a more complex value type (#1109) feat: Track scene event metrics (#1089) style: whitespace fixes (#1117) feat: replace scene registration with scenes in build list (#1080) fix: mtt-857 GitHub issue 915 (#1099) fix: NetworkSceneManager exception when DontDestroyOnLoad NetworkObjects are being synchronized (#1090) feat: NetworkTransform Custom Editor Inspector UI (#1101) refactor: remove TempGlobalObjectIdHashOverride (#1105) fix: MTT-1124 Counters are now reported in sync with other metrics (#1096) refactor: convert using var statements to using var declarations (#1100) chore: updated all of the namespaces to match the tools package change (#1095) refactor!: remove network variable settings, network behaviour cleanup (#1097) fix: mtt-1088 review. Safer handling of out-of-order or old messages (#1091) ... # Conflicts: # com.unity.netcode.gameobjects/Prototyping/NetworkTransform.cs

Unity-Technologies#1097) * refactor!: remove network variable settings, network behaviour cleanup * remove ensure initialized * moved init, removed unused var * refactor!: remove network variable settings, network behaviour cleanup * remove ensure initialized * whitespace / include fixes

refactor!: remove network variable settings, network behaviour cleanup

4e3e196

mattwalsh-unity requested a review from 0xFA11 August 26, 2021 18:52

mattwalsh-unity commented Aug 26, 2021

View reviewed changes

mattwalsh-unity added 2 commits August 26, 2021 12:49

remove ensure initialized

abfcfc0

moved init, removed unused var

a049539

0xFA11 approved these changes Aug 26, 2021

View reviewed changes

mattwalsh-unity and others added 4 commits August 26, 2021 13:45

Merge branch 'develop' into feature/remove_netvar_settings

c1ed83b

refactor!: remove network variable settings, network behaviour cleanup

705ec49

remove ensure initialized

764ae72

moved init, removed unused var

0e84dd3

mattwalsh-unity enabled auto-merge (squash) August 26, 2021 20:47

mattwalsh-unity added 2 commits August 26, 2021 14:31

whitespace / include fixes

08f38f6

merge fix

a14a89b

mattwalsh-unity disabled auto-merge August 26, 2021 21:58

mattwalsh-unity enabled auto-merge (squash) August 26, 2021 21:58

remove unnecessary requires

f0eb64c

mattwalsh-unity merged commit 15d5bef into develop Aug 26, 2021

mattwalsh-unity deleted the feature/remove_netvar_settings branch August 26, 2021 22:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor!: remove network variable settings, network behaviour cleanup #1097

refactor!: remove network variable settings, network behaviour cleanup #1097

Uh oh!

mattwalsh-unity commented Aug 26, 2021

Uh oh!

mattwalsh-unity Aug 26, 2021

Uh oh!

mattwalsh-unity Aug 26, 2021

Uh oh!

mattwalsh-unity Aug 26, 2021

Uh oh!

mattwalsh-unity Aug 26, 2021

Uh oh!

mattwalsh-unity Aug 26, 2021

Uh oh!

mattwalsh-unity Aug 26, 2021

Uh oh!

mattwalsh-unity Aug 26, 2021

Uh oh!

mattwalsh-unity Aug 26, 2021

Uh oh!

mattwalsh-unity Aug 26, 2021

Uh oh!

mattwalsh-unity Aug 26, 2021

Uh oh!

mattwalsh-unity Aug 26, 2021

Uh oh!

mattwalsh-unity Aug 26, 2021

Uh oh!

mattwalsh-unity Aug 26, 2021

Uh oh!

Uh oh!

refactor!: remove network variable settings, network behaviour cleanup #1097

refactor!: remove network variable settings, network behaviour cleanup #1097

Uh oh!

Conversation

mattwalsh-unity commented Aug 26, 2021

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!

Uh oh!