Add/Remove system during iteration

CHANGELOG.md

@@ -1,7 +1,7 @@
 ## Changelog
 
 ### Ashley 1.8.0
-
+* **Bug fix**: Allow add and remove system during iteration, operations are compute at end of update loop Issue #310.
 * **Bug fix**: Poolable Component returned to their ComponentPools even if EntityPool is full. Issue #302.
 * **Update**: Uses libgdx 1.10.0. Commit afa68fc165119a2c79c1709c642e6b620a973ecc.
 

ashley/src/com/badlogic/ashley/core/Engine.java

@@ -139,21 +139,21 @@ public ImmutableArray<Entity> getEntities() {
 	 * the new one will replace the old one.
 	 */
 	public void addSystem(EntitySystem system){
-		systemManager.addSystem(system);
+		systemManager.addSystem(system, updating);
 	}
 
 	/**
 	 * Removes the {@link EntitySystem} from this Engine.
 	 */
 	public void removeSystem(EntitySystem system){
-		systemManager.removeSystem(system);
+		systemManager.removeSystem(system, updating);
 	}
 
 	/**
 	 * Removes all systems from this Engine.
 	 */
 	public void removeAllSystems(){
-		systemManager.removeAllSystems();
+		systemManager.removeAllSystems(updating);
 	}
 
 	/**
@@ -248,6 +248,7 @@ public void update(float deltaTime){
 		}
 		finally {
 			updating = false;
+			systemManager.processPendingOperations();
 		}	
 	}
 

ashley/src/com/badlogic/ashley/core/SystemManager.java

@@ -1,69 +1,125 @@
 package com.badlogic.ashley.core;
 
-import java.util.Comparator;
-
-import com.badlogic.ashley.signals.Listener;
-import com.badlogic.ashley.signals.Signal;
 import com.badlogic.ashley.utils.ImmutableArray;
 import com.badlogic.gdx.utils.Array;
 import com.badlogic.gdx.utils.ObjectMap;
+import com.badlogic.gdx.utils.Pool;
+
+import java.util.Comparator;
 
 class SystemManager {
-	private SystemComparator systemComparator = new SystemComparator();
-	private Array<EntitySystem> systems = new Array<EntitySystem>(true, 16);
-	private ImmutableArray<EntitySystem> immutableSystems = new ImmutableArray<EntitySystem>(systems);
-	private ObjectMap<Class<?>, EntitySystem> systemsByClass = new ObjectMap<Class<?>, EntitySystem>();
-	private SystemListener listener;
-
-	public SystemManager(SystemListener listener) {
-		this.listener = listener;
-	}
-
-	public void addSystem(EntitySystem system){
-		Class<? extends EntitySystem> systemType = system.getClass();		
-		EntitySystem oldSystem = getSystem(systemType);
-
-		if (oldSystem != null) {
-			removeSystem(oldSystem);
-		}
-
-		systems.add(system);
-		systemsByClass.put(systemType, system);		
-		systems.sort(systemComparator);
-		listener.systemAdded(system);
-	}
-
-	public void removeSystem(EntitySystem system){
-		if(systems.removeValue(system, true)) {
-			systemsByClass.remove(system.getClass());
-			listener.systemRemoved(system);
-		}
-	}
-
-	public void removeAllSystems() {
-		while(systems.size > 0) {
-			removeSystem(systems.first());
-		}
-	}
-
-	@SuppressWarnings("unchecked")
-	public <T extends EntitySystem> T getSystem(Class<T> systemType) {
-		return (T) systemsByClass.get(systemType);
-	}
-
-	public ImmutableArray<EntitySystem> getSystems() {
-		return immutableSystems;
-	}
-
-	private static class SystemComparator implements Comparator<EntitySystem>{
-		@Override
-		public int compare(EntitySystem a, EntitySystem b) {
-			return a.priority > b.priority ? 1 : (a.priority == b.priority) ? 0 : -1;
-		}
-	}
-
-	interface SystemListener {
-		void systemAdded(EntitySystem system);
-		void systemRemoved(EntitySystem system);
-	}
+    private SystemComparator systemComparator = new SystemComparator();
+    private Array<EntitySystem> systems = new Array<EntitySystem>(true, 16);
+    private ImmutableArray<EntitySystem> immutableSystems = new ImmutableArray<EntitySystem>(systems);
+    private ObjectMap<Class<?>, EntitySystem> systemsByClass = new ObjectMap<Class<?>, EntitySystem>();
+    private SystemListener listener;
+
+    private Array<SystemOperation> pendingOperations = new Array<>();
+    private SystemManager.SystemOperationPool systemOperationPool = new SystemManager.SystemOperationPool();
+
+    public SystemManager(SystemListener listener) {
+        this.listener = listener;
+    }
+
+    public void addSystem(EntitySystem system, boolean delayed) {
+        if (delayed) {
+            SystemOperation operation = systemOperationPool.obtain();
+            operation.type = SystemOperation.Type.Add;
+            operation.system = system;
+            pendingOperations.add(operation);
+        } else {
+            Class<? extends EntitySystem> systemType = system.getClass();
+            EntitySystem oldSystem = getSystem(systemType);
+
+            if (oldSystem != null) {
+                removeSystem(oldSystem, delayed);
+            }
+
+            systems.add(system);
+            systemsByClass.put(systemType, system);
+            systems.sort(systemComparator);
+            listener.systemAdded(system);
+        }
+    }
+
+    public void removeSystem(EntitySystem system, boolean delayed) {
+        if (delayed) {
+            SystemOperation operation = systemOperationPool.obtain();
+            operation.type = SystemOperation.Type.Remove;
+            operation.system = system;
+        } else {
+            if (systems.removeValue(system, true)) {
+                systemsByClass.remove(system.getClass());
+                listener.systemRemoved(system);
+            }
+        }
+    }
+
+    public void removeAllSystems(boolean delayed) {
+        while (systems.size > 0) {
+            removeSystem(systems.first(), delayed);
+        }
+    }
+
+    public void processPendingOperations() {
+        for (int i = 0; i < pendingOperations.size; ++i) {
+            SystemOperation operation = pendingOperations.get(i);
+            switch(operation.type) {
+                case Add:
+                    addSystem(operation.system, false);
+                    break;
+                case Remove:
+                    removeSystem(operation.system, false);
+                    break;
+                default:
+                    throw new AssertionError("Unexpected EntityOperation type");
+            }
+            systemOperationPool.free(operation);
+        }
+        pendingOperations.clear();
+    }
+
+    @SuppressWarnings("unchecked")
+    public <T extends EntitySystem> T getSystem(Class<T> systemType) {
+        return (T) systemsByClass.get(systemType);
+    }
+
+    public ImmutableArray<EntitySystem> getSystems() {
+        return immutableSystems;
+    }
+
+    private static class SystemComparator implements Comparator<EntitySystem> {
+        @Override
+        public int compare(EntitySystem a, EntitySystem b) {
+            return a.priority > b.priority ? 1 : (a.priority == b.priority) ? 0 : -1;
+        }
+    }
+
+    interface SystemListener {
+        void systemAdded(EntitySystem system);
+
+        void systemRemoved(EntitySystem system);
+    }
+
+
+    private static class SystemOperation implements Pool.Poolable {
+        public enum Type {
+            Add, Remove;
+        }
+
+        public Type type;
+        public EntitySystem system;
+
+        @Override
+        public void reset() {
+            system = null;
+        }
+    }
+
+    private static class SystemOperationPool extends Pool<SystemManager.SystemOperation> {
+        @Override
+        protected SystemManager.SystemOperation newObject() {
+            return new SystemManager.SystemOperation();
+        }
+    }
 }

ashley/tests/com/badlogic/ashley/core/AddRemoveSystemDuringIterationTest.java

@@ -0,0 +1,112 @@
+package com.badlogic.ashley.core;
+
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+
+/**
+ * 3 systems add at engine start with priority 1, 5, 10
+ * System 5 add system with priority 4 during first iteration by system 5
+ *
+ */
+public class AddRemoveSystemDuringIterationTest {
+    Engine engine;
+    List<Integer> systemCallOrder; //updateIndex -> list by priority
+    boolean firstUpdateDone = false;
+
+    private void addSystemUpdateCall(EntitySystem entitySystem) {
+        systemCallOrder.add(entitySystem.priority);
+    }
+
+    @Before
+    public void setUp() {
+        engine = new Engine();
+        systemCallOrder = new ArrayList<>();
+        firstUpdateDone = false;
+    }
+
+    @Test
+    public void addSystemDuringIterationTest() {
+        EntitySystem system1 = new NamedSystem(1) {
+        };
+        EntitySystem system10 = new NamedSystem(10) {
+        };
+        final EntitySystem system4 = new NamedSystem(4) {  //System added during update by system 5
+        };
+        EntitySystem system5 = new NamedSystem(5) {
+
+            @Override
+            public void update(float deltaTime) {
+                super.update(deltaTime);
+                if (!firstUpdateDone) {
+                    getEngine().addSystem(system4);
+                    firstUpdateDone = true;
+                }
+            }
+        };
+
+        engine.addSystem(system1);
+        engine.addSystem(system10);
+        engine.addSystem(system5);
+
+        engine.update(1); //system 5 add system 4 during iteration
+
+        List<Integer> expected = Arrays.asList(1, 5, 10); //system 4 should not be called
+        Assert.assertArrayEquals(expected.toArray(), systemCallOrder.toArray());
+
+        systemCallOrder.clear();
+        engine.update(1);
+        expected = Arrays.asList(1, 4, 5, 10); //second update ok
+        Assert.assertArrayEquals(expected.toArray(), systemCallOrder.toArray());
+    }
+    @Test
+    public void removeSystemDuringIterationTest() {
+        engine = new Engine();
+        EntitySystem system1 = new NamedSystem(1) {
+        };
+        EntitySystem system10 = new NamedSystem(10) {
+        };
+        final EntitySystem system8 = new NamedSystem(8) {  //System remove during update by system 5
+        };
+        EntitySystem system5 = new NamedSystem(5) {
+
+            @Override
+            public void update(float deltaTime) {
+                super.update(deltaTime);
+                if (!firstUpdateDone) {
+                    getEngine().removeSystem(system8);
+                    firstUpdateDone = true;
+                }
+            }
+        };
+
+        engine.addSystem(system1);
+        engine.addSystem(system10);
+        engine.addSystem(system5);
+
+        engine.update(1); //system 5 remove system 8 during iteration
+
+        List<Integer> expected = Arrays.asList(1, 5, 10); //system 8 should not be called
+        Assert.assertArrayEquals(expected.toArray(), systemCallOrder.toArray());
+
+        systemCallOrder.clear();
+        engine.update(1);
+        expected = Arrays.asList(1, 5, 10); //second update ok
+        Assert.assertArrayEquals(expected.toArray(), systemCallOrder.toArray());
+    }
+    private class NamedSystem extends EntitySystem {
+
+        public NamedSystem(int priority) {
+            super(priority);
+        }
+
+        @Override
+        public void update(float deltaTime) {
+            addSystemUpdateCall(this);
+        }
+    }
+}