fix: Attachables destroy order of operations#3931
fix: Attachables destroy order of operations#3931NoelStephensUnity wants to merge 15 commits intodevelop-2.0.0from
Conversation
Adding change log entry. Fixing some typos.
EmandM
left a comment
EmandM
left a comment
There was a problem hiding this comment.
Hmmm, I think I maybe don't understand the full process of what the issue is here
This includes some additional edge case fixes... specifically for the scenario where we know a scene is being unloaded that will result in either an `AttachableNode` or `AttachableBehaviour` being destroyed and they are attached.
Fixing an edge case that requires manually testing due to how integration tests additively load everything to avoid unloading the test runner test scene when loading a scene. Two parts: - When a server is destroying spawned scene objects due to a single mode scene load, it should also include a check for this and handle marking the NetworkObject and associated NetworkBehaviours as being destroyed and then destroy the object. - Just prior to handling a scene loading event, we need to mark anything that will be destroyed (i.e. destroy with scene) as being destroyed when migrating spawned objects into the DDOL.
|
/crc |
![u-pr[bot]](https://avatars.githubusercontent.com/in/1201017?s=60&v=4){kind=small}
There was a problem hiding this comment.
The review has identified several critical issues related to the execution order of destruction hooks and the handling of attached states, particularly in pooling scenarios. Key findings include:
- Execution Timing: The
IsDestroyingflag is set too late inNetworkBehaviour, causing safety checks inInternalDetachto be bypassed. - State Management:
AttachableBehaviourfails to correctly notify/update state during destruction, risking stale data in pooled objects. - Reference Cleanup:
AttachableNodemay leave stale references when behaviours with authority are being destroyed. - Code Quality: Improvements suggested for null safety and documentation of scene-state checks.
🤖 Helpful? 👍/👎
…rors-scene-changes
| // Keeping the property a private set to assure this is | ||
| // the only way it can be set as it should never be reset | ||
| // back to false once invoked. | ||
| childBehaviour.SetIsDestroying(); |
There was a problem hiding this comment.
Hmmm, are we sure that all the child behaviours will be destroyed if the parent NetworkObject is destroyed?
I know most of the time that'll be true, but what happens if just the gameObject the NetworkObject is on is destroyed, and a child gameObject isn't?
There was a problem hiding this comment.
That is actually part of the issue this PR resolves for... the question you just asked can be answered by checking NetworkObject.IsDestroyed if running from the context of a NetworkBehaviour on the child GameObject...assuming the order things are destroyed flows in a hierarchical fashion.
Whether it is a NetworkBehaviour on the root GameObject or one on a child, that is wanting to perform an action on something within its own prefab instance context or another prefab instance's context, being able to check if the thing has already run through the destroy process or is currently running through the destroy process becomes vital to assure you can not perform that action to avoid other forms of internal engine errors from being logged. The engine handles it and it does not (currently) cause issues... but with things in the works... I think this will become pretty important to have in place.
| // We know this instance is going to be destroyed (when it receives the destroy object message). | ||
| // We have to invoke this prior to invoking despawn in order to know that we are de-spawning in | ||
| // preparation of being destroyed by the SceneManager. | ||
| networkObject.SetIsDestroying(); |
There was a problem hiding this comment.
Why not mimic the real SceneManagement and just call Object.Destroy(networkObject)? That way we can be sure that the IsDestroying stuff is triggered properly.
There was a problem hiding this comment.
This is how the integration test originally worked and is required.
I don't change how this part works...because that is one of required, integration test specific, differences between the integration test scene handler (all NetworkManagers are sharing the same root in the scene hierarchy) and the default scene handler (each NetworkManager has its own unique root and application domain).
| // We know this instance is going to be destroyed (for integration testing we simulate the destroy). | ||
| // We have to invoke this prior to invoking despawn in order to know that we are de-spawning in | ||
| // preparation of being destroyed by the SceneManager. | ||
| networkObject.SetIsDestroying(); |
There was a problem hiding this comment.
I think it's better if the OnDestroy calls this function rather than calling it explicitly
There was a problem hiding this comment.
This is required to be called prior... not during.
- Invoking before despawning:
- It is not despawned.
- Its
OnDestroymethod has not been invoked.- When this is invoked...it is too late for the edge case this solves for.
- Invoking while destroying
- It is too late... internally the
GameObjectis marked for destroy.
- It is too late... internally the
| // We know this instance is going to be destroyed (when it receives the destroy object message). | ||
| // We have to invoke this prior to invoking despawn in order to know that we are de-spawning in | ||
| // preparation of being destroyed by the SceneManager. | ||
| networkObject.SetIsDestroying(); |
There was a problem hiding this comment.
This is required to be called prior... not during.
- Invoking before despawning:
- It is not despawned.
- Its
OnDestroymethod has not been invoked.- When this is invoked...it is too late for the edge case this solves for.
- Invoking while destroying
- It is too late... internally the
GameObjectis marked for destroy.
- It is too late... internally the
2 participants
Purpose of this PR
Fixing an issue with attachables where either an
AttachableBehaviouror anAttachableNodecan throw an exception or erroneously log an error message if they are attached together prior to a scene unload event where one of the two persists the scene unload event and the other does not.Jira ticket
MTT-14831
Changelog
AttachableBehaviouror anAttachableNodecan throw an exception or erroneously log an error message if they are attached during a scene unload where one of the two persists the scene unload event and the other does not.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 testsAttachableBehaviourSceneLoadTests.Does 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
This is v2.x.x. specific.