Changed _queryCache to an ordered list #119
Closed
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed changes
The compatibleArchetypes list can become out of order for QueryResults. This makes it skip over entirely valid archetype strings. Changing it from a set to a list can resolve this. Because it already uses the set _entityArchetypeCache to add new archetypes to queryCaches, there's no overhead trying to see if a new archetype has already been added to a queryCache or not.
Related issues
https://discord.com/channels/1234249974959702056/1283058858536992811
(Sorry this isn't a Github Issue, I was expecting to be committing an error myself).
Additional comments
I initially coded this in the current release, which until this worked without issue in my game.
The working version uses a different nextItem() function that in general seems to be erroring for my game, even prior to any changes. I also can't replicate the bug in the new release (but I think this could be due in part to the random erroring); however, the new version of nextItem() still relies on next(compatibleArchetypes) as a set, and it is my intention to resolve this bug and not the other things happening with queryResult.
A potential related issue with the new implementation of nextItem() would be around the removal of optimizeQueries / storage levels:
I may just be misreading the way the storage system is now working, but looking at the new implementation, this would likely happen on the entity level instead of the archetype level as entity archetypes change. E.g. an unordered list of entities {[1] = ..., [8] = ..., [95] = ..., [88] = ...} is not guaranteed to be in order.
This can be seen in use in nextItem() because it uses entityId, entityData = next(storage[currentCompatibleArchetype]). If this data is being modified, I would expect an entity to be removed from the currentCompatibleArchetype and moved to another archetype, causing the ordering next returns to sometimes change randomly.
But this is speculation, and I haven't thoroughly looked into the new storage implementation.