make EntityHashMap and EntityHashSet proper types #16912

Victoronz · 2024-12-20T18:08:33Z

Objective

EntityHashMap and EntityHashSet iterators do not implement EntitySetIterator.

Solution

Make them newtypes instead of aliases. The methods that create the iterators can then produce their own newtypes that carry the Hasher generic and implement EntitySetIterator. Functionality remains the same otherwise.
There are some other small benefits, f.e. the removal of with_hasher associated functions, and the ability to implement more traits ourselves.

MainEntityHashMap and MainEntityHashSet are currently left as the previous type aliases, because supporting general TrustedEntityBorrow hashing is more complex. However, it can also be done.

Testing

Pre-existing EntityHashMap tests.

Migration Guide

Users of with_hasher and with_capacity_and_hasher on EntityHashMap/Set must now use new and with_capacity respectively.
If the non-newtyped versions are required, they can be obtained via Deref, DerefMut or into_inner calls.

crates/bevy_ecs/src/entity/hash_map.rs

chescock · 2024-12-20T18:52:19Z

crates/bevy_ecs/src/entity/hash_map.rs

+/// /// This struct is created by the [`keys`] method on [`EntityHashMap`]. See its documentation for more.
+///
+/// [`keys`]: EntityHashMap::keys
+pub struct Keys<'a, V, S = EntityHash>(hash_map::Keys<'a, Entity, V>, PhantomData<S>);


Why do you need the S type parameter and PhantomData? It looks like this is only ever instantiated with S = EntityHash.

(And the same question for the other types with a PhantomData.)

It is! This S generic parameter is currently only here to express the purpose of these newtypes to the readers:
Technically a newtype alone is enough, but then the type will not communicate why it exists.
This is especially the case for the EntitySetIterator impls.

In the future we might also want to support more hashers, as we currently have multiple "trusted" Hashers in the engine. However I think I'll leave that for when we support hashing more TrustedEntityBorrow types.
(That allows for switching MainEntityHashMap/Set as well)

chescock · 2024-12-20T19:17:53Z

crates/bevy_ecs/src/entity/hash_set.rs

+    /// Clears the set, returning all elements in an iterator.
+    ///
+    /// Equivalent to [`HashSet::drain`].
+    pub fn drain(&mut self) -> Drain<'_> {


Do you also want a wrapper for ExtractIf?

alice-i-cecile

Annoying to need more code for this, but this is well-executed and I agree with the justification.

Victoronz · 2024-12-20T20:58:27Z

Yeah, all this code and the upcoming EntityIndexMap/Set versions could be removed if the hashbrown iterators carried the Hasher generic, which for them would probably be a small non-breaking change to a few dozen lines at most.

# Objective `EntityHashMap` and `EntityHashSet` iterators do not implement `EntitySetIterator`. ## Solution Make them newtypes instead of aliases. The methods that create the iterators can then produce their own newtypes that carry the `Hasher` generic and implement `EntitySetIterator`. Functionality remains the same otherwise. There are some other small benefits, f.e. the removal of `with_hasher` associated functions, and the ability to implement more traits ourselves. `MainEntityHashMap` and `MainEntityHashSet` are currently left as the previous type aliases, because supporting general `TrustedEntityBorrow` hashing is more complex. However, it can also be done. ## Testing Pre-existing `EntityHashMap` tests. ## Migration Guide Users of `with_hasher` and `with_capacity_and_hasher` on `EntityHashMap`/`Set` must now use `new` and `with_capacity` respectively. If the non-newtyped versions are required, they can be obtained via `Deref`, `DerefMut` or `into_inner` calls.

# Objective We do not have `EntityIndexMap`/`EntityIndexSet`. Usual `HashMap`s/`HashSet`s do not guarantee any order, which can be awkward for some use cases. The `indexmap` versions remember insertion order, which then also becomes their iteration order. They can be thought of as a `HashTable` + `Vec`, which means fast iteration and removal, indexing by index (not just key), and slicing! Performance should otherwise be comparable. ## Solution Because `indexmap` is structured to mirror `hashbrown`, it suffers the same issue of not having the `Hasher` generic on their iterators. #16912 solved this issue for `EntityHashMap`/`EntityHashSet` with a wrapper around the hashbrown version, so this PR does the same. Hopefully these wrappers can be removed again in the future by having `hashbrown`/`indexmap` adopt that generic in their iterators themselves!

# Objective #16912 turned `EntityHashMap` and `EntityHashSet` into proper newtypes instead of type aliases. However, this removed the ability to create these collections in const contexts; previously, you could use `EntityHashSet::with_hasher(EntityHash)`, but it doesn't exist anymore. ## Solution Make `EntityHashMap::new` and `EntityHashSet::new` const methods.

# Objective Follow-up to #17615. Bevy's entity collection types like `EntityHashSet` no longer implement serde's `Serialize` and `Deserialize` after becoming newtypes instead of type aliases in #16912. This broke some types that support serde for me in Avian. I also missed creating const constructors for `EntityIndexMap` and `EntityIndexSet` in #17615. Oops! ## Solution Implement `Serialize` and `Deserialize` for Bevy's entity collection types, and add const constructors for `EntityIndexMap` and `EntityIndexSet`. I didn't implement `ReflectSerialize` or `ReflectDeserialize` here, because I had some trouble fixing the resulting errors, and they were not implemented previously either.

# Objective `EntityHashMap` and `EntityHashSet` iterators do not implement `EntitySetIterator`. ## Solution Make them newtypes instead of aliases. The methods that create the iterators can then produce their own newtypes that carry the `Hasher` generic and implement `EntitySetIterator`. Functionality remains the same otherwise. There are some other small benefits, f.e. the removal of `with_hasher` associated functions, and the ability to implement more traits ourselves. `MainEntityHashMap` and `MainEntityHashSet` are currently left as the previous type aliases, because supporting general `TrustedEntityBorrow` hashing is more complex. However, it can also be done. ## Testing Pre-existing `EntityHashMap` tests. ## Migration Guide Users of `with_hasher` and `with_capacity_and_hasher` on `EntityHashMap`/`Set` must now use `new` and `with_capacity` respectively. If the non-newtyped versions are required, they can be obtained via `Deref`, `DerefMut` or `into_inner` calls.

# Objective We do not have `EntityIndexMap`/`EntityIndexSet`. Usual `HashMap`s/`HashSet`s do not guarantee any order, which can be awkward for some use cases. The `indexmap` versions remember insertion order, which then also becomes their iteration order. They can be thought of as a `HashTable` + `Vec`, which means fast iteration and removal, indexing by index (not just key), and slicing! Performance should otherwise be comparable. ## Solution Because `indexmap` is structured to mirror `hashbrown`, it suffers the same issue of not having the `Hasher` generic on their iterators. bevyengine#16912 solved this issue for `EntityHashMap`/`EntityHashSet` with a wrapper around the hashbrown version, so this PR does the same. Hopefully these wrappers can be removed again in the future by having `hashbrown`/`indexmap` adopt that generic in their iterators themselves!

…17615) # Objective bevyengine#16912 turned `EntityHashMap` and `EntityHashSet` into proper newtypes instead of type aliases. However, this removed the ability to create these collections in const contexts; previously, you could use `EntityHashSet::with_hasher(EntityHash)`, but it doesn't exist anymore. ## Solution Make `EntityHashMap::new` and `EntityHashSet::new` const methods.

…e#17620) # Objective Follow-up to bevyengine#17615. Bevy's entity collection types like `EntityHashSet` no longer implement serde's `Serialize` and `Deserialize` after becoming newtypes instead of type aliases in bevyengine#16912. This broke some types that support serde for me in Avian. I also missed creating const constructors for `EntityIndexMap` and `EntityIndexSet` in bevyengine#17615. Oops! ## Solution Implement `Serialize` and `Deserialize` for Bevy's entity collection types, and add const constructors for `EntityIndexMap` and `EntityIndexSet`. I didn't implement `ReflectSerialize` or `ReflectDeserialize` here, because I had some trouble fixing the resulting errors, and they were not implemented previously either.

…17615) # Objective bevyengine#16912 turned `EntityHashMap` and `EntityHashSet` into proper newtypes instead of type aliases. However, this removed the ability to create these collections in const contexts; previously, you could use `EntityHashSet::with_hasher(EntityHash)`, but it doesn't exist anymore. ## Solution Make `EntityHashMap::new` and `EntityHashSet::new` const methods.

…e#17620) # Objective Follow-up to bevyengine#17615. Bevy's entity collection types like `EntityHashSet` no longer implement serde's `Serialize` and `Deserialize` after becoming newtypes instead of type aliases in bevyengine#16912. This broke some types that support serde for me in Avian. I also missed creating const constructors for `EntityIndexMap` and `EntityIndexSet` in bevyengine#17615. Oops! ## Solution Implement `Serialize` and `Deserialize` for Bevy's entity collection types, and add const constructors for `EntityIndexMap` and `EntityIndexSet`. I didn't implement `ReflectSerialize` or `ReflectDeserialize` here, because I had some trouble fixing the resulting errors, and they were not implemented previously either.

Victoronz added 4 commits December 20, 2024 18:53

make EntityHashMap and EntityHashSet proper types

6c70d79

derive Debug on EntityHash

f5586a4

ci

99df261

cargo fmt

77f982f

doc link cleanup

599bf31

chescock reviewed Dec 20, 2024

View reviewed changes

address comments

398f96f

chescock approved these changes Dec 20, 2024

View reviewed changes

Victoronz requested a review from alice-i-cecile December 20, 2024 20:13

alice-i-cecile added the M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Dec 20, 2024

alice-i-cecile approved these changes Dec 20, 2024

View reviewed changes

alice-i-cecile added this pull request to the merge queue Dec 20, 2024

Merged via the queue into bevyengine:main with commit 8ac90ac Dec 20, 2024
29 checks passed

Victoronz mentioned this pull request Jan 20, 2025

implement EntityIndexMap/Set #17449

Merged

Jondolf mentioned this pull request Jan 30, 2025

Make EntityHashMap::new and EntityHashSet::new const #17615

Merged

Jondolf mentioned this pull request Jan 31, 2025

Implement Serialize/Deserialize for entity collections #17620

Merged

Victoronz mentioned this pull request Apr 5, 2025

Newtype hashbrown #18694

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

make EntityHashMap and EntityHashSet proper types #16912

make EntityHashMap and EntityHashSet proper types #16912

Uh oh!

Victoronz commented Dec 20, 2024 •

edited

Loading

Uh oh!

Uh oh!

chescock Dec 20, 2024

Uh oh!

Victoronz Dec 20, 2024

Uh oh!

Victoronz Dec 20, 2024

Uh oh!

chescock Dec 20, 2024

Uh oh!

Victoronz Dec 20, 2024

Uh oh!

alice-i-cecile left a comment

Uh oh!

Victoronz commented Dec 20, 2024 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

make EntityHashMap and EntityHashSet proper types #16912

make EntityHashMap and EntityHashSet proper types #16912

Uh oh!

Conversation

Victoronz commented Dec 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Testing

Migration Guide

Uh oh!

Uh oh!

chescock Dec 20, 2024

Choose a reason for hiding this comment

Uh oh!

Victoronz Dec 20, 2024

Choose a reason for hiding this comment

Uh oh!

Victoronz Dec 20, 2024

Choose a reason for hiding this comment

Uh oh!

chescock Dec 20, 2024

Choose a reason for hiding this comment

Uh oh!

Victoronz Dec 20, 2024

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

Victoronz commented Dec 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Victoronz commented Dec 20, 2024 •

edited

Loading

Victoronz commented Dec 20, 2024 •

edited

Loading