fix: In-scene NetworkObjects disabled but never re-enabled after scene transition

[NoelStephensUnity]

Only when there is an in-scene placed NetworkObject that moves itself into the DDOL scene and there is a full scene transition (i.e. LoadSceneMode.Single) the in-scene placed NetworkObject will get disabled and never re-enabled.
This PR resolves the above issue.

MTT-1479

PR Checklist

• Have you added a backport label (if needed)? For example, the type:backport-release-* label. After you backport the PR, the label changes to stat:backported-release-*.
• Have you updated the changelog? Each package has a CHANGELOG.md file.

Changelog

com.unity.netcode.gameobjects

• Fixed in-scene NetworkObjects that are moved into the DDOL scene not getting restored to their original active state (enabled/disabled) after a full scene transition (i.e. LoadSceneMode.Single).
• Fixed all NetworkObjects marked to persist a full scene transition now are restored to their original active state (enabled/disabled) after a full scene transition (i.e. LoadSceneMode.Single).

Testing and Documentation

• Includes unit tests.
• No documentation changes or additions were necessary.

[JesseOlmer]

I can't tell what this test does by its name.
I also can't really tell what the test does by looking at the test method. Is it testing that MoveObjectsToDontDestroyOnLoad sets them to inactive? Is it testing that MoveObjectsFromDontDestroyOnLoadToScene sets them to active? Is it testing both?

[NoelStephensUnity]

Yeah... I need to come up with a better name for that test probably.
The test uses those two methods when doing a full scene transition (i.e. LoadSceneMode.Single).

• Pre-Scene Transition--> MoveObjectsToDontDestroyOnLoad (disables in-scene placed NOs in DDOL)
• Load Scene
• Post-Scene Transition--> MoveObjectsFromDontDestroyOnLoadToScene (enables in-scene placed NOs in DDOL)

I could just have easily manually disabled the NetworkObject and then just called the MoveObjectsFromDontDestroyOnLoadToScene method to validate the fix, but figured I might as well include the initial method that disables the in-scene placed NetworkObject (MoveObjectsToDontDestroyOnLoad) since that is what disables the NetworkObject(s) prior to doing the full scene transition.

[JesseOlmer]

It appears that these are being made internal so that you can test them directly? This is a testing anti-pattern. To paraphrase a great number of testing authorities: "How do you test internals? Through the public API."

[NoelStephensUnity]

Yeah... the test to validate this fix was a little tricky.
The only place the two methods pertaining to this bug and fix (MoveObjectsToDontDestroyOnLoad and MoveObjectsFromDontDestroyOnLoadToScene ) are called is when there is a full scene transition taking place. Since we cannot do a full scene transition (i.e. it would unload the Test Runner's temporary scene and would break the rest of the tests) in a unit and/or integration test, the only way to write a test (currently) to verify the fix in this PR was to create the same circumstances and call the same methods...which required them to be accessible.
If you want, I could reduce the anti-pattern footprint by just testing the MoveObjectsFromDontDestroyOnLoadToScene method and not including the MoveObjectsToDontDestroyOnLoad in that process (i.e. just disable it in the unit test itself).

[JesseOlmer]

nit: this event system to generic condition tracker feels like overkill. Why not just have an InSceneNetworkObjectDDOLTest field on the network behaviour, assign it when you create it, and have it do something like InSceneNetworkObjectDDOLTest.OnEnabledWasCalled = true;

Then, in the test case, instead of the "waitforcondition" and "conditionmet(false)" stuff you can just do the waiting in-place and increase readability of the test:

            while (!OnEnabledWasCalled)
            {
                yield return new WaitForSeconds(m_ConditionMetFrequency);
            }

[NoelStephensUnity]

Sure, I could just do something like:

            while (networkObject.isActiveAndEnabled)
            {
                yield return new WaitForSeconds(m_ConditionMetFrequency);
            }

and to check it is re-enabled

            while (!networkObject.isActiveAndEnabled)
            {
                yield return new WaitForSeconds(m_ConditionMetFrequency);
            }

in place of the other stuff.

[JesseOlmer]

Is this going to enabled objects which the user disabled manually?

[JesseOlmer]

Maybe worth adding a few test cases:

• Scene-placed objects not in DDOL shouldn't be impacted
• Dynamic objects not in DDOL shouldn't be impacted
• Scene-placed objects in DDOL which are active should stay active
• Scene-Placed objects in DDOL which are inactive should stay inactive
• Dynamic objects in DDOL which are active shouldn't be impacted

[NoelStephensUnity]

I can do that.

[NoelStephensUnity]

The most recent updated test includes the following test cases where all NetworkObjects' DestroyWithScene are set to false:

• Tests for both default enabled and disabled cases:
  • Scene-placed NetworkObjects not already in the DDOL
  • Dynamically spawned NetworkObjects not already in the DDOL
  • Scene-placed NetworkObjects already in the DDOL
  • Dynamically spawned NetworkObjects already in the DDOL

As a side note:
Both dynamically spawned and in-scene placed NetworkObjects that do not have the DestroyWithScene set to false will be despawned before the scene transition.

[0xFA11]

🚀

[unity-cla-assistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}