6c619397d5
13 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Vic
|
6a7fc9ce4b
|
use entity set collections type aliases instead of defaults (#18695)
# 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>` |
||
|
Vic
|
35cfef7cf2
|
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](https://github.com/bevyengine/bevy/pull/18408#issuecomment-2742094176). 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`. |
||
|
Vic
|
f57c7a43c4
|
reexport entity set collections in entity module (#18413)
# Objective Unlike for their helper typers, the import paths for `unique_array::UniqueEntityArray`, `unique_slice::UniqueEntitySlice`, `unique_vec::UniqueEntityVec`, `hash_set::EntityHashSet`, `hash_map::EntityHashMap`, `index_set::EntityIndexSet`, `index_map::EntityIndexMap` are quite redundant. When looking at the structure of `hashbrown`, we can also see that while both `HashSet` and `HashMap` have their own modules, the main types themselves are re-exported to the crate level. ## Solution Re-export the types in their shared `entity` parent module, and simplify the imports where they're used. |
||
|
Vic
|
32d53e7bd3
|
make various entity wrapper type modules public (#18248)
# Objective Part of the #16547 series. The entity wrapper types often have some associated types an aliases with them that cannot be re-exported into an outer module together. Some helper types are best used with part of their path: `bevy::ecs::entity::index_set::Slice` as `index_set::Slice`. This has already been done for `entity::hash_set` and `entity::hash_map`. ## Solution Publicize the `index_set`, `index_map`, `unique_vec`, `unique_slice`, and `unique_array` modules. ## Migration Guide Any mention or import of types in the affected modules have to add the respective module name to the import path. F.e.: `bevy::ecs::entity::EntityIndexSet` -> `bevy::ecs::entity::index_set::EntityIndexSet` |
||
|
Rob Parrett
|
2760692f88
|
Update typos to 1.29.6 (#17850)
# Objective Update typos, fix new typos. 1.29.6 was just released to fix an [issue](https://github.com/crate-ci/typos/issues/1228) where January's corrections were not included in the binaries for the last release. Reminder: typos can be tossed in the monthly [non-critical corrections issue](https://github.com/crate-ci/typos/issues/1221). ## Solution I chose to allow `implementors`, because a good argument seems to be being made [here](https://github.com/crate-ci/typos/issues/1226) and there is now a PR to address that. ## Discussion Should I exclude `bevy_mikktspace`? At one point I think we had an informal policy of "don't mess with mikktspace until https://github.com/bevyengine/bevy/pull/9050 is merged" but it doesn't seem like that is likely to be merged any time soon. I think these particular corrections in mikktspace are fine because - The same typo mistake seems to have been fixed in that PR - The entire file containing these corrections was deleted in that PR ## Typo of the Month correspindong -> corresponding |
||
|
raldone01
|
1b7db895b7
|
Harden proc macro path resolution and add integration tests. (#17330)
This pr uses the `extern crate self as` trick to make proc macros behave the same way inside and outside bevy. # Objective - Removes noise introduced by `crate as` in the whole bevy repo. - Fixes #17004. - Hardens proc macro path resolution. ## TODO - [x] `BevyManifest` needs cleanup. - [x] Cleanup remaining `crate as`. - [x] Add proper integration tests to the ci. ## Notes - `cargo-manifest-proc-macros` is written by me and based/inspired by the old `BevyManifest` implementation and [`bkchr/proc-macro-crate`](https://github.com/bkchr/proc-macro-crate). - What do you think about the new integration test machinery I added to the `ci`? More and better integration tests can be added at a later stage. The goal of these integration tests is to simulate an actual separate crate that uses bevy. Ideally they would lightly touch all bevy crates. ## Testing - Needs RA test - Needs testing from other users - Others need to run at least `cargo run -p ci integration-test` and verify that they work. --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> |
||
|
Vic
|
be9b38e372
|
implement UniqueEntitySlice (#17589)
# Objective Follow-up to #17549 and #16547. A large part of `Vec`s usefulness is behind its ability to be sliced, like sorting f.e., so we want the same to be possible for `UniqueEntityVec`. ## Solution Add a `UniqueEntitySlice` type. It is a wrapper around `[T]`, and itself a DST. Because `mem::swap` has a `Sized` bound, DSTs cannot be swapped, and we can freely hand out mutable subslices without worrying about the uniqueness invariant of the backing collection! `UniqueEntityVec` and the relevant `UniqueEntityIter`s now have methods and trait impls that return `UniqueEntitySlice`s. `UniqueEntitySlice` itself can deref into normal slices, which means we can avoid implementing the vast majority of immutable slice methods. Most of the remaining methods: - split a slice/collection in further unique subsections/slices - reorder the slice: `sort`, `rotate_*`, `swap` - construct/deconstruct/convert pointer-like types: `Box`, `Arc`, `Rc`, `Cow` - are comparison trait impls As this PR is already larger than I'd like, we leave several things to follow-ups: - `UniqueEntityArray` and the related slice methods that would return it - denoted by "chunk", "array_*" for iterators - Methods that return iterators with `UniqueEntitySlice` as their item - `windows`, `chunks` and `split` families - All methods that are capable of actively mutating individual elements. While they could be offered unsafely, subslicing makes their safety contract weird enough to warrant its own discussion. - `fill_with`, `swap_with_slice`, `iter_mut`, `split_first/last_mut`, `select_nth_unstable_*` Note that `Arc`, `Rc` and `Cow` are not fundamental types, so even if they contain `UniqueEntitySlice`, we cannot write direct trait impls for them. On top of that, `Cow` is not a receiver (like `self: Arc<Self>` is) so we cannot write inherent methods for it either. |
||
|
Vic
|
94a238b0ef
|
implement FromEntitySetIterator (#17513)
# Objective Some collections are more efficient to construct when we know that every element is unique in advance. We have `EntitySetIterator`s from #16547, but currently no API to safely make use of them this way. ## Solution Add `FromEntitySetIterator` as a subtrait to `FromIterator`, and implement it for the `EntityHashSet`/`hashbrown::HashSet` types. To match the normal `FromIterator`, we also add a `EntitySetIterator::collect_set` method. It'd be better if these methods could shadow `from_iter` and `collect` completely, but https://github.com/rust-lang/rust/issues/89151 is needed for that. While currently only `HashSet`s implement this trait, future `UniqueEntityVec`/`UniqueEntitySlice` functionality comes with more implementors. Because `HashMap`s are collected from tuples instead of singular types, implementing this same optimization for them is more complex, and has to be done separately. ## Showcase This is basically a free speedup for collecting `EntityHashSet`s! ```rust pub fn collect_milk_dippers(dippers: Query<Entity, (With<Milk>, With<Cookies>)>) { dippers.iter().collect_set::<EntityHashSet>(); // or EntityHashSet::from_entity_set_iter(dippers); } --------- Co-authored-by: SpecificProtagonist <vincentjunge@posteo.net> |
||
|
Zachary Harrold
|
a64446b77e
|
Create bevy_platform_support Crate (#17250)
# Objective - Contributes to #16877 ## Solution - Initial creation of `bevy_platform_support` crate. - Moved `bevy_utils::Instant` into new `bevy_platform_support` crate. - Moved `portable-atomic`, `portable-atomic-util`, and `critical-section` into new `bevy_platform_support` crate. ## Testing - CI --- ## Showcase Instead of needing code like this to import an `Arc`: ```rust #[cfg(feature = "portable-atomic")] use portable_atomic_util::Arc; #[cfg(not(feature = "portable-atomic"))] use alloc::sync::Arc; ``` We can now use: ```rust use bevy_platform_support::sync::Arc; ``` This applies to many other types, but the goal is overall the same: allowing crates to use `std`-like types without the boilerplate of conditional compilation and platform-dependencies. ## Migration Guide - Replace imports of `bevy_utils::Instant` with `bevy_platform_support::time::Instant` - Replace imports of `bevy::utils::Instant` with `bevy::platform_support::time::Instant` ## Notes - `bevy_platform_support` hasn't been reserved on `crates.io` - ~~`bevy_platform_support` is not re-exported from `bevy` at this time. It may be worthwhile exporting this crate, but I am unsure of a reasonable name to export it under (`platform_support` may be a bit wordy for user-facing).~~ - I've included an implementation of `Instant` which is suitable for `no_std` platforms that are not Wasm for the sake of eliminating feature gates around its use. It may be a controversial inclusion, so I'm happy to remove it if required. - There are many other items (`spin`, `bevy_utils::Sync(Unsafe)Cell`, etc.) which should be added to this crate. I have kept the initial scope small to demonstrate utility without making this too unwieldy. --------- Co-authored-by: TimJentzsch <TimJentzsch@users.noreply.github.com> Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com> Co-authored-by: François Mockers <francois.mockers@vleue.com> |
||
|
MichiRecRoom
|
17c46f4add
|
bevy_ecs: Apply #![warn(clippy::allow_attributes, clippy::allow_attributes_without_reason)] (#17335)
# Objective - https://github.com/bevyengine/bevy/issues/17111 ## Solution Set the `clippy::allow_attributes` and `clippy::allow_attributes_without_reason` lints to `warn`, and bring `bevy_ecs` in line with the new restrictions. ## Testing This PR is a WIP; testing will happen after it's finished. |
||
|
Zachary Harrold
|
0403948aa2
|
Remove Implicit std Prelude from no_std Crates (#17086)
# Background In `no_std` compatible crates, there is often an `std` feature which will allow access to the standard library. Currently, with the `std` feature _enabled_, the [`std::prelude`](https://doc.rust-lang.org/std/prelude/index.html) is implicitly imported in all modules. With the feature _disabled_, instead the [`core::prelude`](https://doc.rust-lang.org/core/prelude/index.html) is implicitly imported. This creates a subtle and pervasive issue where `alloc` items _may_ be implicitly included (if `std` is enabled), or must be explicitly included (if `std` is not enabled). # Objective - Make the implicit imports for `no_std` crates consistent regardless of what features are/not enabled. ## Solution - Replace the `cfg_attr` "double negative" `no_std` attribute with conditional compilation to _include_ `std` as an external crate. ```rust // Before #![cfg_attr(not(feature = "std"), no_std)] // After #![no_std] #[cfg(feature = "std")] extern crate std; ``` - Fix imports that are currently broken but are only now visible with the above fix. ## Testing - CI ## Notes I had previously used the "double negative" version of `no_std` based on general consensus that it was "cleaner" within the Rust embedded community. However, this implicit prelude issue likely was considered when forming this consensus. I believe the reason why is the items most affected by this issue are provided by the `alloc` crate, which is rarely used within embedded but extensively used within Bevy. |
||
|
Zachary Harrold
|
21786632c3
|
Remove bevy_core (#16897)
# Objective - Fixes #16892 ## Solution - Removed `TypeRegistryPlugin` (`Name` is now automatically registered with a default `App`) - Moved `TaskPoolPlugin` to `bevy_app` - Moved `FrameCountPlugin` to `bevy_diagnostic` - Deleted now-empty `bevy_core` ## Testing - CI ## Migration Guide - `TypeRegistryPlugin` no longer exists. If you can't use a default `App` but still need `Name` registered, do so manually with `app.register_type::<Name>()`. - References to `TaskPoolPlugin` and associated types will need to import it from `bevy_app` instead of `bevy_core` - References to `FrameCountPlugin` and associated types will need to import it from `bevy_diagnostic` instead of `bevy_core` ## Notes This strategy was agreed upon by Cart and several other members in [Discord](https://discord.com/channels/691052431525675048/692572690833473578/1319137218312278077). |
||
|
Vic
|
a4b89d0d5e
|
implement EntitySet and iter_many_unique methods (#16547)
# Objective
In current Bevy, it is very inconvenient to mutably retrieve a
user-provided list of entities more than one element at a time.
If the list contains any duplicate entities, we risk mutable aliasing.
Users of `Query::iter_many_mut` do not have access to `Iterator` trait,
and thus miss out on common functionality, for instance collecting their
`QueryManyIter`.
We can circumvent this issue with validation, however that entails
checking every entity against all others for inequality, or utilizing an
`EntityHashSet`. Even if an entity list remains unchanged, this
validation is/would have to be redone every time we wish to fetch with
the list.
This presents a lot of wasted work, as we often trivially know an entity
list to be unique f.e.: `QueryIter` will fetch every `Entity` once and
only once.
As more things become entities – assets, components, queries – this
issue will become more pronounced.
`get_many`/`many`/`iter_many`/`par_iter_many`-like functionality is all
affected.
## Solution
The solution this PR proposes is to introduce functionality built around
a new trait: `EntitySet`.
The goal is to preserve the property of "uniqueness" in a list wherever
possible, and then rely on it as a bound within new `*_many_unique`
methods to avoid the need for validation.
This is achieved using `Iterator`:
`EntitySet` is blanket implemented for any `T` that implements
`IntoIterator<IntoIter: EntitySetIterator>`.
`EntitySetIterator` is the unsafe trait that actually guarantees an
iterator to be "unique" via its safety contract.
We define an "Iterator over unique entities" as: "No two entities
returned by the iterator may compare equal."
For iterators that cannot return more than 1 element, this is trivially
true.
Whether an iterator can satisfy this is up to the `EntitySetIterator`
implementor to ensure, hence the unsafe.
However, this is not yet a complete solution. Looking at the signature
of `iter_many`, we find that `IntoIterator::Item` is not `Entity`, but
is instead bounded by the `Borrow<Entity>` trait. That is because
iteration without consuming the collection will often yield us
references, not owned items.
`Borrow<Entity>` presents an issue: The `Borrow` docs state that `x = y`
should equal `x.borrow() = y.borrow()`, but unsafe cannot rely on this
for soundness. We run into similar problems with other trait
implementations of any `Borrow<Entity>` type: `PartialEq`, `Eq`,
`PartialOrd`, `Ord`, `Hash`, `Clone`, `Borrow`, and `BorrowMut`.
This PR solves this with the unsafe `TrustedEntityBorrow` trait:
Any implementor promises that the behavior of the aforementioned traits
matches that of the underlying entity.
While `Borrow<Entity>` was the inspiration, we use our own counterpart
trait `EntityBorrow` as the supertrait to `TrustedEntityBorrow`, so we
can circumvent the limitations of the existing `Borrow<T>` blanket
impls.
All together, these traits allow us to implement `*_many_unique`
functionality with a lone `EntitySet` bound.
`EntitySetIterator` is implemented for all the std iterators and
iterator adapters that guarantee or preserve uniqueness, so we can
filter, skip, take, step, reverse, ... our unique entity iterators
without worry!
Sadly, current `HashSet` iterators do not carry the necessary type
information with them to determine whether the source `HashSet` produces
logic errors; A malicious `Hasher` could compromise a `HashSet`.
`HashSet` iteration is generally discouraged in the first place, so we
also exclude the set operation iterators, even though they do carry the
`Hasher` type parameter.
`BTreeSet` implements `EntitySet` without any problems.
If an iterator type cannot guarantee uniqueness at compile time, then a
user can still attach `EntitySetIterator` to an individual instance of
that type via `UniqueEntityIter::from_iterator_unchecked`.
With this, custom types can use `UniqueEntityIter<I>` as their
`IntoIterator::IntoIter` type, if necessary.
This PR is focused on the base concept, and expansions on it are left
for follow-up PRs. See "Potential Future Work" below.
## Testing
Doctests on `iter_many_unique`/`iter_many_unique_mut` + 2 tests in
entity_set.rs.
## Showcase
```rust
// Before:
fn system(player_list: Res<SomeUniquePlayerList>, players: Query<&mut Player>) {
let value = 0;
while let Some(player) = players.iter_many_mut(player_list).fetch_next() {
value += mem::take(player.value_mut())
}
}
// After:
fn system(player_list: Res<SomeUniquePlayerList>, players: Query<&mut Player>) {
let value = players
.iter_many_unique_mut(player_list)
.map(|player| mem::take(player.value_mut()))
.sum();
}
```
## Changelog
- added `EntityBorrow`, `TrustedEntityBorrow`, `EntitySet` and
`EntitySetIterator` traits
- added `iter_many_unique`, `iter_many_unique_mut`,
`iter_many_unique_unsafe` methods on `Query`
- added `iter_many_unique`, `iter_many_unique_mut`,
`iter_many_unique_manual` and `iter_many_unique_unchecked_manual`
methods on `QueryState`
- added corresponding `QueryManyUniqueIter`
- added `UniqueEntityIter`
## Migration Guide
Any custom type used as a `Borrow<Entity>` entity list item for an
`iter_many` method now has to implement `EntityBorrow` instead. Any type
that implements `Borrow<Entity>` can trivially implement `EntityBorrow`.
## Potential Future Work
- `ToEntitySet` trait for converting any entity iterator into an
`EntitySetIterator`
- `EntityIndexSet/Map` to tie in hashing with `EntitySet`
- add `EntityIndexSetSlice/MapSlice`
- requires: `EntityIndexSet/Map`
- Implementing `par_iter_many_unique_mut` for parallel mutable iteration
- requires: `par_iter_many`
- allow collecting into `UniqueEntityVec` to store entity sets
- add `UniqueEntitySlice`s
- Doesn't require, but should be done after: `UniqueEntityVec`
- add `UniqueEntityArray`s
- Doesn't require, but should be done after: `UniqueEntitySlice`
- `get_many_unique`/`many_unique` methods
- requires: `UniqueEntityArray`
- `World::entity_unique` to match `World::entity` methods
- Doesn't require, but makes sense after:
`get_many_unique`/`many_unique`
- implement `TrustedEntityBorrow` for the `EntityRef` family
- Doesn't require, but makes sense after: `UniqueEntityVec`
|