Fix MultiReactive system retaining entities multiple times. Closes #818

Author: sschmid

Entitas/Entitas/Systems/MultiReactiveSystem.cs

@@ -12,16 +12,19 @@ public abstract class MultiReactiveSystem<TEntity, TContexts> : IReactiveSystem
         where TContexts : class, IContexts {
 
         readonly ICollector[] _collectors;
+        readonly HashSet<TEntity> _collectedEntities;
         readonly List<TEntity> _buffer;
         string _toStringCache;
 
         protected MultiReactiveSystem(TContexts contexts) {
             _collectors = GetTrigger(contexts);
+            _collectedEntities = new HashSet<TEntity>();
             _buffer = new List<TEntity>();
         }
 
         protected MultiReactiveSystem(ICollector[] collectors) {
             _collectors = collectors;
+            _collectedEntities = new HashSet<TEntity>();
             _buffer = new List<TEntity>();
         }
 
@@ -65,24 +68,26 @@ public void Execute() {
             for (int i = 0; i < _collectors.Length; i++) {
                 var collector = _collectors[i];
                 if (collector.count != 0) {
-                    foreach (var e in collector.GetCollectedEntities<TEntity>()) {
-                        if (Filter(e)) {
-                            e.Retain(this);
-                            _buffer.Add(e);
-                        }
-                    }
-
+                    _collectedEntities.UnionWith(collector.GetCollectedEntities<TEntity>());
                     collector.ClearCollectedEntities();
                 }
             }
 
+            foreach (var e in _collectedEntities) {
+                if (Filter(e)) {
+                    e.Retain(this);
+                    _buffer.Add(e);
+                }
+            }
+
             if (_buffer.Count != 0) {
                 try {
                     Execute(_buffer);
                 } finally {
                     for (int i = 0; i < _buffer.Count; i++) {
                         _buffer[i].Release(this);
                     }
+                    _collectedEntities.Clear();
                     _buffer.Clear();
                 }
             }

Tests/Tests/Fixtures/Systems/MultiTriggeredMultiReactiveSystemSpy.cs

@@ -0,0 +1,42 @@
+using System;
+using System.Collections.Generic;
+using Entitas;
+
+public class MultiTriggeredMultiReactiveSystemSpy : MultiReactiveSystem<IMyEntity, Contexts> {
+
+    public int didExecute { get { return _didExecute; } }
+    public IEntity[] entities { get { return _entities; } }
+
+    public Action<List<IMyEntity>> executeAction;
+
+    protected int _didExecute;
+    protected IEntity[] _entities;
+
+    public MultiTriggeredMultiReactiveSystemSpy(Contexts contexts) : base(contexts) {
+    }
+
+    protected override ICollector[] GetTrigger(Contexts contexts) {
+        return new ICollector[] {
+            contexts.test.CreateCollector(TestMatcher.NameAge),
+            contexts.test.CreateCollector(TestMatcher.NameAge.Removed())
+        };
+    }
+
+    protected override bool Filter(IMyEntity entity) {
+        return true;
+    }
+
+    protected override void Execute(List<IMyEntity> entities) {
+        _didExecute += 1;
+
+        if (entities != null) {
+            _entities = entities.ToArray();
+        } else {
+            _entities = null;
+        }
+
+        if (executeAction != null) {
+            executeAction(entities);
+        }
+    }
+}

Tests/Tests/Tests.csproj

@@ -52,6 +52,7 @@
     <Reference Include="System" />
   </ItemGroup>
   <ItemGroup>
+    <Compile Include="Fixtures\Systems\MultiTriggeredMultiReactiveSystemSpy.cs" />
     <Compile Include="Fixtures\Systems\TestJobSystem.cs" />
     <Compile Include="Fixtures\TestProperties.cs" />
     <Compile Include="Tests\Entitas.Migration\describe_M0450.cs" />

Tests/Tests/Tests/Entitas/describe_MultiReactiveSystem.cs

@@ -4,43 +4,82 @@ class describe_MultiReactiveSystem : nspec {
 
     void when_executing() {
 
-        MultiReactiveSystemSpy system = null;
-        TestEntity e1 = null;
-        Test2Entity e2 = null;
+        context["when triggered"] = () => {
 
-        before = () => {
-            var contexts = new Contexts();
-            system = new MultiReactiveSystemSpy(contexts);
-            system.executeAction = entities => {
-                foreach (var e in entities) {
-                    e.nameAge.age += 10;
-                }
+            MultiReactiveSystemSpy system = null;
+            TestEntity e1 = null;
+            Test2Entity e2 = null;
+
+            before = () => {
+                var contexts = new Contexts();
+                system = new MultiReactiveSystemSpy(contexts);
+                system.executeAction = entities => {
+                    foreach (var e in entities) {
+                        e.nameAge.age += 10;
+                    }
+                };
+
+                e1 = contexts.test.CreateEntity();
+                e1.AddNameAge("Max", 42);
+
+                e2 = contexts.test2.CreateEntity();
+                e2.AddNameAge("Jack", 24);
+
+                system.Execute();
             };
 
-            e1 = contexts.test.CreateEntity();
-            e1.AddNameAge("Max", 42);
+            it["processes entities from different contexts"] = () => {
+                system.entities.Length.should_be(2);
+                system.entities.should_contain(e1);
+                system.entities.should_contain(e2);
 
-            e2 = contexts.test2.CreateEntity();
-            e2.AddNameAge("Jack", 24);
+                e1.nameAge.age.should_be(52);
+                e2.nameAge.age.should_be(34);
+            };
 
-            system.Execute();
-        };
+            it["executes once"] = () => {
+                system.didExecute.should_be(1);
+            };
 
-        it["processes entities from different contexts"] = () => {
-            system.entities.Length.should_be(2);
-            system.entities.should_contain(e1);
-            system.entities.should_contain(e2);
+            it["retains once even when multiple collectors contain entity"] = () => {
+                system.didExecute.should_be(1);
+            };
 
-            e1.nameAge.age.should_be(52);
-            e2.nameAge.age.should_be(34);
+            it["can ToString"] = () => {
+                system.ToString().should_be("MultiReactiveSystem(MultiReactiveSystemSpy)");
+            };
         };
 
-        it["executes once"] = () => {
-            system.didExecute.should_be(1);
-        };
 
-        it["can ToString"] = () => {
-            system.ToString().should_be("MultiReactiveSystem(MultiReactiveSystemSpy)");
+
+        context["when multiple collectors are triggered with same entity"] = () => {
+
+            MultiTriggeredMultiReactiveSystemSpy system = null;
+            TestEntity e1 = null;
+
+            before = () => {
+                var contexts = new Contexts();
+                system = new MultiTriggeredMultiReactiveSystemSpy(contexts);
+                e1 = contexts.test.CreateEntity();
+                e1.AddNameAge("Max", 42);
+
+                e1.RemoveNameAge();
+
+                system.Execute();
+            };
+
+            it["executes once"] = () => {
+                system.didExecute.should_be(1);
+            };
+
+            it["merges collected entities and removes duplicates"] = () => {
+                system.entities.Length.should_be(1);
+            };
+
+            it["clears merged collected entities"] = () => {
+                system.Execute();
+                system.didExecute.should_be(1);
+            };
         };
     }
 }