Skip to content

Commit 35cfef7

Browse files
authored
Rename EntityBorrow/TrustedEntityBorrow to ContainsEntity/EntityEquivalent (#18470)
# Objective Fixes #9367. Yet another follow-up to #16547. These traits were initially based on `Borrow<Entity>` because that trait was what they were replacing, and it felt close enough in meaning. However, they ultimately don't quite match: `borrow` always returns references, whereas `EntityBorrow` always returns a plain `Entity`. Additionally, `EntityBorrow` can imply that we are borrowing an `Entity` from the ECS, which is not what it does. Due to its safety contract, `TrustedEntityBorrow` is important an important and widely used trait for `EntitySet` functionality. In contrast, the safe `EntityBorrow` does not see much use, because even outside of `EntitySet`-related functionality, it is a better idea to accept `TrustedEntityBorrow` over `EntityBorrow`. Furthermore, as #9367 points out, abstracting over returning `Entity` from pointers/structs that contain it can skip some ergonomic friction. On top of that, there are aspects of #18319 and #18408 that are relevant to naming: We've run into the issue that relying on a type default can switch generic order. This is livable in some contexts, but unacceptable in others. To remedy that, we'd need to switch to a type alias approach: The "defaulted" `Entity` case becomes a `UniqueEntity*`/`Entity*Map`/`Entity*Set` alias, and the base type receives a more general name. `TrustedEntityBorrow` does not mesh clearly with sensible base type names. ## Solution Replace any `EntityBorrow` bounds with `TrustedEntityBorrow`. + Rename them as such: `EntityBorrow` -> `ContainsEntity` `TrustedEntityBorrow` -> `EntityEquivalent` For `EntityBorrow` we produce a change in meaning; We designate it for types that aren't necessarily strict wrappers around `Entity` or some pointer to `Entity`, but rather any of the myriad of types that contain a single associated `Entity`. This pattern can already be seen in the common `entity`/`id` methods across the engine. We do not mean for `ContainsEntity` to be a trait that abstracts input API (like how `AsRef<T>` is often used, f.e.), because eliding `entity()` would be too implicit in the general case. We prefix "Contains" to match the intuition of a struct with an `Entity` field, like some contain a `length` or `capacity`. It gives the impression of structure, which avoids the implication of a relationship to the `ECS`. `HasEntity` f.e. could be interpreted as "a currently live entity", As an input trait for APIs like #9367 envisioned, `TrustedEntityBorrow` is a better fit, because it *does* restrict itself to strict wrappers and pointers. Which is why we replace any `EntityBorrow`/`ContainsEntity` bounds with `TrustedEntityBorrow`/`EntityEquivalent`. Here, the name `EntityEquivalent` is a lot closer to its actual meaning, which is "A type that is both equivalent to an `Entity`, and forms the same total order when compared". Prior art for this is the [`Equivalent`](https://docs.rs/hashbrown/latest/hashbrown/trait.Equivalent.html) trait in `hashbrown`, which utilizes both `Borrow` and `Eq` for its one blanket impl! Given that we lose the `Borrow` moniker, and `Equivalent` can carry various meanings, we expand on the safety comment of `EntityEquivalent` somewhat. That should help prevent the confusion we saw in [#18408](#18408 (comment)). The new name meshes a lot better with the type aliasing approach in #18408, by aligning with the base name `EntityEquivalentHashMap`. For a consistent scheme among all set types, we can use this scheme for the `UniqueEntity*` wrapper types as well! This allows us to undo the switched generic order that was introduced to `UniqueEntityArray` by its `Entity` default. Even without the type aliases, I think these renames are worth doing! ## Migration Guide Any use of `EntityBorrow` becomes `ContainsEntity`. Any use of `TrustedEntityBorrow` becomes `EntityEquivalent`.
1 parent f57c7a4 commit 35cfef7

File tree

19 files changed

+408
-413
lines changed

19 files changed

+408
-413
lines changed

crates/bevy_ecs/src/entity/entity_set.rs

Lines changed: 94 additions & 78 deletions
Large diffs are not rendered by default.

crates/bevy_ecs/src/entity/hash_map.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@ use bevy_platform_support::collections::hash_map::{self, HashMap};
1313
#[cfg(feature = "bevy_reflect")]
1414
use bevy_reflect::Reflect;
1515

16-
use super::{Entity, EntityHash, EntitySetIterator, TrustedEntityBorrow};
16+
use super::{Entity, EntityEquivalent, EntityHash, EntitySetIterator};
1717

1818
/// A [`HashMap`] pre-configured to use [`EntityHash`] hashing.
1919
#[cfg_attr(feature = "bevy_reflect", derive(Reflect))]
@@ -113,7 +113,7 @@ impl<V> FromIterator<(Entity, V)> for EntityHashMap<V> {
113113
}
114114
}
115115

116-
impl<V, Q: TrustedEntityBorrow + ?Sized> Index<&Q> for EntityHashMap<V> {
116+
impl<V, Q: EntityEquivalent + ?Sized> Index<&Q> for EntityHashMap<V> {
117117
type Output = V;
118118
fn index(&self, key: &Q) -> &V {
119119
self.0.index(&key.entity())

crates/bevy_ecs/src/entity/index_map.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ use core::{
1919
use bevy_reflect::Reflect;
2020
use indexmap::map::{self, IndexMap, IntoValues, ValuesMut};
2121

22-
use super::{Entity, EntityHash, EntitySetIterator, TrustedEntityBorrow};
22+
use super::{Entity, EntityEquivalent, EntityHash, EntitySetIterator};
2323

2424
use bevy_platform_support::prelude::Box;
2525

@@ -176,7 +176,7 @@ impl<V> FromIterator<(Entity, V)> for EntityIndexMap<V> {
176176
}
177177
}
178178

179-
impl<V, Q: TrustedEntityBorrow + ?Sized> Index<&Q> for EntityIndexMap<V> {
179+
impl<V, Q: EntityEquivalent + ?Sized> Index<&Q> for EntityIndexMap<V> {
180180
type Output = V;
181181
fn index(&self, key: &Q) -> &V {
182182
self.0.index(&key.entity())
@@ -246,7 +246,7 @@ impl<V> Index<usize> for EntityIndexMap<V> {
246246
}
247247
}
248248

249-
impl<V, Q: TrustedEntityBorrow + ?Sized> IndexMut<&Q> for EntityIndexMap<V> {
249+
impl<V, Q: EntityEquivalent + ?Sized> IndexMut<&Q> for EntityIndexMap<V> {
250250
fn index_mut(&mut self, key: &Q) -> &mut V {
251251
self.0.index_mut(&key.entity())
252252
}

0 commit comments

Comments
 (0)