[Merged by Bors] - impl `Reflect` for `std::collections::HashMap` instead of only `bevy::utils::HashMap` (#7739) #7782

13ros27 · 2023-02-21T21:19:48Z

Objective

Implement Reflect for std::collections::HashMap<K, V, S> as well as hashbrown::HashMap<K, V, S> rather than just for hashbrown::HashMap<K, V, RandomState>. Fixes #7739.

Solution

Rather than implementing on HashMap<K, V> I instead implemented most of the related traits on HashMap<K, V, S> where S: BuildHasher + Send + Sync + 'static and then FromReflect also needs the extra bound S: Default because it needs to use with_capacity_and_hasher so needs to be able to generate a default hasher.

As the API of hashbrown::HashMap is identical to collections::HashMap making them both work just required creating an impl_reflect_for_hashmap macro like the impl_reflect_for_veclike above and then applying this to both HashMaps.

Changelog

std::collections::HashMap can now be reflected. Also more State generics than just RandomState can now be reflected for both hashbrown::HashMap and collections::HashMap

…omState> (bevyengine#7739) This changes the Map, Reflect, Typed, GetTypeRegistration and FromReflect impls for HashMap<K, V> to HashMap<K, V, S> as mentioned in a comment to bevyengine#7739 so that HashMaps with a State type other than RandomState can be reflected.

Using a similar macro to impl_reflect_for_veclike above, Reflect is now implemented for std::collections::HashMap as well as hashbrown::HashMap. Fixes bevyengine#7739.

github-actions · 2023-02-21T21:20:02Z

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

PROMETHIA-27

If I'm not mistaken, bevy_utils::hashbrown::HashMap and the std one are the same type. Can you just implement on std with the appropriate bounds and cover both?

13ros27 · 2023-02-21T21:58:21Z

If I remove the explicit hashbrown impl and just leave the collections one it starts erroring in bevy_input/src/gamepad with 'the trait Reflect is not implemented for bevy_utils::hashbrown::HashMap<GamepadButton, ButtonAxisSettings>'. From what I can tell although hashbrown is used in rust now (since 1.36) bevy still imports hashbrown and re-exports that from bevy_utils.

PROMETHIA-27 · 2023-02-21T22:00:31Z

I think we should probably stop using hashbrown directly then, I thought all utils was doing was swapping out the hasher, not the entire type.

13ros27 · 2023-02-21T23:30:03Z

With a couple of changes to the example (which I think is just due to breaking changes), this also fixes #1044 (makes it compile). Specifically the changes are App::build() -> App::new() and deriving FromReflect on Thing.

MrGVSV · 2023-02-22T01:35:19Z

I think we should probably stop using hashbrown directly then, I thought all utils was doing was swapping out the hasher, not the entire type.

This can probably be done in a separate PR. I think this should at least solve the immediate issue of not being able to use the standard library HashMap with Reflect.

james7132 · 2023-02-22T03:12:50Z

I think we should probably stop using hashbrown directly then, I thought all utils was doing was swapping out the hasher, not the entire type.

Currently impossible, we're using the unstable/unexposed entry API of hashbrown for performance reasons. It also removes one std dependency on lower level crates that might be no_std compatible (i.e. potentially bevy_ecs).

james7132

Generally LGTM. The PR has quite a bit a noise due to the increased indentation, but seems to be identical sans being generic over the HashMap state type and the macro to support both hashbrown and std.

PROMETHIA-27

LGTM

james7132 · 2023-02-27T21:04:22Z

bors r+

…:utils::HashMap` (#7739) (#7782) # Objective Implement `Reflect` for `std::collections::HashMap<K, V, S>` as well as `hashbrown::HashMap<K, V, S>` rather than just for `hashbrown::HashMap<K, V, RandomState>`. Fixes #7739. ## Solution Rather than implementing on `HashMap<K, V>` I instead implemented most of the related traits on `HashMap<K, V, S> where S: BuildHasher + Send + Sync + 'static` and then `FromReflect` also needs the extra bound `S: Default` because it needs to use `with_capacity_and_hasher` so needs to be able to generate a default hasher. As the API of `hashbrown::HashMap` is identical to `collections::HashMap` making them both work just required creating an `impl_reflect_for_hashmap` macro like the `impl_reflect_for_veclike` above and then applying this to both HashMaps. --- ## Changelog `std::collections::HashMap` can now be reflected. Also more `State` generics than just `RandomState` can now be reflected for both `hashbrown::HashMap` and `collections::HashMap`

bors · 2023-02-27T21:07:37Z

Build failed:

markdownlint

james7132 · 2023-02-27T21:37:19Z

bors retry

…:utils::HashMap` (#7739) (#7782) # Objective Implement `Reflect` for `std::collections::HashMap<K, V, S>` as well as `hashbrown::HashMap<K, V, S>` rather than just for `hashbrown::HashMap<K, V, RandomState>`. Fixes #7739. ## Solution Rather than implementing on `HashMap<K, V>` I instead implemented most of the related traits on `HashMap<K, V, S> where S: BuildHasher + Send + Sync + 'static` and then `FromReflect` also needs the extra bound `S: Default` because it needs to use `with_capacity_and_hasher` so needs to be able to generate a default hasher. As the API of `hashbrown::HashMap` is identical to `collections::HashMap` making them both work just required creating an `impl_reflect_for_hashmap` macro like the `impl_reflect_for_veclike` above and then applying this to both HashMaps. --- ## Changelog `std::collections::HashMap` can now be reflected. Also more `State` generics than just `RandomState` can now be reflected for both `hashbrown::HashMap` and `collections::HashMap`

bors · 2023-02-27T21:54:30Z

Pull request successfully merged into main.

Build succeeded:

build-and-install-on-iOS
build-android
build (macos-latest)
build (ubuntu-latest)
build-wasm
build (windows-latest)
build-without-default-features (bevy)
build-without-default-features (bevy_ecs)
build-without-default-features (bevy_reflect)
check-compiles
check-doc
check-missing-examples-in-docs
ci
markdownlint
msrv
run-examples
run-examples-on-wasm
run-examples-on-windows-dx12

…:utils::HashMap` (bevyengine#7739) (bevyengine#7782) # Objective Implement `Reflect` for `std::collections::HashMap<K, V, S>` as well as `hashbrown::HashMap<K, V, S>` rather than just for `hashbrown::HashMap<K, V, RandomState>`. Fixes bevyengine#7739. ## Solution Rather than implementing on `HashMap<K, V>` I instead implemented most of the related traits on `HashMap<K, V, S> where S: BuildHasher + Send + Sync + 'static` and then `FromReflect` also needs the extra bound `S: Default` because it needs to use `with_capacity_and_hasher` so needs to be able to generate a default hasher. As the API of `hashbrown::HashMap` is identical to `collections::HashMap` making them both work just required creating an `impl_reflect_for_hashmap` macro like the `impl_reflect_for_veclike` above and then applying this to both HashMaps. --- ## Changelog `std::collections::HashMap` can now be reflected. Also more `State` generics than just `RandomState` can now be reflected for both `hashbrown::HashMap` and `collections::HashMap`

13ros27 added 2 commits February 21, 2023 20:38

Implement Reflect for std::collections::HashMap (bevyengine#7739).

abcbf85

Using a similar macro to impl_reflect_for_veclike above, Reflect is now implemented for std::collections::HashMap as well as hashbrown::HashMap. Fixes bevyengine#7739.

13ros27 changed the title ~~impl Reflect for std::collections::HashMap instead of only bevy::utils::HashMap~~ impl Reflect for std::collections::HashMap instead of only bevy::utils::HashMap (#7739) Feb 21, 2023

alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types labels Feb 21, 2023

alice-i-cecile requested review from MrGVSV and PROMETHIA-27 February 21, 2023 21:36

PROMETHIA-27 suggested changes Feb 21, 2023

View reviewed changes

MrGVSV approved these changes Feb 22, 2023

View reviewed changes

james7132 self-requested a review February 22, 2023 03:13

james7132 approved these changes Feb 27, 2023

View reviewed changes

james7132 requested a review from PROMETHIA-27 February 27, 2023 07:16

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 Feb 27, 2023

james7132 added this to the 0.10 milestone Feb 27, 2023

PROMETHIA-27 approved these changes Feb 27, 2023

View reviewed changes

bors bot changed the title ~~impl Reflect for std::collections::HashMap instead of only bevy::utils::HashMap (#7739)~~ [Merged by Bors] - impl Reflect for std::collections::HashMap instead of only bevy::utils::HashMap (#7739) Feb 27, 2023

bors bot closed this Feb 27, 2023

13ros27 deleted the hashmap-reflect branch February 27, 2023 22:01

Uh oh!

[Merged by Bors] - impl Reflect for std::collections::HashMap instead of only bevy::utils::HashMap (#7739) #7782

[Merged by Bors] - impl Reflect for std::collections::HashMap instead of only bevy::utils::HashMap (#7739) #7782

Uh oh!

Conversation

13ros27 commented Feb 21, 2023

Objective

Solution

Changelog

Uh oh!

github-actions bot commented Feb 21, 2023

Uh oh!

PROMETHIA-27 left a comment

Choose a reason for hiding this comment

Uh oh!

13ros27 commented Feb 21, 2023

Uh oh!

PROMETHIA-27 commented Feb 21, 2023

Uh oh!

13ros27 commented Feb 21, 2023

Uh oh!

MrGVSV commented Feb 22, 2023

Uh oh!

james7132 commented Feb 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

james7132 left a comment

Choose a reason for hiding this comment

Uh oh!

PROMETHIA-27 left a comment

Choose a reason for hiding this comment

Uh oh!

james7132 commented Feb 27, 2023

Uh oh!

bors bot commented Feb 27, 2023

Uh oh!

james7132 commented Feb 27, 2023

Uh oh!

bors bot commented Feb 27, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

[Merged by Bors] - impl `Reflect` for `std::collections::HashMap` instead of only `bevy::utils::HashMap` (#7739) #7782

[Merged by Bors] - impl `Reflect` for `std::collections::HashMap` instead of only `bevy::utils::HashMap` (#7739) #7782

james7132 commented Feb 22, 2023 •

edited

Loading