Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
mattjennings opened this issue Jul 17, 2022 · 10 comments · Fixed by #2430
Closed

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

mattjennings opened this issue Jul 17, 2022 · 10 comments · Fixed by #2430
Labels
bug This issue describes undesirable, incorrect, or unexpected behavior

Comments

@mattjennings
Copy link
Contributor

mattjennings commented Jul 17, 2022

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:

public kill() {
if (this.scene) {
this._prekill(this.scene);
this.emit('kill', new KillEvent(this));
super.kill();
this._postkill(this.scene);
} else {
this.logger.warn('Cannot kill actor, it was never added to the Scene');
}
}

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
Copy link
Member

@mattjennings Thanks for the issue!

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

@eonarheim eonarheim added the bug This issue describes undesirable, incorrect, or unexpected behavior label Jul 17, 2022
@chrisk-7777
Copy link

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
Copy link
Member

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

eonarheim added a commit that referenced this issue Jul 22, 2022
Closes #2419

## Changes:

- `Actor.scene` is now moved to the base type `Entity.scene` functionality is the same
- Adds scene to entity add/remove processing
@chrisk-7777
Copy link

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

eonarheim added a commit that referenced this issue Jul 23, 2022
Related #2419 (comment)

## Changes:

- Update log message to be more helpful
@chrisk-7777
Copy link

chrisk-7777 commented Aug 7, 2022

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
Copy link
Member

@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
Copy link
Member

Posted a PR 👍 #2454

@chrisk-7777
Copy link

Great, thanks for looking into it @eonarheim 🙌

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

@KokoDoko
Copy link

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
Copy link

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()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue describes undesirable, incorrect, or unexpected behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants