genaray/Arch

`Archetype` does not grow after `World.Remove(QueryDescription)`

mikhail-dcl opened this issue · 7 comments

Description

  • Use bulk Remove
  • Create new entities in the affected archetype
  • When the existing capacity has reached the archetype does not extend it properly and throws an exception

It's super critical for us at Decentraland and we are seeking for help ASAP.
In the meantime we will try dig deeper to understand the underlying logic.

P.S. If individual Remove(entity) is used it does not happen and the archetype works as expected

How to reproduce:

    [Test]
    public void ChunkDoesNotOverflowOnBulkRemoval()
    {
        for (var i = 0; i < 100; i++)
        {
            // Create entities with a single archetype
            _world.Create(new Transform());
        }

        var archetype = _world.Archetypes[0];

        var entityCapacity = archetype.EntityCapacity;

        for (var i = 0; i <= entityCapacity; i++)
        {
            // Create entities with a duplex archetype
            _world.Create(new Transform(), new Rotation());
        }

        // both archetypes should be created

        // Try move entities from duplex archetype to single archetype
        _world.Remove<Rotation>(new QueryDescription().WithAll<Transform, Rotation>());

        // Just a big value to ensure it goes beyond the original capacity
        entityCapacity = archetype.EntityCapacity;

        // Capacity should grow properly
        for (var i = 0; i <= entityCapacity; i++)
        {
            // Create entities with a duplex archetype
            Assert.DoesNotThrow(() => _world.Create(new Transform(), new Rotation()), "Overflow at {0}", i);
        }
    }

Observe the exception

Overflow at 1638
  Expected: No Exception to be thrown
  But was:  <System.IndexOutOfRangeException: Index was outside the bounds of the array.
   at Arch.Tests.OverflowTest.<ChunkDoesNotOverflowOnBulkRemoval>b__3_0() in D:\Decentraland\ArchECS\Arch\src\Arch.Tests\OverflowTest.cs:line 54
   at InvokeStub_TestDelegate.Invoke(Object, Object, IntPtr*)
   at System.Reflection.MethodInvoker.Invoke(Object obj, IntPtr* args, BindingFlags invokeAttr)
--- End of stack trace from previous location ---
   at NUnit.Framework.Internal.ExceptionHelper.Rethrow(Exception exception)
   at NUnit.Framework.Internal.Reflect.DynamicInvokeWithTransparentExceptions(Delegate delegate)
   at NUnit.Framework.Internal.ExceptionHelper.RecordException(Delegate parameterlessDelegate, String parameterName)>

Original Exception:

image

Gonna Look at this tomorrow, once im Home. What Version are you using? Have you tried the latest Version or the latest Commit?

Latest commit

Think i found the reason and a possible fix for it.

The potential fix is to call "EntityInfo.EnsureCapacity" for before each "EntityInfo.Add"... that should fix the issue, however that's just a temporary fix. Its not that clean, I'm looking to optimize this.

Hi @genaray are you able to commit the work around until you find the optimization? Or should we open a PR?

We still have the critical issue our side.

Thank you in advance!

Hi @genaray are you able to commit the work around until you find the optimization? Or should we open a PR?

We still have the critical issue our side.

Thank you in advance!

I will commit it today until i found an more optimized approach to this. Thanks for your patience ^^

Closed in 87f2105 without any performance drawbacks.
The issue was that the capacity did not recalculate after a Add/Remove-Query operation which resulted in too few slots for possible entities.

Lemme know if this solved your issues! @mikhail-dcl @m3taphysics