Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions crates/bevy_ecs/src/system/adapter_system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,14 @@ where
fn set_last_run(&mut self, last_run: crate::component::Tick) {
self.system.set_last_run(last_run);
}

unsafe fn should_react_unsafe(
&self,
world: UnsafeWorldCell,
this_run: crate::component::Tick,
) -> bool {
self.system.should_react_unsafe(world, this_run)
}
}

// SAFETY: The inner system is read-only.
Expand Down
8 changes: 8 additions & 0 deletions crates/bevy_ecs/src/system/combinator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,10 @@ where
self.a.set_last_run(last_run);
self.b.set_last_run(last_run);
}

unsafe fn should_react_unsafe(&self, world: UnsafeWorldCell, this_run: Tick) -> bool {
self.a.should_react_unsafe(world, this_run) || self.b.should_react_unsafe(world, this_run)
}
}

/// SAFETY: Both systems are read-only, so any system created by combining them will only read from the world.
Expand Down Expand Up @@ -425,6 +429,10 @@ where
self.a.set_last_run(last_run);
self.b.set_last_run(last_run);
}

unsafe fn should_react_unsafe(&self, world: UnsafeWorldCell, this_run: Tick) -> bool {
self.a.should_react_unsafe(world, this_run) || self.b.should_react_unsafe(world, this_run)
}
}

/// SAFETY: Both systems are read-only, so any system created by piping them will only read from the world.
Expand Down
15 changes: 15 additions & 0 deletions crates/bevy_ecs/src/system/function_system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -728,6 +728,21 @@ where
fn set_last_run(&mut self, last_run: Tick) {
self.system_meta.last_run = last_run;
}

unsafe fn should_react_unsafe(&self, world: UnsafeWorldCell, this_run: Tick) -> bool {
let last_run = self.get_last_run();
if let Some(state) = &self.state {
<F::Param as SystemParam>::should_react(
&state.param,
&self.system_meta,
world,
last_run,
this_run,
)
} else {
false
}
Comment on lines +734 to +744
Copy link
Contributor

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

Suggested change
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)
})

}
}

/// SAFETY: `F`'s param is [`ReadOnlySystemParam`], so this system will only read from the world.
Expand Down
4 changes: 4 additions & 0 deletions crates/bevy_ecs/src/system/observer_system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,10 @@ where
fn default_system_sets(&self) -> Vec<crate::schedule::InternedSystemSet> {
self.observer.default_system_sets()
}

unsafe fn should_react_unsafe(&self, world: UnsafeWorldCell, this_run: Tick) -> bool {
self.observer.should_react_unsafe(world, this_run)
}
}

#[cfg(test)]
Expand Down
12 changes: 12 additions & 0 deletions crates/bevy_ecs/src/system/schedule_system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,10 @@ impl<S: System<In = ()>> System for InfallibleSystemWrapper<S> {
fn default_system_sets(&self) -> Vec<crate::schedule::InternedSystemSet> {
self.0.default_system_sets()
}

unsafe fn should_react_unsafe(&self, world: UnsafeWorldCell, this_run: Tick) -> bool {
self.0.should_react_unsafe(world, this_run)
}
}

/// See [`IntoSystem::with_input`] for details.
Expand Down Expand Up @@ -192,6 +196,10 @@ where
fn set_last_run(&mut self, last_run: Tick) {
self.system.set_last_run(last_run);
}

unsafe fn should_react_unsafe(&self, world: UnsafeWorldCell, this_run: Tick) -> bool {
self.system.should_react_unsafe(world, this_run)
}
}

/// Constructed in [`IntoSystem::with_input_from`].
Expand Down Expand Up @@ -292,6 +300,10 @@ where
fn set_last_run(&mut self, last_run: Tick) {
self.system.set_last_run(last_run);
}

unsafe fn should_react_unsafe(&self, world: UnsafeWorldCell, this_run: Tick) -> bool {
self.system.should_react_unsafe(world, this_run)
}
}

/// Type alias for a `BoxedSystem` that a `Schedule` can store.
Expand Down
73 changes: 73 additions & 0 deletions crates/bevy_ecs/src/system/system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,25 @@ pub trait System: Send + Sync + 'static {
/// However, it can be an essential escape hatch when, for example,
/// you are trying to synchronize representations using change detection and need to avoid infinite recursion.
fn set_last_run(&mut self, last_run: Tick);

/// Returns true if this system's input has changed since its last run and should therefore be run again to "react" to those changes.
///
/// # Safety
///
/// - The caller must ensure that [`world`](UnsafeWorldCell) has permission to access any world data
/// registered in the access returned from [`System::initialize`]. There must be no conflicting
/// simultaneous accesses while the system is running.
/// - 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
Copy link
Contributor

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.

}

/// 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 {
Copy link
Contributor

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.

// SAFETY: We have exclusive access to the entire world.
unsafe { self.should_react_unsafe(world.as_unsafe_world_cell(), this_run) }
}
}

/// [`System`] types that do not modify the [`World`] when run.
Expand Down Expand Up @@ -484,4 +503,58 @@ mod tests {
let expected = "System bevy_ecs::system::system::tests::run_system_once_invalid_params::system did not run due to failed parameter validation: Parameter `Res<T>` failed validation: Resource does not exist\nIf this is an expected state, wrap the parameter in `Option<T>` and handle `None` when it happens, or wrap the parameter in `When<T>` to skip the system when it happens.";
assert_eq!(expected, result.unwrap_err().to_string());
}

#[test]
fn system_should_react() {
struct A(usize);
impl Resource for A {}
struct B(usize);
impl Resource for B {}
let mut world = World::new();
world.insert_resource(A(0));
world.insert_resource(B(0));

let mut system = IntoSystem::into_system(|_a: Res<A>, _b: Res<B>| {});
system.initialize(&mut world);
let current_tick = world.change_tick();
assert!(
system.should_react(&mut world, current_tick),
"system should react because the system has not yet seen the initial values of A or B"
);
// run the system as a "reaction"
system.run((), &mut world);
let current_tick = world.change_tick();
assert!(
!system.should_react(&mut world, current_tick),
"system should not react because nothing has changed since the last run",
);

world.resource_mut::<A>().0 += 1;
let current_tick = world.change_tick();
assert!(
system.should_react(&mut world, current_tick),
"system should react because A changed since the last system run"
);
system.run((), &mut world);

let current_tick = world.change_tick();
assert!(
!system.should_react(&mut world, current_tick),
"system should not react because nothing has changed since the last run"
);

world.resource_mut::<B>().0 += 1;
let current_tick = world.change_tick();
assert!(
system.should_react(&mut world, current_tick),
"system should react because B changed since the last system run"
);
system.run((), &mut world);

let current_tick = world.change_tick();
assert!(
!system.should_react(&mut world, current_tick),
"system should not react because nothing has changed since the last run"
);
}
}
61 changes: 61 additions & 0 deletions crates/bevy_ecs/src/system/system_param.rs
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,23 @@ pub unsafe trait SystemParam: Sized {
world: UnsafeWorldCell<'world>,
change_tick: Tick,
) -> Self::Item<'world, 'state>;

/// Returns true if this system param has changed since the system was last run.
///
/// # Safety
///
/// - The passed [`UnsafeWorldCell`] must have access to any world data registered
/// in [`init_access`](SystemParam::init_access).
/// - `world` must be the same [`World`] that was used to initialize [`state`](SystemParam::init_state).
unsafe fn should_react(
_state: &Self::State,
_system_meta: &SystemMeta,
_world: UnsafeWorldCell,
_last_run: Tick,
_this_run: Tick,
) -> bool {
false
}
}

/// A [`SystemParam`] that only reads a given [`World`].
Expand Down Expand Up @@ -821,6 +838,20 @@ unsafe impl<'a, T: Resource> SystemParam for Res<'a, T> {
changed_by: caller.map(|caller| caller.deref()),
}
}

unsafe fn should_react(
state: &Self::State,
_system_meta: &SystemMeta,
world: UnsafeWorldCell,
last_run: Tick,
this_run: Tick,
) -> bool {
let resource_data = world.storages().resources.get(*state).unwrap();
resource_data
.get_ticks()
.unwrap()
.is_changed(last_run, this_run)
}
}

// SAFETY: Res ComponentId access is applied to SystemMeta. If this Res
Expand Down Expand Up @@ -899,6 +930,20 @@ unsafe impl<'a, T: Resource> SystemParam for ResMut<'a, T> {
changed_by: value.changed_by,
}
}

unsafe fn should_react(
state: &Self::State,
_system_meta: &SystemMeta,
world: UnsafeWorldCell,
last_run: Tick,
this_run: Tick,
) -> bool {
let resource_data = world.storages().resources.get(*state).unwrap();
Copy link
Contributor

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.

resource_data
.get_ticks()
.unwrap()
.is_changed(last_run, this_run)
}
}

/// SAFETY: only reads world
Expand Down Expand Up @@ -2163,6 +2208,12 @@ macro_rules! impl_system_param_tuple {
)]
($($param::get_param($param, system_meta, world, change_tick),)*)
}

#[inline]
unsafe fn should_react(state: &Self::State, system_meta: &SystemMeta, _world: UnsafeWorldCell, _last_run: Tick, _this_run: Tick) -> bool {
let ($($param,)*) = &state;
false $(|| <$param as SystemParam>::should_react($param, system_meta, _world, _last_run, _this_run))*
}
}
};
}
Expand Down Expand Up @@ -2327,6 +2378,16 @@ unsafe impl<P: SystemParam + 'static> SystemParam for StaticSystemParam<'_, '_,
// SAFETY: Defer to the safety of P::SystemParam
StaticSystemParam(unsafe { P::get_param(state, system_meta, world, change_tick) })
}

unsafe fn should_react(
state: &Self::State,
system_meta: &SystemMeta,
world: UnsafeWorldCell,
last_run: Tick,
this_run: Tick,
) -> bool {
P::should_react(state, system_meta, world, last_run, this_run)
Copy link
Contributor

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.)

}
}

// SAFETY: No world access.
Expand Down