add Entity default to the entity set wrappers #18319
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Victoronz
added
A-ECS
alice-i-cecile
approved these changes
Mar 14, 2025
alice-i-cecile
added
the
C-Usability
chescock
approved these changes
Mar 14, 2025
Victoronz
added
S-Ready-For-Final-Review
joshua-holmes
pushed a commit
to joshua-holmes/bevy
that referenced
this pull request
Mar 24, 2025
# Objective Installment of the bevyengine#16547 series. The vast majority of uses for these types will be the `Entity` case, so it makes sense for it to be the default. ## Solution `UniqueEntityVec`, `UniqueEntitySlice`, `UniqueEntityArray` and their helper iterator aliases now have `Entity` as a default. Unfortunately, this means the the `T` parameter for `UniqueEntityArray` now has to be ordered after the `N` constant, which breaks the consistency to `[T; N]`. Same with about a dozen iterator aliases that take some `P`/`F` predicate/function parameter. This should however be an ergonomic improvement in most cases, so we'll just have to live with this inconsistency. ## Migration Guide Switch type parameter order for the relevant wrapper types/aliases.
16 tasks
github-merge-queue bot
pushed a commit
that referenced
this pull request
Mar 30, 2025
…alent (#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`.
mockersf
pushed a commit
that referenced
this pull request
Mar 30, 2025
…alent (#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`.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Apr 3, 2025
# Objective Newest installment of the #16547 series. In #18319 we introduced `Entity` defaults to accomodate the most common use case for these types, however that resulted in the switch of the `T` and `N` generics of `UniqueEntityArray`. Swapping generics might be somewhat acceptable for `UniqueEntityArray`, it is not at all acceptable for map and set types, which we would make generic over `T: EntityEquivalent` in #18408. Leaving these defaults in place would result in a glaring inconsistency between these set collections and the others. Additionally, the current standard in the engine is for "entity" to mean `Entity`. APIs could be changed to accept `EntityEquivalent`, however that is a separate and contentious discussion. ## Solution Name these set collections `UniqueEntityEquivalent*`, and retain the `UniqueEntity*` name for an alias of the `Entity` case. While more verbose, this allows for all generics to be in proper order, full consistency between all set types*, and the "entity" name to be restricted to `Entity`. On top of that, `UniqueEntity*` now always have 1 generic less, when previously this was not enforced for the default case. *`UniqueEntityIter<I: Iterator<T: EntityEquivalent>>` is the sole exception to this. Aliases are unable to enforce bounds (`lazy_type_alias` is needed for this), so for this type, doing this split would be a mere suggestion, and in no way enforced. Iterator types are rarely ever named, and this specific one is intended to be aliased when it sees more use, like we do for the corresponding set collection iterators. Furthermore, the `EntityEquivalent` precursor `Borrow<Entity>` was used exactly because of such iterator bounds! Because of that, we leave it as is. While no migration guide for 0.15 users, for those that upgrade from main: `UniqueEntityVec<T>` -> `UniqueEntityEquivalentVec<T>` `UniqueEntitySlice<T>` -> `UniqueEntityEquivalentSlice<T>` `UniqueEntityArray<N, T>` -> `UniqueEntityEquivalentArray<T, N>`
mockersf
pushed a commit
that referenced
this pull request
Apr 3, 2025
# Objective Newest installment of the #16547 series. In #18319 we introduced `Entity` defaults to accomodate the most common use case for these types, however that resulted in the switch of the `T` and `N` generics of `UniqueEntityArray`. Swapping generics might be somewhat acceptable for `UniqueEntityArray`, it is not at all acceptable for map and set types, which we would make generic over `T: EntityEquivalent` in #18408. Leaving these defaults in place would result in a glaring inconsistency between these set collections and the others. Additionally, the current standard in the engine is for "entity" to mean `Entity`. APIs could be changed to accept `EntityEquivalent`, however that is a separate and contentious discussion. ## Solution Name these set collections `UniqueEntityEquivalent*`, and retain the `UniqueEntity*` name for an alias of the `Entity` case. While more verbose, this allows for all generics to be in proper order, full consistency between all set types*, and the "entity" name to be restricted to `Entity`. On top of that, `UniqueEntity*` now always have 1 generic less, when previously this was not enforced for the default case. *`UniqueEntityIter<I: Iterator<T: EntityEquivalent>>` is the sole exception to this. Aliases are unable to enforce bounds (`lazy_type_alias` is needed for this), so for this type, doing this split would be a mere suggestion, and in no way enforced. Iterator types are rarely ever named, and this specific one is intended to be aliased when it sees more use, like we do for the corresponding set collection iterators. Furthermore, the `EntityEquivalent` precursor `Borrow<Entity>` was used exactly because of such iterator bounds! Because of that, we leave it as is. While no migration guide for 0.15 users, for those that upgrade from main: `UniqueEntityVec<T>` -> `UniqueEntityEquivalentVec<T>` `UniqueEntitySlice<T>` -> `UniqueEntityEquivalentSlice<T>` `UniqueEntityArray<N, T>` -> `UniqueEntityEquivalentArray<T, N>`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
Installment of the #16547 series.
The vast majority of uses for these types will be the
Entitycase, so it makes sense for it to be the default.Solution
UniqueEntityVec,UniqueEntitySlice,UniqueEntityArrayand their helper iterator aliases now haveEntityas a default.Unfortunately, this means the the
Tparameter forUniqueEntityArraynow has to be ordered after theNconstant, which breaks the consistency to[T; N].Same with about a dozen iterator aliases that take some
P/Fpredicate/function parameter.This should however be an ergonomic improvement in most cases, so we'll just have to live with this inconsistency.
Migration Guide
Switch type parameter order for the relevant wrapper types/aliases.