refactor!: remove network variable settings, network behaviour cleanup#1097
refactor!: remove network variable settings, network behaviour cleanup#1097mattwalsh-unity merged 10 commits intodevelopfrom
Conversation
| } | ||
|
|
||
| instance.NetworkBehaviour = this; | ||
| instance.Initialize(this); |
There was a problem hiding this comment.
Don't want NetworkBehaviour to be exposed, want tighter control over this
| } | ||
|
|
||
| internal static void HandleNetworkVariableDeltas(List<NetworkVariableBase> networkVariableList, Stream stream, ulong clientId, NetworkBehaviour logInstance, NetworkManager networkManager) | ||
| internal void HandleNetworkVariableDeltas(Stream stream, ulong clientId, NetworkBehaviour logInstance) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Yes, this appears to have been jammed in for logging messages so I can clean up further
| } | ||
|
|
||
| internal static void WriteNetworkVariableData(List<NetworkVariableBase> networkVariableList, Stream stream, ulong clientId, NetworkManager networkManager) | ||
| internal void WriteNetworkVariableData(Stream stream, ulong clientId) |
There was a problem hiding this comment.
Same story here, no good reason to make this static
| } | ||
|
|
||
| internal static void SetNetworkVariableData(List<NetworkVariableBase> networkVariableList, Stream stream, NetworkManager networkManager) | ||
| internal void SetNetworkVariableData(Stream stream) |
There was a problem hiding this comment.
Same story here, cleaner as instance method
| else | ||
| { | ||
| NetworkBehaviour.HandleNetworkVariableDeltas(instance.NetworkVariableFields, stream, clientId, instance, NetworkManager); | ||
| instance.HandleNetworkVariableDeltas(stream, clientId, instance); |
There was a problem hiding this comment.
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?
| m_DirtyEvents.Add(dictionaryEvent); | ||
| } | ||
|
|
||
| m_DirtyEvents.Add(dictionaryEvent); |
There was a problem hiding this comment.
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
| /// <summary> | ||
| /// The temporary accessor to enable struct element access until [MTT-1020] complete | ||
| /// </summary> | ||
| public ref T ValueRef |
There was a problem hiding this comment.
Forgot to remove this in my prev PR
| // demolish me | ||
| // or better setter? | ||
| internal protected NetworkBehaviour NetworkBehaviour { get; internal set; } | ||
| private protected void EnsureInitialized() |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Again I think we can nuke this from @MFatihMAR 's observation
| // 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; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Matt will reconsider if we want to be this daring to move this up here
| } | ||
| } | ||
|
|
||
| private void EnsureInitialized() |
There was a problem hiding this comment.
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
No description provided.