forestia/bevy
2
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Remove deprecated component_reads_and_writes (#16348)

Browse Source 
# Objective

- Fixes #16339

## Solution

- Replaced `component_reads_and_writes` and `component_writes` with
`try_iter_component_access`.

## Testing

- Ran `dynamic` example to confirm behaviour is unchanged.
- CI

---

## Migration Guide

The following methods (some removed in previous PRs) are now replaced by
`Access::try_iter_component_access`:

* `Access::component_reads_and_writes`
* `Access::component_reads`
* `Access::component_writes`

As `try_iter_component_access` returns a `Result`, you'll now need to
handle the failing case (e.g., `unwrap()`). There is currently a single
failure mode, `UnboundedAccess`, which occurs when the `Access` is for
all `Components` _except_ certain exclusions. Since this list is
infinite, there is no meaningful way for `Access` to provide an
iterator. Instead, get a list of components (e.g., from the `Components`
structure) and iterate over that instead, filtering using
`Access::has_component_read`, `Access::has_component_write`, etc.

Additionally, you'll need to `filter_map` the accesses based on which
method you're attempting to replace:

* `Access::component_reads_and_writes` -> `Exclusive(_) | Shared(_)`
* `Access::component_reads` -> `Shared(_)`
* `Access::component_writes` -> `Exclusive(_)`

To ease migration, please consider the below extension trait which you
can include in your project:

```rust
pub trait AccessCompatibilityExt {
    /// Returns the indices of the components this has access to.
    fn component_reads_and_writes(&self) -> impl Iterator<Item = T> + '_;

    /// Returns the indices of the components this has non-exclusive access to.
    fn component_reads(&self) -> impl Iterator<Item = T> + '_;

    /// Returns the indices of the components this has exclusive access to.
    fn component_writes(&self) -> impl Iterator<Item = T> + '_;
}

impl<T: SparseSetIndex> AccessCompatibilityExt for Access<T> {
    fn component_reads_and_writes(&self) -> impl Iterator<Item = T> + '_ {
        self
            .try_iter_component_access()
            .expect("Access is unbounded. Please refactor the usage of this method to directly use try_iter_component_access")
            .filter_map(|component_access| {
                let index = component_access.index().sparse_set_index();

                match component_access {
                    ComponentAccessKind::Archetypal(_) => None,
                    ComponentAccessKind::Shared(_) => Some(index),
                    ComponentAccessKind::Exclusive(_) => Some(index),
                }
            })
    }

    fn component_reads(&self) -> impl Iterator<Item = T> + '_ {
        self
            .try_iter_component_access()
            .expect("Access is unbounded. Please refactor the usage of this method to directly use try_iter_component_access")
            .filter_map(|component_access| {
                let index = component_access.index().sparse_set_index();

                match component_access {
                    ComponentAccessKind::Archetypal(_) => None,
                    ComponentAccessKind::Shared(_) => Some(index),
                    ComponentAccessKind::Exclusive(_) => None,
                }
            })
    }

    fn component_writes(&self) -> impl Iterator<Item = T> + '_ {
        self
            .try_iter_component_access()
            .expect("Access is unbounded. Please refactor the usage of this method to directly use try_iter_component_access")
            .filter_map(|component_access| {
                let index = component_access.index().sparse_set_index();

                match component_access {
                    ComponentAccessKind::Archetypal(_) => None,
                    ComponentAccessKind::Shared(_) => None,
                    ComponentAccessKind::Exclusive(_) => Some(index),
                }
            })
    }
}
```

Please take note of the use of `expect(...)` in these methods. You
should consider using these as a starting point for a more appropriate
migration based on your specific needs.

## Notes

- This new method is fallible based on whether the `Access` is bounded
or unbounded (unbounded occurring with inverted component sets). If
bounded, will return an iterator of every item and its access level. I
believe this makes sense without exposing implementation details around
`Access`.
- The access level is defined by an `enum` `ComponentAccessKind<T>`,
either `Archetypical`, `Shared`, or `Exclusive`. As a convenience, this
`enum` has a method `index` to get the inner `T` value without a match
statement. It does add more code, but the API is clearer.
- Within `QueryBuilder` this new method simplifies several pieces of
logic without changing behaviour.
- Within `QueryState` the logic is simplified and the amount of
iteration is reduced, potentially improving performance.
- Within the `dynamic` example it has identical behaviour, with the
inversion footgun explicitly highlighted by an `unwrap`.

---------

Co-authored-by: Chris Russell <8494645+chescock@users.noreply.github.com>
Co-authored-by: Mike <2180432+hymm@users.noreply.github.com>
This commit is contained in:
Zachary Harrold 2025-03-04 19:22:29 +11:00 committed by GitHub
parent 8a87a51c54
commit ff1143ec87
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 190 additions and 61 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

193
crates/bevy_ecs/src/query/access.rs
View File

@ -3,9 +3,10 @@ use crate::storage::SparseSetIndex;
use crate::world::World;
use alloc::{format, string::String, vec, vec::Vec};
use core::{fmt, fmt::Debug, marker::PhantomData};
use derive_more::derive::From;
use derive_more::From;
use disqualified::ShortName;
use fixedbitset::FixedBitSet;
use thiserror::Error;
/// A wrapper struct to make Debug representations of [`FixedBitSet`] easier
/// to read, when used to store [`SparseSetIndex`].
@ -773,38 +774,99 @@ impl<T: SparseSetIndex> Access<T> {
self.archetypal.ones().map(T::get_sparse_set_index)
}
/// Returns an iterator over the component IDs that this `Access` either
/// reads and writes or can't read or write.
/// Returns an iterator over the component IDs and their [`ComponentAccessKind`].
///
/// The returned flag specifies whether the list consists of the components
/// that the access *can* read or write (false) or whether the list consists
/// of the components that the access *can't* read or write (true).
/// Returns `Err(UnboundedAccess)` if the access is unbounded.
/// This typically occurs when an [`Access`] is marked as accessing all
/// components, and then adding exceptions.
///
/// Because this method depends on internal implementation details of
/// `Access`, it's not recommended. Prefer to manage your own lists of
/// accessible components if your application needs to do that.
#[doc(hidden)]
// TODO: this should be deprecated and removed, see https://github.com/bevyengine/bevy/issues/16339
pub fn component_reads_and_writes(&self) -> (impl Iterator<Item = T> + '_, bool) {
(
self.component_read_and_writes
.ones()
.map(T::get_sparse_set_index),
self.component_read_and_writes_inverted,
)
}
/// # Examples
///
/// ```rust
/// # use bevy_ecs::query::{Access, ComponentAccessKind};
/// let mut access = Access::<usize>::default();
///
/// access.add_component_read(1);
/// access.add_component_write(2);
/// access.add_archetypal(3);
///
/// let result = access
/// .try_iter_component_access()
/// .map(Iterator::collect::<Vec<_>>);
///
/// assert_eq!(
/// result,
/// Ok(vec![
/// ComponentAccessKind::Shared(1),
/// ComponentAccessKind::Exclusive(2),
/// ComponentAccessKind::Archetypal(3),
/// ]),
/// );
/// ```
pub fn try_iter_component_access(
&self,
) -> Result<impl Iterator<Item = ComponentAccessKind<T>> + '_, UnboundedAccessError> {
// component_writes_inverted is only ever true when component_read_and_writes_inverted is
// also true. Therefore it is sufficient to check just component_read_and_writes_inverted.
if self.component_read_and_writes_inverted {
return Err(UnboundedAccessError {
writes_inverted: self.component_writes_inverted,
read_and_writes_inverted: self.component_read_and_writes_inverted,
});
}
/// Returns an iterator over the component IDs that this `Access` either
/// writes or can't write.
///
/// The returned flag specifies whether the list consists of the components
/// that the access *can* write (false) or whether the list consists of the
/// components that the access *can't* write (true).
pub(crate) fn component_writes(&self) -> (impl Iterator<Item = T> + '_, bool) {
(
self.component_writes.ones().map(T::get_sparse_set_index),
self.component_writes_inverted,
)
let reads_and_writes = self.component_read_and_writes.ones().map(|index| {
let sparse_index = T::get_sparse_set_index(index);
if self.component_writes.contains(index) {
ComponentAccessKind::Exclusive(sparse_index)
} else {
ComponentAccessKind::Shared(sparse_index)
}
});
let archetypal = self
.archetypal
.ones()
.filter(|&index| {
!self.component_writes.contains(index)
&& !self.component_read_and_writes.contains(index)
})
.map(|index| ComponentAccessKind::Archetypal(T::get_sparse_set_index(index)));
Ok(reads_and_writes.chain(archetypal))
}
}
/// Error returned when attempting to iterate over items included in an [`Access`]
/// if the access excludes items rather than including them.
#[derive(Clone, Copy, PartialEq, Eq, Debug, Error)]
#[error("Access is unbounded")]
pub struct UnboundedAccessError {
/// [`Access`] is defined in terms of _excluding_ [exclusive](ComponentAccessKind::Exclusive)
/// access.
pub writes_inverted: bool,
/// [`Access`] is defined in terms of _excluding_ [shared](ComponentAccessKind::Shared) and
/// [exclusive](ComponentAccessKind::Exclusive) access.
pub read_and_writes_inverted: bool,
}
/// Describes the level of access for a particular component as defined in an [`Access`].
#[derive(PartialEq, Eq, Hash, Debug, Clone, Copy)]
pub enum ComponentAccessKind<T> {
/// Archetypical access, such as `Has<Foo>`.
Archetypal(T),
/// Shared access, such as `&Foo`.
Shared(T),
/// Exclusive access, such as `&mut Foo`.
Exclusive(T),
}
impl<T> ComponentAccessKind<T> {
/// Gets the index of this `ComponentAccessKind`.
pub fn index(&self) -> &T {
let (Self::Archetypal(value) | Self::Shared(value) | Self::Exclusive(value)) = self;
value
}
}
@ -1360,9 +1422,10 @@ impl<T: SparseSetIndex> Default for FilteredAccessSet<T> {
#[cfg(test)]
mod tests {
use crate::query::{
access::AccessFilters, Access, AccessConflicts, FilteredAccess, FilteredAccessSet,
access::AccessFilters, Access, AccessConflicts, ComponentAccessKind, FilteredAccess,
FilteredAccessSet, UnboundedAccessError,
};
use alloc::vec;
use alloc::{vec, vec::Vec};
use core::marker::PhantomData;
use fixedbitset::FixedBitSet;
@ -1634,4 +1697,70 @@ mod tests {
assert_eq!(access_a, expected);
}
#[test]
fn try_iter_component_access_simple() {
let mut access = Access::<usize>::default();
access.add_component_read(1);
access.add_component_read(2);
access.add_component_write(3);
access.add_archetypal(5);
let result = access
.try_iter_component_access()
.map(Iterator::collect::<Vec<_>>);
assert_eq!(
result,
Ok(vec![
ComponentAccessKind::Shared(1),
ComponentAccessKind::Shared(2),
ComponentAccessKind::Exclusive(3),
ComponentAccessKind::Archetypal(5),
]),
);
}
#[test]
fn try_iter_component_access_unbounded_write_all() {
let mut access = Access::<usize>::default();
access.add_component_read(1);
access.add_component_read(2);
access.write_all();
let result = access
.try_iter_component_access()
.map(Iterator::collect::<Vec<_>>);
assert_eq!(
result,
Err(UnboundedAccessError {
writes_inverted: true,
read_and_writes_inverted: true
}),
);
}
#[test]
fn try_iter_component_access_unbounded_read_all() {
let mut access = Access::<usize>::default();
access.add_component_read(1);
access.add_component_read(2);
access.read_all();
let result = access
.try_iter_component_access()
.map(Iterator::collect::<Vec<_>>);
assert_eq!(
result,
Err(UnboundedAccessError {
writes_inverted: false,
read_and_writes_inverted: true
}),
);
}
}

12
crates/bevy_ecs/src/query/builder.rs
View File

@ -81,14 +81,14 @@ impl<'w, D: QueryData, F: QueryFilter> QueryBuilder<'w, D, F> {
.is_some_and(|info| info.storage_type() == StorageType::Table)
};
let (mut component_reads_and_writes, component_reads_and_writes_inverted) =
self.access.access().component_reads_and_writes();
if component_reads_and_writes_inverted {
let Ok(component_accesses) = self.access.access().try_iter_component_access() else {
// Access is unbounded, pessimistically assume it's sparse.
return false;
}
};
component_reads_and_writes.all(is_dense)
&& self.access.access().archetypal().all(is_dense)
component_accesses
.map(|access| *access.index())
.all(is_dense)
&& !self.access.access().has_read_all_components()
&& self.access.with_filters().all(is_dense)
&& self.access.without_filters().all(is_dense)

35
crates/bevy_ecs/src/query/state.rs
View File

@ -21,8 +21,8 @@ use log::warn;
use tracing::Span;
use super::{
NopWorldQuery, QueryBuilder, QueryData, QueryEntityError, QueryFilter, QueryManyIter,
QueryManyUniqueIter, QuerySingleError, ROQueryItem, ReadOnlyQueryData,
ComponentAccessKind, NopWorldQuery, QueryBuilder, QueryData, QueryEntityError, QueryFilter,
QueryManyIter, QueryManyUniqueIter, QuerySingleError, ROQueryItem, ReadOnlyQueryData,
};
/// An ID for either a table or an archetype. Used for Query iteration.
@ -681,23 +681,22 @@ impl<D: QueryData, F: QueryFilter> QueryState<D, F> {
access: &mut Access<ArchetypeComponentId>,
) {
// As a fast path, we can iterate directly over the components involved
// if the `access` isn't inverted.
let (component_reads_and_writes, component_reads_and_writes_inverted) =
self.component_access.access.component_reads_and_writes();
let (component_writes, component_writes_inverted) =
self.component_access.access.component_writes();
// if the `access` is finite.
if let Ok(iter) = self.component_access.access.try_iter_component_access() {
iter.for_each(|component_access| {
if let Some(id) = archetype.get_archetype_component_id(*component_access.index()) {
match component_access {
ComponentAccessKind::Archetypal(_) => {}
ComponentAccessKind::Shared(_) => {
access.add_component_read(id);
}
ComponentAccessKind::Exclusive(_) => {
access.add_component_write(id);
}
}
}
});
if !component_reads_and_writes_inverted && !component_writes_inverted {
component_reads_and_writes.for_each(|id| {
if let Some(id) = archetype.get_archetype_component_id(id) {
access.add_component_read(id);
}
});
component_writes.for_each(|id| {
if let Some(id) = archetype.get_archetype_component_id(id) {
access.add_component_write(id);
}
});
return;
}

11
examples/ecs/dynamic.rs
View File

@ -13,7 +13,7 @@ use bevy::{
component::{
ComponentCloneBehavior, ComponentDescriptor, ComponentId, ComponentInfo, StorageType,
},
query::QueryData,
query::{ComponentAccessKind, QueryData},
world::FilteredEntityMut,
},
prelude::*,
@ -157,9 +157,10 @@ fn main() {
query.iter_mut(&mut world).for_each(|filtered_entity| {
let terms = filtered_entity
.access()
.component_reads_and_writes()
.0
.map(|id| {
.try_iter_component_access()
.unwrap()
.map(|component_access| {
let id = *component_access.index();
let ptr = filtered_entity.get_by_id(id).unwrap();
let info = component_info.get(&id).unwrap();
let len = info.layout().size() / size_of::<u64>();
@ -175,7 +176,7 @@ fn main() {
};
// If we have write access, increment each value once
if filtered_entity.access().has_component_write(id) {
if matches!(component_access, ComponentAccessKind::Exclusive(_)) {
data.iter_mut().for_each(|data| {
*data += 1;
});