From fc3ab68ecd572cc3ee43f3f2e1dba61366822a76 Mon Sep 17 00:00:00 2001
From: Erik Onarheim <erik.onarheim@gmail.com>
Date: Mon, 27 May 2024 23:41:05 -0500
Subject: [PATCH] fix: Issue where removing and re-adding an actor would cause
 subsequent children to not function properly

---
 CHANGELOG.md                                  |  1 +
 .../EntityComponentSystem/EntityManager.ts    | 36 ++++++++++++-------
 src/spec/EntityManagerSpec.ts                 | 17 +++++++++
 src/spec/TransformComponentSpec.ts            | 20 +++++++++++
 4 files changed, 62 insertions(+), 12 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index c5fef7156..f4edc6d15 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -25,6 +25,7 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 
 - Fixed issue where `ex.SpriteFont` did not respect scale when measuring text
 - Fixed issue where negative transforms would cause collision issues because polygon winding would change.
+- Fixed issue where removing and re-adding an actor would cause subsequent children added not to function properly with regards to their parent/child transforms
 
 ### Updates
 
diff --git a/src/engine/EntityComponentSystem/EntityManager.ts b/src/engine/EntityComponentSystem/EntityManager.ts
index 0371cc93d..f07e4315a 100644
--- a/src/engine/EntityComponentSystem/EntityManager.ts
+++ b/src/engine/EntityComponentSystem/EntityManager.ts
@@ -8,6 +8,8 @@ import { Scene } from '../Scene';
 export class EntityManager {
   public entities: Entity[] = [];
   public _entityIndex: { [entityId: string]: Entity } = {};
+  private _childAddedHandlerMap = new Map<Entity, (entity: Entity) => void>();
+  private _childRemovedHandlerMap = new Map<Entity, (entity: Entity) => void>();
 
   constructor(private _world: World) {}
 
@@ -33,6 +35,14 @@ export class EntityManager {
     }
   }
 
+  private _createChildAddedHandler = () => (e: Entity) => {
+    this.addEntity(e);
+  };
+
+  private _createChildRemovedHandler = () => (e: Entity) => {
+    this.removeEntity(e, false);
+  };
+
   /**
    * Adds an entity to be tracked by the EntityManager
    * @param entity
@@ -50,16 +60,12 @@ export class EntityManager {
         c.scene = entity.scene;
         this.addEntity(c);
       });
-      entity.childrenAdded$.register({
-        notify: (e) => {
-          this.addEntity(e);
-        }
-      });
-      entity.childrenRemoved$.register({
-        notify: (e) => {
-          this.removeEntity(e, false);
-        }
-      });
+      const childAdded = this._createChildAddedHandler();
+      this._childAddedHandlerMap.set(entity, childAdded);
+      const childRemoved = this._createChildRemovedHandler();
+      this._childRemovedHandlerMap.set(entity, childRemoved);
+      entity.childrenAdded$.subscribe(childAdded);
+      entity.childrenRemoved$.subscribe(childRemoved);
     }
   }
 
@@ -93,8 +99,14 @@ export class EntityManager {
         c.scene = null;
         this.removeEntity(c, deferred);
       });
-      entity.childrenAdded$.clear();
-      entity.childrenRemoved$.clear();
+      const childAddedHandler = this._childAddedHandlerMap.get(entity);
+      if (childAddedHandler) {
+        entity.childrenAdded$.unsubscribe(childAddedHandler);
+      }
+      const childRemovedHandler = this._childRemovedHandlerMap.get(entity);
+      if (childRemovedHandler) {
+        entity.childrenRemoved$.unsubscribe(childRemovedHandler);
+      }
 
       // stats
       if (this._world?.scene?.engine) {
diff --git a/src/spec/EntityManagerSpec.ts b/src/spec/EntityManagerSpec.ts
index 1a6773005..e57446db1 100644
--- a/src/spec/EntityManagerSpec.ts
+++ b/src/spec/EntityManagerSpec.ts
@@ -64,4 +64,21 @@ describe('An EntityManager', () => {
     expect(entityManager.entities.length).toBe(0);
     expect((entityManager as any)._entitiesToRemove.length).toBe(0);
   });
+
+  it('should only remove the handlers that the manager added on removal', () => {
+    const entityManager = new ex.EntityManager(new ex.World(null));
+    const entity = new ex.Entity([new ex.TransformComponent()], 'some-e');
+    // Transform component listens to child add/remove
+    const entity2 = new ex.Entity();
+    entityManager.addEntity(entity);
+    entityManager.addEntity(entity2);
+
+    expect(entity.childrenAdded$.subscriptions.length).toBe(2);
+    expect(entity2.childrenAdded$.subscriptions.length).toBe(1);
+
+    entityManager.removeEntity(entity, false);
+    entityManager.removeEntity(entity2, false);
+    expect(entity.childrenAdded$.subscriptions.length).toBe(1);
+    expect(entity2.childrenAdded$.subscriptions.length).toBe(0);
+  });
 });
diff --git a/src/spec/TransformComponentSpec.ts b/src/spec/TransformComponentSpec.ts
index 2ddbff4f2..0328b8f44 100644
--- a/src/spec/TransformComponentSpec.ts
+++ b/src/spec/TransformComponentSpec.ts
@@ -287,6 +287,26 @@ describe('A TransformComponent', () => {
     expect(child2.get(TransformComponent).get().parent).toBe(null);
   });
 
+  it('can be parented to a previously deleted entity with transform component', () => {
+    const entityManager = new ex.EntityManager(new ex.World(null));
+
+    const child1 = new ex.Entity([new TransformComponent()]);
+    const child2 = new ex.Entity([new TransformComponent()]);
+    const parent = new ex.Entity([new TransformComponent()]);
+
+    parent.get(TransformComponent).pos = ex.vec(100, 500);
+    parent.addChild(child1);
+
+    entityManager.addEntity(parent);
+    entityManager.removeEntity(parent, false);
+
+    entityManager.addEntity(parent);
+    parent.addChild(child2);
+
+    expect(child1.get(TransformComponent).globalPos).toBeVector(ex.vec(100, 500));
+    expect(child2.get(TransformComponent).globalPos).toBeVector(ex.vec(100, 500));
+  });
+
   it('children inherit the top most parent coordinate plane', () => {
     const logger = ex.Logger.getInstance();
     spyOn(logger, 'warn');