-
Notifications
You must be signed in to change notification settings - Fork 5
Add methods to collect entities of a query #320
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Maybe we can spare |
Yeah, probably. It could also be called like this: entities = collect_entities!(Query(...), Entity[])I think this feature is useful enough to be added. |
Removed the collect_entities function that was previously used to gather entities from a query.
Is it really useful there? Both loops involve drawing random numbers, so the operation it not applied to all matching entities. If an operation is applied to all, using batch operations (when implemented...) would be way more performant. But there are definitely still use cases where collecting entities makes sense. |
Click to expand benchmark resultsTime is per entity/N, allocations are totals. Allocations are only shown for current.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| count | ||
| end | ||
|
|
||
| function collect_entities!(q::Query, all_entities::AbstractVector{Entity}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the Julia style guide, the mutated arg should be the first one.
|
Looking a bit ahead, maybe at this point it would be good to already implement EDIT: Thinking a bit further, maybe it would be good then to postpone filters and collect to v0.3.0? |
|
mmh, if I understand it one could do something like |
No, that's not what I mean by filters. What I mean is just the masks part of queries in a separate object. They could be stored, created without locking the world, and used for batch operations and registered/cached filters. But there could be an implicit conversion from queries to filters, so both can be used where a filter is the actual argument. EDIT: predicate-like filters make no sense for batch operations, as we do not really get the advantage of batching then. Or at least not all advantages. |
|
is there this feature in the Go Ark package? I'm reading your edit and I'm not getting if you are saying that a |
|
ah okay maybe you are saying to do something like |
Yes, kind of. But with the difference that queries are always created from filters there, because of the required runtime lookup of component IDs. https://mlange-42.github.io/ark/batch/#components Predicates may be useful, maybe even with batch operations. We will not get the speedup of mass copying when they are used, but at least we still don't need the repeated archetype graph traversal. But I still think that a predicate (e.g. for batches or collect) should be separated from the kind of filters I mean. These are really just a lighweight, re-usable part of the query's filtering mechanism/masks. |
|
Ah okay got it maybe. This is caching the already in-place mechanisms for filtering. Though, if this is the case, then collecting entities is a separate feature a bit unrelated to filtering or not? |
Yes, kind of, but it would make sense then to use a |
What do you think about this @mlange-42 ? It should be useful sometimes to reduce the boilerplate.
E.g. in the little example SIR which I would like to add to the demos