bevyengine
Migrate BuildChildren to impl IntoIterator<Item = impl Borrow<Entity>>
Objective
Fixes https://github.com/bevyengine/bevy/issues/7229
This PR picks up where https://github.com/bevyengine/bevy/pull/7244 leaves off. In this PR we migrate the remaining methods on the BuildChildren trait to use children: impl IntoIterator<Item = Entity> instead of children: &[Entity]. In the process we change the move semantics. (I only created this PR when I saw that the earlier one had been dormant for several weeks.)
cc @LamequeFernandes, @adam-shih, @XaurDesu
Open Questions
- [x] Should other traits in this file should be migrated as well?
- [x] Should some of the methods use
impl Iterator<Item = &'e Entity>instead, in order to avoid consuming the input? - [x] What if anything should go in the Migration Guide section?
Changelog
Changed: Methods on BuildWorldChildren and BuildChildren that accept &[Entity] updated to take impl IntoIterator<Item = impl Borrow<Entity>> instead.
Migration Guide
N/A
Welcome, new contributor!
Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
It looks like your PR is a breaking change, but you didn't provide a migration guide.
Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.
@devil-ira sounds good. Just so I don't go off on a tangent, can you link to an example or two of methods you want to modify? I looked in crates/bevy_ecs/src/world/entity_ref.rs, and the only function I found there with a slice argument was sorted_remove. But perhaps you have something more general in mind.
@devil-ira, @james7132 any thoughts on the questions I added to the description?
Ah, sorry I should have been more clear. I was referring to the BuildWorldChildren impl for EntityMut in the same file (child_builder.rs)
Seems there's some errors that are being picked up in CI that I don't see in my smoke tests when running things locally. This evening or tomorrow I'll try to reproduce what CI is doing.
I was able to mostly repro what CI is doing by updating to the latest stable version using rustup and then running this command:
$ cargo run -p ci -- lints
I say "mostly" because the linter wanted me to fix a linting issue in crates/bevy_reflect/src/enums/dynamic_enum.rs, which wasn't touched in this PR, so I suspect that my local run isn't quite the same as CI on GitHub.
@devil-ira is this good to go, or is there something else that will be needed?
Should some of the methods use impl Iterator<Item = &'e Entity> instead, in order to avoid consuming the input?
What you could use is impl IntoIterator<Item = impl Borrow<Entity>> which accepts both owned values and references. That should avoid this being breaking change as well.
This does mean you have to do SmallVec::from_iter(children.into_iter().map(|e| *e.borrow())) instead.
What you could use is impl IntoIterator<Item = impl Borrow<Entity>> which accepts both owned values and references.
Done.
As I work through this PR, I'm beginning to wonder whether it improves upon the existing ergonomics, or whether it makes it harder to work with the children argument than when it was passed as a &[Entity] slice.
Closing per this discussion: https://github.com/bevyengine/bevy/issues/7229#issuecomment-1465434249