Repository: playcanvas/engine (js)

Issue:
Allow entity components enabling order on hierarchy change
When working with custom components (e.g. added via code), we need some components to be enabled before others. At the moment the components are called in random order:

https://github.com/playcanvas/engine/blob/55a3ff8845f0cf30829aeec515f24ac98b6baf4d/src/framework/entity.js#L503-L520

For example, a body component and a constraint component for physics. The constraint component needs a body to be attached to. If the body component is enabled later, it will be added to the physics after the constraint, which will fail creating the constraint as it will fail to locate the body.

There is no elegant way of doing it today. We have to introduce a component dependency into another component that actually doesn't need to be aware if other components and their state. We also have to add some sort of systems to queue up the creation commands to wait for the other component to be enabled, which is pretty brittle.

Pull request description:
Allow components ordering
Fixes #6647 

The PR allows components to specify in which order they get enabled when multiple components are present on an Entity.

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).


Patch:
diff --git a/src/core/sort.js b/src/core/sort.js
index 062667ced..627891ff0 100644
--- a/src/core/sort.js
+++ b/src/core/sort.js
@@ -6,6 +6,15 @@
  */
 const cmpPriority = (a, b) => a.priority - b.priority;
 
+
+/**
+ * @param {{order: number}} a - First object with `order` property.
+ * @param {{order: number}} b - Second object with `order` property.
+ * @returns {number} A number indicating the relative position.
+ * @ignore
+ */
+const cmpStaticOrder = (a, b) => a.constructor.order - b.constructor.order;
+
 /**
  * @param {Array<{priority: number}>} arr - Array to be sorted in place where each element contains
  * an object with at least a priority property.
@@ -13,3 +22,11 @@ const cmpPriority = (a, b) => a.priority - b.priority;
  * @ignore
  */
 export const sortPriority = arr => arr.sort(cmpPriority);
+
+/**
+ * @param {Array<{order: number}>} arr - Array to be sorted in place where each element contains
+ * an object with a static `order` property.
+ * @returns {Array<{order: number}>} In place sorted array.
+ * @ignore
+ */
+export const sortStaticOrder = arr => arr.sort(cmpStaticOrder);
diff --git a/src/framework/components/component.js b/src/framework/components/component.js
index a034fa226..062c4e5ba 100644
--- a/src/framework/components/component.js
+++ b/src/framework/components/component.js
@@ -5,6 +5,15 @@ import { EventHandler } from '../../core/event-handler.js';
  * events each frame, and expose properties to the PlayCanvas Editor.
  */
 class Component extends EventHandler {
+    /**
+     * Component order. When an entity with multiple components gets enabled, this order specifies
+     * in which order the components get enabled. The lowest number gets enabled first.
+     *
+     * @type {number} - Component order number.
+     * @private
+     */
+    static order = 0;
+
     /**
      * The ComponentSystem used to create this Component.
      *
diff --git a/src/framework/components/rigid-body/component.js b/src/framework/components/rigid-body/component.js
index 226cc5a5a..2d8d9d579 100644
--- a/src/framework/components/rigid-body/component.js
+++ b/src/framework/components/rigid-body/component.js
@@ -114,6 +114,8 @@ class RigidBodyComponent extends Component {
      */
     static EVENT_TRIGGERLEAVE = 'triggerleave';
 
+    static order = -1;
+
     /** @private */
     _angularDamping = 0;
 
diff --git a/src/framework/components/system.js b/src/framework/components/system.js
index 506d68032..07679e438 100644
--- a/src/framework/components/system.js
+++ b/src/framework/components/system.js
@@ -67,15 +67,16 @@ class ComponentSystem extends EventHandler {
      * @ignore
      */
     removeComponent(entity) {
+        const id = this.id;
         const record = this.store[entity.getGuid()];
-        const component = entity.c[this.id];
+        const component = entity.c[id];
 
         this.fire('beforeremove', entity, component);
 
         delete this.store[entity.getGuid()];
 
-        entity[this.id] = undefined;
-        delete entity.c[this.id];
+        entity[id] = undefined;
+        delete entity.c[id];
 
         this.fire('remove', entity, record.data);
     }
diff --git a/src/framework/entity.js b/src/framework/entity.js
index de2ad5d2e..059b1302c 100644
--- a/src/framework/entity.js
+++ b/src/framework/entity.js
@@ -1,5 +1,6 @@
 import { Debug } from '../core/debug.js';
 import { guid } from '../core/guid.js';
+import { sortStaticOrder } from '../core/sort.js';
 
 import { GraphNode } from '../scene/graph-node.js';
 
@@ -11,6 +12,12 @@ import { getApplication } from './globals.js';
  */
 const _enableList = [];
 
+/**
+ * @type {Array<import('./components/component.js').Component>}
+ * @ignore
+ */
+const _sortedArray = [];
+
 /**
  * The Entity is the core primitive of a PlayCanvas game. Generally speaking an object in your game
  * will consist of an {@link Entity}, and a set of {@link Component}s which are managed by their
@@ -503,17 +510,14 @@ class Entity extends GraphNode {
     _onHierarchyStateChanged(enabled) {
         super._onHierarchyStateChanged(enabled);
 
-        // enable / disable all the components
-        const components = this.c;
-        for (const type in components) {
-            if (components.hasOwnProperty(type)) {
-                const component = components[type];
-                if (component.enabled) {
-                    if (enabled) {
-                        component.onEnable();
-                    } else {
-                        component.onDisable();
-                    }
+        const components = this._getSortedComponents();
+        for (let i = 0; i < components.length; i++) {
+            const component = components[i];
+            if (component.enabled) {
+                if (enabled) {
+                    component.onEnable();
+                } else {
+                    component.onDisable();
                 }
             }
         }
@@ -522,10 +526,9 @@ class Entity extends GraphNode {
     /** @private */
     _onHierarchyStatePostChanged() {
         // post enable all the components
-        const components = this.c;
-        for (const type in components) {
-            if (components.hasOwnProperty(type))
-                components[type].onPostStateChange();
+        const components = this._getSortedComponents();
+        for (let i = 0; i < components.length; i++) {
+            components[i].onPostStateChange();
         }
     }
 
@@ -601,6 +604,26 @@ class Entity extends GraphNode {
         return clone;
     }
 
+    _getSortedComponents() {
+        _sortedArray.length = 0;
+
+        const components = this.c;
+        let needSort = 0;
+        for (const type in components) {
+            if (components.hasOwnProperty(type)) {
+                const component = components[type];
+                needSort |= component.constructor.order !== 0;
+                _sortedArray.push(component);
+            }
+        }
+
+        if (needSort && _sortedArray.length > 1) {
+            sortStaticOrder(_sortedArray);
+        }
+
+        return _sortedArray;
+    }
+
     /**
      * @param {Object<string, Entity>} duplicatedIdsMap - A map of original entity GUIDs to cloned
      * entities.


Test patch:
diff --git a/test/framework/entity.test.mjs b/test/framework/entity.test.mjs
index 51dd33c1e..1cd638507 100644
--- a/test/framework/entity.test.mjs
+++ b/test/framework/entity.test.mjs
@@ -32,6 +32,7 @@ import { DummyComponentSystem } from './test-component/system.mjs';
 import { HTMLCanvasElement } from '@playcanvas/canvas-mock';
 
 import { expect } from 'chai';
+import { stub } from 'sinon';
 
 describe('Entity', function () {
 
@@ -121,6 +122,68 @@ describe('Entity', function () {
             });
         }
 
+        it('respects components order on disable', function () {
+            const entity = new Entity();
+            entity.enabled = true;
+
+            entity.addComponent('collision');
+            entity.addComponent('rigidbody');
+
+            const colOnDisable = stub();
+            const rbOnDisable = stub();
+            let disableOrder = 0;
+
+            entity.collision.onDisable = colOnDisable;
+            entity.rigidbody.onDisable = rbOnDisable;
+
+            colOnDisable.onFirstCall().callsFake(() => {
+                disableOrder = 2;
+            });
+            rbOnDisable.onFirstCall().callsFake(() => {
+                disableOrder = 1;
+            });
+
+            entity.enabled = false;
+
+            expect(disableOrder).to.equal(2);
+
+            entity.destroy();
+        });
+
+        it('respects components order on enable', function () {
+            const entity = new Entity('Child');
+            const parent = new Entity('Parent');
+
+            parent.addChild(entity);
+            parent._enabled = true;
+            parent._enabledInHierarchy = true;
+
+            entity.addComponent('collision');
+            entity.addComponent('rigidbody');
+
+            entity.enabled = false;
+
+            const rbOnEnable = stub();
+            const colOnEnable = stub();
+            let enableOrder = 0;
+
+            entity.collision.onEnable = colOnEnable;
+            entity.rigidbody.onEnable = rbOnEnable;
+
+            colOnEnable.onFirstCall().callsFake(() => {
+                enableOrder = 2;
+            });
+            rbOnEnable.onFirstCall().callsFake(() => {
+                enableOrder = 1;
+            });
+
+            entity.enabled = true;
+
+            expect(enableOrder).to.equal(2);
+
+            parent.destroy();
+        });
+
     });
 
     const createSubtree = () => {