feat!: Network Time (RFC #14) #845
Conversation
# Conflicts: # com.unity.multiplayer.mlapi/Editor/NetworkManagerEditor.cs # com.unity.multiplayer.mlapi/Prototyping/NetworkAnimator.cs # com.unity.multiplayer.mlapi/Prototyping/NetworkNavMeshAgent.cs # com.unity.multiplayer.mlapi/Prototyping/NetworkTransform.cs # com.unity.multiplayer.mlapi/Runtime/Core/NetworkBehaviour.cs # com.unity.multiplayer.mlapi/Runtime/Core/NetworkManager.cs # com.unity.multiplayer.mlapi/Runtime/NetworkVariable/Collections/NetworkDictionary.cs # com.unity.multiplayer.mlapi/Runtime/NetworkVariable/Collections/NetworkList.cs # com.unity.multiplayer.mlapi/Runtime/NetworkVariable/Collections/NetworkSet.cs
# Conflicts: # com.unity.multiplayer.mlapi/Runtime/Core/SnapshotSystem.cs
| { | ||
| throw new InvalidOperationException("MultiInstanceHelper is not started"); |
There was a problem hiding this comment.
@TwoTenPvP I think you added this a while ago. I'm removing it again to allow for save TearDown in case multi instance helpers are created inside a test and not during Setup
| [Tooltip("The amount of times per second the internal event loop will run. This includes for example NetworkVariable checking.")] | ||
| public int EventTickrate = 64; | ||
| [Tooltip("The tickrate. This value controls how often MLAPI runs user code and sends out data. The value is in 'ticks per seconds' which means a value of 50 will result in 50 ticks being executed per second or a fixed delta time of 0.02.")] | ||
| public int TickRate = 30; |
There was a problem hiding this comment.
Are we confident an int will always do the job ? Let's say some game wanted 30 ms fixed tick rate, for [unspecified reasons], they'd be shooting for 33.33... TickRate. Pie-in-the-sky, there would be a way to specify either an integral tick per second or an integral tick duration in ms, but a more immediate fix is to offer the TickRate as a floating-point value.
(mostly asking, not really arguing for)
There was a problem hiding this comment.
Not confident. But floating point tickrate introduces some potential problems. DOTS Netcode also only support integral tickrate. MLAPI also only supported integral tickrate before. I will note it down and mention it to UX.
| @@ -289,7 +294,7 @@ private void OnValidate() | |||
| { | |||
| { | |||
| var childNetworkObjects = new List<NetworkObject>(); | |||
| networkPrefab.Prefab.GetComponentsInChildren(/* includeInactive = */ true, childNetworkObjects); | |||
| networkPrefab.Prefab.GetComponentsInChildren( /* includeInactive = */ true, childNetworkObjects); | |||
There was a problem hiding this comment.
very minor pet peeve: those whitespace changes can be removed before committing/pushing. It's really not an issue often, but sometimes it can be the unlucky line that conflicts with another change and causes merge issues down the road.
There was a problem hiding this comment.
always rely on ./standard.py --fix to have consistent whitespaces! :)
| } | ||
| else | ||
| { | ||
| NetworkTimeSystem = new NetworkTimeSystem(1d / NetworkConfig.TickRate, 1d / NetworkConfig.TickRate, 0.2); |
There was a problem hiding this comment.
For readability, can we have 1.0 /? Seems silly, but at first, I parsed 1d as 1 day, in the context of time. Similarly, 0.2 could be in a variable with a name, otherwise, as we glance, it's not obvious what it is. All minor stuff.
There was a problem hiding this comment.
Adjusted to use 1.0 instead of 1d for the 0.2 I expect people to use an IDE like Rider or VS which writes parameter names in front of the values.
| float maxDelta = Mathf.Max(0.001f, 0.2f * Time.unscaledDeltaTime); | ||
| m_CurrentNetworkTimeOffset += Mathf.Clamp(m_NetworkTimeOffset - m_CurrentNetworkTimeOffset, -maxDelta, maxDelta); | ||
| } | ||
| if (IsServer == false) |
There was a problem hiding this comment.
Is this the still we like ? e.g. as opposed to if (!IsServer) ?
There was a problem hiding this comment.
We don't have a guideline for this and I prefer == false because it makes it easier to quickly skim over a piece of code and understand the flow correctly. A single ! can easily missed.
com.unity.multiplayer.mlapi/Runtime/NetworkVariable/Collections/NetworkDictionary.cs
Outdated
Show resolved
Hide resolved
| @@ -55,13 +57,15 @@ public class NetworkManager : MonoBehaviour, INetworkUpdateSystem, IProfilableTr | |||
| internal static bool UseClassicDelta = true; | |||
| internal static bool UseSnapshot = false; | |||
|
|
|||
| private const int k_TimeSyncFrequency = 30; // sync every 30 ticks, TODO will be removed once timesync is done via snapshots | |||
There was a problem hiding this comment.
since tick length is configurable by users, shouldn't this const be in seconds? This way we have predictable behaviour no matter what users set?
There was a problem hiding this comment.
Fixed, syncing every second now regardless of tickrate.
| }); | ||
|
|
||
| // check how we close we are to target time. | ||
| var offsetToTarget = (timeSystem.LocalTime - timeSystem.ServerTime) - 0.1f - timeSystem.ServerBufferSec - timeSystem.LocalBufferSec; |
There was a problem hiding this comment.
nit: 0.1f magic number
There was a problem hiding this comment.
fixed by introducing named variable
|
Ok, I'm good with this. Got to writing independent multi-process tests and it seems to be doing exactly what it's supposed to. |
This PR implements RFC #14: NetworkTime.