Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Merged by Bors] - Deprecate ChangeTrackers<T> in favor of Ref<T> #7306

Closed
wants to merge 12 commits into from

Conversation

JoJoJet
Copy link
Member

@JoJoJet JoJoJet commented Jan 20, 2023

Objective

ChangeTrackers<> is a WorldQuery type that lets you access the change ticks for a component. #7097 has added Ref<>, which gives access to a component's value in addition to its change ticks. Since bevy's access model does not separate a component's value from its change ticks, there is no benefit to using ChangeTrackers<T> over Ref<T>.

Solution

Deprecate ChangeTrackers<>.


Changelog

  • ChangeTrackers<T> has been deprecated. It will be removed in Bevy 0.11.

Migration Guide

ChangeTrackers<T> has been deprecated, and will be removed in the next release. Any usage should be replaced with Ref<T>.

// Before (0.9)
fn my_system(q: Query<(&MyComponent, ChangeTrackers<MyComponent>)>) {
    for (value, trackers) in &q {
        if trackers.is_changed() {
            // Do something with `value`.
        }
    }
}

// After (0.10)
fn my_system(q: Query<Ref<MyComponent>>) {
    for value in &q {
        if value.is_changed() {
            // Do something with `value`.
        }
    }
}

@JoJoJet JoJoJet added A-ECS Entities, components, systems, and events M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Jan 20, 2023
) {
for (entity, component, trackers) in &query {
info!("{:?}: {:?} -> {:?}", entity, component, trackers);
// By using `Ref`, the query is not filtered but the information is available
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment seems incorrect. I'm not sure that it was ever correct however.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the original intended meaning is that the query is not filtered based on change detection, but you can still access the change detection info. I tried to preserve that meaning when rewriting this section of code.

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Jan 21, 2023
@alice-i-cecile
Copy link
Member

This is a bit of a controversial decision. I think it's the right one, but there's a possibility that we split change ticks from the data in a meaningful way later. Ping @maniwani @BoxyUwU for a quick second opinion?

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jan 21, 2023
@james7132
Copy link
Member

james7132 commented Jan 21, 2023

A potentially more controversial option is just to have &T return Ref<T> instead. Though that might require some benchmarking to ensure there's no perf regression in doing so.

@BoxyUwU
Copy link
Member

BoxyUwU commented Jan 21, 2023

A nice thing about making &T return Ref<T> is that it would make the ReadOnly assoc type on &mut T have an "obvious answer", with this PR it could realistically be &T or Ref<T>. I think the only downside that Mut<T> has that Ref<T> would also have is the fact that you cant pattern match on it, the other issues (shoving mut everywhere and borrowck annoyances from disjoint borrows) dont apply to Ref<T> which is cool.

@alice-i-cecile
Copy link
Member

Yeah, I think I'm sold on doing that, but it should be in a new PR. Are you two sold on deprecating ChangeTrackers then?

@BoxyUwU
Copy link
Member

BoxyUwU commented Jan 24, 2023

I was a bit concerned that someone might re-add this WorldQuery in the future as a "perf optimization for if you don't need the component data" but according to james, rust is able to optimize that use case fine as is.

I do think we should avoid actually removing the ChangeTrackers<T> type though, it doesn't have a lifetime like Ref does so it could be more flexible to pass around and store than Ref<'_, T> (for example it is not possible to store a Ref in a component, but you could store ChangeTrackers<T> in a component)

@BoxyUwU
Copy link
Member

BoxyUwU commented Jan 24, 2023

I guess there is also a code clarity question, is Query<ChangeTrackers<T>> clearer than Query<Ref<T>> if semantically all you care about is the change ticks 🤔

@alice-i-cecile
Copy link
Member

It's a bit clearer, but it also adds yet another concept to learn.

@JoJoJet JoJoJet added this to the 0.10 milestone Feb 19, 2023
@JoJoJet
Copy link
Member Author

JoJoJet commented Feb 19, 2023

I'd like to consider this for 0.10 so we don't miss out on a deprecation cycle. Obviously it's not super important, though.

@maniwani
Copy link
Contributor

maniwani commented Feb 19, 2023

A more controversial option is just to have &T return Ref<T> instead.

I want this tbh. Better symmetry.

Once we separate !Send resources from World, we have the option to remove Res(Mut)<T> (NonSend(Mut)<T> can go away). Getting resources in a system signature could just use Ref<T> and Mut<T>. Or maybe Res can be like Query.

@alice-i-cecile
Copy link
Member

That's two SMEs: merging :)

bors r+

bors bot pushed a commit that referenced this pull request Feb 19, 2023
# Objective

`ChangeTrackers<>` is a `WorldQuery` type that lets you access the change ticks for a component. #7097 has added `Ref<>`, which gives access to a component's value in addition to its change ticks. Since bevy's access model does not separate a component's value from its change ticks, there is no benefit to using `ChangeTrackers<T>` over `Ref<T>`.

## Solution

Deprecate `ChangeTrackers<>`.

---

## Changelog

* `ChangeTrackers<T>` has been deprecated. It will be removed in Bevy 0.11.

## Migration Guide

`ChangeTrackers<T>` has been deprecated, and will be removed in the next release. Any usage should be replaced with `Ref<T>`.

```rust
// Before (0.9)
fn my_system(q: Query<(&MyComponent, ChangeTrackers<MyComponent>)>) {
    for (value, trackers) in &q {
        if trackers.is_changed() {
            // Do something with `value`.
        }
    }
}

// After (0.10)
fn my_system(q: Query<Ref<MyComponent>>) {
    for value in &q {
        if value.is_changed() {
            // Do something with `value`.
        }
    }
}
```
@bors bors bot changed the title Deprecate ChangeTrackers<T> in favor of Ref<T> [Merged by Bors] - Deprecate ChangeTrackers<T> in favor of Ref<T> Feb 19, 2023
@bors bors bot closed this Feb 19, 2023
@JoJoJet JoJoJet deleted the no-change-trackers branch February 19, 2023 16:37
myreprise1 pushed a commit to myreprise1/bevy that referenced this pull request Mar 23, 2023
`ChangeTrackers<>` is a `WorldQuery` type that lets you access the change ticks for a component. bevyengine#7097 has added `Ref<>`, which gives access to a component's value in addition to its change ticks. Since bevy's access model does not separate a component's value from its change ticks, there is no benefit to using `ChangeTrackers<T>` over `Ref<T>`.

Deprecate `ChangeTrackers<>`.

---

* `ChangeTrackers<T>` has been deprecated. It will be removed in Bevy 0.11.

`ChangeTrackers<T>` has been deprecated, and will be removed in the next release. Any usage should be replaced with `Ref<T>`.

```rust
// Before (0.9)
fn my_system(q: Query<(&MyComponent, ChangeTrackers<MyComponent>)>) {
    for (value, trackers) in &q {
        if trackers.is_changed() {
            // Do something with `value`.
        }
    }
}

// After (0.10)
fn my_system(q: Query<Ref<MyComponent>>) {
    for value in &q {
        if value.is_changed() {
            // Do something with `value`.
        }
    }
}
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants