fix: Use Time.unscaledDeltaTime instead of Time.deltaTime as time step#2187
fix: Use Time.unscaledDeltaTime instead of Time.deltaTime as time step#2187jeffreyrainy merged 15 commits intodevelopfrom
Conversation
#2171) This fixes the case of Time.timeScale = 0 stopping the NetworkManager to update. This as no influence on all projects that always have Time.timeScale = 1. + Add tests + Change existing test using Assert.Equal(boolExpr) to the correct Assert function to display real values expected instead of just "true" or "false"
…ement (netcode.standards)
…logies/com.unity.multiplayer.mlapi into fix/unscaled-deltatime-2183
…logies/com.unity.multiplayer.mlapi into fix/unscaled-deltatime-2183
…sed by NetworkManager.Singleton (can we get rid of that completely soon?)
| foreach (var networkManager in NetworkManagerInstances) | ||
| { | ||
| networkManager.Shutdown(); | ||
| networkManager.ShutdownInternal(); |
There was a problem hiding this comment.
To explain why this is there... It comes down to NetworkManager.Singleton.
Shutdown doesn't do anything when it's called but set a flag for later. Singleton doesn't get unset until ShutdownInternal is called. Which means timing can happen as follows:
- Test 1 runs
- Test 1 stops and shuts down the NetworkManager
- Test 2 starts running and OnSetup creates a new prefab
- NetworkManager.Singleton is still set, so the new NetworkObject gets its NetworkManager set to the current Singleton which is already shutdown and its GameObject destroyed
- Setup yields
- NetworkManager.ShutdownInternal is called
- NetworkManager.OnDestroy is called and destroys all NetworkObjects associated with it... the newly created prefab gets destroyed out of nowhere!
- Test 2 resumes and tries to use the prefab, and can't because it has now been destroyed... Test 2 fails, but if it's run on its own, it succeeds
Adding an explicit call to ShutdownInternal at the end of the test ensures the Singleton gets unset so the objects in the next test don't get associated with it.
There was a problem hiding this comment.
The ENTIRE point of the "mostly remove the singleton" work we did was to enable more reliable testing. Why are these tests using the singleton at all?
Any/all cases depending on the singleton should be considered a high priority bug IMO. Calling an internal API to work around this is just throwing more fuel on the fire.
There was a problem hiding this comment.
The tests aren't using the singleton. The SDK code is using the singleton in a way that creates non-deterministic timing artifacts that interfere with test stability.
There was a problem hiding this comment.
Kitty is correct. The NetcodeIntegrationTest doesn't use NetworkManager.Singleton , but there are still a few tests that were not upgraded to use NetcodeIntegrationTest and/or still should be refactored to not use NetworkManager.Singleton:
NetworkTimeSystemTests
NetworkTransformOwnershipTests
NetworkObjectSceneSerializationTests
BufferDataValidationComponent
Regarding the SDK, there are still a few places that use it (like Kitty was mentioning)...some places are just really legacy systems (i.e. NetworkLog) and other places just need to be updated (i.e. SceneEventData line 631...not sure why that didn't get updated to use m_NetworkManager) but only a handful.
|
|
||
| this.UnregisterAllNetworkUpdates(); | ||
|
|
||
| if (Singleton == this) |
There was a problem hiding this comment.
Seems like a bad idea to unregister the singleton before you've finished shutting it down. Why was this necessary?
| foreach (var networkManager in NetworkManagerInstances) | ||
| { | ||
| networkManager.Shutdown(); | ||
| networkManager.ShutdownInternal(); |
There was a problem hiding this comment.
The ENTIRE point of the "mostly remove the singleton" work we did was to enable more reliable testing. Why are these tests using the singleton at all?
Any/all cases depending on the singleton should be considered a high priority bug IMO. Calling an internal API to work around this is just throwing more fuel on the fire.
…ties caused by NetworkManager.Singleton (can we get rid of that completely soon?)" This reverts commit 02a222e.
| [SetUp] | ||
| public void Setup() | ||
| { | ||
| m_OriginalTimeScale = Time.timeScale; |
NoelStephensUnity
left a comment
NoelStephensUnity
left a comment
There was a problem hiding this comment.
Looks good to me!
👍
Unity-Technologies#2187) * fix: Use Time.unscaledDeltaTime instead of Time.deltaTime as time step (Unity-Technologies#2171) This fixes the case of Time.timeScale = 0 stopping the NetworkManager to update. This as no influence on all projects that always have Time.timeScale = 1. + Add tests + Change existing test using Assert.Equal(boolExpr) to the correct Assert function to display real values expected instead of just "true" or "false" * style: removing unrelated stylistic differences. Fixing brackets placement (netcode.standards) * test: test adjustments * test: test adjustments * test: Applying PR review suggestion of saving the time scale * test: Applying PR review suggestion of saving the time scale, second location * Reverted changes to AddNetworkPrefabTests and fixed instabilities caused by NetworkManager.Singleton (can we get rid of that completely soon?) * reverting unwanted whitespace change * reducing the test cases to 0.0f, 1.0f and 2.0f timescales * Revert "Reverted changes to AddNetworkPrefabTests and fixed instabilities caused by NetworkManager.Singleton (can we get rid of that completely soon?)" This reverts commit 02a222e. Co-authored-by: PitouGames <pitou.games@gmail.com> Co-authored-by: Kitty Draper <kitty.draper@unity3d.com>
5 participants
This PR fixes the case of Time.timeScale = 0 that stops the NetworkManager to send updates.
It only change the NetworkManager to use Time.unscaledDeltaTime instead of Time.deltaTime (scaled) when advancing the time system.
It has no influence on all projects that always use Time.timeScale = 1.
Changelog
Time.timeScaleis set to 0 #2171)Testing and Documentation