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

Implement FromIterator/IntoIterator for dynamic types #14250

Merged

Conversation

SpecificProtagonist
Copy link
Contributor

@SpecificProtagonist SpecificProtagonist commented Jul 9, 2024

Objective

Implement FromIterator/IntoIterator for dynamic types where missing

Note:

  • can't impl IntoIterator for &Array & co because of orphan rules
  • into_iter().collect() is a no-op for Vecs because of specialization

Migration Guide

  • Change DynamicArray::from_vec to DynamicArray::from_iter

@MrGVSV MrGVSV added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 9, 2024
Copy link
Member

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

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

Awesome! I think these changes will be super useful!

@@ -508,6 +508,44 @@ impl Debug for DynamicStruct {
}
}

impl FromIterator<(String, Box<dyn Reflect>)> for DynamicStruct {
Copy link
Member

Choose a reason for hiding this comment

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

These are cool! I've been wanting a succinct way to create DynamicStruct and friends for a long time haha

crates/bevy_reflect/src/struct_trait.rs Outdated Show resolved Hide resolved
crates/bevy_reflect/src/struct_trait.rs Outdated Show resolved Hide resolved
@MrGVSV
Copy link
Member

MrGVSV commented Jul 9, 2024

@SpecificProtagonist could you also update the title and description to account for your new changes?

@SpecificProtagonist SpecificProtagonist changed the title DynamicArray::from_vec: Take IntoIterator Implement FromIterator/IntoIterator for dynamic types Jul 9, 2024
@SpecificProtagonist
Copy link
Contributor Author

SpecificProtagonist commented Jul 9, 2024

@SpecificProtagonist could you also update the title and description to account for your new changes?

Yep – already updated the description; accidentally cancelled the edit to the title :3

Do you know what's going on with the compile error? It doesn't appear locally for me, and doesn't make any sense to me either: It says that the return type of Box::<[Box<dyn Reflect>]>::into_iter is [Box<dyn Reflect>], which isn't the case.

@MrGVSV
Copy link
Member

MrGVSV commented Jul 10, 2024

Do you know what's going on with the compile error? It doesn't appear locally for me, and doesn't make any sense to me either: It says that the return type of Box::<[Box<dyn Reflect>]>::into_iter is [Box<dyn Reflect>], which isn't the case.

Looks like IntoIterator for Box<[T]> was recently merged and set to land in Rust 1.80.

For now, I'd recommend just doing:

self.values.into_vec().into_iter()

It might be a good idea also leaving a FIXME comment that links to that PR or maybe a Bevy issue to update it once 1.80 is out (in the next few weeks I think).

@SpecificProtagonist
Copy link
Contributor Author

Thanks! Fixed it. Didn't leave a todo as the resulting code is fine.

I've also removed the homogeneous FromIterator impls from the types intended to be heterogeneous.

crates/bevy_reflect/src/array.rs Show resolved Hide resolved
crates/bevy_reflect/src/map.rs Show resolved Hide resolved
Copy link
Contributor

@mweatherley mweatherley left a comment

Choose a reason for hiding this comment

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

A couple minor comments, but nothing worth blocking over. Good feature!

@mweatherley mweatherley added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 12, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jul 15, 2024
Merged via the queue into bevyengine:main with commit ab255ae Jul 15, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Reflection Runtime information about types C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

4 participants