`Mut`/`ResMut` mutable access without triggering change detection

[NathanSWard]

What problem does this solve or what need does it fill?

There are a set of uses cases where a user may want to mutably access some type that implements change detection (e.g. Mut) without actually triggering change detection.

What solution would you like?

An API exposed for these type e.g. get_untracked() or something like that which returns a &mut T to the inner type but does not cause is_changed() to return true.

What alternative(s) have you considered?

Using unsafe primitives UnsafeCell, or wrapping them in something like a Mutex to get mutable access immutably. However, these are quite a bit overkill and add unnecessary unsafe and overhead.

[TheRawMeatball]

Something else quite similar to this would be a map function, for getting a tracked mutable reference to a field on an object.

[mockersf]

This feels like an easy footgun to avoid fixing a bad resource modelling

[NathanSWard]

This feels like an easy footgun to avoid fixing a bad resource modelling

Sure I can see this as being a potential footgun, however there are steps you can take to avoid this. E.g. not automatically exporting the associated trait, adding docs etc...

(Edited comment to only include relevant info for this issue)

[NathanSWard]

Something else quite similar to this would be a map function, for getting a tracked mutable reference to a field on an object

Ohh I actually like this more. Instead of

fn get_untracked(&mut self) -> &mut T;

do

fn bikeshedding(&mut self, f: impl FnOnce(&mut T));

[mockersf]

As I still disagree on your assessment on the State issue, I also disagree on how we should fix it 😄

For disabling change detection, its cost is very light, even more so on resource. And to get completely free of it you would also have to disable it on the system

Something that I could find interesting is having a way to disable change detection on a field at the struct declaration

#[derive(Resource)]
struct MyResource {
    field1: u8,
    #[resource(untracked)]
    field2: u8
}

[alice-i-cecile]

Something that I could find interesting is having a way to disable change detection on a field at the struct declaration

This is a much more interesting route IMO: it allows bypassing in an explicit way, defined on the struct itself. You can get the same effect with interior mutability, so I prefer the explicitness of @mockersf proposal.

I really don't like the idea of allowing consumers to make changes in an untracked way: it seriously violates the guarantees provided in a way that seems virtually impossible to debug since it could be occurring anywhere in your code base.

[NathanSWard]

I really like the API of #[resource(untracked)] @mockersf :) This definitely adds quite a bit of complexity to the implementation though.

I really don't like the idea of allowing consumers to make changes in an untracked way: it seriously violates the guarantees provided in a way that seems virtually impossible to debug since it could be occurring anywhere in your code base.

I don't think is necessarily a valid argument though against allowing users the ability to do so. I'm not saying we encourage users to do this, however, we shouldn't prohibit this finer level of control. And it's not anywhere in your code base, it's only where you explicitly use the get_unchecked method.

#[derive(Resource)]
struct MyResource {
    field1: u8,
    #[resource(untracked)]
    field2: u8
}

This is ideally something we'd want not just for Resources though? We should probably support this for Components as well.

[alice-i-cecile]

Agreed: this should work for components (and relations) too.

[TheRawMeatball]

Hmm, while the macro approach looks interesting, I'm not sure how we can implement it. The only thing I can think of is generating a trait and implementing it on Mut to add a acessor method that bypasses triggering change detection, but this causes a massive footgun where using field access instead of a method call suddenly re introduces change detection, and generating a trait fully implicitly is also rather ugly IMO. Furthermore, implementing this approach would require a full bypass anyways. So, I think we should just add the simple bypass now, and consider the much more complicated macro version after #2254 lands.

[jakobhellermann]

Having a way to mutably access a struct without change detection is very useful in bevy-inspector-egui. Since egui expects e.g. &mut f32 for a slider widget, you have to give out a mutable reference which makes change detection not work.

To fix that I compied the Mut implementation and added get_mut_silent and mark_changed functions: https://github.com/jakobhellermann/bevy-inspector-egui/blob/3a0fd8970df3337a069778ecbeee47db7cd30961/src/plugin.rs#L242-L254.

This usecase wouldn't be solved by #[resource(untracked)].

[TheRawMeatball]

Wouldn't this case be easily solved by just copying the float into a mutable temporary, giving a ref to that and writing back if it changed? I don't see why get untracked is needed here.

[jakobhellermann]

Wouldn't this case be easily solved by just copying the float into a mutable temporary, giving a ref to that and writing back if it changed? I don't see why get untracked is needed here.

The case of the float could be solved that way, but not every type that is inspectable is also Copy or Clone.

[TheRawMeatball]

But egui doesn't want &mut T?

[jakobhellermann]

The Inspectable trait has a fn ui(&mut self, ui: &mut egui::Ui) function, which borrows the data mutably. I don't see a way to work around this that isn't annoying to use.

[TheRawMeatball]

Why not change the signature to fn ui<T: Deref<Target = Self> + DerefMut>(this: &mut T, ui: &mut egui::Ui)? Ideally it'd use something like https://github.com/rust-lang/rust/issues/44874 but that's a long ways off, and this workaround doesn't seem particularly annoying.

[jakobhellermann]

I think that wouldn't work for nested inspectable types. Also it would require PartialEq impls for the struct members.

struct InpsectorData {
  float: f32,
  noise_options: NoiseOptions,
}

impl Inspectable for InspectorData {
  fn ui<T: Deref<Target = Self> + DerefMut>(this: &mut T, ui: &mut egui::Ui) {
    let mut float_copy = this.float;
    ui.slider(&mut float_copy);
    if float_copy != this.float {
      this.float = float_copy;
    }

    // how to do it for noise options?
  }
}

#[derive(Inspectable]
struct NoiseOptions {
  ..
}

I guess you could do it if there was a Res::map(value, |val| &mut val.noise_options) -> Res<NoiseOptions> but that still leaves the required PartialEq implementations.

[TheRawMeatball]

Also it would require PartialEq impls for the struct members.

Why? If there was a map function, only primitives which get passed to egui need to be clonable / comparable. Also, thanks for the nice use case demonstration for a map function :)

[NathanSWard]

The only thing I can think of is generating a trait and implementing it on Mut to add a acessor method that bypasses triggering change detection, but this causes a massive footgun where using field access instead of a method call suddenly re introduces change detection, and generating a trait fully implicitly is also rather ugly IMO

Yea, the more I think about it, the more complicated the macro version becomes. There might be some magic you can do where you wrap each field in a proxy object that holds a reference to the change ticks so you can still do

let mref = &mut my_resource.my_field;

and not need to add a get_my_field() function.

Also, something to consider is with this approach having a field that sometimes triggers change detection is more difficult. As you now have to call set_changed explicitly (which actually may be desired?). Where as with the get_unchecked approach, you would call get_unchecked if you don't want to trigger change detection, and just use the regular DerefMut impl if you do want change detection.

[NathanSWard]

Also just so I can layout the current paths forward we have:

1) add a get_untracked(&mut self) -> &mut T function 2) add a map_untracked(&mut self, f: impl FnOnce(&mut T)); 3) add a macro attribute `#[resource(untracked)]| 4) none of the above and do not allow this untracked mutable change detection.

[alice-i-cecile]

3 > 4 > 1 > 2 for me

[TheRawMeatball]

I'd rather not touch any macro stuff until #2254 lands, so for me I'd first do 2, and then do an opt-in get_untracked:

trait AllowGetUntracked {}

impl<'a, T: Component + AllowGetUntracked> Mut<'a, T> {
    fn get_untracked(&mut self) -> &mut T {
        &mut self.value
    }
}

[Ratysz]

Juxtapose with #1661.

And it's not anywhere in your code base, it's only where you explicitly use the get_unchecked method.

What about third-party plugins?

I disagree with this idea as written: it changes the API contract of change detection from reliable "this will trip on mutable access" to ambiguous "this will trip on mutable access unless it's been opted out of", which just screams "footgun" to me. Fancy macro looks more palatable, but even with an implementation like that: how would the user know it's been opted out of, before just running into it and without going to the docs or source to look up specifically this?

Most importantly: what actual problem would this solve? If it's about #2343, I strongly disagree with this being a solution. Are there any more concrete examples? I don't like the idea of complicating an API contract just because, especially if we feel like we have to hide it ("Sure I can see this as being a potential footgun, however there are steps you can take to avoid this. E.g. not automatically exporting the associated trait, adding docs etc...").

[NathanSWard]

trait AllowGetUntracked {}

I really like this trait as it allows you to opt in.

Most importantly: what actual problem would this solve? If it's about #2343, I strongly disagree with this being a solution.

Sure, #2343 we can forget about, as it's not a good example.

In #2275 we ran into a similar problem. Specifically with asset events. The Assets collection contains asset events which it sends to the related EventReader<T>. The Assets<T> collection gets flagged as changed when .assets.events.drain() is called. However, a user is unable to view any of the mutations directly in Assets when the mutation is due to the events. So this extra if is thrown in. (However it still triggers change detection when events do exist)

if !assets.events.is_empty() {
    events.send_batch(assets.events.drain())
}

Ideally I'd like to write:

events.send_batch(assets.get_unchecked().events.drain());

[NathanSWard]

Granted maybe the correct solution here is to instead de-couple the events from the Assets collection.

I think an overarching problem I see is that users can often get confused as to what change detection actually conveys.

As the engine-devs we know that this means the resource/component was accessed mutably, however as a users, sometimes it's assumed that this means the resource/component was changed in a meaningful and observably way.

If we can somehow make this distinction simpler or implement a way to avoid this confusion (e.g. opt-out of change detection for StateHandler or even a more extreme solution is to not have change detection for resources by default 🤷).

As I've seen with some of the linked issues, that the problems only really arise with change-detection of resources (this problem doesn't seem to appear with components)

(And yes I know resources are technically stored like components however Im purposely ignoring that)

[mockersf]

I think it's because resources are often used for more complex things: assets, state, thread pools, input, events, ...

Either splitting resources between observable/internal, or using interior mutability pattern sounds good to me 👍

[NathanSWard]

interior mutability pattern sounds good to me

@mockersf is this the get_unchecked stuff I proposed before or something else you're referring to? (Just clarifying 😅)

[mockersf]

no, interior mutability is https://doc.rust-lang.org/book/ch15-05-interior-mutability.html 😄

[NathanSWard]

no, interior mutability is https://doc.rust-lang.org/book/ch15-05-interior-mutability.html 😄

👌

[NathanSWard]

I'm pretty in favor of going the interior mutability route (or splitting up the resources into parts) paths.

Thanks everyone for discussing this issue with me, and now I feel ok to close this issue since I prefer the proposed paths over the get_unchecked one :)

[TheRawMeatball]

Personally, I still think an opt-in version of get_unchecked would be worth the trouble, so I'm opening this again. Interior mutability is not a very nice solution imo, as it breaks multiple very nice guarantees which rust makes and we rely on.

[TheRawMeatball]

Something else I thought of is this: if change detection is causing issues with where it's circumvented, we could extend the AllowGetUnchecked trait with a type, which would let crates opt into change detection circumvention, but only for their crates:

trait AllowGetUntracked<Marker = ()> {}

impl<'a, T: Component> Mut<'a, T> {
    fn get_untracked(&mut self) -> &mut T where T: AllowGetUntracked {
        &mut self.value
    }

    fn get_untracked_with_token<Token>(&mut self, token: Token) -> &mut T where T: AllowGetUntracked<Token> {
        &mut self.value
    }
}

By making the token type private, a crate can circumvent change detection internally without letting others do the same and potentially breaking invariants.

[FeldrinH]

One use case I would have for some form of get_unchecked is having some component which the entity uses to receive messages. I would then use change detection to know which entites need to check for new messages and clear the messages. However, without get_unchecked clearing the messages would trigger a change, causing the system to check and clear the messages again, which would cause anther change and so one, rendering the change detection useless. With get_unchecked I could essentially mark clearing the messages as a change which is not relevant to track.

I suppose there are workarounds to this, like first running a read-only query to check if changed messages are empty and then running a mutable query for those that weren't, however, get_unchecked feels at least to me like a much neater and definitely more performant solution.

[alice-i-cecile]

Closed by #5635.