junkdog / artemis-odb

A continuation of the popular Artemis ECS framework
BSD 2-Clause "Simplified" License
776 stars 111 forks source link

Removing entites causes nullPointerException #10

Closed lopho closed 10 years ago

lopho commented 10 years ago

oh shit. Well seems like the performer update wasn't very nice.

I wrote up a small unit test with one system and one type of component, the only thing this system does is this:

TestSystem#process

protected void process(Entity e) {
    e.deleteFromWorld();
}

It throws a NullPointerException exactly after HALF of the entites have been processed.

I guess it's because how the iteration in EntityProcessingSystem works, as it gets the data from the bag once, then gets the size once, but during the iteration things get removed, thus the size is changing, and the data is slipping through because the empty spots get filled with the last entity.

There is no way, I know of, to ensure during the loop, that all entites will get processed, and to leave entites out that have been deleted but not processed yet.

EntitySystem#removeFromSystem

private void removeFromSystem(Entity e) {
    actives.remove(e);
    e.getSystemBits().clear(systemIndex);
    removed(e);
}

EntityProcessingSystem#processEntities (passes actives as bag)

protected final void processEntities(ImmutableBag<Entity> entities) {
    Object[] array = ((Bag<Entity>)entities).getData();
    for (int i = 0, s = entities.size(); s > i; i++) {
        process((Entity)array[i]);
    }
}

I'll give a pull request with the UnitTest right away.

lopho commented 10 years ago

I see quite a problem with concurrency here, the change to call the performers directly instead of once before World#process.

I will try to wrap the array used for the iteration in EntityProcessingSystem#processEntities in some sort of linked list. hopefully it will work out.

junkdog commented 10 years ago

LinkedLists have terrible performance characteristics (see http://www.java-gaming.org/index.php?topic=27017.0). Maybe it's better to reinstate bags associated with each performer?

I haven't tried it myself (at work atm), but I think we might run into problems when an EntitySystem removes one of its own entities during

process, since Bags don't preserve order.

On Mon, Sep 30, 2013 at 5:12 PM, lopho notifications@github.com wrote:

I see quite a problem with concurrency here, the change to call the performers directly instead of once before World#process.

I will try to wrap the array used for the iteration in EntityProcessingSystem#processEntities in some sort of linked list. hopefully it will work out.

— Reply to this email directly or view it on GitHubhttps://github.com/junkdog/artemis-odb/issues/10#issuecomment-25371049 .

lopho commented 10 years ago

Yes that's the main problem (the only one really). Systems removing entities from themselves in process.

junkdog commented 10 years ago

Hmm... so, either: 1) reintroduce multiple bags and only remove/change entities after world#process is done, or 2) only remove(/change) entities after each system's process.

Alternatively, don't remove/add entities from a running system until end() is called. Would require a flag, EntitySystem#isProcessng or something and an if-check.

On Mon, Sep 30, 2013 at 5:51 PM, lopho notifications@github.com wrote:

Yes that's the main problem (the only one really). Systems removing entities from themselves in process.

— Reply to this email directly or view it on GitHubhttps://github.com/junkdog/artemis-odb/issues/10#issuecomment-25375231 .

lopho commented 10 years ago

Aight. Now I've put a isProcessing check into EntitySystem#deleted, if the system is processing, it collects to entities to be removed from actives. It will then remove them after EntitySystem#process but before EntitySystem#end().

I think this is a compromise between keeping the altering of entities with instant feedback and keeping performance.

Doing some performance profiling now. If everything is in shape, I'll do a pull request.

greets.

junkdog commented 10 years ago

Cool, looking forward to it!

On Mon, Sep 30, 2013 at 8:02 PM, lopho notifications@github.com wrote:

Aight. Now I've put a isProcessing check into EntitySystem#deleted, if the system is processing, it collects to entities to be removed from actives. It will then remove them after EntitySystem#process but before EntitySystem#end().

I think this is a compromise between keeping the altering of entities with instant feedback and keeping performance.

Doing some performance profiling now. If everything is in shape, I'll do a pull request.

greets.

— Reply to this email directly or view it on GitHubhttps://github.com/junkdog/artemis-odb/issues/10#issuecomment-25387118 .

ghost commented 10 years ago

I have a similar problem:

TestSystem#process

Entity e1 = ...
e1.deleteFromWorld();
Entity e2 = world.createEntity();

After this e2 get the e1 id and lose all his components.

junkdog commented 10 years ago

Did you try this with 0.4.0 or HEAD version?

I've created a new issue for your problem: #36 - let's continue any discussion there.