fix: ServerDestroySpawnedSceneObjects throws an exception before switching scene - #618
Merged
Merged
Conversation
If you have in-scene placed NetworkObjects and you transition to a new scene then a crash will occur in NetworkSpawnManager.ServerDestroySpawnedSceneObjects due to NetworkObjects being removed from the SpawnedObjectsList during the "OnDestroy" invocation. This fixes that crash.
Adding some comments as to why the code is added, camelCase for locals, and generic Var as opposed to specific type for declaration.
| { | ||
| //This Allocation is "ok" for now because this code only executes when a new scene is switched to | ||
| //We need to keep track of GameObjects and NetworkObjects to be destroyed in the current scene | ||
| //but only destroy them once outside of the enumerative foreach loop |
Contributor
There was a problem hiding this comment.
Ok...would you also capture the 'why' here too in the comment?
Member
Author
There was a problem hiding this comment.
Simplified the code required and adjusted the comment.
This applies a better approach to the issue of removing objects from a list that is being iterated over without throwing the "Collection was modified; enumeration operation may not execute" exception due to NetworkObjects removing themselves from the SpawnedObjectList during OnDestroy.
Minor tweak to the comment as to why the code was added.
NoelStephensUnity
marked this pull request as ready for review
March 15, 2021 18:49
| internal static void ServerDestroySpawnedSceneObjects() | ||
| { | ||
| foreach (var sobj in SpawnedObjectsList) | ||
| //This Allocation is "ok" for now because this code only executes when a new scene is switched to |
Contributor
There was a problem hiding this comment.
Is this comment still real? I don't see an allocation per se...or do you mean the ToList() call?
Contributor
There was a problem hiding this comment.
Yes ToList creates a copy of the collection and allocates a new list object in the process.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is the fix for MTT-544 where transitioning from a scene that has InSceneObjects the Host-Server will throw an exception due to a list in the middle of an enumerative processing loop is modified.
The Jira Bug MTT-544 describes the steps to reproduce the exception and to verify this branch fixes those specific exceptions.