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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Replace WorldQueryGats trait with actual gats #6319

Closed
wants to merge 1 commit into from
Closed

[Merged by Bors] - Replace WorldQueryGats trait with actual gats #6319

wants to merge 1 commit into from

Conversation

BoxyUwU
Copy link
Member

@BoxyUwU BoxyUwU commented Oct 20, 2022

Objective

Replace WorldQueryGats trait with actual gats

Solution

Replace WorldQueryGats trait with actual gats

Changelog

  • Replaced WorldQueryGats trait with actual gats

Migration Guide

  • Replace usage of WorldQueryGats assoc types with the actual gats on WorldQuery trait

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Oct 20, 2022
alice-i-cecile
alice-i-cecile approved these changes Oct 20, 2022
View reviewed changes
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.

Looks good! The WorldQuery trait impls are way more legible.

Great use of deprecation. The actual changes are very mechanical; I'm glad to see you hit the doc strings too though.

@alice-i-cecile alice-i-cecile added the S-Blocked This cannot move forward until something else changes label Oct 20, 2022
@alice-i-cecile
Copy link
Member

This PR is blocked on the next release of Rust, which will stabilize basic GATs.

james7132
james7132 approved these changes Oct 21, 2022
View reviewed changes
Copy link
Member

@james7132 james7132 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

This is so much nicer. My only gripe is that ROQueryItem<'w, Q> to <Q::ReadOnly as WorldQuery>::Item is a readability regression.

crates/bevy_ecs/src/query/fetch.rs Show resolved Hide resolved
@BoxyUwU
Copy link
Member Author

BoxyUwU commented Oct 22, 2022

I'd be fine just keeping QueryItem so that <Q::ReadOnly as WorldQuery>::Item<''w> can be shorter i dont really care what exactly we do 😅

@james7132
Copy link
Member

It's in these cases I wish traits supported default type aliases.

@BoxyUwU
Copy link
Member Author

BoxyUwU commented Oct 23, 2022

default type alias isnt really what we want since that would allow it to be overriden. What you'd want it something that behaves more like:

trait GetReadOnlyItem {
    type ReadOnlyItem<'w>;
    type ReadOnlyFetch<'w>;
}

impl<T: WorldQuery> GetReadOnlyItem for T {
    type ReadOnlyItem<'w> = <T::ReadOnly as WorldQuery>::Item<'w>;
    type ReadOnlyItem<'w> = <T::ReadOnly as WorldQuery>::Fetch<'w>; 
}

@BoxyUwU
Copy link
Member Author

BoxyUwU commented Oct 23, 2022

Actually thats tiny enough of a change that im just Gonna Do That instead of type ROQueryItem<'a, Q>

@BoxyUwU
Copy link
Member Author

BoxyUwU commented Oct 23, 2022

Wow that incredibly did not work and I have no idea why, guess I'm not doing that lol

@BoxyUwU BoxyUwU force-pushed the remove_worldquerygats branch 3 times, most recently from ff44f19 to 32d4125 Compare October 24, 2022 23:49
@BoxyUwU
Copy link
Member Author

BoxyUwU commented Oct 25, 2022

decided against removing QueryFetch and ROQueryFetch as until my PR to remove the Clone bounds on Fetch from iter_combinations people will be required to put where ROQueryFetch<'a, Q>: Clone or where QueryFetch<'a, Q>: Clone in functions that call iter_combinations on generic queries. Would rather not force people to do <<Q as WorldQuery>::ReadOnly as WorldQuery>::Fetch<'a>: Clone

@BoxyUwU BoxyUwU force-pushed the remove_worldquerygats branch 3 times, most recently from 4239f26 to 6f6746c Compare October 26, 2022 21:23
@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 Oct 30, 2022
@james7132 james7132 added this to the Bevy 0.9 milestone Nov 2, 2022
@BoxyUwU BoxyUwU force-pushed the remove_worldquerygats branch from 6f6746c to 687800e Compare November 3, 2022 14:39
@BoxyUwU
Copy link
Member Author

BoxyUwU commented Nov 3, 2022

looked over the changes after rebasing and they still seem correct to me

@alice-i-cecile alice-i-cecile removed the S-Blocked This cannot move forward until something else changes label Nov 3, 2022
@BoxyUwU BoxyUwU force-pushed the remove_worldquerygats branch from 687800e to 47381fc Compare November 3, 2022 15:33
alice-i-cecile
alice-i-cecile approved these changes Nov 3, 2022
View reviewed changes
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.

Rebase LGTM.

@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Nov 3, 2022
@BoxyUwU
Replace WorldQueryGats trait with actual gats (#6319)
30e3576 
# Objective

Replace `WorldQueryGats` trait with actual gats

## Solution

Replace `WorldQueryGats` trait with actual gats

---

## Changelog

- Replaced `WorldQueryGats` trait with actual gats

## Migration Guide

- Replace usage of `WorldQueryGats` assoc types with the actual gats on `WorldQuery` trait
@bors
Copy link
Contributor

bors bot commented Nov 3, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Replace WorldQueryGats trait with actual gats [Merged by Bors] - Replace WorldQueryGats trait with actual gats Nov 3, 2022
@bors bors bot closed this Nov 3, 2022
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@BoxyUwU @ItsDoot
Replace WorldQueryGats trait with actual gats (bevyengine#6319)
af5962c 
# Objective

Replace `WorldQueryGats` trait with actual gats

## Solution

Replace `WorldQueryGats` trait with actual gats

---

## Changelog

- Replaced `WorldQueryGats` trait with actual gats

## Migration Guide

- Replace usage of `WorldQueryGats` assoc types with the actual gats on `WorldQuery` trait
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tim-blackbird tim-blackbird tim-blackbird approved these changes

@JoJoJet JoJoJet JoJoJet approved these changes

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@james7132 james7132 james7132 approved these changes

@TheRawMeatball TheRawMeatball Awaiting requested review from TheRawMeatball

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide 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
Milestone
Bevy 0.9
Development

Successfully merging this pull request may close these issues.
5 participants
@BoxyUwU @alice-i-cecile @james7132 @JoJoJet @tim-blackbird