#113 entity version

[Quillraven]

PR for #113 (WIP).

@dragbone: I quickly run the benchmarks when changing Entity from a value class to a data class.

// from this
@JvmInline
@Serializable
value class Entity(val id: Int)

// to this
@Serializable
data class Entity(val id: Int, val version: Int = 0)

To my surprise, this does not seem to have any impact on the performance. Maybe because everything internally still works with the id (=int) and somehow that already gets optimized to perfection by the compiler/JVM? Don't know about the otheer platforms.

Here is the result:

# value class
jvmBenchmarks summary:
Benchmark                  Mode  Cnt     Score     Error  Units
FleksBenchmark.addRemove  thrpt    5  3005,834 ± 104,474  ops/s
FleksBenchmark.complex    thrpt    5     1,515 ±   0,308  ops/s
FleksBenchmark.simple     thrpt    5    91,181 ±   5,372  ops/s

# data class
jvmBenchmarks summary:
Benchmark                  Mode  Cnt     Score     Error  Units
FleksBenchmark.addRemove  thrpt    5  3011,912 ± 216,568  ops/s
FleksBenchmark.complex    thrpt    5     1,788 ±   0,094  ops/s
FleksBenchmark.simple     thrpt    5    87,405 ±  32,696  ops/s

Imo we can go ahead with this change as it is a nice feature when it comes to safe referencing entities in components and it seems to have no negative effect from what I can tell.

---

One thing that comes to my mind is serialization. Before it was just the id of the entity but now you get an id + version. The version is imo irrelevant for seralization because when you load a save state you most likely start again with version 0 for all entities?

@jobe-m: do you have any concerns about changing Entity from a value class to a data class?

@dragbone: If you like, you can start from this branch and add the logic to increase the version when an entity gets reused and add the tests for it. The correct place should be the create method of the DefaultEntityProvider. And also the contains method of the DefaultEntityProvider to check for the correct version.

If you don't have time then I will try to finalize it in October.

[dragbone]

@Quillraven I think the version has to be serialized as well or else any references to entities might break due to versions no longer matching. I have looked into how fleks works internally a bit and the change looks like it might be more complicated than expected. There are many places where the Entity value class gets created from just an index (e.g. Family.foreach creates Entity references from its BitArray), which now would need access to the current entity version to create a valid reference. I think these lookups will have a performance impact, how much is hard to say without implementing it...

[Quillraven]

Ah okay, that explains why my change did not have any bigger impact. So we need to test that again, when all areas that create an Entity like Entity(someindex) are adjusted as well.

About the serialization: I still think it is not necessary because what is the use-case here? Usually you want to save a state of the world and then load it later on. When you load it you start from an empty world or "override" the current state of the world meaning that all entities are new and should start with version 0 again.

Why would we need the version?

[dragbone]

About the serialization: I still think it is not necessary because what is the use-case here? Usually you want to save a state of the world and then load it later on. When you load it you start from an empty world or "override" the current state of the world meaning that all entities are new and should start with version 0 again.

Why would we need the version?

I image users expect that creating a snapshot and restoring it would result in a world that is exactly the same as it was before. That includes that all entity references that might be stored in some Components are still valid after the restore and that is only possible if all entities have their version restored. I haven't used the snapshot feature yet so that might be a wrong assumption on my part.

[Quillraven]

@dragbone: I quickly drafted the idea of the handle that I mentioned in your PR. I called it EntityRef instead because I liked that name better. It is not fully tested nor documented but should hopefully explain the idea :)

In short:

• There is a new ref() extension function for an Entity that returns an EntityRef
• This reference is valid until the entity gets removed

Also added a test to quickly verify the behavior. A component needs to use EntityRef instead of Entity when it needs a safe reference:

data class EntityRefComponent(val ref: EntityRef) : Component<EntityRefComponent> {
    override fun type() = EntityRefComponent

    companion object : ComponentType<EntityRefComponent>()
}

What do you think about this approach? Which one do you like more? The versioning or the ref? Which one feels better in your specific game?

[dragbone]

From a usability point of view I'd prefer directly getting entity references that i can store and use without having to think about it. Having to remember to call ref() is somewhat error prone and not very self-explanatory. But in the end I can live with both ¯\_(ツ)_/¯

[Quillraven]

From a usability point of view I'd prefer directly getting entity references that i can store and use without having to think about it. Having to remember to call ref() is somewhat error prone and not very self-explanatory. But in the end I can live with both ¯_(ツ)_/¯

I thought so and this is also my personal opinion. So, I think we should stick to the versioning idea but make it work with keeping the entity value class because of the reasons I explained in the review and your PR.

I try to find some time this week to maybe come up with a solution. If you have time and motivation, feel free to help! 🙂 Maybe we can simply work with the encoded version+idx id everywhere and only in the internal Fleks parts where the entity idx is needed for the data structures we extract the idx part out of it ...

[Quillraven]

@dragbone:

I fiddled around a little bit now and this would be a working entity value class. With that approach we'd also have 16 bits left that we could use for special flags on the entity directly without requiring components. E.g. simple tagging like "is visible" or "is player controlled", etc.. I thought about such a feature once and that would be theoretically possible with this approach.

/**
 * An entity of a [world][World]. It represents a unique identifier that contains several infos:
 * - index of the entity (first 32 bits)
 * - version of the entity (next 16 bits)
 * - optional flags (last 16 bits)
 */
@JvmInline
@Serializable
value class Entity(val id: Long) {

    // To explain the bitwise operations:
    // To extract n bits of a long from a specific offset we can use following approach:
    //
    // val rightShifted = long >>> offset
    // val mask = (1L << nBits) - 1L
    // val result = rightShifted and mask
    //
    // 32bits mask can be represented as 0xFF FF FF FF
    // 16bits mask can be represented as 0xFF FF

    fun nextVersion(): Entity {
        val v: Long = version + 1L
        val newId = id and 0xFFFF0000FFFFFF
        return Entity(newId or (v shl 32))
    }

    val idx: Int
        get() = (id and 0xFFFFFFFF).toInt()

    val version: Int
        get() = ((id ushr 32) and 0xFFFF).toInt()

    companion object {
        val NONE = Entity(-1)
    }
}

HOWEVER, I now understand better what you meant with the different lookups and imo it makes the codebase harder to understand and at least in my branch the addRemove benchmark performance went down quite a lot. Simple and complex were the same. Also, the EntityProvider needs the version function, like you have it in your PR as well, which I also don't like because imo users should not be bothered with such details and it will be most likely tricky to get that right when implementing their own EntityProvider.

Therefore, I change my mind and would vote for the ref solution for a quick-win solution which is easier to implement, does not affect performance and also does not "polute" the codebase that much.

Fine for you? Or do you want to investigate the value class approach some more? Maybe you can get it to work without killing addRemove performance :D

[dragbone]

I had one last idea last night which I want to test maybe today if I have time or else tomorrow.

[Quillraven]

115 has a more convenient solution for the entity version. We will go for that approach. I close this PR draft.