-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Mut
/ResMut
mutable access without triggering change detection
#2348
Comments
Something else quite similar to this would be a |
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) |
Ohh I actually like this more. fn get_untracked(&mut self) -> &mut T; do fn bikeshedding(&mut self, f: impl FnOnce(&mut T)); |
As I still disagree on your assessment on the 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
} |
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. |
I really like the API of
I don't think is necessarily a valid argument though against allowing users the ability to do so. #[derive(Resource)]
struct MyResource {
field1: u8,
#[resource(untracked)]
field2: u8
} This is ideally something we'd want not just for |
Agreed: this should work for components (and relations) too. |
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. |
Having a way to mutably access a struct without change detection is very useful in To fix that I compied the This usecase wouldn't be solved by |
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 |
But egui doesn't want &mut T? |
The |
Why not change the signature to |
I think that wouldn't work for nested inspectable types. 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 |
Why? If there was a |
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 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 |
Also just so I can layout the current paths forward we have:
|
3 > 4 > 1 > 2 for me |
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
}
} |
Juxtapose with #1661.
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..."). |
trait AllowGetUntracked {} I really like this trait as it allows you to opt in.
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 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()); |
Granted maybe the correct solution here is to instead de-couple the events from the 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 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) |
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 👍 |
@mockersf is this the |
no, interior mutability is https://doc.rust-lang.org/book/ch15-05-interior-mutability.html 😄 |
👌 |
I'm pretty in favor of going the 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 |
Personally, I still think an opt-in version of |
Something else I thought of is this: if change detection is causing issues with where it's circumvented, we could extend the 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. |
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. |
Closed by #5635. |
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 causeis_changed()
to return true.What alternative(s) have you considered?
Using unsafe primitives
UnsafeCell
, or wrapping them in something like aMutex
to get mutable access immutably. However, these are quite a bit overkill and add unnecessaryunsafe
and overhead.The text was updated successfully, but these errors were encountered: