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] - Implement IntoIterator for ECS wrapper types. #5096

Closed
wants to merge 6 commits into from
Closed

[Merged by Bors] - Implement IntoIterator for ECS wrapper types. #5096

wants to merge 6 commits into from

Conversation

tim-blackbird
Copy link
Contributor

@tim-blackbird tim-blackbird commented Jun 25, 2022 •
edited
Loading

Objective

Improve ergonomics by passing on the IntoIterator impl of the underlying type to wrapper types.

Solution

Implement IntoIterator for ECS wrapper types (Mut, Local, Res, etc.).

@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 labels Jun 25, 2022
alice-i-cecile
alice-i-cecile requested changes Jun 25, 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.

I'm on board once the trait bounds are cleaned up. Calling IntoIterator on a collection of Mut / ResMut will trigger change detection on all of them, but that should be relatively obvious to users who are at a point where they care.

alice-i-cecile
alice-i-cecile approved these changes Jun 25, 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.

Formatting failure :p You have my failure, but you still need the machine's.

@tim-blackbird
Copy link
Contributor Author

tim-blackbird commented Jul 3, 2022 •
edited
Loading

I've decided to yeet the IntoIterator impl on Windows as it's impossible to do without exposing implementation details,
and there are a ton of other types that should implement IntoIterator too (EventReader, etc.) but It's a giant pain to do without TAIT.
Lets keep this simple and leave all that for the future.

@tim-blackbird tim-blackbird mentioned this pull request Jul 10, 2022
Closed
james7132
james7132 approved these changes Oct 20, 2022
View reviewed changes
Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

LGTM. Love the improvement in ergonomics. Should save some extra fenangling when using these traits.

crates/bevy_ecs/src/change_detection.rs Outdated Show resolved Hide resolved
@james7132 james7132 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 20, 2022
@alice-i-cecile
Copy link
Member

bors r+

1 similar comment
@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Oct 24, 2022
@tim-blackbird
Implement IntoIterator for ECS wrapper types. (#5096)
b291223 
# Objective

Improve ergonomics by passing on the `IntoIterator` impl of the underlying type to wrapper types.

## Solution

Implement `IntoIterator` for ECS wrapper types (Mut, Local, Res, etc.).

Co-authored-by: devil-ira <justthecooldude@gmail.com>
@bors
Copy link
Contributor

bors bot commented Oct 24, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Implement IntoIterator for ECS wrapper types. [Merged by Bors] - Implement IntoIterator for ECS wrapper types. Oct 24, 2022
@bors bors bot closed this Oct 24, 2022
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
@tim-blackbird @james7132
Implement IntoIterator for ECS wrapper types. (bevyengine#5096)
6cfc2d9 
# Objective

Improve ergonomics by passing on the `IntoIterator` impl of the underlying type to wrapper types.

## Solution

Implement `IntoIterator` for ECS wrapper types (Mut, Local, Res, etc.).

Co-authored-by: devil-ira <justthecooldude@gmail.com>
@tim-blackbird tim-blackbird deleted the intoiterator-ergo branch November 6, 2022 18:22
Pietrek14 pushed a commit to Pietrek14/bevy that referenced this pull request Dec 17, 2022
@tim-blackbird @Pietrek14
Implement IntoIterator for ECS wrapper types. (bevyengine#5096)
f91e5af 
# Objective

Improve ergonomics by passing on the `IntoIterator` impl of the underlying type to wrapper types.

## Solution

Implement `IntoIterator` for ECS wrapper types (Mut, Local, Res, etc.).

Co-authored-by: devil-ira <justthecooldude@gmail.com>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@tim-blackbird @ItsDoot
Implement IntoIterator for ECS wrapper types. (bevyengine#5096)
2b1ae15 
# Objective

Improve ergonomics by passing on the `IntoIterator` impl of the underlying type to wrapper types.

## Solution

Implement `IntoIterator` for ECS wrapper types (Mut, Local, Res, etc.).

Co-authored-by: devil-ira <justthecooldude@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoJoJet JoJoJet JoJoJet left review comments

@mockersf mockersf mockersf approved these changes

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

@james7132 james7132 james7132 approved these changes

@DJMcNab DJMcNab Awaiting requested review from DJMcNab

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use 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
No milestone
Development

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