From 25789e7c9d040279864696b59c905dbb44b9ed02 Mon Sep 17 00:00:00 2001 From: hypercross Date: Sat, 18 Jul 2026 19:58:23 +0800 Subject: [PATCH] fix: improve stability and add edge case tests - Fix SparseSet resize logic to correctly initialize new slots - Implement Equals and GetHashCode for QueryDescriptor - Prevent collection modification exceptions in World.DestroyEntity - Update CommandQueue to accumulate errors across drain cycles - Add comprehensive edge case tests in EdgeCaseTests.cs --- src/OECS/CommandQueue.cs | 13 +- src/OECS/QueryDescriptor.cs | 18 ++ src/OECS/SparseSet.cs | 3 +- src/OECS/World.cs | 16 +- tests/OECS.Tests/EdgeCaseTests.cs | 473 ++++++++++++++++++++++++++++++ 5 files changed, 517 insertions(+), 6 deletions(-) create mode 100644 tests/OECS.Tests/EdgeCaseTests.cs diff --git a/src/OECS/CommandQueue.cs b/src/OECS/CommandQueue.cs index 09d2a8f..8dedcff 100644 --- a/src/OECS/CommandQueue.cs +++ b/src/OECS/CommandQueue.cs @@ -21,8 +21,8 @@ public class CommandQueue public int Count => _commands.Count; /// - /// Exceptions thrown by commands during the most recent call. - /// Cleared at the start of each drain cycle. + /// Exceptions thrown by commands during calls. + /// Accumulates across drain cycles. Call to reset. /// public IReadOnlyList Errors => _errors; @@ -42,8 +42,6 @@ public class CommandQueue /// public void ExecuteAll(World world) { - _errors.Clear(); - int index = 0; while (index < _commands.Count) { @@ -69,6 +67,13 @@ public class CommandQueue public void Clear() { _commands.Clear(); + } + + /// + /// Clears accumulated error history. + /// + public void ClearErrors() + { _errors.Clear(); } } diff --git a/src/OECS/QueryDescriptor.cs b/src/OECS/QueryDescriptor.cs index 5e98fff..489beb1 100644 --- a/src/OECS/QueryDescriptor.cs +++ b/src/OECS/QueryDescriptor.cs @@ -27,4 +27,22 @@ public class QueryDescriptor /// Returns true if this query has no "with" components (matches nothing). /// internal bool IsEmpty => With.Count == 0; + + public override bool Equals(object? obj) + { + if (obj is not QueryDescriptor other) return false; + if (With.Count != other.With.Count || Without.Count != other.Without.Count) + return false; + return With.SetEquals(other.With) && Without.SetEquals(other.Without); + } + + public override int GetHashCode() + { + var hash = new HashCode(); + foreach (var t in With.OrderBy(t => t.FullName)) + hash.Add(t); + foreach (var t in Without.OrderBy(t => t.FullName)) + hash.Add(t); + return hash.ToHashCode(); + } } \ No newline at end of file diff --git a/src/OECS/SparseSet.cs b/src/OECS/SparseSet.cs index c351091..ae27835 100644 --- a/src/OECS/SparseSet.cs +++ b/src/OECS/SparseSet.cs @@ -140,9 +140,10 @@ internal class SparseSet : ISparseSet where T : struct { if (entityId >= (uint)_sparse.Length) { + int oldSize = _sparse.Length; int newSize = Math.Max((int)entityId + 1, _sparse.Length * 2); Array.Resize(ref _sparse, newSize); - Array.Fill(_sparse, -1, _sparse.Length - (newSize - _sparse.Length), newSize - _sparse.Length); + Array.Fill(_sparse, -1, oldSize, newSize - oldSize); } } diff --git a/src/OECS/World.cs b/src/OECS/World.cs index ba0d2de..a4fb1d0 100644 --- a/src/OECS/World.cs +++ b/src/OECS/World.cs @@ -71,7 +71,12 @@ public class World : IDisposable return; // Remove all relationships where this entity is the target. - foreach (var (relType, sources) in _relationships.GetIncomingRelationships(entity)) + // Materialize to avoid collection-modified-during-enumeration when + // OnRemoved mutates the same HashSet we're iterating. + var incoming = _relationships.GetIncomingRelationships(entity) + .Select(r => (r.RelationshipType, r.Sources.ToList())) + .ToList(); + foreach (var (relType, sources) in incoming) { foreach (var source in sources) { @@ -83,6 +88,15 @@ public class World : IDisposable // Remove all relationships where this entity is the source. _relationships.RemoveAllSourcesForEntity(entity); + // Mark component removals for each component type before removing them. + foreach (var componentType in _components.ComponentTypes.ToList()) + { + if (_components.GetSet(componentType)?.Contains(entity) == true) + { + _changes.Pending.MarkComponentRemoved(entity, componentType); + } + } + _components.RemoveAll(entity); _allocator.Free(entity); diff --git a/tests/OECS.Tests/EdgeCaseTests.cs b/tests/OECS.Tests/EdgeCaseTests.cs new file mode 100644 index 0000000..fe1119a --- /dev/null +++ b/tests/OECS.Tests/EdgeCaseTests.cs @@ -0,0 +1,473 @@ +using FluentAssertions; +using R3; +using Xunit; + +namespace OECS.Tests; + +/// +/// Tests for edge cases and previously uncovered code paths. +/// +public class EdgeCaseTests +{ + // ── Test components ────────────────────────────────────────────── + + private struct Position { public float X; public float Y; } + private struct Velocity { public float X; public float Y; } + private struct Health { public int Value; } + private struct Frozen { } + private struct Burning { } + private struct Poisoned { } + private struct Cursed { } + + // ── SparseSet resize correctness ───────────────────────────────── + + [Fact] + public void SparseSet_Resize_InitializesNewSlotsToMinusOne() + { + // This test verifies that when the sparse array grows, new slots + // are filled with -1 (not left as default 0, which would falsely + // indicate the entity has a component). + var world = new World(); + + // Create many entities to force sparse array resizes. + var entities = new List(); + for (int i = 0; i < 2000; i++) + { + entities.Add(world.CreateEntity()); + } + + // Add Position to every other entity. + for (int i = 0; i < entities.Count; i += 2) + { + world.AddComponent(entities[i], new Position { X = i, Y = i }); + } + + // Entities without Position should correctly report false. + for (int i = 1; i < entities.Count; i += 2) + { + world.HasComponent(entities[i]).Should().BeFalse( + $"entity {entities[i]} should not have Position"); + } + } + + [Fact] + public void SparseSet_Resize_PreservesExistingData() + { + var world = new World(); + + // Create entities before any component adds to trigger resizes + // after data is already present. + var first = world.CreateEntity(); + world.AddComponent(first, new Position { X = 1, Y = 2 }); + + // Create many more to force resize. + for (int i = 0; i < 2000; i++) + { + var e = world.CreateEntity(); + world.AddComponent(e, new Position { X = i, Y = i }); + } + + // Original entity should still have its data. + ref var pos = ref world.GetComponent(first); + pos.X.Should().Be(1); + pos.Y.Should().Be(2); + } + + // ── Entity version wrapping ────────────────────────────────────── + + [Fact] + public void Entity_VersionWraps_From255To1() + { + var world = new World(); + var original = world.CreateEntity(); + uint id = original.Id; + + // Destroy and recreate the entity 255 times to wrap the version. + for (int i = 0; i < 255; i++) + { + world.DestroyEntity(original); + original = world.CreateEntity(); + original.Id.Should().Be(id); + } + + // After 255 recycles, version should have wrapped from 255 to 1. + original.Version.Should().Be(1); + world.IsAlive(original).Should().BeTrue(); + } + + // ── DestroyEntity with multiple incoming relationships ─────────── + + [Fact] + public void DestroyEntity_WithMultipleIncomingRelationships_DoesNotThrow() + { + var world = new World(); + var target = world.CreateEntity(); + + // Create multiple sources pointing to the same target. + var sources = new List(); + for (int i = 0; i < 10; i++) + { + var source = world.CreateEntity(); + sources.Add(source); + world.AddComponent(source, new Relationship + { + Source = source, + Target = target + }); + } + + // Destroying the target should cascade-remove all incoming + // relationships without throwing. + var act = () => world.DestroyEntity(target); + act.Should().NotThrow(); + + // All source entities should have had their relationship removed. + foreach (var source in sources) + { + world.HasComponent>(source).Should().BeFalse(); + } + } + + // ── Component removal change tracking on destroy ───────────────── + + [Fact] + public void DestroyEntity_PostsComponentRemoved_ForEachComponent() + { + var world = new World(); + var entity = world.CreateEntity(); + world.AddComponent(entity, new Position { X = 1, Y = 2 }); + world.AddComponent(entity, new Health { Value = 100 }); + world.PostChanges(); // Clear pending + + var collector = new ChangeCollector(); + using var sub = world.ObserveEntityChanges().Subscribe(collector.ToObserver()); + + world.DestroyEntity(entity); + world.PostChanges(); + + // Should see: ComponentRemoved(Position), ComponentRemoved(Health), EntityRemoved + collector.Changes.Should().Contain(c => + c.Kind == ChangeKind.ComponentRemoved && c.ComponentType == typeof(Position)); + collector.Changes.Should().Contain(c => + c.Kind == ChangeKind.ComponentRemoved && c.ComponentType == typeof(Health)); + collector.Changes.Should().Contain(c => + c.Kind == ChangeKind.EntityRemoved && c.Entity == entity); + } + + // ── QueryDescriptor equality ───────────────────────────────────── + + [Fact] + public void QueryDescriptor_EqualQueries_AreEqual() + { + var q1 = new QueryBuilder().With().With().Without().Build(); + var q2 = new QueryBuilder().With().With().Without().Build(); + + q1.Should().Be(q2); + q1.GetHashCode().Should().Be(q2.GetHashCode()); + } + + [Fact] + public void QueryDescriptor_DifferentWith_AreNotEqual() + { + var q1 = new QueryBuilder().With().Build(); + var q2 = new QueryBuilder().With().Build(); + + q1.Should().NotBe(q2); + } + + [Fact] + public void QueryDescriptor_DifferentWithout_AreNotEqual() + { + var q1 = new QueryBuilder().With().Without().Build(); + var q2 = new QueryBuilder().With().Without().Build(); + + q1.Should().NotBe(q2); + } + + [Fact] + public void QueryDescriptor_DifferentCount_AreNotEqual() + { + var q1 = new QueryBuilder().With().Build(); + var q2 = new QueryBuilder().With().With().Build(); + + q1.Should().NotBe(q2); + } + + // ── Query with 4, 5, 6 components ──────────────────────────────── + + [Fact] + public void Query_FourComponents_Works() + { + var world = new World(); + var entity = world.CreateEntity(); + world.AddComponent(entity, new Position { X = 1, Y = 2 }); + world.AddComponent(entity, new Velocity { X = 3, Y = 4 }); + world.AddComponent(entity, new Health { Value = 100 }); + world.AddComponent(entity, new Frozen()); + + var query = world.Query() + .With().With().With().With() + .Build(); + + var count = 0; + world.ForEach(query, (Entity e, ref Position pos, ref Velocity vel, ref Health hp, ref Frozen f) => + { + count++; + pos.X.Should().Be(1); + vel.Y.Should().Be(4); + hp.Value.Should().Be(100); + }); + count.Should().Be(1); + } + + [Fact] + public void Query_FiveComponents_Works() + { + var world = new World(); + var entity = world.CreateEntity(); + world.AddComponent(entity, new Position { X = 1, Y = 2 }); + world.AddComponent(entity, new Velocity { X = 3, Y = 4 }); + world.AddComponent(entity, new Health { Value = 100 }); + world.AddComponent(entity, new Frozen()); + world.AddComponent(entity, new Burning()); + + var query = world.Query() + .With().With().With().With().With() + .Build(); + + var count = 0; + world.ForEach(query, (Entity e, ref Position pos, ref Velocity vel, ref Health hp, ref Frozen f, ref Burning b) => + { + count++; + }); + count.Should().Be(1); + } + + [Fact] + public void Query_SixComponents_Works() + { + var world = new World(); + var entity = world.CreateEntity(); + world.AddComponent(entity, new Position { X = 1, Y = 2 }); + world.AddComponent(entity, new Velocity { X = 3, Y = 4 }); + world.AddComponent(entity, new Health { Value = 100 }); + world.AddComponent(entity, new Frozen()); + world.AddComponent(entity, new Burning()); + world.AddComponent(entity, new Poisoned()); + + var query = world.Query() + .With().With().With() + .With().With().With() + .Build(); + + var count = 0; + world.ForEach(query, (Entity e, ref Position pos, ref Velocity vel, ref Health hp, + ref Frozen f, ref Burning b, ref Poisoned p) => + { + count++; + }); + count.Should().Be(1); + } + + // ── Query with only Without ────────────────────────────────────── + + [Fact] + public void Query_OnlyWithout_FiltersCorrectly() + { + var world = new World(); + var a = world.CreateEntity(); + var b = world.CreateEntity(); + + world.AddComponent(a, new Position { X = 1, Y = 2 }); + world.AddComponent(b, new Position { X = 3, Y = 4 }); + world.AddComponent(b, new Frozen()); + + // Without filters out entity b which has Frozen. + var query = world.Query().Without().Build(); + var results = new List(); + + world.ForEach(query, (Entity e, ref Position pos) => + { + results.Add(e); + }); + + results.Should().BeEquivalentTo([a]); + } + + // ── World.Dispose ──────────────────────────────────────────────── + + [Fact] + public void World_Dispose_DoesNotThrow() + { + var world = new World(); + var entity = world.CreateEntity(); + world.AddComponent(entity, new Position { X = 1, Y = 2 }); + + var act = () => world.Dispose(); + act.Should().NotThrow(); + } + + [Fact] + public void World_Dispose_CanBeCalledMultipleTimes() + { + var world = new World(); + + world.Dispose(); + var act = () => world.Dispose(); + act.Should().NotThrow(); + } + + // ── CommandQueue error accumulation ────────────────────────────── + + [Fact] + public void CommandQueue_Errors_AccumulateAcrossDrainCycles() + { + var world = new World(); + + world.Commands.Enqueue(new FailingCommand { Message = "first" }); + world.ExecuteCommands(); + world.Commands.Errors.Should().HaveCount(1); + + world.Commands.Enqueue(new FailingCommand { Message = "second" }); + world.ExecuteCommands(); + world.Commands.Errors.Should().HaveCount(2); + + world.Commands.Errors[0].Message.Should().Be("first"); + world.Commands.Errors[1].Message.Should().Be("second"); + } + + [Fact] + public void CommandQueue_ClearErrors_ResetsErrorList() + { + var world = new World(); + + world.Commands.Enqueue(new FailingCommand { Message = "boom" }); + world.ExecuteCommands(); + world.Commands.Errors.Should().HaveCount(1); + + world.Commands.ClearErrors(); + world.Commands.Errors.Should().BeEmpty(); + } + + // ── Multiple World instances ────────────────────────────────────── + + [Fact] + public void MultipleWorlds_AreIndependent() + { + var world1 = new World(); + var world2 = new World(); + + var e1 = world1.CreateEntity(); + var e2 = world2.CreateEntity(); + + world1.AddComponent(e1, new Position { X = 1, Y = 2 }); + world2.AddComponent(e2, new Position { X = 3, Y = 4 }); + + // Each world sees its own entity's components. + ref var pos1 = ref world1.GetComponent(e1); + pos1.X.Should().Be(1); + + ref var pos2 = ref world2.GetComponent(e2); + pos2.X.Should().Be(3); + + // Worlds are independent: destroying in one doesn't affect the other. + world1.DestroyEntity(e1); + world1.IsAlive(e1).Should().BeFalse(); + world2.IsAlive(e2).Should().BeTrue(); + } + + // ── EntityAllocator large scale ────────────────────────────────── + + [Fact] + public void EntityAllocator_HandlesManyEntities() + { + var world = new World(); + var entities = new List(); + + for (int i = 0; i < 5000; i++) + { + var e = world.CreateEntity(); + entities.Add(e); + world.IsAlive(e).Should().BeTrue(); + } + + // Destroy half. + for (int i = 0; i < 2500; i++) + { + world.DestroyEntity(entities[i]); + world.IsAlive(entities[i]).Should().BeFalse(); + } + + // Create more — should reuse freed IDs. + for (int i = 0; i < 2500; i++) + { + var e = world.CreateEntity(); + world.IsAlive(e).Should().BeTrue(); + } + } + + // ── Structural mutation during ForEach ──────────────────────────── + + [Fact] + public void ForEach_AddingComponent_DoesNotAffectCurrentIteration() + { + var world = new World(); + var a = world.CreateEntity(); + var b = world.CreateEntity(); + world.AddComponent(a, new Position { X = 1, Y = 2 }); + world.AddComponent(b, new Position { X = 3, Y = 4 }); + + var query = world.Query().With().Build(); + var seen = new List(); + + world.ForEach(query, (Entity e, ref Position pos) => + { + seen.Add(e); + // Add a component to the other entity during iteration. + // This should not affect the current iteration. + world.AddComponent(e, new Velocity { X = 1, Y = 1 }); + }); + + seen.Should().BeEquivalentTo([a, b]); + } + + [Fact] + public void ForEach_DestroyingEntity_DoesNotThrow() + { + var world = new World(); + var a = world.CreateEntity(); + var b = world.CreateEntity(); + world.AddComponent(a, new Position { X = 1, Y = 2 }); + world.AddComponent(b, new Position { X = 3, Y = 4 }); + + var query = world.Query().With().Build(); + + // Destroying an entity during iteration: the sparse set uses + // swap-remove, so the destroyed entity's slot is replaced by + // the last element. This is safe as long as we don't re-iterate + // the destroyed entity (which we won't since it's swap-removed). + var act = () => + { + world.ForEach(query, (Entity e, ref Position pos) => + { + world.DestroyEntity(e); + }); + }; + act.Should().NotThrow(); + } + + // ── Helpers ────────────────────────────────────────────────────── + + private sealed class ChangeCollector : IObserver + { + public List Changes = new(); + + public void OnNext(EntityChange value) => Changes.Add(value); + public void OnError(Exception error) { } + public void OnCompleted() { } + } + + // Phantom types for relationship tests. + public struct ChildOf { } + public struct ParentOf { } +}