Calling engine.removeEntity() on item in ImmutableArray from Engine.getEntitiesFor mutates array

[Barryrowe]

Use Case:

• Use engine.getEntitiesFor(Family...) on input event to remove all entities in the family

Code that fails:

ImmutableArray<Entity> currentGunEntities = getEngine().getEntitiesFor(Family.one(GunComponent.class, WeaponDecorationComponent.class).get());


for(Entity e:currentGunEntities){
     getEngine.removeEntity(e);
}

The above code runs without error, but only half of of the entities are removed, as it apperas the underlying Array<> is modified immediately on the removeEntity() call, which appears to cause the iterator to adjust positions.

Work Around for now:

//Declared at System class level
Array<Entity> removableEntities = new Array<Entity>();

//...
//event code
ImmutableArray<Entity> currentGunEntities = getEngine().getEntitiesFor(Family.one(GunComponent.class, WeaponDecorationComponent.class).get());

removableEntities.clear();
for(int i=0;i<currentGunEntities.size();i++){
   removableEntities.add(currentGunEntities.get(i));
}

for(Entity e:removableEntities){
    getEngine().removeEntity(e);
}

This caught me by surprise, as I expected to be able to trust the ImmutableArray to remain unchanged. I have not tested this same code inside of and update of an EntitySystem, I'm currently doing this on an InputProcessor keydown event. I'm not sure if this is the expected behavior outside of a system update() method.

[dsaltares]

We are not doing snapshots on these immutable arrays. They are immutable in the sense that they cannot be used to affect the state of the engine. However, their state can, and will change, whenever the engine does.

Maybe adding that to the wiki/docs would be useful?

[Barryrowe]

Ok thanks. I think a note in the docs would be good, as just reading the type name, I wouldn't have expected the behavior that is intended.

I wonder if changing the name to something like "ManagedArray" instead of ImmutableArray might make that clearer as well?

I'm not sure if there are other use cases for this, and allowing leaking of entity references may be enough to shoot this down, but how would you feel about a getSnapshot() method on the ImmutableArray<> type that returned an "unmanaged" copy of the internal Array? The items in the snapshot array would be references (so any engine modifications to them would still be picked up), but the array would always contain the references to the objects at the time of the snapshot. It would definitely need to be documented with notes that it could be dangerous and unpredictable if kept through multiple engine steps.

[Barryrowe]

I was looking through this, and while I submitted a pull request to add documentation around on the Engine.getEntities method, it seems like in the long run implementing snapshots seems like it would prevent confusion and unexpected behavior.

[LennartH]

I ran into the same problem, when I tried to remove all entities of a family. My workaround is (shamelessly borrowed from Engine.removeAllEntities()):

ImmutableArray<Entity> entities = engine.getEntitiesFor(family);
while (entities.size() > 0) {
    engine.removeEntity(entities.first());
}

I suggest to add the method Engine.removeAllEntities(Family family). With that, the functionality to snapshot "ImmutableArrays" could be unnecessary, since the most common (if not only) operations that change the engines state are adding and removing entities.

I agree that the name ImmutableArray is misleading. In my opinion something like ReadOnlyArray would be better, since this doesn't indicate that the content never changes.

[dsaltares]

@LennartH that sounds good, do you wanna send a PR for that?

[clintdoriot]

FWIW, java.util.collections has similar wrappers, designated using the term "unmodifiable"

https://docs.oracle.com/javase/7/docs/api/java/util/Collections.html#unmodifiableList(java.util.List)