Spy-Shifty / BrokenBricksECS

Rebuild of Unity3D upcoming Entity Component System !OUT OF DEVELOPMENT!
MIT License
162 stars 28 forks source link

OnEntityRemoved/Removing called when Component is removed doesn't allow accessing the removed component. #21

Open acoppes opened 6 years ago

acoppes commented 6 years ago

When you remove a Component calling the manager.RemoveComponent(), the entity removing and remove are called to all groups interested in that component, however, they are called without that component.

I believe the system may want to react to the component being removed to remove data related to that component, in some cases with a reference to that data stored in the component itself or being used as identifier, etc.

To test it I am using the following code:

    class ListenerMock : IEntityAddedEventListener, IEntityRemovedEventListener, IEntityRemovingEventListener
    {
        public int addedCalls;

        public int removedCalls;

        public int removingCalls;

        public event Action<Entity> RemovedEvent;

        public event Action<Entity> RemovingEvent;

        #region IEntityAddedEventListener implementation
        public void OnEntityAdded (object sender, Entity entity)
        {
            addedCalls++;
        }

        public void OnEntityRemoved(object sender, Entity entity)
        {
            removedCalls++;
            if (RemovedEvent != null)
                RemovedEvent(entity);
        }

        public void OnEntityRemoving(object sender, Entity entity)
        {
            removingCalls++;
            if (RemovingEvent != null)
                RemovingEvent(entity);
        }
        #endregion

    }

    [Test]
    public void EntityRemovedCallbackShouldBeCalledWithComponents() {
        var entityManager = new EntityManager ();

        var group = entityManager.GetComponentGroup (typeof(Component1), typeof(Component2));

        var listenerMock = new ListenerMock ();
        group.SubscribeOnEntityAdded(listenerMock);
        group.SubscribeOnEntityRemoved (listenerMock);
        group.SubscribeOnEntityRemoving(listenerMock);

        var e = entityManager.CreateEntity ();

        entityManager.AddComponent (e, new Component1());
        entityManager.AddComponent (e, new Component2());

        Assert.That (listenerMock.addedCalls, Is.EqualTo (1));

        listenerMock.RemovingEvent += delegate (Entity e1) {
            Assert.True(entityManager.HasComponent<Component1>(e1));
            Assert.True(entityManager.HasComponent<Component2>(e1));
        };

        entityManager.RemoveComponentImmediate<Component1>(e);

        Assert.That (listenerMock.removingCalls, Is.EqualTo (1));
    }

It fails also using the normal remove:

    [Test]
    public void EntityRemovedCallbackShouldBeCalledWithComponents2() {
        var entityManager = new EntityManager ();

        var systemRoot = new SystemRoot<EntityManager>(entityManager);

        var group = entityManager.GetComponentGroup (typeof(Component1), typeof(Component2));

        var listenerMock = new ListenerMock ();
        // group.SubscribeOnEntityAdded(listenerMock);
        // group.SubscribeOnEntityRemoved (listenerMock);
        group.SubscribeOnEntityRemoving(listenerMock);

        // group.component

        var e = entityManager.CreateEntity ();

        entityManager.AddComponent (e, new Component1());
        entityManager.AddComponent (e, new Component2());

        // Assert.That (listenerMock.addedCalls, Is.EqualTo (1));

        // listenerMock.RemovedEvent += delegate (Entity e1) {
        //  Assert.True(entityManager.HasComponent<Component1>(e1));
        //  Assert.True(entityManager.HasComponent<Component2>(e1));
        // };

        listenerMock.RemovingEvent += delegate (Entity e1) {
            Assert.True(entityManager.HasComponent<Component1>(e1));
            Assert.True(entityManager.HasComponent<Component2>(e1));
        };

        entityManager.RemoveComponent<Component1>(e);
        // entityManager.RemoveComponentImmediate<Component1>(e);

        systemRoot.Update();

        Assert.That (listenerMock.removingCalls, Is.EqualTo (1));
    }
Spy-Shifty commented 6 years ago

Thank you I'll fix this

Gotcab commented 6 years ago

Is it fixed yet?

dfraska commented 6 years ago

I was able to address this by changing ComponentWrapper.cs lines 82 and 149 from: gameObjectEntity.EntityManager.RemoveComponent(gameObjectEntity.Entity); to: gameObjectEntity.EntityManager.RemoveComponentImmediate(gameObjectEntity.Entity);

I'm not 100% sure if this will have any other effects, but so far it has worked for me.

dfraska commented 6 years ago

The above fix causes issues with setting GameObjects inactive. I'm no longer seeing the original issue in my code... but I've made other bug fixes in my own non-public branch.