Child entities don't inherit scene from parent, logs warning when killed

[mattjennings]

It appears that child entities do not inherit this.scene from its parent. I first noticed this when killing an actor with children and getting the following warning:

Cannot kill actor, it was never added to the Scene

After digging into it I noticed it was the child entity logging the warning, as this.scene was null here:

https://github.com/excaliburjs/Excalibur/blob/730d621805d32104c18dc7ca8d9e97c7f212bb2b/src/engine/Actor.ts#L795-L804

If the child entities aren't truly killed it's possible this might be a memory leak, but I haven't dug much further.

Steps to Reproduce

repro: https://codesandbox.io/s/exaclibur-child-entities-bug-5r0w3e?file=/src/index.js

• Add children to an actor via actor.addChild()
• Kill the actor
• See the warning in console

Expected Result

Children should inherit scene from parent and no warning log should occur when killed

Actual Result

Children do not inherit scene and a warning is logged when parent is killed

Environment

• browsers and versions: Chrome 103.0.5060.114
• operating system: macoS 12.4
• Excalibur versions: 0.27

Current Workaround

None

[eonarheim]

@mattjennings Thanks for the issue!

Definitely a bug, it should 100% have the scene attached that is an oversight.

[chrisk-7777]

I was just about to raise something similar, so good timing!

My suggestion was to improve the warning message to include the actor's name to help narrow down the warning when it does pop up. But a fix to the underlying issue would also be ace.

Something like:

this.logger.warn(`Cannot kill actor ${this.name}, it was never added to the Scene`); 

[eonarheim]

@chrisk-7777 this is a great idea, I think adding this context to the log will help a ton.

[chrisk-7777]

Thanks for the fix! It's amazing the speed of development on this project

[chrisk-7777]

Just a follow up to this one. I'm getting a strange issue, its somewhat related to the above - I started by calling killChild but it seems the parent actor .children is left "as is" (still populated).

So as an alternative, I tried removeAllChildren instead, but I believe there might be an issue there. See demo:

https://codepen.io/chrisk7777/pen/gOeKOOy?editors=0010

Note how every second child is removed? Not all of them. This had me scratching my head, but after some digging, and I could be wrong, but I believe its because we're altering the array inside a loop:

https://github.com/excaliburjs/Excalibur/blob/main/src/engine/EntityComponentSystem/Entity.ts#L258

+

https://github.com/excaliburjs/Excalibur/blob/main/src/engine/EntityComponentSystem/Entity.ts#L246

(sorry late night edit: @eonarheim if you prefer this as a new issue or simply a discussion post, let me know)

[eonarheim]

@chrisk-7777 thanks for the follow up! I think we definitely have a couple bugs! These should be easy to fix quick, thanks for spotting! I can make a quick issue for these 👍

Bug 1: Doh! You're 100% right about the removeAllChildren that is definitely a problematic way to remove them all. Classic removing elements while iterating bug 💣

Bug 2: Child actors should definitely not be present in the parent .children after being killed.

[eonarheim]

Posted a PR 👍 https://github.com/excaliburjs/Excalibur/pull/2454

[chrisk-7777]

Great, thanks for looking into it @eonarheim 🙌

Bug 1: Haha, yup I've been there.

[KokoDoko]

It seems the bug is still present when you add child actors to other actors, using

let actor = new Actor()
let anotherActor = new Actor()

actor.addChild(anotherActor())
anotherActor.kill()

[chrisk-7777]

I haven't tested the above, but just confirming the extra () is intended?

let actor = new Actor()
let anotherActor = new Actor()

actor.addChild(anotherActor()) // <-- the extra () after anotherActor
anotherActor.kill()