Remove deprecated component_reads_and_writes

[bushrat011899]

Objective

• Fixes Migrate component_reads_and_writes #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:

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.

[bushrat011899]

This didn't have to be removed, but its signature is pretty awful to work with, and the new method shadows this one entirely.

[bushrat011899]

Could be a non_exhaustive enum instead, but I just wanted to keep it simple. Open to changing it.

[bushrat011899]

Previously this example assumed that the iteration was always going to be over non-inverted access, which would silently misbehave at runtime if the assumption broke. Instead, the unwrap() explicitly crashes the program if that invalid state is reached.

[BenjaminBrienen]

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.

[chescock]

Adding the components from self.archetypal will add more work in QueryState::update_archetype_component_access(), which doesn't currently need to iterate archetypal at all. I don't know how to tell whether that will have any meaningful impact on performance.

[bushrat011899]

Chasing it around, the only items self.archetypal contains are components explicitly included in a Query with Has<C>. As such, I'd expect this list to be very short. Additionally, this method was already "wasting" time, since it iterated all &mut C items twice (since they're included in component_writes and component_reads_and_writes). I imagine the net effect here is zero.

My thinking was I could also remove the archetypal() method as well, since it's not being used in engine code (or examples). But I decided to just leave them separate just in case to reduce some of the controversy here.

[chescock]

It's a little odd that we have no way to enumerate component writes for an access that reads all components and writes some, but we don't have anyone trying to do that so we must not need it.

[bushrat011899]

Yeah I actually considered changing the function signature to take an iterator of "source" components, so you could always get an iterator since that source would bound the unbounded cases. But that wouldn't have worked for some of the engine code, so I decided to just pessimistically fail instead. In the future we could update the method to return a meaningful result if there's desire to.

[chescock]

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.

[bushrat011899]

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.

[bushrat011899]

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'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.

[mockersf]

I would prefer to just undeprecate it for the 0.15, to have more time for a proper fix #16357

[bushrat011899]

Closed in favour of #16357

[mockersf]

#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

[bushrat011899]

Oh ok sorry I misunderstood! I'll reopen but just remove it from the 0.15 milestone.