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

Add QueryState::get_single_unchecked_manual and its family #3159

Closed
wants to merge 19 commits into from

Conversation

2ne1ugly
Copy link
Contributor

Hey people! This is my first PR to bevy. Definitely this issue helped me get some insights to bevy ecs.
I hope I followed the correct conventions! Tho, don't mind to correct them :)

Objective

  • Fixes Add single() to QueryState #3156
  • add #[inline] to single related functions so that they matches with other function defs
  • elide one lifetime in Query::single, Query::get_single

Solution

  • added functions to QueryState
    • get_single_unchecked_manual
    • get_single_unchecked
    • get_single
    • get_single_mut
    • single
    • single_mut
  • make Query::get_single use QueryState::get_single_unchecked_manual
  • added #[inline] & elided lifetimes
  • change #[derive(Debug, Error)] to #[derive(Error, Debug)] because it tilts me

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Nov 21, 2021
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Needs-Review and removed S-Needs-Triage This issue needs to be labelled labels Nov 21, 2021
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.

Awesome first PR, thank you! I have not a single nit to pick.

Other than the broken import that CI caught of course :p You should check out https://github.com/bevyengine/bevy/blob/main/CONTRIBUTING.md if you haven't yet, and welcome aboard :)

Copy link
Contributor

@MinerSebas MinerSebas left a comment

Choose a reason for hiding this comment

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

This PR needs to be updated due to the changes from #2305

crates/bevy_ecs/src/system/query.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/query.rs Outdated Show resolved Hide resolved
@Shatur
Copy link
Contributor

Shatur commented Mar 10, 2022

@2ne1ugly could you rebase the PR ?

@2ne1ugly
Copy link
Contributor Author

@2ne1ugly could you rebase the PR ?

Done

@Shatur
Copy link
Contributor

Shatur commented Mar 13, 2022

Thanks! Cart said that he will back to this in about a week.

@cart cart added this to the Bevy 0.7 milestone Apr 4, 2022
@alice-i-cecile
Copy link
Member

I just double-checked, and the lifetimes here appear to be correct: compare to

pub fn get<'w, 's>(

crates/bevy_ecs/src/query/state.rs Show resolved Hide resolved
crates/bevy_ecs/src/query/state.rs Show resolved Hide resolved
crates/bevy_ecs/src/query/state.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/query.rs Outdated Show resolved Hide resolved
@2ne1ugly
Copy link
Contributor Author

2ne1ugly commented Apr 24, 2022

Sorry for being a bit late! I'm doing unversity on weekends and it's the mid-term season.
Not sure about the check-markdown-links CI failure 🤔 Should I leave it be? It looks like it's happening to other branches too.

@alice-i-cecile alice-i-cecile modified the milestones: Bevy 0.7, Bevy 0.8 Apr 25, 2022
@alice-i-cecile
Copy link
Member

@Shatur @MinerSebas can I get another review on this?

@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 May 25, 2022
@alice-i-cecile
Copy link
Member

bors try

bors bot added a commit that referenced this pull request May 25, 2022
@bors
Copy link
Contributor

bors bot commented May 25, 2022

try

Build failed:

@alice-i-cecile
Copy link
Member

@2ne1ugly can you please rebase? I think the CI failure is spurious based on an old breakage we had.

@alice-i-cecile alice-i-cecile 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 25, 2022
@alice-i-cecile
Copy link
Member

Currently broken; the lifetimes must be fixed. Once this compiles again, I'm happy to slap the Ready-For-Final-Review label back on.

@2ne1ugly
Copy link
Contributor Author

Currently broken; the lifetimes must be fixed. Once this compiles again, I'm happy to slap the Ready-For-Final-Review label back on.

I'm on vacation rn so I won't be able to help until june 6th. If we need this earlier, feel free to do it for me :)

@Shatur
Copy link
Contributor

Shatur commented May 26, 2022

I'm on vacation rn so I won't be able to help until june 6th. If we need this earlier, feel free to do it for me :)

No problem, fixed it: #4841.

bors bot pushed a commit that referenced this pull request May 30, 2022
# Objective

- Rebase of #3159.
- Fixes #3156
- add #[inline] to single related functions so that they matches with other function defs

## Solution

* added functions to QueryState
  *  get_single_unchecked_manual
  *  get_single_unchecked
  *  get_single
  *  get_single_mut
  *  single
  *  single_mut
* make Query::get_single use QueryState::get_single_unchecked_manual
* added #[inline]

---

## Changelog

### Added

Functions `QueryState::single`, `QueryState::get_single`, `QueryState::single_mut`, `QueryState::get_single_mut`, `QueryState::get_single_unchecked`, `QueryState::get_single_unchecked_manual`.

### Changed

`QuerySingleError` is now in the `state` module.

## Migration Guide

Change `query::QuerySingleError` to `state::QuerySingleError`


Co-authored-by: 2ne1ugly <chattermin@gmail.com>
Co-authored-by: 2ne1ugly <47616772+2ne1ugly@users.noreply.github.com>
@james7132
Copy link
Member

As #4841 has been merged, closing this one.

@james7132 james7132 closed this May 31, 2022
james7132 pushed a commit to james7132/bevy that referenced this pull request Jun 7, 2022
…e#4841)

# Objective

- Rebase of bevyengine#3159.
- Fixes bevyengine#3156
- add #[inline] to single related functions so that they matches with other function defs

## Solution

* added functions to QueryState
  *  get_single_unchecked_manual
  *  get_single_unchecked
  *  get_single
  *  get_single_mut
  *  single
  *  single_mut
* make Query::get_single use QueryState::get_single_unchecked_manual
* added #[inline]

---

## Changelog

### Added

Functions `QueryState::single`, `QueryState::get_single`, `QueryState::single_mut`, `QueryState::get_single_mut`, `QueryState::get_single_unchecked`, `QueryState::get_single_unchecked_manual`.

### Changed

`QuerySingleError` is now in the `state` module.

## Migration Guide

Change `query::QuerySingleError` to `state::QuerySingleError`


Co-authored-by: 2ne1ugly <chattermin@gmail.com>
Co-authored-by: 2ne1ugly <47616772+2ne1ugly@users.noreply.github.com>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
…e#4841)

# Objective

- Rebase of bevyengine#3159.
- Fixes bevyengine#3156
- add #[inline] to single related functions so that they matches with other function defs

## Solution

* added functions to QueryState
  *  get_single_unchecked_manual
  *  get_single_unchecked
  *  get_single
  *  get_single_mut
  *  single
  *  single_mut
* make Query::get_single use QueryState::get_single_unchecked_manual
* added #[inline]

---

## Changelog

### Added

Functions `QueryState::single`, `QueryState::get_single`, `QueryState::single_mut`, `QueryState::get_single_mut`, `QueryState::get_single_unchecked`, `QueryState::get_single_unchecked_manual`.

### Changed

`QuerySingleError` is now in the `state` module.

## Migration Guide

Change `query::QuerySingleError` to `state::QuerySingleError`


Co-authored-by: 2ne1ugly <chattermin@gmail.com>
Co-authored-by: 2ne1ugly <47616772+2ne1ugly@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-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add single() to QueryState
6 participants