Fix Entity#destroy (#5414)

* Fix Entity#destroy

* Implement `GraphNode#destroy`

* Reflect `GraphNode#destroy` in `SceneRegistry#changeScene`
Add assertion to make sure it worked

* remove debugging line

* Update src/scene/graph-node.js

Co-authored-by: Martin Valigursky <59932779+mvaligursky@users.noreply.github.com>

* SceneRegistry#changeScene: remove Debug.assert

* Destroy Model graph instead of just removing it
Revert order in Entity#destroy

* Update src/scene/graph-node.js

Co-authored-by: Will Eastcott <will@playcanvas.com>

* Update src/scene/graph-node.js

Co-authored-by: Will Eastcott <will@playcanvas.com>

---------

Co-authored-by: Martin Valigursky <59932779+mvaligursky@users.noreply.github.com>
Co-authored-by: Will Eastcott <will@playcanvas.com>

src/framework/components/model/component.js

@@ -300,7 +300,7 @@ class ModelComponent extends Component {
             this._model._immutable = false;
 
             this.removeModelFromLayers();
-            this.entity.removeChild(this._model.getGraph());
+            this._model.getGraph().destroy();
             delete this._model._entity;
 
             if (this._clonedModel) {

src/framework/entity.js

@@ -519,28 +519,7 @@ class Entity extends GraphNode {
             this.c[name].system.removeComponent(this);
         }
 
-        // Detach from parent
-        if (this._parent)
-            this._parent.removeChild(this);
-
-        // recursively destroy all children
-        const children = this._children;
-        while (children.length) {
-
-            // remove a child from the array and disconnect it from the parent
-            const child = children.pop();
-            child._parent = null;
-
-            if (child instanceof Entity) {
-                child.destroy();
-            }
-        }
-
-        // fire destroy event
-        this.fire('destroy', this);
-
-        // clear all events
-        this.off();
+        super.destroy();
 
         // remove from entity index
         if (this._guid) {

src/framework/scene-registry.js

@@ -376,14 +376,11 @@ class SceneRegistry {
         const app = this._app;
 
         const onBeforeAddHierarchy = (sceneItem) => {
-            // Destroy/Remove all nodes on the app.root
-            const rootChildren = app.root.children;
-            while (rootChildren.length > 0) {
-                const child = rootChildren[0];
-                child.reparent(null);
-                child.destroy?.();
+            // Destroy all nodes on the app.root
+            const { children } = app.root;
+            while (children.length) {
+                children[0].destroy();
             }
-
             app.applySceneSettings(sceneItem.data.settings);
         };
 

src/scene/graph-node.js

@@ -480,6 +480,37 @@ class GraphNode extends EventHandler {
         return this;
     }
 
+
+    /**
+     * Detach a GraphNode from the hierarchy and recursively destroy all children.
+     *
+     * @example
+     * const firstChild = this.entity.children[0];
+     * firstChild.destroy(); // delete child, all components and remove from hierarchy
+     */
+    destroy() {
+        // Detach from parent
+        this._parent?.removeChild(this);
+
+        // Recursively destroy all children
+        const children = this._children;
+        while (children.length) {
+            // Remove last child from the array
+            const child = children.pop();
+            // Disconnect it from the parent: this is only an optimization step, to prevent calling
+            // GraphNode#removeChild which would try to refind it via this._children.indexOf (which
+            // will fail, because we just removed it).
+            child._parent = null;
+            child.destroy();
+        }
+
+        // fire destroy event
+        this.fire('destroy', this);
+
+        // clear all events
+        this.off();
+    }
+
     /**
      * Search the graph node and all of its descendants for the nodes that satisfy some search
      * criteria.