fix: unregister NetworkPlayerLoop systems from PlayerLoop by 0xFA11 · Pull Request #588 · Unity-Technologies/com.unity.netcode.gameobjects

0xFA11 · 2021-03-11T01:34:41Z

implement UnregisterLoopSystems() to unregister systems injected into the PlayerLoop by the NetworkUpdateLoop
implement a register-then-unregister test to verify NetworkUpdateLoop does NOT cause any side-effects on PlayerLoop
refactor for performance (use array ops instead of lists), for better namings (inject → register) and styling (extract duplicate code into helper methods: TryAddLoopSystem & TryRemoveLoopSystem)

… the Editor

NoelStephensUnity

LGTM!
I like how you simplified!

mattwalsh-unity · 2021-03-11T03:56:45Z

Can you add a description of how a tester would test this? I realize it would have to be manual, but can we say something like "hit play on a project, wait for it to get into a certain state, then stop and observe the console to have / not have _____"?

JesseOlmer

Help me understand what's going on here. Why do you say NetworkUpdateLoop will still tick systems after we stop? NUL doesn't do the ticking - it just adds systems to the PlayerLoop before Awake... why are our systems still running but systems like behaviourupdate, audio, input etc. aren't having issues?

JesseOlmer · 2021-03-11T04:42:44Z

com.unity.multiplayer.mlapi/Runtime/Core/NetworkUpdateLoop.cs

+            {
+                if (stateChange == PlayModeStateChange.ExitingPlayMode)
+                {
+                    PlayerLoop.SetPlayerLoop(PlayerLoop.GetDefaultPlayerLoop());


This doesn't feel right. We start with CurrentPlayerLoop, not DefaultPlayerLoop, so feels odd overwriting any other customizations with the default loop.

Why do you say NetworkUpdateLoop will still tick systems after we stop?

DomainReload happens when we enter PlayMode and that'd cause NUL to be injected into PlayerLoop.
However, when we exit PlayMode (stop playing), nothing reloads so that PlayerLoop still executes its systems and NetworkUpdateLoop systems are still being ticked therefore NUL still ticks INetworkUpdateSystem instances underneath.
As I mentioned in the PR description, the proper handling of this scenario is to unregister from NUL on OnApplicationQuit, OnDestroy, OnDisable but sometimes that's not the case, people might forget and/or somehow unregister codepath might not be executed.

If this is supposed to be a fix for Noel's #558 / MTT-529 can you please note that in the PR description or title somewhere.

No, we're just playing safe here and revert PlayerLoop changes back when we exit PlayMode in the Editor.
This is not a fix for #558 — this is just hardening.

why are our systems still running but systems like behaviourupdate, audio, input etc. aren't having issues?

I can't paste codes here for obvious reasons :) but I can say that they're being protected if not playing. That's not a PlayerLoop logic, that's systems' own logic to execute or skip. PlayerLoop is always around and being ticked, it never stops. Systems under PlayerLoop might pause/stop/start or do whatever they want. They're being registered into low-level engine ticking loop, in this case NetworkUpdateLoop is still being ticked by the PlayerLoop — that's completely normal.

This doesn't feel right. We start with CurrentPlayerLoop, not DefaultPlayerLoop, so feels odd overwriting any other customizations with the default loop.

We only do that when we stop playing in the Editor (exit PlayMode), not when we enter PlayMode.
(We start with the default playerloop after domainreload anyway, systems relying on playerloop injection should already re-register after domainreload).

JesseOlmer · 2021-03-11T04:48:03Z

If this is supposed to be a fix for Noel's #558 / MTT-529 can you please note that in the PR description or title somewhere.

… exiting PlayMode in the Editor

0xFA11 · 2021-03-11T16:09:26Z

com.unity.multiplayer.mlapi/Runtime/Core/NetworkUpdateLoop.cs

        [RuntimeInitializeOnLoadMethod]
        private static void Initialize()
        {
+#if UNITY_EDITOR
+            EditorApplication.playModeStateChanged += stateChange =>
+            {
+                switch (stateChange)
+                {
+                    case PlayModeStateChange.EnteredPlayMode:
+                        InjectSystems();
+                        break;
+                    case PlayModeStateChange.ExitingPlayMode:
+                        UninjectSystems();
+                        break;
+                }
+            };
+#else
+            InjectSystems();
+#endif
+        }


hmm, I think we should have a good comment block explaining what's going on here :P
will put that in too!

NoelStephensUnity · 2021-03-11T17:16:11Z

com.unity.multiplayer.mlapi/Runtime/Core/NetworkUpdateLoop.cs

        }
+
+#if UNITY_EDITOR
+        private static void UninjectSystems()


We might want to comment on what this is doing and why this is important?
(or at least replicate the comments in Initialize)? (maybe)

I did comment in the Initialize() method above, do you think that's not enough?

JesseOlmer

Today seems like a PERFECT day for a couple of unit tests :)

JesseOlmer · 2021-03-11T19:05:32Z

com.unity.multiplayer.mlapi/Runtime/Core/NetworkUpdateLoop.cs

+            {
+                var playerLoopSystem = customPlayerLoop.subSystemList[i];
+
+                if (playerLoopSystem.type == typeof(Initialization))


nit: there is a ton of copy/paste and weird { scoping here. Could just extract method that takes a type for the system and a type for the network subsystem :)

sure, I'll try to make it look fancier :)

…tems()

com.unity.multiplayer.mlapi/Tests/Runtime/NetworkUpdateLoopTests.cs

… namings

wackoisgod · 2021-03-14T01:07:59Z

com.unity.multiplayer.mlapi/Runtime/Core/NetworkUpdateLoop.cs

+            // + childSystem
+            newSubsystemList[systemPosition] = childLoopSystem;
+            // + systemsAfter
+            Array.Copy(parentLoopSystem.subSystemList, systemPosition, newSubsystemList, systemPosition + 1, parentLoopSystem.subSystemList.Length - systemPosition);


Just so I am clear is there a reason we are doing a copy twice? I feel like there is a missing check here.

we copy from 0 to systemPosition (systemsBefore)
then we set childLoopSystem at systemPosition (childSystem)
and finally copy from systemPosition + 1 to the end (systemsAfter)

we don't double-copy, it is basically: half + 1 + half = all + 1
if code doesn't read like that, I might comment more and/or introduce more local variables to indicate what's going on more clearly.

wackoisgod

LGMT

com.unity.multiplayer.mlapi/Tests/Runtime/NetworkUpdateLoopTests.cs

fix: reset PlayerLoop back to DefaultPlayerLoop on ExitingPlayMode in…

505fe8f

… the Editor

0xFA11 requested review from LukeStampfli, NoelStephensUnity and mattwalsh-unity March 11, 2021 01:34

NoelStephensUnity approved these changes Mar 11, 2021

View reviewed changes

mattwalsh-unity approved these changes Mar 11, 2021

View reviewed changes

JesseOlmer suggested changes Mar 11, 2021

View reviewed changes

fix: only uninject NetworkPlayerLoop systems from the PlayerLoop when…

3548f5f

… exiting PlayMode in the Editor

0xFA11 commented Mar 11, 2021

View reviewed changes

0xFA11 changed the title ~~fix: reset PlayerLoop back to DefaultPlayerLoop on ExitingPlayMode in the Editor~~ fix: uninject NetworkPlayerLoop systems from PlayerLoop when exiting PlayMode in the Editor Mar 11, 2021

comment in NetworkUpdateLoop.Initialize() method explaining the flow

cb1d918

NoelStephensUnity reviewed Mar 11, 2021

View reviewed changes

JesseOlmer reviewed Mar 11, 2021

View reviewed changes

0xFA11 added 2 commits March 12, 2021 02:05

implement tests for NetworkUpdateLoop InjectSystems() and UninjectSys…

de21a54

…tems()

refactor NetworkUpdateLoop.UninjectSystems()

8b1334c

0xFA11 commented Mar 12, 2021

View reviewed changes

com.unity.multiplayer.mlapi/Tests/Runtime/NetworkUpdateLoopTests.cs Show resolved Hide resolved

0xFA11 requested review from JesseOlmer, NoelStephensUnity and mattwalsh-unity March 12, 2021 02:26

refactor NetworkUpdateLoop: more performance, less duplicates, better…

3ac4360

… namings

0xFA11 changed the title ~~fix: uninject NetworkPlayerLoop systems from PlayerLoop when exiting PlayMode in the Editor~~ fix: unregister NetworkPlayerLoop systems from PlayerLoop Mar 13, 2021

0xFA11 requested a review from wackoisgod March 13, 2021 18:25

wackoisgod reviewed Mar 14, 2021

View reviewed changes

micro-optimization

1b6df9b

wackoisgod approved these changes Mar 14, 2021

View reviewed changes

JesseOlmer suggested changes Mar 15, 2021

View reviewed changes

0xFA11 added 2 commits March 15, 2021 19:14

implement RegisterCustomLoopInTheMiddle test

6cf6331

RegisterCustomLoopInTheMiddle test to operate on a default PlayerLoop

5895919

JesseOlmer approved these changes Mar 15, 2021

View reviewed changes

JesseOlmer merged commit 8b2e771 into release/0.1.0 Mar 15, 2021

JesseOlmer deleted the fix/editor-unreg-netupdateloop branch March 15, 2021 20:50

jeffreyrainy mentioned this pull request Mar 15, 2021

Random error messages in editor #558

Closed

Conversation

0xFA11 commented Mar 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NoelStephensUnity left a comment • 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!

mattwalsh-unity commented Mar 11, 2021

Uh oh!

JesseOlmer left a comment

Choose a reason for hiding this comment

Uh oh!

JesseOlmer Mar 11, 2021

Choose a reason for hiding this comment

Uh oh!

0xFA11 Mar 11, 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!

JesseOlmer commented Mar 11, 2021

Uh oh!

0xFA11 Mar 11, 2021

Choose a reason for hiding this comment

Uh oh!

0xFA11 Mar 11, 2021

Choose a reason for hiding this comment

Uh oh!

NoelStephensUnity Mar 11, 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!

0xFA11 Mar 11, 2021

Choose a reason for hiding this comment

Uh oh!

JesseOlmer left a comment

Choose a reason for hiding this comment

Uh oh!

JesseOlmer Mar 11, 2021

Choose a reason for hiding this comment

Uh oh!

0xFA11 Mar 12, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wackoisgod Mar 14, 2021

Choose a reason for hiding this comment

Uh oh!

0xFA11 Mar 14, 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!

wackoisgod left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

0xFA11 commented Mar 11, 2021 •

edited

Loading

NoelStephensUnity left a comment •

edited

Loading

0xFA11 Mar 11, 2021 •

edited

Loading

NoelStephensUnity Mar 11, 2021 •

edited

Loading

0xFA11 Mar 14, 2021 •

edited

Loading