Repository: playcanvas/engine
Pull request #5469 (2023-07-08 13:32:19)

Implement/use `GraphNode#remove`
This is a follow-up of this PR, to make it a bit cleaner/shorter:

https://github.com/playcanvas/engine/pull/5414#issuecomment-1598473727

`GraphNode#remove` works just like `HTMLElement#remove`:

```js
const div = document.createElement("div");
div.remove();
div.remove();
document.body.append(div);
div.remove(); // only call which does something
div.remove();
```

Equivalent API:

```js
const ent = new pc.Entity();
ent.remove();
ent.remove();
pc.app.root.addChild(ent);
ent.remove(); // only call which does something
ent.remove();
```



I confirm I have read the [contributing guidelines](https://github.com/playcanvas/engine/blob/master/.github/CONTRIBUTING.md) and signed the [Contributor License Agreement](https://docs.google.com/a/playcanvas.com/forms/d/1Ih69zQfJG-QDLIEpHr6CsaAs6fPORNOVnMv5nuo0cjk/viewform).


LeXXik: Shouldn't this be a private API? I mean, we already have `entity.destroy()`, which presumably does the same?
kungfooman: > Shouldn't this be a private API? I mean, we already have `entity.destroy()`, which presumably does the same?

No, this reflects how HTML works:

```js
const div = document.createElement("div");
div.innerHTML = "innerhtml of div";
div.remove();
div.remove();
document.body.append(div);
div.remove();
div.remove();
div.remove();
div.remove();
document.body.append(div);
div.remove();
document.body.append(div);
```

You can parent/unparent a `HTMLElement`/`GraphNode` as much as you want, it's not destroyed. Just because you remove a cookie out of the box, it's not destroyed/eaten. You can just put it back or put it in another box.

Just like you can have this code in HTML, you can do the same in PlayCanvas... `destroy` of course also unparents your entity, but completely destroys it recursively aswell (hence the name implying destruction). `GraphNode#remove` is simply removing it from the scene graph.

Patch:
diff --git a/src/framework/components/sprite/component.js b/src/framework/components/sprite/component.js
index 8e24addf3..d61d83151 100644
--- a/src/framework/components/sprite/component.js
+++ b/src/framework/components/sprite/component.js
@@ -596,11 +596,8 @@ class SpriteComponent extends Component {
         this._hideModel();
         this._model = null;
 
-        if (this._node) {
-            if (this._node.parent)
-                this._node.parent.removeChild(this._node);
-            this._node = null;
-        }
+        this._node?.remove();
+        this._node = null;
 
         if (this._meshInstance) {
             // make sure we decrease the ref counts materials and meshes
diff --git a/src/scene/graph-node.js b/src/scene/graph-node.js
index cd32bfdcc..5eb85f7e2 100644
--- a/src/scene/graph-node.js
+++ b/src/scene/graph-node.js
@@ -490,7 +490,7 @@ class GraphNode extends EventHandler {
      */
     destroy() {
         // Detach from parent
-        this._parent?.removeChild(this);
+        this.remove();
 
         // Recursively destroy all children
         const children = this._children;
@@ -955,6 +955,13 @@ class GraphNode extends EventHandler {
         return this._worldScaleSign;
     }
 
+    /**
+     * Remove graph node from current parent.
+     */
+    remove() {
+        this._parent?.removeChild(this);
+    }
+
     /**
      * Remove graph node from current parent and add as child to new parent.
      *
@@ -962,11 +969,7 @@ class GraphNode extends EventHandler {
      * @param {number} [index] - The child index where the child node should be placed.
      */
     reparent(parent, index) {
-        const current = this._parent;
-
-        if (current)
-            current.removeChild(this);
-
+        this.remove();
         if (parent) {
             if (index >= 0) {
                 parent.insertChild(this, index);
@@ -1294,9 +1297,7 @@ class GraphNode extends EventHandler {
     _prepareInsertChild(node) {
 
         // remove it from the existing parent
-        if (node._parent) {
-            node._parent.removeChild(node);
-        }
+        node.remove();
 
         Debug.assert(node !== this, `GraphNode ${node?.name} cannot be a child of itself`);
         Debug.assert(!this.isDescendantOf(node), `GraphNode ${node?.name} cannot add an ancestor as a child`);
diff --git a/types-fixup.mjs b/types-fixup.mjs
index 6f1740131..767390e59 100644
--- a/types-fixup.mjs
+++ b/types-fixup.mjs
@@ -347,6 +347,7 @@ const standardMaterialProps = [
     ['bumpiness', 'number'],
     ['clearCoat', 'number'],
     ['clearCoatBumpiness', 'number'],
+    ['clearCoatGlossInvert', 'boolean'],
     ['clearCoatGlossMap', 'Texture|null'],
     ['clearCoatGlossMapChannel', 'string'],
     ['clearCoatGlossMapOffset', 'Vec2'],
@@ -402,8 +403,10 @@ const standardMaterialProps = [
     ['emissiveVertexColor', 'boolean'],
     ['emissiveVertexColorChannel', 'string'],
     ['enableGGXSpecular', 'boolean'],
+    ['envAtlas', 'Texture|null'],
     ['fresnelModel', 'number'],
     ['gloss', 'number'],
+    ['glossInvert', 'boolean'],
     ['glossMap', 'Texture|null'],
     ['glossMapChannel', 'string'],
     ['glossMapOffset', 'Vec2'],


Test patch:
diff --git a/test/scene/graph-node.test.mjs b/test/scene/graph-node.test.mjs
index 1741c1d22..74a87b0f3 100644
--- a/test/scene/graph-node.test.mjs
+++ b/test/scene/graph-node.test.mjs
@@ -634,6 +634,19 @@ describe('GraphNode', function () {
 
     });
 
+    describe('#remove', function () {
+
+        it('removes the node from its parent, unparenting it', function () {
+            const node = new GraphNode();
+            const child = new GraphNode();
+            node.addChild(child);
+            child.remove();
+            expect(node.children).to.be.an('array').with.lengthOf(0);
+            expect(child.parent).to.equal(null);
+        });
+
+    });
+
     describe('#removeChild()', function () {
 
         it('removes a child node', function () {