fix: reset static fields for Fast Enter Play Mode#3956
Conversation
# Conflicts: # com.unity.netcode.gameobjects/Runtime/Core/NetworkObject.cs
simon-lemay-unity
left a comment
simon-lemay-unity
left a comment
There was a problem hiding this comment.
Changes to UnityTransport look good to me.
# Conflicts: # com.unity.netcode.gameobjects/Runtime/Logging/NetworkLog.cs
…gies/com.unity.netcode.gameobjects into chore/fast-enter-playmode
EmandM
left a comment
EmandM
left a comment
There was a problem hiding this comment.
This is looking incredible! You've done such good thorough work on this PR. You should be really proud of yourself. 
| IsDestroying = true; | ||
| } | ||
|
|
||
| private void OnDisable() |
There was a problem hiding this comment.
This unsubscribe should be in OnDestroy instead. It is (kind of vaguely) valid to have a disabled NetworkObject so we should still be tracking that object.
| using System; | ||
| using System.Collections.Generic; | ||
| #if UNITY_EDITOR | ||
| using UnityEditor; |
There was a problem hiding this comment.
This is used often enough that the using define still has value in this file. Do you mind putting it back?
| s_SceneEventTypeNames[(uint)type] = type.ToString(); | ||
| k_SceneEventTypeNames[(uint)type] = type.ToString(); | ||
| } | ||
| s_FrameDispatch = new ProfilerMarker($"{nameof(NetworkMetrics)}.DispatchFrame"); |
There was a problem hiding this comment.
This is never reset so it can match the pattern of k_SceneEventTypeNames. (i.e. static readonly that's only set here).
| /// <summary> | ||
| /// Command-line options singleton | ||
| /// </summary> | ||
| public static CommandLineOptions Instance |
There was a problem hiding this comment.
Something in here is breaking some tests. We maybe don't need to worry about resetting the CommandLineOptions in the editor as command line args are very hard to use from the editor.
I wonder if instead of the usage pattern of CommandLineOptions.Instance.GetArg(<arg>) as string <varName> it'd be nicer to just have a single static function called TryGetArg.
Something like
public static bool TryGetArg(string arg, out string argValue)That can then be used like
CommandLineOptions.TryGetArg(k_OverridePortArg, out var argValue)| return 0; | ||
| } | ||
|
|
||
| internal bool EnableSerializationLogs = false; |
There was a problem hiding this comment.
This field wasn't static. We don't want lose the functionality of being able to turn on logging per client, so might be best to put this one back for this PR.
I'll updated it properly when I implement the new logger through here.
| internal class FastEnterPlayModeTests | ||
| { | ||
| [UnityTest] | ||
| public IEnumerator NetworkManagerSingletonResetsOnPlayModeEnter( |
There was a problem hiding this comment.
This test doesn't seem to be testing anything.
I know I said we needed tests for this change, but looking at the changes in this PR I think there's not really any risky behaviour being reset here that we need a test to ensure it's reset. So it looks like we might not need a test after all.
4 participants
Purpose of this PR
Ensure com.unity.netcode.gameobjects package supports Fast Enter Play Mode
Jira ticket
MTT-14665
Changelog
Documentation
Testing & QA (How your changes can be verified during release Playtest)
Functional Testing
Manual testing :
Manual testing doneAutomated tests:
Covered by existing automated testsCovered by new automated testsDoes the change require QA team to:
Review automated tests?Execute manual tests?Provide feedback about the PR?If any boxes above are checked the QA team will be automatically added as a PR reviewer.
Backports
N/A