-
Notifications
You must be signed in to change notification settings - Fork 19
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
feat: single & first entity queries #386
base: main
Are you sure you want to change the base?
Conversation
89cb9a6
to
17ca456
Compare
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.
This is great, looks correct + great tests and other bug fixes caught in here!
Left a couple comments regarding use of bitsets - let me know if any questions on those, I think I understand how this is working but I could definitely have my wires crossed too.
let mut bitset = self.bitset().clone(); | ||
query.apply_bitset(&mut bitset); | ||
|
||
let 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.
I'm wondering if we could instead of iterating over entities here, apply the entity's bitset in query.get_single_with_bitset and filter by entity's bitset once while iterating through it.
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.
Without iterating over the entities bitset we wouldn't know the index of the found value to construct the Entity
to return. We could make UntypedComponentStore::get_single_with_bitset
return a (u32, SchemaRef)
instead of just the ref and pass it up the chain. idk is that fine or is it a code smell?
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.
Ah gotcha, I understand.
I'm thinking that maybe we can do query.iter_with_bitset(bitset)
first to get iterator, and then do similar logic you have in which you check if iterator is_some, and that next is_none to confirm is single.
In the case that we know it is single, I'm wondering if we can get the current_id
off of the iterator, and then lookup the entity id using this current_id (should be index in bitset?). This way we can skip the iterating over entities by relying on its bitset being applied to query item, but hopefully still get entity id for mapping single to (entity, item)
.
I think this is sound but definitely check me on that if doesn't go well or if ends up being a pain. Definitely ok merging this as is, if we ever profile and find there are perf issues with this (I kind of doubt we will) can rethink it, but not too worried.
dac10eb
to
75aafd2
Compare
The `UntypedComponent(Optional)BitsetIterator(Mut)` structs may panic in their impl for `Iterator::next` if the current iteration id equals the maximum id, and the maximum id equals the length of the bitset. This is very unlikely to happen in the non-`Optional` variants since the max id starts at 0 and increments as entities are inserted. However, this is much more likely to happen in the `Optional` variants due to the max id being the length of the bitset.
There is no need for a `World` in these tests. The `impl QueryItem` for `&Comp<T>`, `&CompMut<T>`, and `&mut CompMut<T>` do not contain any logic -- they are merely passthroughs to the real implementations -- so it is not necessary to test all of them. The `impl QueryItem` for `OptionalQueryItem(Mut)` does, however, contain some minimal logic so it might be useful to test both the mutable and immutable variants in the tests.
75aafd2
to
1195f2b
Compare
Closes #289.
Changes
Entities::(get_)single_with
methods to get a single entity by a queryEntities::(get_)first_with_bitset
methods to get the first entity in the given bitsetEntities::(get_)first_with
methods to get the first entity in the given querySummary
Add convenience methods to get a single entity (and optionally its components) when there should only be one of the thing you're looking for. This could be useful, for example, to check if there is only one player alive in the game. You could use
Entities::get_single_with
which would allow you to detect when there is only one player (Ok
) or when there are none/multiple (Err
).All methods have a panicking and non-panicking variant -- e.g.
get_single_with
returns aResult<_, QuerySingleError>
, whilesingle_with
panics if the return value would not beOk
. The*first_with*
methods either return anOption
or panic.