bevyengine / bevy

A refreshingly simple data-driven game engine built in Rust
https://bevyengine.org
Apache License 2.0
36.46k stars 3.6k forks source link

Remove deprecated `component_reads_and_writes` #16348

Open bushrat011899 opened 2 weeks ago

bushrat011899 commented 2 weeks ago

Objective

Solution

Testing


Migration Guide

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

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:

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

BenjaminBrienen commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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

bushrat011899 commented 2 weeks ago

Closed in favour of #16357

mockersf commented 2 weeks ago

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 commented 2 weeks ago

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