Remove deprecated `component_reads_and_writes`
Objective
- Fixes #16339
Solution
- Replaced
component_reads_and_writesandcomponent_writeswithtry_iter_component_access.
Testing
- Ran
dynamicexample 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_writesAccess::component_readsAccess::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:
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
Accessis 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 aroundAccess. - The access level is defined by an
enumComponentAccessKind<T>, eitherArchetypical,Shared, orExclusive. As a convenience, thisenumhas a methodindexto get the innerTvalue without a match statement. It does add more code, but the API is clearer. - Within
QueryBuilderthis new method simplifies several pieces of logic without changing behaviour. - Within
QueryStatethe logic is simplified and the amount of iteration is reduced, potentially improving performance. - Within the
dynamicexample it has identical behaviour, with the inversion footgun explicitly highlighted by anunwrap.
I think the function is supposed to stay deprecated, not removed. It's just bevy itself needs to not use the function. It can be removed in 0.16.
The migration guide is written relative to main. If this is for 0.15, should it be written relative to 0.14 instead? In 0.14, there were separate component_read(), component_writes(), and component_reads_and_writes() methods that returned plain impl Iterators, and they returned empty iterators when the result would have been unbounded.
I think the function is supposed to stay deprecated, not removed. It's just bevy itself needs to not use the function. It can be removed in 0.16.
I decided to remove them entirely, since they don't actually work as users expect them to in their current form. Basically, the breaking change already happened, so we might as well see it through to a meaningful replacement.
The migration guide is written relative to main. If this is for 0.15, should it be written relative to 0.14 instead? In 0.14, there were separate
component_read(),component_writes(), andcomponent_reads_and_writes()methods that returned plainimpl Iterators, and they returned empty iterators when the result would have been unbounded.
I've updated the migration guide to explicitly reference the 0.14 state of these methods, thanks for calling that out! To help, I've provided an implementation of an extension trait which should allow the migration to be a drop-in replacement.
I would prefer to just undeprecate it for the 0.15, to have more time for a proper fix https://github.com/bevyengine/bevy/pull/16357
Closed in favour of #16357
#16357 is not a fix for the underlying issue, just a way to make it less needed for the 0.15, and this PR seemed a possibility to fix it for longer term
Oh ok sorry I misunderstood! I'll reopen but just remove it from the 0.15 milestone.
@bushrat011899 I think this is a good idea, but it needs a bit of testing and merge conflict resolution still.
Added some unit tests and an example in the documentation. I've also improved the information provided by the UnboundedAccess error, and resolved merge conflicts.