-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Reactive Systems / SystemParams and Resource impl #19723
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
base: main
Are you sure you want to change the base?
Conversation
|
Upon review, this isn't usable enough to warrant a release note yet :) Still a very nice, clean step forward! |
I would like this to be opt-out using a wrapper param. We recently merged |
Yeah I think we need to sort out opt-in/out param UX first |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the right shape of solution (and much more thought out than my proposal): I'm happy to land this as an incremental step forward.
|
Very nice, I like the ability to poll a System if any of its SystemParams have Updated/Changed. |
I agree, it took me a while to figure out what Com was supposed to mean. Abbreviations are always dangerous in that aspect. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool!
- Should we use the word "react" here (ex:
System::should_react)? "Reactivity" intersects with the UI space, and this feature may or may not be used for the official reactive Bevy UI solution.
A more neutral option here might be has_param_changed, in analogy to the existing is_changed terminology. (Or maybe you're planning to extend this in a way where that would no longer match?)
| last_run: Tick, | ||
| this_run: Tick, | ||
| ) -> bool { | ||
| P::should_react(state, system_meta, world, last_run, this_run) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You propagated should_react through tuples and StaticSystemParam, but there are a handful of other wrapper types that we might also want to propagate through: When, Option, Result, Vec, DynSystemParam, ParamSet<(tuples)>, and ParamSet<Vec>. We'll probably want to propagate through #[derive(SystemParam)], too.
Or maybe you were already counting those under "Next Steps: Make more params reactive".
(Hmm, it might be hard to make Option<Res<T>> react to the resource being removed, so maybe that one is even trickier.)
| if let Some(state) = &self.state { | ||
| <F::Param as SystemParam>::should_react( | ||
| &state.param, | ||
| &self.system_meta, | ||
| world, | ||
| last_run, | ||
| this_run, | ||
| ) | ||
| } else { | ||
| false | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might be a bit more idiomatic using is_some_and
| if let Some(state) = &self.state { | |
| <F::Param as SystemParam>::should_react( | |
| &state.param, | |
| &self.system_meta, | |
| world, | |
| last_run, | |
| this_run, | |
| ) | |
| } else { | |
| false | |
| } | |
| self.state.as_ref().is_some_and(|state| { | |
| F::Param::should_react(&state.param, &self.system_meta, world, last_run, this_run) | |
| }) |
| } | ||
|
|
||
| /// Returns true if this system's input has changed since its last run and should therefore be run again to "react" to those changes. | ||
| fn should_react(&self, world: &mut World, this_run: Tick) -> bool { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really want to allow mutable access in should_react? In contrast, validate_param only permits read access, so that it can be called with &World instead of needing &mut World.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks like a good, uncontroversial, small step forward. IIUC, this is effectively going to turn into pull style observers. To be honest, I don't know how useful that would be over normal observers, but I've seen you're reactivity ideas, so I'm trusting that you know where this is headed.
On the subject of including "react" in the names, here's my two cents: Systems params will have either changed since the last run or not (a bool). Wether or not a system should react or not feels more like three options: ("I'm not reactive", "I'm reactive and should run", "I'm reactive but shouldn't run".) So personally, I would prefer either doing should_react() -> Option<bool>, where None would mean the system should run since it's not reactive, or you could try any_params_changed() -> bool and is_reactive() -> bool. I don't think it's a huge deal either way, but that's my thought.
For what it's worth, Com and ComMut confused me at first too. Two ideas to consider: If this really is just a component value, why not Ref and Mut. Or even &T and &mut T? Or, if it is intended to support all QueryData types, maybe This<Q: QueryData> or OfTriggeredEntity<Q> or OfWatched<Q>, etc. I don't what what we'd land on, and we don't need to decide now, but those are some ideas.
In any event, this looks good to me. It's nice to see incremental improvements like this.
| last_run: Tick, | ||
| this_run: Tick, | ||
| ) -> bool { | ||
| let resource_data = world.storages().resources.get(*state).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a better way to handle this besides unwrapping? Other unwraps here give a nice error message. It seems possible that the should_react could cause a crash with an opaque error message before get_param would crash with a user friendly error message. It may also be worth providing a nice error message when getting the ticks fails. Same goes for Res as well.
| /// - If [`System::is_exclusive`] returns `true`, then it must be valid to call | ||
| /// [`UnsafeWorldCell::world_mut`] on `world`. | ||
| unsafe fn should_react_unsafe(&self, _world: UnsafeWorldCell, _this_run: Tick) -> bool { | ||
| false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
False is probably the correct default if we plan to make reactive runs (like those "run if any params changed" ideas) fully opt-in. It's worth mentioning that if we default this to true, we could just make every system run if any params change. That's maybe not what you're shooting for, but I figured I'd point it out.
|
I was going to suggest adapting the The current code updates widgets whose state have changed, using a combination of To do this efficiently, I'd need to be able to use The real problem, then, is that the These small-scoped systems have their own problem, which is what I think you were getting at with |
|
I'm happy to merge this as an incremental step, but we're going to need a lot more experimentation here :) |
|
Haha forgive me for removing it from the queue. I'd like to respond to some of the feedback and consider some terminology stuff :) |
|
I've been thinking about this a lot lately. I don't think this solution, as formulated, will be effective; however I think I know a possible variation that might. The problem with the approach presented here is that the change detection on queries is too coarse-grained, and will likely lead to (potentially massive) overreaction: formulas getting recomputed too often because of changes to unrelated entities. As I have written elsewhere (#20821), the problem has to do with the way that we use queries in Bevy: we often cast a broad net and then pick out one or a handful of entities that we are really interested in. If we react whenever any entity in the query changes, then we end up reacting even for entities we don't care about. The solution, I think, is a hybrid approach which combines dependency injection with tracking scopes. We start by defining tracking variants of all of the normal injectable types: For queries, accessing an individual entity gives us an Internally, these objects contain a We can detect whether the reaction should be run by comparing the set of tracked ids to the set of changed resources and components. Every tracking scope has a tick count for the previous reaction; a reaction should happen whenever there are changes more recent than this. Exactly how the list of dependent ids should be stored is a design question. In my current prototypes, I use a regular HashMap. I have thought about whether it would be possible to use a bloom filter, but I don't think that will work. This is too bad as it would allow tracking scopes to be a fixed size. In either case, what we are attempting to calculate is whether two sets intersect: the set of tracked components and the set of changed components. Whether this is done by iterating over the tracked components and looking up the tick count for each component, or by looking up the tracking scopes directly after a mutation, will determine whether we use a forward index or a reverse index. For queries specifically, there are three kinds of changes we care about:
A secondary advantage of this scheme is that we know when the reactions are done reacting, we can use the "run to convergence" strategy rather than depending on the order of spawning mentioned above. The problem with relying on spawn order is that there are some rare edge cases where we get an unavoidable 1-frame delay. This can happen if there's a level of indirection between the reaction and its dependency, such that the indirect reference was mutated after the reaction was created to point to a dependency that was spawned later. This scheme does not work particularly well for mutable injections. To avoid issues around mutation in a reactive context, I have been dividing up my reactions into a "read" or "lens" (as the term is used in functional programming) phase, followed by a non-reactive "effect" phase. Mutation is strictly confined to the effect phase, which does not do any tracking. |
Objective
We increasingly would like to write "reactive systems" that can detect when their inputs have been changed. This would enable things like "reactive UI" patterns.
Relates to #19712 (but does not implement a run condition ... see Next Steps)
Solution
This is the implementation from my Reactivity Proposal (without the other tangential bits like system entity targets or controversial bits like the reactive Com SystemParam).
SystemParam::should_reactandSystem::should_react, making it possible to detect whether a param or system has changed since the last system run in such a way that should prompt a reactive run of a system.System::should_react_unsafe, which uses UnsafeWorldCell and can be run in parallel contexts.This notably does not add new traits. Reactivity is something that SystemParams and Systems opt in to by overriding default impls that return
falseforshould_react. It is implemented this way for a number of reasons:Note that I do not think this is necessarily the solution to the "reactive UI" problem. It might be a piece of the puzzle. This PR is about extending the system API to support reacting to input changes.
Open Questions:
ResandResMutare currently reactive in this PR. Would we use something likeNoReact<Res<Time>>, Res<Counter>to prevent reacting to the time parameter while reacting to the Counter parameter? Should we opt parameters in to reactivity viaReact<Res<Time>>, Res<Counter>? Should we have reactive variants (ex:Res<Time>, ReactRes<Counter>).System::should_react)? "Reactivity" intersects with the UI space, and this feature may or may not be used for the official reactive Bevy UI solution.Next Steps
should_reactis true (aka the ShouldReactRunCondition returns true), because we can't be sure the system will actually run (as the run condition could be combined with other run conditions). We would need to be able to somehow access the actual target system's change ticks from within run condition systems, which I'm not sure is something we want to do. I'm not sure we even want/need a run condition, as most "reactive" scenarios I can imagine would be their own system execution orchestrators (ex: reactive UI running in an exclusive system that polls reactive systems for changes and runs reactions to completion).should_react()