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

Added Has<T> WorldQuery type #8844

Merged
merged 14 commits into from
Jun 19, 2023

Conversation

wainwrightmark
Copy link
Contributor

Objective

Solution

  • I added Has<T> (and HasFetch<T> ) and implemented WorldQuery, ReadonlyWorldQuery, and ArchetypeFilter it
  • I also added documentation with an example and a unit test

I believe I've done everything right but this is my first contribution and I'm not an ECS expert so someone who is should probably check my implementation. I based it on what Or<With<T>,>, would do. The only difference is that Has does not update component access - adding Has to a query should never affect whether or not it is disjoint with another query I think.


Changelog

Added

  • Added Has<T> WorldQuery to find out whether or not an entity has a particular component.

@github-actions
Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible C-Examples An addition or correction to our examples A-ECS Entities, components, systems, and events and removed C-Examples An addition or correction to our examples labels Jun 15, 2023
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! I've often wanted this.

Can we have a couple more tests to verify that the accesses are set up correctly? We should be able to do Have + &mut in two queries in the same system.

Perhaps as a doc test: this is a nice property to teach about.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A small suggestion to help call out the important property to the reader, but this LGTM now :)

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
crates/bevy_ecs/src/query/fetch.rs Outdated Show resolved Hide resolved
@alice-i-cecile
Copy link
Member

@wainwrightmark can you try out the suggestion from James and report back? Once that's done (success or failure), I'm happy to merge this and ship it for 0.11 :)

@wainwrightmark
Copy link
Contributor Author

Replacing HasFetch<T> with bool worked great!

@alice-i-cecile alice-i-cecile added 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 Jun 16, 2023
Co-authored-by: JoJoJet <21144246+JoJoJet@users.noreply.github.com>
@wainwrightmark
Copy link
Contributor Author

I realize I have made a mistake in making Has<T> and ArcheTypeFilter - it's technically true in a sense but completely unhelpful as you should never be using it in a filter position. I've reverted this.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 19, 2023
Merged via the queue into bevyengine:main with commit 6529d2e Jun 19, 2023
james7132 pushed a commit to james7132/bevy that referenced this pull request Jun 19, 2023
# Objective

- Fixes bevyengine#7811 

## Solution

- I added `Has<T>` (and `HasFetch<T>` ) and implemented `WorldQuery`,
`ReadonlyWorldQuery`, and `ArchetypeFilter` it
- I also added documentation with an example and a unit test


I believe I've done everything right but this is my first contribution
and I'm not an ECS expert so someone who is should probably check my
implementation. I based it on what `Or<With<T>,>`, would do. The only
difference is that `Has` does not update component access - adding `Has`
to a query should never affect whether or not it is disjoint with
another query *I think*.

---

## Changelog

## Added
- Added `Has<T>` WorldQuery to find out whether or not an entity has a
particular component.

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: JoJoJet <21144246+JoJoJet@users.noreply.github.com>
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-Feature A new feature, making something new possible S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Has<C> WorldQuery type to replace Option<&C> when checking existence
6 participants