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

Mutate Observer #16183

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Mutate Observer #16183

wants to merge 2 commits into from

Conversation

pyhinox
Copy link

@pyhinox pyhinox commented Oct 31, 2024

Objective

Solution

A EntityChanges which stores the change list is added to World. When Mut::set_changed is called, a change record will be added to the EntityChanges.
When World::flush is called, EntityChanges will be clear and trigger OnMutate event to each relative entity.

Example

world.observe(|trigger: Trigger<OnMutate, Transform>, query: Query<&Transform>| {
    let transform = query.get(trigger.entity()).unwrap();
});

Testing

  • Did you test these changes? If so, how?
    Just test for the base function, I don't know which test i should do.
  • Are there any parts that need more testing?
    Query::iter and Query::get_mut's benchmark
  • How can other people (reviewers) test your changes? Is there anything specific they need to know?
    Just treat the OnMutate as a common observer. Nothing is specific.
  • If relevant, what platforms did you test these changes on, and are there any important ones you can't test?
    Mac

Possible Follow-ups

  1. Add an OnMutate hook

Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@BenjaminBrienen BenjaminBrienen added D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged A-ECS Entities, components, systems, and events labels Oct 31, 2024
@pyhinox pyhinox force-pushed the mutate_observer branch 6 times, most recently from 5cb7832 to 61511de Compare October 31, 2024 13:22
Copy link
Contributor

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

@alice-i-cecile alice-i-cecile added the M-Needs-Release-Note Work that should be called out in the blog due to impact label Oct 31, 2024
Copy link
Contributor

github-actions bot commented Nov 1, 2024

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

Copy link
Contributor

github-actions bot commented Nov 4, 2024

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

Copy link
Contributor

github-actions bot commented Nov 4, 2024

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

@pyhinox pyhinox force-pushed the mutate_observer branch 2 times, most recently from 0e4cdd7 to 5933212 Compare November 4, 2024 08:04
Copy link
Contributor

github-actions bot commented Nov 4, 2024

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

@pyhinox pyhinox force-pushed the mutate_observer branch 3 times, most recently from f836647 to 4888eec Compare November 4, 2024 08:53
Copy link
Contributor

github-actions bot commented Nov 4, 2024

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

@pyhinox pyhinox force-pushed the mutate_observer branch 2 times, most recently from 313690e to 13bdcf9 Compare November 4, 2024 09:18
Copy link
Contributor

github-actions bot commented Nov 4, 2024

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

Copy link
Contributor

github-actions bot commented Nov 4, 2024

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

@pyhinox pyhinox changed the title mutate observer Mutate Observer Nov 4, 2024
@pyhinox
Copy link
Author

pyhinox commented Nov 4, 2024

@alice-i-cecile Hi alice, i thought this pr is ready to review now. What should I do next?

@BenjaminBrienen BenjaminBrienen added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Nov 9, 2024
@BenjaminBrienen
Copy link
Contributor

@pyhinox there are some merge conflicts, but I'll go ahead and mark it for review to start getting eyes on it. 🙂

@mockersf
Copy link
Member

mockersf commented Nov 9, 2024

Could you use core::cell::RefCell instead of std::cell::RefCell?

CI is failing for a few invalid links in documentation, mostly things that are not in scope for automatic doc links

@BenjaminBrienen
Copy link
Contributor

I thought we had a lint enabled to catch that as well. Good eye.

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Nov 13, 2024

I think that we can use #16208 to remove the need for this. If certain components can't be mutated, we can do all of our reactive logic in the insertion / removal stage 🎉

@bushrat011899
Copy link
Contributor

I thought we had a lint enabled to catch that as well. Good eye.

I don't believe clippy lints apply to documentation sadly. I got bit by that when adding the lints!

@JMS55
Copy link
Contributor

JMS55 commented Nov 14, 2024

@alice-i-cecile how does #16208 remove the need for this? What happens if you want to observe a component changing?

@bushrat011899
Copy link
Contributor

@alice-i-cecile how does #16208 remove the need for this? What happens if you want to observe a component changing?

I believe the idea was if you want to watch a component, make that component immutable so that the current hooks can capture all state changes. It doesn't cover the case where you want to observe a component which isn't immutable though.

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Nov 14, 2024

I believe the idea was if you want to watch a component, make that component immutable so that the current hooks can capture all state changes.

Precisely.

It doesn't cover the case where you want to observe a component which isn't immutable though.

Yeah. Ultimately, virtually all of the same drawbacks apply to both the OnMutate and the immutable components approaches to this problem:

  • can only be safely mutated during exclusive world access to ensure side effects are processed immediately
  • changes are relatively expensive
  • must be opted in (otherwise the current design would incur an unacceptable global perf penalty)

I'd like to see if we can get away with only using a single new concept to achieve these benefits, and then re-evaluate once we have a clearer picture of the final performance and ergonomics.

EDIT: on reflection, I think that this is probably still useful, because it allows users to do reactivity things with any arbitrary component, which is really valuable for UI. I suspect that we should have on-mutate observers fill a "not very strict + deferred reactions" niche, while reserving immutable components + hooks for really critical invariants that must be constantly upheld.

@cart
Copy link
Member

cart commented Dec 11, 2024

on reflection, I think that this is probably still useful, because it allows users to do reactivity things with any arbitrary component, which is really valuable for UI. I suspect that we should have on-mutate observers fill a "not very strict + deferred reactions" niche, while reserving immutable components + hooks for really critical invariants that must be constantly upheld.

Yeah I'm starting to want something like this as part of our reactivity solution, over less event driven / poll based approaches (tbd though).

But yeah the current unconditional impl should not be merged. If we land this, we can't affect the default Mut path. That is hot / costly enough as it is.

The Option also appears to exist to accommodate resources. But I'd rather stop using Mut with resources (in favor of ResMut or alternatives) than pay the unnecessary size cost of the Option for resources and the unnecessary branching cost of the option for components.

@alice-i-cecile alice-i-cecile added X-Controversial There is active debate or serious implications around merging this PR S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Dec 11, 2024
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 D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes M-Needs-Release-Note Work that should be called out in the blog due to impact S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged 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