-
Notifications
You must be signed in to change notification settings - Fork 17
Add queries that filter based on change detection #42
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
Conversation
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.
Sorry, this took a while! I have the lay of the land again, and the tests are passing, so that's good.
I used this review to address some of the comments you left on my pre-rebase PR, so there are some code/comment suggestions I have provided. Mostly they are doc improvements.
The biggest issue I think is that we should be using TableRow
instead of usize
in all.rs
, and merging the rebased PR as is will cause at least one incorrect regression there (see comments).
I also have a lot of nits with the original code I wrote...just so much unnecessary code duplication? Or maybe I am being overly sensitive to it, but I worry that as is, the code will be extremely hard/painful to maintain.
Depending on which code duplications you think are reasonable/valuable to eliminate, we could decide to:
-
merge as is, and make issues for their elimination, which I am happy to spend the rest of the week eliminating/addressing
-
I can submit PRs to the
change-detection-rebase
that address them, before we merge.
src/query_filter.rs
Outdated
) -> Self::Item<'w> { | ||
let ticks_ptr = match fetch.storage { | ||
ChangeDetectionStorage::Uninit => { | ||
// set_archetype must have been called already |
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 noticed that in lib.rs
we have "registry sealing" tracking whether a registry is sealed or open: https://github.com/JoJoJet/bevy-trait-query/blob/2071f9c948358f0959b538ecf54f3a54cc6de923/src/lib.rs#L286
If the seal check fails, a panic is issued: https://github.com/JoJoJet/bevy-trait-query/blob/main/src/lib.rs#L311
I am wondering: is it worth putting a similar, explicit panic-check here that enforces the conditions the debug_unreachable
s are based on?
The |
Co-authored-by: Brian Merchant <[email protected]>
I've removed much of the code duplication by turning the I'd appreciate another look @bzm3r :). Also, I think that your idea of removing the added/changed |
a7d25b4
to
c515851
Compare
I've slimmed down the API for the |
I took @bzm3r's suggestion of removing the iterator filters in favor of simple |
Rebase of #30.
Resolves #23.
Adds support for trait queries that filter based on change detection.
TODO: describe the different kinds of query supported