feat: implement NetworkTickSystem based on NetworkUpdateLoop by jeffreyrainy · Pull Request #502 · Unity-Technologies/com.unity.netcode.gameobjects

jeffreyrainy · 2021-02-26T03:38:48Z

Implements the very minimal part of a tick system we need for synced networked variables.

I tried to keep it to the bare minimum and avoid any difficult design decision. This would be a first stepping stone.

Later on, we can consider what it means to pause/resume the tick system, to scale time, etc. Tests are pending, I'll get started on them.

0xFA11 · 2021-02-26T12:33:57Z

+        public static NetworkTickSystem Instance
+        {
+            get
+            {
+                if (m_Instance == null)
+                {
+                    m_Instance = new NetworkTickSystem();
+                }
+                return m_Instance;
+            }
+        }


singleton on its own vs an instance attached to a specific NetworkManager? I prefer the latter, what do you think? :)

Attached to a specific NetworkManager is cleaner.
But the singleton PR has not landed yet. So, I'd rather push this as-is, and when we have converged on the correct way to deal with singletons, we replicate the pattern for this, too? Workable?

you can still put instances under NetworkManager and access via NetworkManager.Singleton later on right?

Why not instantiate it within the NetworkUpdateLoop itself? That was my original plan, have it accessible via NetworkUpdateLoop. Really, this is not the final version for the tick system, so NetworkManager or NetworkUpdateLoop... one of the two would be nice.

NetworkUpdateLoop will always be a static class but NetworkManager is not a static class and will not be a singleton in the future so I'd suggest putting this instance under NetworkManager instead of NetworkUpdateLoop — also I don't want to repeat singleton pattern, we should be trying to avoid and remove them, not introduce them IMHO.

See how the RPCQueueContainer is instantiated

Addressed in latest update. Thanks!

0xFA11 · 2021-02-26T12:35:33Z

+        /// <summary>
+        /// Constructor
+        /// Defaults to k_DefaultTickDuration if no tick duration is specified
+        /// </summary>
+        /// <param name="tickDuration">Duration of a network tick</param>
+        private NetworkTickSystem(double tickDuration = k_DefaultTickDuration)
+        {
+            NetworkUpdateLoop.RegisterNetworkUpdate(this, NetworkUpdateStage.EarlyUpdate);
+
+            //Assure we don't specify a value less than or equal to zero for tick frequency
+            m_TickDuration = (tickDuration <= 0d) ? k_DefaultTickDuration : tickDuration;
+
+            // ticks might not start at 0, so let's update right away at construction
+            UpdateNetworkTick();
+        }


super-minor: should we move this constructor block up above Dispose()? it took me a CTRL+F to find .RegisterNetworkUpdate() and constructor :P

yeah, will move constructor and Dispose at the top. 👍

0xFA11 · 2021-02-26T12:36:57Z

+        /// </summary>
+        private void UpdateNetworkTick()
+        {
+            m_NetworkTick = (int)(Time.unscaledTime / m_TickDuration);


why (int)(float / double) instead of (int)(float / float)? m_TickDuration could be a float no?

I had tried to keep the design for the original branch and thus kept the doubles. But yeah, you raise a good point if our source data is float, no point going over-board with double. I've got in my plans to eventually review the float usage for precision. But float would give us already ~2ms resolution after 5hours game time. Good enough for now. Will change.

Let's add a comment saying we plan to move to FixedTime, etc.

0xFA11 · 2021-02-26T12:40:08Z

+{
+    public class NetworkTickSystem : INetworkUpdateSystem, IDisposable
+    {
+        private const double k_DefaultTickDuration = 0.016;


hmm, 62.5 ticks per sec. do you mind inlining 1/0.016 math here and also explain what this variable is and why 62.5 tps instead of 20 or 30 or something else for now?

The games I shipped in the past used integer number of millis to be able to do all computation on integers. So, that's why 16 milliseconds felt natural. But I don't really care since we're using floats now. Do you insist on making this 1/60f ?

you can keep 0.016 if you want but I'm asking for a comment explaining its math + why 62.5fps specifically instead of any other number.

0xFA11 · 2021-02-26T12:43:35Z

+        private double m_TickDuration;
+        private int m_NetworkTick; //How many network ticks have passed?


Suggested change

private double m_TickDuration;

private int m_NetworkTick; //How many network ticks have passed?

private float m_TickInternalMs;

private int m_NetworkTickCount;

I feel a little bit mixed here. if variable name is not good enough, we should probably change its name rather than comment next to it IMHO. I'd propose m_NetworkTickCount instead of m_NetworkTick and m_TickIntervalMs instead of m_TickDuration here. what do you think?

Yeah, will change. I was trying to stay aligned with original branch, but it's worth improving upon.

Noted. I think the long-term goal will be:
Read float unscaledtime, because that's the precision we have.
Maintain an integral number of millis for precision alignment and then surface out any time via APIs in double

Or better: accumulate FixedUpdate time as integral, quantize it during EarlyUpdate in ticks. Offer API for people who want to know the time in float/double.

…ing properly

0xFA11

🚀

NoelStephensUnity

LGMT!

jeffreyrainy requested review from 0xFA11, NoelStephensUnity, TwoTenPvP and mattwalsh-unity February 26, 2021 03:38

0xFA11 reviewed Feb 26, 2021

View reviewed changes

0xFA11 changed the title ~~feat: Feature/networktick~~ feat: NetworkTickSystem Feb 26, 2021

0xFA11 changed the title ~~feat: NetworkTickSystem~~ feat: implement NetworkTickSystem based on NetworkUpdateLoop with INetworkUpdateSystem interface Feb 26, 2021

0xFA11 changed the title ~~feat: implement NetworkTickSystem based on NetworkUpdateLoop with INetworkUpdateSystem interface~~ feat: implement NetworkTickSystem based on NetworkUpdateLoop Feb 26, 2021

jeffreyrainy added 3 commits February 26, 2021 10:39

feat: Network Tick System early prototype

a7159f4

feat: Registering the tick system with the update loop system, dispos…

7aad8c6

…ing properly

feat: Network Tick System PR comments

a983537

jeffreyrainy force-pushed the feature/networktick branch from c8f7c23 to a983537 Compare February 26, 2021 15:40

0xFA11 reviewed Feb 26, 2021

View reviewed changes

Comment thread com.unity.multiplayer.mlapi/Runtime/Core/NetworkTickSystem.cs Outdated

jeffreyrainy force-pushed the feature/networktick branch from faa53b6 to a983537 Compare February 26, 2021 16:04

jeffreyrainy added 3 commits February 26, 2021 11:11

fix: PR comments on comments

86b543d

fix: last comments on PR

e78da7d

fix: no singletons, extra doc

3632b94

0xFA11 approved these changes Feb 26, 2021

View reviewed changes

mattwalsh-unity approved these changes Feb 26, 2021

View reviewed changes

NoelStephensUnity approved these changes Feb 26, 2021

View reviewed changes

jeffreyrainy merged commit 45ec6bf into develop Feb 26, 2021

jeffreyrainy deleted the feature/networktick branch February 26, 2021 18:26

0xFA11 mentioned this pull request Mar 6, 2021

MTTDOC-95 Draft release notes Unity-Technologies/com.unity.multiplayer.docs#46

Merged

3 tasks

		private double m_TickDuration;
		private int m_NetworkTick; //How many network ticks have passed?

Conversation

jeffreyrainy commented Feb 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!

jeffreyrainy Feb 26, 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!

jeffreyrainy Feb 26, 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!

Uh oh!

0xFA11 left a comment

Choose a reason for hiding this comment

Uh oh!

NoelStephensUnity left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jeffreyrainy Feb 26, 2021 •

edited

Loading

jeffreyrainy Feb 26, 2021 •

edited

Loading