bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Remove deprecated `component_reads_and_writes`

Open bushrat011899 opened this issue 1 year ago • 8 comments

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:

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 avatar Nov 11 '24 08:11 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.

BenjaminBrienen avatar Nov 11 '24 15:11 BenjaminBrienen

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.

chescock avatar Nov 11 '24 16:11 chescock

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 avatar Nov 11 '24 20:11 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.

bushrat011899 avatar Nov 11 '24 21:11 bushrat011899

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

mockersf avatar Nov 11 '24 22:11 mockersf

Closed in favour of #16357

bushrat011899 avatar Nov 11 '24 23:11 bushrat011899

#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

mockersf avatar Nov 11 '24 23:11 mockersf

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

bushrat011899 avatar Nov 11 '24 23:11 bushrat011899

@bushrat011899 I think this is a good idea, but it needs a bit of testing and merge conflict resolution still.

alice-i-cecile avatar Feb 26 '25 00:02 alice-i-cecile

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.

bushrat011899 avatar Feb 26 '25 10:02 bushrat011899