Fix Entity#destroy

[kungfooman]

Fixes #4649

The main issue is simply that GraphNode has no destroy method and if a Entity is inside a GraphNode... it will never be deleted.

I can imagine many solutions, but simply to get the conversations going I propose this one...

I confirm I have read the contributing guidelines and signed the Contributor License Agreement.

[yaustar]

Just to confirm, that scene basically has:

entity
-> graph node
- -> entity

[kungfooman]

@yaustar yep exactly, testable like:

const isEntityChildOfGraphNode = _ => _ instanceof pc.Entity && _.parent?.constructor === pc.GraphNode;
const entitiesInGraphNodes = pc.app.root.find(isEntityChildOfGraphNode);
entitiesInGraphNodes.map(ent => {
    const {name, path, parent} = ent;
    return {name, path, ent, parent};
});

Output in Seemore:

More ideas for (possible?) solutions:

1. As in PR, find these special cases before recursively destroy entities "normally" (as it always was)
2. Implement GraphNode#destroy?
3. Simply find every entity using GraphNode#find and delete them all iteratively?

Probably everything has pros/cons, but so far I tend to (1) or (2)

[mvaligursky]

personally I would find the Implement GraphNode#destroy option a lot cleaner, any reasons not to go that route?

[kungfooman]

I just thought about performance, lets say having 20 skeletons with ~70 GraphNode's and GraphNode is a bit "lower level" and calling destroy on them even tho they may have no entities might be useless. But I guess in real-life people put entities in GraphNode's all the time to track animated positions or attach particle effects, so being "pure" with GraphNode's is not really a thing.

personally I would find the Implement GraphNode#destroy option a lot cleaner, any reasons not to go that route?

So I pretty much agree, I would also like to see GraphNode#detachFromParent, because I see it all over the place handled via nested if's:

Or other tricks are GraphNode#reparent(null)... which feels "okayish" once you know it, but still not 100% intuitive. I guess therefore it's only used once:

Would it be okay to implement GraphNode#detachFromParent and GraphNode#destroy?

[mvaligursky]

Would it be okay to implement GraphNode#detachFromParent and GraphNode#destroy?

Absolutely, I'd like that. If possible, create two separate PRs please.

[kungfooman]

Debugging and debugging 🤓

Found another reason why it fails...

Called from:

Simply said, the "component removal code" is snatching away our entities (which we want to delete recursively) before our recursion ever has a chance to get to them (they just "disappear" 🤯👻💥).

This is easily fixed though, simply first disabled all component, then do the recursion, and then to component deletion.

Or 2nd possible solution: mark the entire hierarchy we want to delete (something like _destroying flag) beforehand and then make GraphNode#removeChild do nothing, because we know better (namely that this is not a removal but a destruction).

But I like the 1st solution, simply destroying the components after the recursion, so I will research that route a bit more... I realize that historically this stuff was a mine field, a change here breaks a project there etc... please just link me all scenes I shall test this with 🔍 (after all, we want to fix bugs, not introduce new ones).

[kungfooman]

Ready for review... pretty happy about the result.

I'm testing with Seemore and besides the event bug I also found a room-loading bug in demo-loader.js:

kungfooman/seemore@25e565c

[kungfooman]

Context for latest commit:

Output of this code in src/framework/components/model/component.js:

            this.removeModelFromLayers();
            const ents = this._model.getGraph().find(_ => _ instanceof Entity);
            if (ents.length) {
                console.log(ents);
            }
            this._model.getGraph().destroy();

Edit:

This looks like a potential other entity orphan trap:

engine/src/framework/components/sprite/component.js

Lines 599 to 603 in 039da2e

	if (this._node) {
	if (this._node.parent)
	this._node.parent.removeChild(this._node);
	this._node = null;
	}

(but I have no test scene right now to validate quickly)

[mvaligursky]

Approving, looks great to me, I don't see any problems now. Thanks!