Merge pull request #42 from thejayman/master

Fixed ComputedGroup refresh firing unecessary remove events
EcsRx · Apr 25, 2024 · 7f6abf4 · 7f6abf4
2 parents 93fcc00 + 41a673b
commit 7f6abf4
Show file tree

Hide file tree

Showing 2 changed files with 69 additions and 1 deletion.
diff --git a/src/EcsRx.Plugins.Computeds/Groups/ComputedGroup.cs b/src/EcsRx.Plugins.Computeds/Groups/ComputedGroup.cs
@@ -71,7 +71,7 @@ public void OnEntityRemovingFromGroup(IEntity entity)
         public void RefreshEntities()
         {
             var applicableEntities = InternalObservableGroup.Where(IsEntityApplicable).ToArray();
-            var entitiesToRemove = InternalObservableGroup.Where(x => applicableEntities.All(y => y.Id != x.Id)).ToArray();
+            var entitiesToRemove = CachedEntities.Where(x => applicableEntities.All(y => y.Id != x.Id)).ToArray();
             var entitiesToAdd = applicableEntities.Where(x => !CachedEntities.Contains(x.Id)).ToArray();
 
             for (var i = entitiesToAdd.Length - 1; i >= 0; i--)

diff --git a/src/EcsRx.Tests/Plugins/Computeds/ComputedGroupTests.cs b/src/EcsRx.Tests/Plugins/Computeds/ComputedGroupTests.cs
@@ -129,5 +129,73 @@ public void should_only_remove_and_fire_events_when_non_applicable_entity_remove
             Assert.Equal(1, removingFiredTimes);
             Assert.Equal(1, removedFiredTimes);
         }
+
+        [Fact]
+        public void should_only_fire_events_from_refresh_when_cache_changed()
+        {
+
+            var applicableEntity = Substitute.For<IEntity>();
+            applicableEntity.Id.Returns(1);
+            applicableEntity.HasComponent<TestComponentOne>().Returns(false);
+
+            var inapplicableEntity = Substitute.For<IEntity>();
+            inapplicableEntity.Id.Returns(2);
+            inapplicableEntity.HasComponent<TestComponentOne>().Returns(false);
+
+            var dummyEntitySnapshot = new List<IEntity> { applicableEntity, inapplicableEntity };
+
+            var mockObservableGroup = Substitute.For<IObservableGroup>();
+            mockObservableGroup.GetEnumerator().Returns(x => dummyEntitySnapshot.GetEnumerator());
+            mockObservableGroup.OnEntityAdded.Returns(Observable.Empty<IEntity>());
+            mockObservableGroup.OnEntityRemoving.Returns(Observable.Empty<IEntity>());
+            mockObservableGroup.OnEntityRemoved.Returns(Observable.Empty<IEntity>());
+
+            var computedGroup = new TestComputedGroup(mockObservableGroup);
+
+            var addedFiredTimes = 0;
+            computedGroup.OnEntityAdded.Subscribe(x =>
+            {
+                addedFiredTimes++;
+            });
+
+            var removingFiredTimes = 0;
+            computedGroup.OnEntityRemoving.Subscribe(x =>
+            {
+                removingFiredTimes++;
+            });
+
+            var removedFiredTimes = 0;
+            computedGroup.OnEntityRemoved.Subscribe(x =>
+            {
+                removedFiredTimes++;
+            });
+
+            //No events should fire
+            computedGroup.RefreshEntities();
+
+            Assert.Equal(0, addedFiredTimes);
+            Assert.Equal(0, removingFiredTimes);
+            Assert.Equal(0, removedFiredTimes);
+
+            applicableEntity.HasComponent<TestComponentOne>().Returns(true);
+
+            //Add should fire only once
+            computedGroup.RefreshEntities();
+
+            Assert.Single(computedGroup.CachedEntities);
+            Assert.Equal(1, addedFiredTimes);
+            Assert.Equal(0, removingFiredTimes);
+            Assert.Equal(0, removedFiredTimes);
+
+            applicableEntity.HasComponent<TestComponentOne>().Returns(false);
+
+            //Remove should fire only once
+            computedGroup.RefreshEntities();
+
+            Assert.Empty(computedGroup.CachedEntities);
+            Assert.Equal(1, addedFiredTimes);
+            Assert.Equal(1, removingFiredTimes);
+            Assert.Equal(1, removedFiredTimes);
+        }
     }
 }