Skip to content
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 by Bors] - [ecs] implement is_empty for queries #2271

Closed
wants to merge 15 commits into from

Conversation

NathanSWard
Copy link
Contributor

@NathanSWard NathanSWard commented May 28, 2021

Problem

Solution

  • Implement an is_empty function for queries to more easily check if the query is empty.

@NathanSWard NathanSWard added A-ECS Entities, components, systems, and events C-Enhancement A new feature labels May 28, 2021
@alice-i-cecile
Copy link
Member

I wouldn't call this "Fixes #2270"; the Query<(), F> syntax is also a nice boost to this pattern that isn't addressed in this PR.

@NathanSWard
Copy link
Contributor Author

NathanSWard commented May 28, 2021

I wouldn't call this "Fixes #2270"; the Query<(), F> syntax is also a nice boost to this pattern that isn't addressed in this PR.

Agreed, though the issue is titled Add .empty() convenience function to Query....
So it's probably splitting apart these two issues.

@NathanSWard NathanSWard requested a review from mockersf May 28, 2021 21:19
@NathanSWard NathanSWard added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels May 28, 2021
@NathanSWard NathanSWard removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label May 29, 2021
@NathanSWard NathanSWard requested a review from cart May 29, 2021 03:32
@cart
Copy link
Member

cart commented May 29, 2021

I do have concerns that this is "yet another" complicated iterator impl. We now have 5 variants: for_each, par_for_each, iter, iter_combinations, and is_empty. Which makes logic thats already pretty hard to maintain even harder. We need to draw the line somewhere, I'm just not sure yet if its before this, or after this :)

We have a couple of ways to "unify" things:

  1. iter_combinations introduced a QueryIterationCursor, which can replace iter duplication once we adopt derive(Component), which should solve perf regressions we hit the last time we tried.
  2. This code can be replaced with query.iter().next() if we sort out how to convert "write" queries to "read" queries.

I guess as long as we add a TODO comment above this implementation calling out (2), I'm cool with this impl as an interim solution.

@alice-i-cecile
Copy link
Member

This code can be replaced with query.iter().next() if we sort out how to convert "write" queries to "read" queries.

TBH this is a reasonably high priority quality issue; maybe we should just try and solve this instead?

@NathanSWard
Copy link
Contributor Author

I guess as long as we add a TODO comment above this implementation calling out (2), I'm cool with this impl as an interim solution

Done.

@cart
Copy link
Member

cart commented May 29, 2021

TBH this is a reasonably high priority quality issue; maybe we should just try and solve this instead?

Yup this both significantly improves UX for query consumption (no need for both read and write queries in querysets to work around lifetime restrictions when both aliased reads and unique writes are needed) and a reduction in maintenance burden.

Its just also a "relatively advanced" problem to solve as it involves both Fetch (which has lots of nuance) and advanced rust type system usage. Theres probably also multiple ways to do it, so its should have a design discussion first (maybe doesnt need an rfc, but we should survey the space).

I don't see much of a reason to hold off on this, given that we can't count on a quick solve for the "harder" problem. However if someone volunteers to pick up this work immediately, I'm down to hold off on merging this.

@NathanSWard
Copy link
Contributor Author

NathanSWard commented May 29, 2021

However if someone volunteers to pick up this work immediately, I'm down to hold off on merging this.

This is something I would love to help out with, however I can't work on it immediately as I have a bunch of other things I'm currently attending to. (It would probably be a couple weeks or so...)

@cart
Copy link
Member

cart commented Jun 2, 2021

bors r+

bors bot pushed a commit that referenced this pull request Jun 2, 2021
## Problem
- The `Query` struct does not provide an easy way to check if it is empty. 
- Specifically, users have to use `.iter().peekable()` or `.iter().next().is_none()` which is not very ergonomic. 
- Fixes: #2270 

## Solution
- Implement an `is_empty` function for queries to more easily check if the query is empty.
@bors bors bot changed the title [ecs] implement is_empty for queries [Merged by Bors] - [ecs] implement is_empty for queries Jun 2, 2021
@bors bors bot closed this Jun 2, 2021
ostwilkens pushed a commit to ostwilkens/bevy that referenced this pull request Jul 27, 2021
## Problem
- The `Query` struct does not provide an easy way to check if it is empty. 
- Specifically, users have to use `.iter().peekable()` or `.iter().next().is_none()` which is not very ergonomic. 
- Fixes: bevyengine#2270 

## Solution
- Implement an `is_empty` function for queries to more easily check if the query is empty.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Enhancement A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add .empty() convenience function to Query
4 participants