Skip to content

General entity set cleanup #21498

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

Merged
alice-i-cecile merged 12 commits into from
May 4, 2026
Merged

General entity set cleanup #21498

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
513da58
privatize entity set type fields
Victoronz Oct 10, 2025
4e08725
entity set constructor consistency
Victoronz Oct 10, 2025
bb95355
inline entity set logic
Victoronz Oct 10, 2025
dceec17
stop internally skipping entity set type construction safety
Victoronz Oct 10, 2025
68546a9
turn various entity set functions const
Victoronz Oct 10, 2025
1f1d205
miscellaneous simplifications
Victoronz Oct 10, 2025
a1530b0
doc cleanup
Victoronz Oct 10, 2025
3c0c67a
update bevy_ecs msrv to 1.89
Victoronz Oct 10, 2025
ac73915
Merge branch 'main' into general-entity-set-cleanup
Victoronz Apr 26, 2026
f390dd3
Merge branch 'main' into general-entity-set-cleanup
Victoronz Apr 30, 2026
a77aaaa
remove the inlines again
Victoronz Apr 30, 2026
4c5ccaa
remove unnecessary turbofish
Victoronz May 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 38 additions & 10 deletions crates/bevy_ecs/src/entity/entity_set.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use core::{
fmt::{Debug, Formatter},
hash::{BuildHasher, Hash},
iter::{self, FusedIterator},
option, result,
option, ptr, result,
};

use super::{Entity, UniqueEntityEquivalentSlice};
Expand Down Expand Up @@ -58,7 +58,7 @@ pub trait ContainsEntity {
/// To obtain hash values forming the same total order as [`Entity`], any [`Hasher`] used must be
/// deterministic and concerning [`Entity`], collisionless.
/// Standard library hash collections handle collisions with an [`Eq`] fallback, but do not account for
/// determinism when [`BuildHasher`] is unspecified,.
/// determinism when [`BuildHasher`] is unspecified.
///
/// [`Hash`]: core::hash::Hash
/// [`Hasher`]: core::hash::Hasher
Expand Down Expand Up @@ -143,12 +143,12 @@ unsafe impl<T: EntityEquivalent> EntityEquivalent for Arc<T> {}
/// As a consequence, [`into_iter()`] on `EntitySet` will always produce another `EntitySet`.
///
/// Implementing this trait allows for unique query iteration over a list of entities.
/// See [`iter_many_unique`] and [`iter_many_unique_mut`]
/// See [`iter_many_unique`] and [`iter_many_unique_mut`].
///
/// Note that there is no guarantee of the [`IntoIterator`] impl being deterministic,
/// it might return different iterators when called multiple times.
/// Neither is there a guarantee that the comparison trait impls of `EntitySet` match that
/// of the respective [`EntitySetIterator`] (or of a [`Vec`] collected from its elements)
/// of the respective [`EntitySetIterator`] (or of a [`Vec`] collected from its elements).
///
/// [`Self::IntoIter`]: IntoIterator::IntoIter
/// [`into_iter()`]: IntoIterator::into_iter
Expand Down Expand Up @@ -342,6 +342,7 @@ pub trait FromEntitySetIterator<A: EntityEquivalent>: FromIterator<A> {
impl<T: EntityEquivalent + Hash, S: BuildHasher + Default> FromEntitySetIterator<T>
for HashSet<T, S>
{
#[inline]
fn from_entity_set_iter<I: EntitySet<Item = T>>(set_iter: I) -> Self {
let iter = set_iter.into_iter();
let set = HashSet::<T, S>::with_capacity_and_hasher(iter.size_hint().0, S::default());
Expand All @@ -358,14 +359,17 @@ impl<T: EntityEquivalent + Hash, S: BuildHasher + Default> FromEntitySetIterator
/// An iterator that yields unique entities.
///
/// This wrapper can provide an [`EntitySetIterator`] implementation when an instance of `I` is known to uphold uniqueness.
#[repr(transparent)]
Comment thread
Victoronz marked this conversation as resolved.
pub struct UniqueEntityIter<I: Iterator<Item: EntityEquivalent>> {
iter: I,
}

impl<I: EntitySetIterator> UniqueEntityIter<I> {
/// Constructs a `UniqueEntityIter` from an [`EntitySetIterator`].
pub fn from_entity_set_iterator<S>(iter: I) -> Self {
Self { iter }
#[inline]
pub const fn from_entity_set_iter(iter: I) -> Self {
// SAFETY: iter implements `EntitySetIterator`.
unsafe { Self::from_iter_unchecked(iter) }
}
}

Expand All @@ -375,17 +379,40 @@ impl<I: Iterator<Item: EntityEquivalent>> UniqueEntityIter<I> {
/// # Safety
/// `iter` must only yield unique elements.
/// As in, the resulting iterator must adhere to the safety contract of [`EntitySetIterator`].
pub unsafe fn from_iterator_unchecked(iter: I) -> Self {
#[inline]
pub const unsafe fn from_iter_unchecked(iter: I) -> Self {
Self { iter }
}

/// Constructs a [`UniqueEntityIter`] from an iterator unsafely.
///
/// # Safety
/// `iter` must only yield unique elements.
/// As in, the resulting iterator must adhere to the safety contract of [`EntitySetIterator`].
#[inline]
pub const unsafe fn from_iter_ref_unchecked(iter: &I) -> &Self {
Copy link
Copy Markdown
Contributor

@chescock chescock Oct 10, 2025

Choose a reason for hiding this comment

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

I guess this makes sense for completeness, but there's not much you can do with an &impl Iterator.

Same thing for UniqueEntityEquivalentVec::from_vec_ref_unchecked. Are there ever any cases where you need that instead of UniqueEntityEquivalentSlice::from_slice_unchecked? I guess you can call capacity()?

Copy link
Copy Markdown
Contributor Author

@Victoronz Victoronz Oct 12, 2025

Choose a reason for hiding this comment

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

For iterators, remember that is a UniqueEntityIter construction method, its main purpose is to be able to mark any iterator as an EntitySetIterator. This is not just for iteration itself! Sometimes, there are &SomeIterator can return underlying views into a collection, like f.e. as_slice()/AsRef<[T]>.

For Vecs, it has to do with safety around the uniqueness invariant:
If you know that you have borrowed the full collection, you have stronger guarantees about its subsections.
F.e. a slice can always have adjacent elements you have no awareness of/access to. If you have a Vec, this is never the case.

Right now, mutable UniqueEntitySlice logic is not yet implemented, so we do not yet have safety comments that talk about this subtlety.
Interestingly enough, the need to reference collections while maintaining a "no superslice" guarantee is not one I've really heard of before, which seems to be corroborated by some triggered lints surrounding &Box<[T]> and the like.

// SAFETY: UniqueEntityIter is a transparent wrapper around I.
unsafe { &*ptr::from_ref(iter).cast() }
}

/// Constructs a [`UniqueEntityIter`] from an iterator unsafely.
///
/// # Safety
/// `iter` must only yield unique elements.
/// As in, the resulting iterator must adhere to the safety contract of [`EntitySetIterator`].
#[inline]
pub const unsafe fn from_iter_mut_unchecked(iter: &mut I) -> &mut Self {
// SAFETY: UniqueEntityIter is a transparent wrapper around I.
unsafe { &mut *ptr::from_mut(iter).cast() }
}

/// Returns the inner `I`.
pub fn into_inner(self) -> I {
self.iter
}

/// Returns a reference to the inner `I`.
pub fn as_inner(&self) -> &I {
pub const fn as_inner(&self) -> &I {
&self.iter
}

Expand All @@ -395,7 +422,7 @@ impl<I: Iterator<Item: EntityEquivalent>> UniqueEntityIter<I> {
///
/// `self` must always contain an iterator that yields unique elements,
/// even while this reference is live.
pub unsafe fn as_mut_inner(&mut self) -> &mut I {
pub const unsafe fn as_mut_inner(&mut self) -> &mut I {
&mut self.iter
}
}
Expand All @@ -415,6 +442,7 @@ impl<I: Iterator<Item: EntityEquivalent>> Iterator for UniqueEntityIter<I> {
impl<I: ExactSizeIterator<Item: EntityEquivalent>> ExactSizeIterator for UniqueEntityIter<I> {}

impl<I: DoubleEndedIterator<Item: EntityEquivalent>> DoubleEndedIterator for UniqueEntityIter<I> {
#[inline]
fn next_back(&mut self) -> Option<Self::Item> {
self.iter.next_back()
}
Expand Down Expand Up @@ -506,7 +534,7 @@ mod tests {

// SAFETY: SpawnBatchIter is `EntitySetIterator`,
let mut unique_entity_iter =
unsafe { UniqueEntityIter::from_iterator_unchecked(spawn_batch.iter()) };
unsafe { UniqueEntityIter::from_iter_unchecked(spawn_batch.iter()) };

let entity_set = unique_entity_iter
.by_ref()
Expand Down
1 change: 1 addition & 0 deletions crates/bevy_ecs/src/entity/hash.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ pub struct EntityHash;
impl BuildHasher for EntityHash {
type Hasher = EntityHasher;

#[inline]
fn build_hasher(&self) -> Self::Hasher {
Self::Hasher::default()
}
Expand Down
51 changes: 45 additions & 6 deletions crates/bevy_ecs/src/entity/hash_map.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use super::{Entity, EntityEquivalent, EntityHash, EntitySetIterator};
#[cfg_attr(feature = "bevy_reflect", derive(Reflect))]
#[cfg_attr(feature = "serialize", derive(serde::Deserialize, serde::Serialize))]
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct EntityHashMap<V>(pub(crate) HashMap<Entity, V, EntityHash>);
pub struct EntityHashMap<V>(HashMap<Entity, V, EntityHash>);

impl<V> EntityHashMap<V> {
/// Creates an empty `EntityHashMap`.
Expand All @@ -35,11 +35,16 @@ impl<V> EntityHashMap<V> {
///
/// Equivalent to [`HashMap::with_capacity_and_hasher(n, EntityHash)`].
///
/// [`HashMap:with_capacity_and_hasher(n, EntityHash)`]: HashMap::with_capacity_and_hasher
/// [`HashMap::with_capacity_and_hasher(n, EntityHash)`]: HashMap::with_capacity_and_hasher
pub fn with_capacity(n: usize) -> Self {
Self(HashMap::with_capacity_and_hasher(n, EntityHash))
}

/// Constructs an `EntityHashMap` from an [`HashMap`].
pub const fn from_index_map(set: HashMap<Entity, V, EntityHash>) -> Self {
Self(set)
}

/// Returns the inner [`HashMap`].
pub fn into_inner(self) -> HashMap<Entity, V, EntityHash> {
self.0
Expand Down Expand Up @@ -113,8 +118,15 @@ impl<V> FromIterator<(Entity, V)> for EntityHashMap<V> {
}
}

impl<V> From<HashMap<Entity, V, EntityHash>> for EntityHashMap<V> {
fn from(value: HashMap<Entity, V, EntityHash>) -> Self {
Self(value)
}
}

impl<V, Q: EntityEquivalent + ?Sized> Index<&Q> for EntityHashMap<V> {
type Output = V;

fn index(&self, key: &Q) -> &V {
self.0.index(&key.entity())
}
Expand Down Expand Up @@ -156,8 +168,20 @@ impl<V> IntoIterator for EntityHashMap<V> {
pub struct Keys<'a, V, S = EntityHash>(hash_map::Keys<'a, Entity, V>, PhantomData<S>);

impl<'a, V> Keys<'a, V> {
/// Constructs a [`Keys<'a, V, S>`] from a [`hash_map::Keys<'a, V>`] unsafely.
///
/// # Safety
///
/// `keys` must either be empty, or have been obtained from a
/// [`hash_map::HashMap`] using the `S` hasher.
pub const unsafe fn from_keys_unchecked<S>(
keys: hash_map::Keys<'a, Entity, V>,
) -> Keys<'a, V, S> {
Copy link
Copy Markdown
Contributor

@chescock chescock Oct 10, 2025

Choose a reason for hiding this comment

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

It might make sense to put this in a separate impl block so that it can use Self. (And similarly for the other from_foo_unchecked methods.)

Oh, or is the point to be able to call it as Keys::from_keys_unchecked<SomeType> instead of Keys::<_, SomeType>::from_keys_unchecked?

Copy link
Copy Markdown
Contributor Author

@Victoronz Victoronz Apr 30, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Yup, if either Self or just Keys is used (in the same impl block), it will simply fill in EntityHash as S, instead of the generic declared when calling the function.
If a separate impl<'a, V, S> Keys<'a, V, S> impl block is used, it will not compile at all, because S is declared twice.

Copy link
Copy Markdown
Contributor

@chescock chescock May 1, 2026

Choose a reason for hiding this comment

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

Nice to see you active again!!

Yup, if either Self or just Keys is used (in the same impl block), it will simply fill in EntityHash as S, instead of the generic declared when calling the function.
If a separate impl<'a, V, S> Keys<'a, V, S> impl block is used, it will not compile at all, because S is declared twice.

Right, I meant to have S just on the impl and not on the fn, like

impl<'a, V, S> Keys<'a, V, S> {
    pub const unsafe fn from_keys_unchecked(keys: map::Keys<'a, Entity, V>) -> Self {
        Self(keys, PhantomData)
    }
}

That seems like the more idiomatic way to write a constructor function, and then Self uses S instead of the default of EntityHash.

... Hmm, for that matter, why is into_inner only defined for S = EntityHash? You could generalize that, too, by putting it in that same impl<'a, V, S> Keys<'a, V, S> block.

Copy link
Copy Markdown
Contributor Author

@Victoronz Victoronz May 1, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Nice to see you active again!!

Thank you!!

Yup, if either Self or just Keys is used (in the same impl block), it will simply fill in EntityHash as S, instead of the generic declared when calling the function.
If a separate impl<'a, V, S> Keys<'a, V, S> impl block is used, it will not compile at all, because S is declared twice.

Right, I meant to have S just on the impl and not on the fn, like

impl<'a, V, S> Keys<'a, V, S> {
pub const unsafe fn from_keys_unchecked(keys: map::Keys<'a, Entity, V>) -> Self {
Self(keys, PhantomData)
}
}

That seems like the more idiomatic way to write a constructor function, and then Self uses S instead of the default of EntityHash.

Hmm, I think I wasn't clear here.
The purpose of these constructors is to be explicitly turbofished!
IIRC, my intent here was for S to not be inferred, so the user would have to be explicit about it. The turbofish in Keys::<...>::from_keys_unchecked() can just be inferred, but in Keys::from_keys_unchecked<...>() it cannot! As far as my understanding goes at least. (It's been some time, so I am not fully certain about it though, this might also have changed in the meantime)

... Hmm, for that matter, why is into_inner only defined for S = EntityHash? You could generalize that, too, by putting it in that same impl<'a, V, S> Keys<'a, V, S> block.

Because all of this infrastructure is only for EntityHash! The S exists to define what types do not stem from EntityHash.
Of course, with the hypothetical DeterministicBuildHasher extension, we would change that.

Copy link
Copy Markdown
Contributor

@chescock chescock May 1, 2026

Choose a reason for hiding this comment

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

The purpose of these constructors is to be explicitly turbofished!
IIRC, my intent here was for S to not be inferred, so the user would have to be explicit about it. The turbofish in Keys::<...>::from_keys_unchecked() can just be inferred, but in Keys::from_keys_unchecked<...>() it cannot!

Maybe I'm just not entirely clear on what the expected use case of this method looks like.

It's certainly possible for Rust to infer generic parameters on both types and methods! So something like

fn foo<V, S>(map: &HashMap<Entity, V>) -> Keys<'_, V, S> {
    unsafe { Keys::from_keys_unchecked(map.keys()) }
}

where S is known will work with either formulation. And if S isn't known, then you need to specify it in either formulation. The way you specify it just changes from Keys::from_keys_unchecked::<S> vs Keys::<_ , S>::from_keys_unchecked.

Anyway, this isn't a big deal either way! It seems odd to have an associated function that doesn't make any use of Self, so my instinct is to move it to an impl block where it can, but it's not actually a problem.

Because all of this infrastructure is only for EntityHash! The S exists to define what types do not stem from EntityHash.

I'm not sure I follow. If we always want S = EntityHash, then it wouldn't even be a type parameter, right? If we're making it generic, then we expect someone to create a Keys<V, SomeOtherHash>. And why wouldn't we want into_inner() to be available on that type?

Copy link
Copy Markdown
Contributor Author

@Victoronz Victoronz May 2, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure I follow. If we always want S = EntityHash, then it wouldn't even be a type parameter, right? If we're making it generic, then we expect someone to create a Keys<V, SomeOtherHash>. And why wouldn't we want into_inner() to be available on that type?

We do currently always want S = EntityHash, the reason why we use a generic here at all is to mark it as such!
The usual set and map iterator types have no way of carrying the original property with them, so we do it with the newtypes instead. What the S generic does it to communicate this the same way the source types do.

As soon as you see the Keys<V, SomeOtherHash> wrapper, something has already gone wrong, because it should never be necessary. If it simply was Keys<V>, this would be confusing. (Naming the newtypes differently would also introduce unnecessary confusion)
Because these variants should not see use, the into_inner methods should not work for them either, was my reasoning.

Keys::<'_, _, S>(keys, PhantomData)
Comment thread
Victoronz marked this conversation as resolved.
Outdated
}

/// Returns the inner [`Keys`](hash_map::Keys).
pub fn into_inner(self) -> hash_map::Keys<'a, Entity, V> {
pub const fn into_inner(self) -> hash_map::Keys<'a, Entity, V> {
self.0
}
}
Expand Down Expand Up @@ -188,7 +212,8 @@ impl<V> FusedIterator for Keys<'_, V> {}

impl<V> Clone for Keys<'_, V> {
fn clone(&self) -> Self {
Self(self.0.clone(), PhantomData)
// SAFETY: We are cloning an already valid `Keys`.
unsafe { Self::from_keys_unchecked(self.0.clone()) }
}
}

Expand All @@ -200,7 +225,8 @@ impl<V: Debug> Debug for Keys<'_, V> {

impl<V> Default for Keys<'_, V> {
fn default() -> Self {
Self(Default::default(), PhantomData)
// SAFETY: `Keys` is empty.
unsafe { Self::from_keys_unchecked(Default::default()) }
}
}

Expand All @@ -218,6 +244,18 @@ unsafe impl<V> EntitySetIterator for Keys<'_, V> {}
pub struct IntoKeys<V, S = EntityHash>(hash_map::IntoKeys<Entity, V>, PhantomData<S>);

impl<V> IntoKeys<V> {
/// Constructs a [`IntoKeys<V, S>`] from a [`hash_map::IntoKeys<V>`] unsafely.
///
/// # Safety
///
/// `into_keys` must either be empty, or have been obtained from a
/// [`hash_map::HashMap`] using the `S` hasher.
pub const unsafe fn from_into_keys_unchecked<S>(
into_keys: hash_map::IntoKeys<Entity, V>,
) -> IntoKeys<V, S> {
IntoKeys::<_, S>(into_keys, PhantomData)
}

/// Returns the inner [`IntoKeys`](hash_map::IntoKeys).
pub fn into_inner(self) -> hash_map::IntoKeys<Entity, V> {
self.0
Expand Down Expand Up @@ -259,7 +297,8 @@ impl<V: Debug> Debug for IntoKeys<V> {

impl<V> Default for IntoKeys<V> {
fn default() -> Self {
Self(Default::default(), PhantomData)
// SAFETY: `IntoKeys` is empty.
unsafe { Self::from_into_keys_unchecked(Default::default()) }
}
}

Expand Down
Loading