Entity Id Version

[mtsr]

Was it ever considered for Artemis to include a version in the Entity Id? Since the Entity Id's are reused, it's unsafe to store them anywhere, making it much harder to keep references to specific entities (you need to use something like the included uuids).

I'm sure you are familiar with this scheme, since it's used in a number of other ECS systems: If instead some of the bits of the Id were used to keep track of the current version number of a specific Id, then entityId's can be checked for validity, and it becomes safe to store them, instead of some additional uuid.

[junkdog]

It's perfectly fine storing entities as long as you intercept when they're deleted. There are basically two (three) ways of doing this:

• A manager can override void deleted(Entity e) and intercept when the entity is deleted.
• If the entity can be defined and grouped by an Aspect, you probably already have a system for them - override inserted(entity)/removed(entity).
• Alternatively, get an EntitySubscription from the AspectSubscpritionManager, implement and register a SubscriptionListener (this is effectively what EntitySystems do).

edit: Actually, disregard second and third alternative, if you explicitly want to track the full lifetime of the entitity, you need to go with the manager approach. EntitySystems and EntitySubscriptions don't distinguish between deleted entities and entities with a non-matching component composition.

[mtsr]

True that works, I just think it would be nicer for the library to enforce entity Id correctness, probably because that's what I was working with in the past.

I don't expect it to just change, I just wanted to bring it up for discussion.

[DaanVanYperen]

Just observing the devs here, pooling and reused ids are a common source of bugs. A feature like this would make the framework more accessible to devs less experienced with Artemis.

Alternatively, just having a short (configurable?) cooldown on limbo entities might help developers catch reference issues faster.

[junkdog]

Was it ever considered for Artemis to include a version in the Entity Id?

I just wanted to bring it up for discussion.

Sorry, I inferred too much and read too hastily...

But if ids are guaranteed to be unique (either by versioning or by always incrementing the id), wouldn't you still need to track entity deletions? Otherwise it seems there'd be a lot of memory still allocated?

The primary reason for artemis' reused id:s (this has been this way from the vanilla/original artemis days btw) is mainly performance; artemis has a Bag<Bag<Component>>, the indices in the outer Bag map to the componentTypeIndex while the inner - component - bags' indices correspond to entitiyIds. If incremented ids had been used, we would have required a mapping between actualId->componentLookupIndex(entityId), or kept it as a separate field. Both scenarios negatively impact performance.

You can get around it by implementing your own manager; something similar to the UuidEntityManager, but pref using plain integers and not UUID:s. This would allow for uniquely identified entities, as it's essentially adding another level/layer of entity ids to the framework.

pooling and reused id's are a common source of bugs.

Yeah, I'm hoping some of that can be remedied by #269 - I'll add a mental note about pooling too.

Alternatively, just having a short (configurable?) cooldown on limbo entities might help developers catch reference issues faster.

But this would essentially still lead to the same bugs, but expressed as NPE:s instead, as the components themselves have already been freed. Once dead, it's just the shell of its former component-composed self.

[DaanVanYperen]

But this would essentially still lead to the same bugs, but expressed as NPE:s instead, as the components themselves have already been freed. Once dead, it's just the shell of its former component-composed self.

not in our asserted debug build. ;)

[DaanVanYperen]

Coming back to the cooldown, the goal would be fail quickly with NPE instead of having weird unintended entity interactions. ;) I agree it's a crutch.

Managed 'EntityReference` that nulls out on entity removal is how I handle it really, not the prettiest but it works.

Love to stare at some examples of entity versioning!

[snorrees]

I can say from personal experience that pooled EntityRef-wrapper using a supporting IdManager still requires some developer discipline. You have to be vigilante in nulling out the wrapper reference, or it will hurt real bad :fearful:

I've been thinking about how to manage this problem better. One thought is to solve it by using callbacks for cleanup. I just typed this up, so there might be glaring errors/lapses in logic.

The gist of it is that you cannot retrieve a ref without EntityReferee interface, which should remind you to null out/ remove the EntityRef.

class EntityRefManager extends Manager{
    Pool<EntityRef> pool;
    Map<Entity, EntityRef> refs;
    IdManager idManager;

    public EntityRef getRef(Entity e, EntityReferee referee) {
        EntityRef ref = pool.obtain();
        ref.init(e, referee, idManager, Pool<EntityRef> pool);
        return ref;
    }
}

class EntityRef implements Poolable {
    Entity entity;
    EntityReferee referee;
    int uniqueId;
    IdManager idManager;
    Pool<EntityRef> pool;

    void init(entity, referee, idManager, Pool<EntityRef> pool) {
        this.entity = entity;
        this.referee = referee;
        this.uniqueId = idManager.getId(entity);
        this.pool = pool;
    }

    Entity getEntity() {
        return isValid() ? entity : null;
    }

    boolean isValid() {
        boolean valid = entity != null && idManager.getId(entity) == uniqueId;
        if(!valid){
            referee.removed(ref);
            pool.free(this);
        }
        return valid;
    }

    void reset() {
        referee.removed(this);
        entity = null;
        uniqueId = -1;
        idManager = null;
        pool = null;
    }
}

interface EntityReferee {
 /*EntityReferee will have to null out the reference/remove it from collections ect 
 when removed(EntityRef ref) is called.*/
    void removed(EntityRef ref);
}

[mtsr]

But if ids are guaranteed to be unique (either by versioning or by always incrementing the id), wouldn't you still need to track entity deletions? Otherwise it seems there'd be a lot of memory still allocated?

Sure, this remains, but at least you can guarantee that you cannot accidentally access the wrong entity, similar to how memory managed languages prevent accessing the wrong bit of memory.

The primary reason for artemis' reused id:s (this has been this way from the vanilla/original artemis days btw) is mainly performance; artemis has a Bag<Bag>, the indices in the outer Bag map to the componentTypeIndex while the inner - component - bags' indices correspond to entitiyIds. If incremented ids had been used, we would have required a mapping between actualId->componentLookupIndex(entityId), or kept it as a separate field. Both scenarios negatively impact performance.

I agree that reused id's are essential. If I'm not mistaken, the whole bag-construct is meant to prevent cache misses by keeping components packed close together in memory, and not reusing id's will surely introduce lots of gaps and more cache-misses. That's why what I propose is either keeping a separate version int, or reserving some bits in the id int, but still indexing only by the id. The exact choice is a bit of a trade-off, of course, in terms of memory use.

There are two more tradeoffs. The number of id bits determines the max number of concurrently existing entities. The number of version bits together with the minimum size of the id-free-list (FILO + minimum size guarantees how many entities you can create/destroy before id+version start getting recycled.

You can get around it by implementing your own manager; something similar to the UuidEntityManager, but pref using plain integers and not UUID:s. This would allow for uniquely identified entities, as it's essentially adding another level/layer of entity ids to the framework.

It works, but doesn't address the issue of this being a common source of error.

[DaanVanYperen]

You have to be vigilante in nulling out the wrapper reference, or it will hurt real bad

Ah the joys of targeting android. XD

[DaanVanYperen]

or reserving some bits in the id int, but still indexing only by the id

:+1: sorta. Odb's focus on 'entity is int' makes this the most realistic option, and this will certainly help devs diagnose common pitfalls, Bit of a smell on the framework (and extension frameworks) end of things though.

Edit: Don't want to give users a false sense of security, eventually version is going to cycle. How many bits? ;)

[antag99]

I can say from personal experience that pooled EntityRef-wrapper using a supporting IdManager still requires some developer discipline. You have to be vigilante in nulling out the wrapper reference, or it will hurt real bad :fearful:

Why do you need to pool EntityRefs? It should be enough to simply hold on to the same EntityRef and set a new entity when the object is reused; this will ensure that it's not possible to dereference an invalid entity.

public final class EntityReference {
    private Entity entity;
    private UUID uuid;

    public void set(Entity entity) {
        this.entity = entity;
        this.uuid = entity.getUuid();
    }

    public Entity get() {
        return entity != null && entity.getUuid().equals(uuid) ? entity : null;
    }
}

EDIT: forgot that artemis-odb uses UUID rather than long, fixed. NOTE: in the real world, you probably want to avoid returning null, and instead check if an entity is present first.

[snorrees]

Why do you need to pool EntityRefs?

In my case I have a need for collection of refs. The alternative is using "realSize" variable to manage the iteration of said collection (which to be honest I do for some other classes, so yay for consitency). Or am I forgetting something?

Well damned, I just spelled out the solution. A EntityReferenceCollection class. Damn - thats what I should do.

[mtsr]

The EntityRef solution is pretty nice for the use-case, at the expense of memory usage and significant cache misses, most likely.

[DaanVanYperen]

@junkdog go/no go on version in id within the foreseeable future, if not we'll put in freezer and revisit later.

[junkdog]

EDIT: forgot that artemis-odb uses UUID rather than long, fixed.

Don't want ot use UUID:s if you don't have to. They're only in the framework for historical reasons, and quite heavy-weight.

Adding fields to entity would be problematic, similarly messing with entity ids would make IntBag's prone to programmer error (and I sometimes embed metadata in those bits myself).

I'd rather handle this in the special debug build: easiest solution would probably be to pre-pool a big stack of entities (256k?); thereby always spawning entities with new id:s, at least for a while.

[antag99]

Don't want ot use UUID:s if you don't have to. They're only in the framework for historical reasons, and quite heavy-weight.

One ugly solution could be to deprecate UUID:s and introduce GUIDs/UIDs, which are long.

I'd rather handle this in the special debug build: easiest solution would probably be to pre-pool a big stack of entities (256k?); thereby always spawning entities with new id:s, at least for a while.

What happens with the code that relies on this when running under a release build?

[DaanVanYperen]

Keeping the UUID system is cheap, though for 1.0 it should probably be removed from World. might be nice to roll a long variant.

[DaanVanYperen]

Closing ticket, merged with #300 debug build. Feel free to continue discussion there!