Check if gameobjects going to be removed in OnShutdown-Method

[ChristianGreiner]

Summary

The engine throws an exception when you remove gameobjects in the OnShutdown-Method.

How to reproduce [Bugs]

Here is my code:

private List<GameObject> enemies = new List<GameObject>();

public void OnInit(InitContext context)
{
    if (context == InitContext.Activate)
    {
        // add some gameobjects and put them into the list.
    }
}

public void OnShutdown(ShutdownContext context)
{
    if (context == ShutdownContext.Saving)
    {
        foreach (var enemy in enemies)
        {
            Scene.Current.RemoveObject(enemy);
        }
    }
}

What's my plan?
I like to keep track of "enemies" and I want to remove them after exiting the sandbox-mode.

Attachments

The Exception:

[Core] Error:   OnSaving() of Scene "Data\Scenes\TestLevel" failed: InvalidOperationException: Collection was modified; enumeration operation may not execute.
CallStack:
   at System.Collections.Generic.HashSet`1.Enumerator.MoveNext()
   at System.Linq.Enumerable.WhereEnumerableIterator`1.MoveNext()
   at Duality.Resources.Scene.OnSaving(String saveAsPath) in c:\projects\duality\Duality\Resources\Scene.cs:line 942
   at Duality.Resource.CheckedOnSaving(String saveAsPath) in c:\projects\duality\Duality\Resource.cs:line 215

Jeah, I know that I try to remove gameobjects when the engine wants to save them.

[ChristianGreiner]

I just realized that I needn't keep track of them... 🙈 I had a logic error in my code... so.. jeah..
Nevertheless, the bug should be fixed, shouldn't he? :)

[ilexp]

I just realized that I needn't keep track of them... 🙈 I had a logic error in my code... so.. jeah..

Yep, sandbox does that already by reloading the Scene :)

Nevertheless, the bug should be fixed, shouldn't he? :)

Yep.

[SirePi]

I don't think this can be categorized as a bug, and honestly I don't see how else you are supposed to fix it. Removing objects from a collection that is being enumerated is an error in and by itself (the Exception itself is coming from System, not from Duality). Even if you do that in basic c# you will get the same error, and there's nothing else to do besides review your code :)

[ilexp]

Now that you're commenting on the issue, I think it has been solved inadvertently by implementing the deterministic component update order in issue #303, since all ICmpInitializable calls are now performed on a (sorted) copy of the actual data storage.

It wouldn't hurt to write a unit test for this issue though to verify it and guard against regressions. A complete test should check for this "self-modifying scene graph" behavior to be valid for all init and shutdown contexts, as well as all situations in which they can occur.

The existing ICmpInitializableTest class covers them all and it would make sense to extend that class to perform all relevant once more in a different form where the component removes its own object from the scene when the tested event is received. Instead of checking whether the event was received (existing tests already cover this), simply assert that the removal was successful at the end.