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

Implement opt-in change detection bypass #2363

Closed
wants to merge 2 commits into from

Conversation

TheRawMeatball
Copy link
Member

Objective

Implements #2348

Solution

Implements the solution outlined here

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jun 19, 2021
@TheRawMeatball TheRawMeatball added A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible and removed S-Needs-Triage This issue needs to be labelled labels Jun 19, 2021
@@ -41,6 +41,9 @@ pub trait DetectChanges {
fn set_changed(&mut self);
}

// TODO: Add some nice docs here
Copy link
Member Author

Choose a reason for hiding this comment

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

If someone would contribute to this point, it'd be highly appreciated

Copy link
Member

Choose a reason for hiding this comment

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

/// When implemented for a component or resource type, this trait allows end users to manually bypass change detection within the ECS using the [`get_untracked`] method on `Query` and `World`.

I think that's where those methods live?

Copy link
Member

Choose a reason for hiding this comment

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

I don't see the get_untracked methods in Query or World.

Are they found elsewhere? Or are they part of an unmerged PR?

Copy link
Member

Choose a reason for hiding this comment

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

It was a "please add these methods too" comment for @TheRawMeatball ;)

@Ratysz
Copy link
Contributor

Ratysz commented Jun 19, 2021

This looks delightfully overengineered. I would be on board, but my sentiment from linked issue remains: do we actually need it?

The two examples so far can be (and best be, if you ask me) solved by splitting resources/components not just by the data they store, but also by the mutation events that happen to them, if that makes sense. I think that's a universal solution, be it inside a specific plugin or between several plugins.

@NathanSWard
Copy link
Contributor

NathanSWard commented Jun 20, 2021

but also by the mutation events that happen to them

@Ratysz Would you mind clarifying exactly what you mean by this?
(I assume it just mean on when they trigger change detection via DerefMut for example?)

@Ratysz
Copy link
Contributor

Ratysz commented Jun 20, 2021

but also by the mutation events that happen to them

@Ratysz Would you mind clarifying exactly what you mean by this?
(I assume it just mean on when they trigger change detection via DerefMut for example?)

Yes, pretty much. E.g., we could split Assets into AssetStorage and AssetEvents, separating the concern of providing assets from that of processing asset-related events.

@TheRawMeatball
Copy link
Member Author

While this can sometimes be an option, I don't think it can be applied globally as it requires changing the external API to do something that only bothers the internal impl. For the asset case for example, the external API shouldn't need to care about how the Assets type also accumulates events and sends them back. This impl allows for building such abstractions.

@cart cart added the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 23, 2021
@mockersf mockersf removed the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 24, 2021
@TheRawMeatball TheRawMeatball changed the title Implement opt-in change detection Implement opt-in change detection bypass Aug 18, 2021
@alice-i-cecile
Copy link
Member

Ran into a user with a serious need for this earlier today. Effectively, they wanted to make a change detecting that system that responded to all changes other than its own. I spent a while helping them and couldn't come up with a clean workaround.

This now has my 👍🏽 in terms of use case. Lets take a look at the specific design now...

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Dec 7, 2021

Gave you some docs ;) The impl looks reasonable to me. Two outstanding concerns:

  1. From an API perspective, I would also be interested in supporting Query<Untracked<&mut A>>, which better communicates the intent in the function signature and should allow ordinary iteration in ways that the current design does not.
  2. This is reasonably non-trivial: it should have tests to verify that it acts as promised.

@MrGVSV
Copy link
Member

MrGVSV commented Dec 10, 2021

I haven't used Reflect all that much, but is there a reason it doesn't have the other impl macros? (i.e., impl_into_inner, impl_change_detection_circumvention, impl_debug)

change_detection_impl!(ReflectMut<'a>, dyn Reflect,);

@TheRawMeatball
Copy link
Member Author

Closing in favor of the upcoming permissions work

@alice-i-cecile
Copy link
Member

I ran into a very compelling use case of this today.

I have two components: Direction and Rotation, that I want to keep synchronized. If one has changed, update the other to match.

However, this creates an infinite ping-pong, as doing so causes the other to be changed, thus triggering another update. The workaround is to check whether the new value is equal to the old value before setting it.

@alice-i-cecile
Copy link
Member

Reopening this: permissions has serious open questions, and this is at least an important stop-gap.

@alice-i-cecile alice-i-cecile reopened this Apr 4, 2022
nicopap pushed a commit to nicopap/bevy that referenced this pull request Sep 12, 2022
…evyengine#5635)

# Objective

- Our existing change detection API is not flexible enough for advanced users: particularly those attempting to do rollback networking.
- This is an important use case, and with adequate warnings we can make mucking about with change ticks scary enough that users generally won't do it.
- Fixes bevyengine#5633.
- Closes bevyengine#2363.

## Changelog

- added `ChangeDetection::set_last_changed` to manually mutate the `last_change_ticks` field"
- the `ChangeDetection` trait now requires an `Inner` associated type, which contains the value being wrapped.
- added `ChangeDetection::bypass_change_detection`, which hands out a raw `&mut Inner`

## Migration Guide

Add the `Inner` associated type and new methods to any type that you've implemented `DetectChanges` for.
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
…evyengine#5635)

# Objective

- Our existing change detection API is not flexible enough for advanced users: particularly those attempting to do rollback networking.
- This is an important use case, and with adequate warnings we can make mucking about with change ticks scary enough that users generally won't do it.
- Fixes bevyengine#5633.
- Closes bevyengine#2363.

## Changelog

- added `ChangeDetection::set_last_changed` to manually mutate the `last_change_ticks` field"
- the `ChangeDetection` trait now requires an `Inner` associated type, which contains the value being wrapped.
- added `ChangeDetection::bypass_change_detection`, which hands out a raw `&mut Inner`

## Migration Guide

Add the `Inner` associated type and new methods to any type that you've implemented `DetectChanges` for.
bors bot pushed a commit that referenced this pull request Nov 28, 2022
# Objective

Change detection can be spuriously triggered by setting a field to the same value as before.
As a result, a common pattern is to write:

```rust
if *foo != value {
  *foo = value;
}
```

This is confusing to read, and heavy on boilerplate.

## Solution

1. Add a method to the `DetectChanges` trait that implements this boilerplate when the appropriate trait bounds are met.
2. Document this minor footgun, and point users to it.

## Changelog

- added the `set_if_neq` method to avoid triggering change detection when the new and previous values are equal. This will work on both components and resources.

## Migration Guide

If you are manually checking if a component or resource's value is equal to its new value before setting it to avoid triggering change detection, migrate to the clearer and more convenient `set_if_neq` method.

## Context

Related to #2363 as it avoids triggering change detection, but not a complete solution (as it still requires triggering it when real changes are made).
bors bot pushed a commit that referenced this pull request Dec 11, 2022
# Objective

Change detection can be spuriously triggered by setting a field to the same value as before. As a result, a common pattern is to write:

```rust
if *foo != value {
  *foo = value;
}
```

This is confusing to read, and heavy on boilerplate.

Adopted from #5373, but untangled and rebased to current `bevy/main`.

## Solution

    1. Add a method to the `DetectChanges` trait that implements this boilerplate when the appropriate trait bounds are met.

    2. Document this minor footgun, and point users to it.


## Changelog

    * added the `set_if_neq` method to avoid triggering change detection when the new and previous values are equal. This will work on both components and resources.


## Migration Guide

If you are manually checking if a component or resource's value is equal to its new value before setting it to avoid triggering change detection, migrate to the clearer and more convenient `set_if_neq` method.
## Context

Related to #2363 as it avoids triggering change detection, but not a complete solution (as it still requires triggering it when real changes are made).



Co-authored-by: Zoey <Dessix@Dessix.net>
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
…e#6853)

# Objective

Change detection can be spuriously triggered by setting a field to the same value as before. As a result, a common pattern is to write:

```rust
if *foo != value {
  *foo = value;
}
```

This is confusing to read, and heavy on boilerplate.

Adopted from bevyengine#5373, but untangled and rebased to current `bevy/main`.

## Solution

    1. Add a method to the `DetectChanges` trait that implements this boilerplate when the appropriate trait bounds are met.

    2. Document this minor footgun, and point users to it.


## Changelog

    * added the `set_if_neq` method to avoid triggering change detection when the new and previous values are equal. This will work on both components and resources.


## Migration Guide

If you are manually checking if a component or resource's value is equal to its new value before setting it to avoid triggering change detection, migrate to the clearer and more convenient `set_if_neq` method.
## Context

Related to bevyengine#2363 as it avoids triggering change detection, but not a complete solution (as it still requires triggering it when real changes are made).



Co-authored-by: Zoey <Dessix@Dessix.net>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
…evyengine#5635)

# Objective

- Our existing change detection API is not flexible enough for advanced users: particularly those attempting to do rollback networking.
- This is an important use case, and with adequate warnings we can make mucking about with change ticks scary enough that users generally won't do it.
- Fixes bevyengine#5633.
- Closes bevyengine#2363.

## Changelog

- added `ChangeDetection::set_last_changed` to manually mutate the `last_change_ticks` field"
- the `ChangeDetection` trait now requires an `Inner` associated type, which contains the value being wrapped.
- added `ChangeDetection::bypass_change_detection`, which hands out a raw `&mut Inner`

## Migration Guide

Add the `Inner` associated type and new methods to any type that you've implemented `DetectChanges` for.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
…e#6853)

# Objective

Change detection can be spuriously triggered by setting a field to the same value as before. As a result, a common pattern is to write:

```rust
if *foo != value {
  *foo = value;
}
```

This is confusing to read, and heavy on boilerplate.

Adopted from bevyengine#5373, but untangled and rebased to current `bevy/main`.

## Solution

    1. Add a method to the `DetectChanges` trait that implements this boilerplate when the appropriate trait bounds are met.

    2. Document this minor footgun, and point users to it.


## Changelog

    * added the `set_if_neq` method to avoid triggering change detection when the new and previous values are equal. This will work on both components and resources.


## Migration Guide

If you are manually checking if a component or resource's value is equal to its new value before setting it to avoid triggering change detection, migrate to the clearer and more convenient `set_if_neq` method.
## Context

Related to bevyengine#2363 as it avoids triggering change detection, but not a complete solution (as it still requires triggering it when real changes are made).



Co-authored-by: Zoey <Dessix@Dessix.net>
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 C-Feature A new feature, making something new possible 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.

7 participants