Skip to content

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

Merged
merged 55 commits into from
Sep 8, 2023

Conversation

joseph-gio
Copy link
Owner

Rebase of #30.
Resolves #23.

Adds support for trait queries that filter based on change detection.

TODO: describe the different kinds of query supported

Copy link

@bzm3r bzm3r left a 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:

  1. merge as is, and make issues for their elimination, which I am happy to spend the rest of the week eliminating/addressing

  2. I can submit PRs to the change-detection-rebase that address them, before we merge.

) -> Self::Item<'w> {
let ticks_ptr = match fetch.storage {
ChangeDetectionStorage::Uninit => {
// set_archetype must have been called already
Copy link

@bzm3r bzm3r Jun 28, 2023

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_unreachables are based on?

@bzm3r bzm3r mentioned this pull request Jun 28, 2023
2 tasks
@bzm3r
Copy link

bzm3r commented Jun 28, 2023

@joseph-gio
Copy link
Owner Author

joseph-gio commented Jun 29, 2023

I've removed much of the code duplication by turning the All filters into iterator adapters over the standard All form. This keeps mostly the same API while removing many lines of code. There's still a bit more deduplication that can be done (specifically in the actual WorldQuery impls), but I think this is an improvement. I still need to think of some approaches for improving the One adapters (maybe even by just removing them as mentioned previously).

I'd appreciate another look @bzm3r :). Also, I think that your idea of removing the added/changed WorldQuerys in favor of just adding methods to ReadTraits and WriteTraits is attractive in its simplicity, though I haven't tried it yet.

@joseph-gio joseph-gio force-pushed the change-detection-rebase branch from a7d25b4 to c515851 Compare July 15, 2023 21:51
@joseph-gio
Copy link
Owner Author

I've slimmed down the API for the One filters, and renamed ChangedAll back to AllChanged, since I think it reads better. I originally asked @bzm3r to rename them, but I think I was being a bit too particular.

@joseph-gio joseph-gio marked this pull request as ready for review July 15, 2023 22:37
@joseph-gio
Copy link
Owner Author

I took @bzm3r's suggestion of removing the iterator filters in favor of simple iter_added() and iter_changed() methods, which remove a lot of bloat from this PR. I plan on merging this soon.

@joseph-gio joseph-gio merged commit 491328e into main Sep 8, 2023
@joseph-gio joseph-gio deleted the change-detection-rebase branch September 8, 2023 05:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for change detection filters Changed<>, Added<>, etc
2 participants