#113 entity version #115
#113 entity version #115
Conversation
|
Thanks for the contribution but I will need some time to check it. I hope that I can give an answer/review by Monday. If only the first benchmark is affected then I think it is not a big deal because thanks to @tommyettinger that got recently improved by a lot and imo adding a clean solution for entity versioning is more important. Just an idea that I got from an article. I think an int has 32 bits in Kotlin for all platforms? Or maybe even 64? Maybe we can again do some bitwise magic stuff to encode the entity version into the ID as well. My idea is that e.g. we use the first 16 bits for the version and the last 16 bits for the id itself. Of course that reduces the total amount of possible entities but I think it is still a big number that won't be reached by most games using Fleks. That way we safe an additional lookup for the version and we can keep the value class and therefore still working with an Int instead of a class object. I am not 100% sure if that works out but it is just an idea that popped up today in my head. What do you think? Could that work out and maybe simplify the current aporoach and keep performance and memory consumption on the same level? |
|
The problem as I see it is not really with how we encode or store the versions. The storing aspect is mostly a non-issue because for usual use cases the memory consumption is dominated by data in components. Performance wise I don't think we gain much by changing the encoding (might even be a loss if we need to decode and encode too often). |
|
@dragbone: Sorry once more! I will have a look tomorrow morning and give you feedback to the PR. Coming back to the idea with splitting the 32 bit int into two parts for version and id I still think it might be simpler and does not require most of the logic that we came up with right now. But let me elaborate and please let me know, if my thought process is wrong somewhere. For simplicity reason, let's say we only have a 4 bit number for entities where 0000 is 0 and 1111 = 15. In total 16 possible values. If we now want to use half for ids and half for versions, we end up supporting only up to 8 entities. Now imagine following scenario:
The only additional thing that is happening is that a separate bag of entities is used and that a bit shift operation is happening when removing entities, which should be not a big deal imo because direct lookup of an array index should not be that expensive. My idea would be to have something like this, but not sure if that is possible in Kotlin: // entity is still a value class like before
@JvmInline
@Serializable
value class Entity(val versionId: Int) {
val id : Int // add id property to make the code compile. However, this should not be a real property because we are in a value class so it is just some syntax sugar in Kotlin
get() = versionId and and 0xFFFF
fun incVersion() : Entity {
// Mask out the last 16 bits and increment the first 16 bits
val last16Bits = versionId and 0xFFFF
val first16Bits = (versionId ushr 16) + 1
// Combine the first 16 bits with the last 16 bits
return Entity((first16Bits shl 16) or last16Bits)
}
}With that change the entire codebase should still compile and all tests should pass because Maybe that is what you did in your implementation already. Again, I will have a look at it tomorrow. |
|
Me again! I also chatted with ChatGPT on my phone now a little bit and it gave me two other ideas:
Both strategies don't require the versioning stuff at all. For solution 1 we'd need some platform specific implementations afaik because there is no build in UUID for kotlin multiplatform. Solution 2 sounds interesting to me and my idea would be the following:
What I like about this approach is that this new functionality is used "lazily" and does not require the lookups of our previous solution (e.g. in family iterations). The downside is that users need to be aware to work with What do you think about that approach? |
src/commonTest/kotlin/com/github/quillraven/fleks/ComponentTest.kt
Outdated
Show resolved
Hide resolved
|
@Quillraven I moved some stuff around and replaced some data structures to get rid of the lookup. At least on my machine the performance is now pretty much the same as it is on master. Short summary of what I did:
I want to write some additional tests to make sure that everything still works, I think there are some gaps in the test coverage that's relevant for this change. I also want to add a benchmark to test if there is a memory/GC issue (I think there isn't, but I'd prefer proof). |
That looks neat! If everything is still working and performance is close (or maybe even better? ;)) to what we have before then let's use your PR instead of the Really appreciate your time and effort for this - thank you! I added some comments as a review and please also share your info, how you do a memory/GC test and how data class compares to value class. Would be cool if we get this solution up and running. Btw, the usage of |
|
I did some memory tests which didn't show much difference between this version and the one on master. I tested the performance under very harsh memory limitation by setting the maximum heap to 8 megabytes ( Both didn't indicate much difference between the versions. I'll add some more tests and clean up any comments later this week/on the weekend. |
|
I also run the benchmarks now on my notebook for master and your fork. I adjusted the versions of your fork before to match master because I updated them a few days ago: [versions]
kotlin = "1.9.10"
kotlinxSerialization = "1.6.0"
kotlinDokka = "1.9.0"
kotlinxBenchmark = "0.4.9"Result of master: Result of your fork: Simple and complex seem to have no impact. AdddRemove is slowed down quite a bit BUT it is still super fast and in that case I would vote to introduce the feature and ignore the performance impact in that particular use-case because it is a very nice and important feature. I also modified your fork and used the TL;DR: your implementation looks good and the addRemove performance impact should not be that critical imo. Thank you a lot for your contribution and time investment into this feature! |
|
Remember that the addRemove benchmark is not comparable to master because I had to adjust it to handle entity versions correctly. But I fixed it just now :D https://github.com/Quillraven/Fleks/pull/115/files#diff-e8658b624510082e4daf11283b3a6ac32ee654d77b4eaa8b86fa78ef5034e642 Here are my results from benchmarking with GC profiler: master
branch with version
If you compare the two it seems that this branch actually requires less garbage collection (gc.count is lower), probably because we no longer need to create Entity classes for iterations. |
|
Very cool stuff! There was also another issue and PR today (#117) that further reduces GC. I will have a final look over the weekend but once the documentation/comments are ready we can merge it and I will try it out in my three example games that I always take as a reference. If everything looks good I will release version 2.5 in the near future. Once more - thank you a lot for your time and help <3 |
# Conflicts: # build.gradle.kts
|
@Quillraven I think I'm done! I added some tests, adjusted comments and answered the last two open questions. |
|
I thank you for taking your time and implement this feature. It is a great improvement because before, as mentioned in your issue, I always used workarounds for the use case and now we have an elegant solution imo. Let's change the version to an Int and then I will verify it in my three example games and merge it. Btw.: Yesterday I tried a few things on your fork. One of them was to remove the compasks of the entity service and put it directly into the entity. I thought this will improve the performance further because the compmask of the entity doesn't need to be looked up anymore. However at least for me it had a negative Impact. I found that interesting and don't understand it to be honest. Maybe you have an explanation for it? Also, since an entity is now a class we could potentially add a world reference to it and remove the EntityContext concept. But that is something for the future. And maybe that will also have a negative impact for whatever reason 😅 Anyway, once more, great job and thanks! |
|
Verified it in my example games and everything looks good -> merged |
@Quillraven here is my attempt at implementing issue #113. I'm not entirely happy with the solution... I'm open to any suggestion.
Notable changes
fun getCurrentVersion(id: Int): Entityto retrieve the current version of an entityEntity.NONEas a shorthand to create an invalid entity (replacesEntity(-1))Open tasks
EntityProvider.getCurrentVersion(id: Int)DefaultEntityProvider.minusAssign(entity: Entity)has to update the entity version lookup for snapshots to work correctly, I think that's an ugly workaround. Snapshot loading seems kinda hacky in general, maybe a refactoring is in order now.Benchmarks
Benchmarking this is a bit different because the benchmark now needs to track the references to the entities. I have run the adjusted benchmark on master as well to get a result that can be compared, you can expect 20%-25% slowdown in the addRemove benchmark, the other two benchmarks seem unaffected:
master with benchmark adjustment
branch with entity version tracking