Skip to content

System param validation for observers, system registry and run once #15526

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

Merged
merged 10 commits into from
Sep 30, 2024
Merged
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
21 changes: 21 additions & 0 deletions crates/bevy_ecs/src/observer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1190,4 +1190,25 @@ mod tests {
// after the observer's spawn_empty.
world.despawn(ent);
}

#[test]
fn observer_invalid_params() {
#[derive(Event)]
struct EventA;

#[derive(Resource)]
struct ResA;

#[derive(Resource)]
struct ResB;

let mut world = World::new();
// This fails because `ResA` is not present in the world
world.observe(|_: Trigger<EventA>, _: Res<ResA>, mut commands: Commands| {
commands.insert_resource(ResB);
});
world.trigger(EventA);

assert!(world.get_resource::<ResB>().is_none());
}
}
6 changes: 4 additions & 2 deletions crates/bevy_ecs/src/observer/runner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -374,8 +374,10 @@ fn observer_system_runner<E: Event, B: Bundle, S: ObserverSystem<E, B>>(
// - system is the same type erased system from above
unsafe {
(*system).update_archetype_component_access(world);
(*system).run_unsafe(trigger, world);
(*system).queue_deferred(world.into_deferred());
if (*system).validate_param_unsafe(world) {
(*system).run_unsafe(trigger, world);
(*system).queue_deferred(world.into_deferred());
}
}
}

Expand Down
40 changes: 20 additions & 20 deletions crates/bevy_ecs/src/system/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -386,8 +386,8 @@ mod tests {
.build_state(&mut world)
.build_system(local_system);

let result = world.run_system_once(system);
assert_eq!(result, 10);
let output = world.run_system_once(system).unwrap();
assert_eq!(output, 10);
}

#[test]
Expand All @@ -403,8 +403,8 @@ mod tests {
.build_state(&mut world)
.build_system(query_system);

let result = world.run_system_once(system);
assert_eq!(result, 1);
let output = world.run_system_once(system).unwrap();
assert_eq!(output, 1);
}

#[test]
Expand All @@ -418,8 +418,8 @@ mod tests {

let system = (state,).build_state(&mut world).build_system(query_system);

let result = world.run_system_once(system);
assert_eq!(result, 1);
let output = world.run_system_once(system).unwrap();
assert_eq!(output, 1);
}

#[test]
Expand All @@ -433,8 +433,8 @@ mod tests {
.build_state(&mut world)
.build_system(multi_param_system);

let result = world.run_system_once(system);
assert_eq!(result, 1);
let output = world.run_system_once(system).unwrap();
assert_eq!(output, 1);
}

#[test]
Expand Down Expand Up @@ -464,8 +464,8 @@ mod tests {
count
});

let result = world.run_system_once(system);
assert_eq!(result, 3);
let output = world.run_system_once(system).unwrap();
assert_eq!(output, 3);
}

#[test]
Expand All @@ -479,8 +479,8 @@ mod tests {
.build_state(&mut world)
.build_system(|a, b| *a + *b + 1);

let result = world.run_system_once(system);
assert_eq!(result, 1);
let output = world.run_system_once(system).unwrap();
assert_eq!(output, 1);
}

#[test]
Expand All @@ -506,8 +506,8 @@ mod tests {
params.p0().iter().count() + params.p1().iter().count()
});

let result = world.run_system_once(system);
assert_eq!(result, 5);
let output = world.run_system_once(system).unwrap();
assert_eq!(output, 5);
}

#[test]
Expand Down Expand Up @@ -535,8 +535,8 @@ mod tests {
count
});

let result = world.run_system_once(system);
assert_eq!(result, 5);
let output = world.run_system_once(system).unwrap();
assert_eq!(output, 5);
}

#[test]
Expand Down Expand Up @@ -564,8 +564,8 @@ mod tests {
},
);

let result = world.run_system_once(system);
assert_eq!(result, 4);
let output = world.run_system_once(system).unwrap();
assert_eq!(output, 4);
}

#[derive(SystemParam)]
Expand All @@ -591,7 +591,7 @@ mod tests {
.build_state(&mut world)
.build_system(|param: CustomParam| *param.local + param.query.iter().count());

let result = world.run_system_once(system);
assert_eq!(result, 101);
let output = world.run_system_once(system).unwrap();
assert_eq!(output, 101);
}
}
64 changes: 50 additions & 14 deletions crates/bevy_ecs/src/system/system.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use bevy_utils::tracing::warn;
use core::fmt::Debug;
use thiserror::Error;

use crate::{
archetype::ArchetypeComponentId,
Expand Down Expand Up @@ -269,7 +270,7 @@ where
/// let mut world = World::default();
/// let entity = world.run_system_once(|mut commands: Commands| {
/// commands.spawn_empty().id()
/// });
/// }).unwrap();
/// # assert!(world.get_entity(entity).is_some());
/// ```
///
Expand All @@ -289,7 +290,7 @@ where
/// world.spawn(T(1));
/// let count = world.run_system_once(|query: Query<&T>| {
/// query.iter().filter(|t| t.0 == 1).count()
/// });
/// }).unwrap();
///
/// # assert_eq!(count, 2);
/// ```
Expand All @@ -311,25 +312,25 @@ where
/// world.spawn(T(0));
/// world.spawn(T(1));
/// world.spawn(T(1));
/// let count = world.run_system_once(count);
/// let count = world.run_system_once(count).unwrap();
///
/// # assert_eq!(count, 2);
/// ```
pub trait RunSystemOnce: Sized {
/// Runs a system and applies its deferred parameters.
fn run_system_once<T, Out, Marker>(self, system: T) -> Out
/// Tries to run a system and apply its deferred parameters.
fn run_system_once<T, Out, Marker>(self, system: T) -> Result<Out, RunSystemError>
where
T: IntoSystem<(), Out, Marker>,
{
self.run_system_once_with((), system)
}

/// Runs a system with given input and applies its deferred parameters.
/// Tries to run a system with given input and apply deferred parameters.
fn run_system_once_with<T, In, Out, Marker>(
self,
input: SystemIn<'_, T::System>,
system: T,
) -> Out
) -> Result<Out, RunSystemError>
where
T: IntoSystem<In, Out, Marker>,
In: SystemInput;
Expand All @@ -340,14 +341,36 @@ impl RunSystemOnce for &mut World {
self,
input: SystemIn<'_, T::System>,
system: T,
) -> Out
) -> Result<Out, RunSystemError>
where
T: IntoSystem<In, Out, Marker>,
In: SystemInput,
{
let mut system: T::System = IntoSystem::into_system(system);
system.initialize(self);
system.run(input, self)
if system.validate_param(self) {
Ok(system.run(input, self))
} else {
Err(RunSystemError::InvalidParams(system.name()))
}
}
}

/// Running system failed.
#[derive(Error)]
pub enum RunSystemError {
/// System could not be run due to parameters that failed validation.
///
/// This can occur because the data required by the system was not present in the world.
#[error("The data required by the system {0:?} was not found in the world and the system did not run due to failed parameter validation.")]
InvalidParams(Cow<'static, str>),
}

impl Debug for RunSystemError {
fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
match self {
Self::InvalidParams(arg0) => f.debug_tuple("InvalidParams").field(arg0).finish(),
}
}
}

Expand All @@ -369,7 +392,7 @@ mod tests {
}

let mut world = World::default();
let n = world.run_system_once_with(1, system);
let n = world.run_system_once_with(1, system).unwrap();
assert_eq!(n, 2);
assert_eq!(world.resource::<T>().0, 1);
}
Expand All @@ -387,9 +410,9 @@ mod tests {
let mut world = World::new();
world.init_resource::<Counter>();
assert_eq!(*world.resource::<Counter>(), Counter(0));
world.run_system_once(count_up);
world.run_system_once(count_up).unwrap();
assert_eq!(*world.resource::<Counter>(), Counter(1));
world.run_system_once(count_up);
world.run_system_once(count_up).unwrap();
assert_eq!(*world.resource::<Counter>(), Counter(2));
}

Expand All @@ -402,7 +425,7 @@ mod tests {
fn command_processing() {
let mut world = World::new();
assert_eq!(world.entities.len(), 0);
world.run_system_once(spawn_entity);
world.run_system_once(spawn_entity).unwrap();
assert_eq!(world.entities.len(), 1);
}

Expand All @@ -415,7 +438,20 @@ mod tests {
let mut world = World::new();
world.insert_non_send_resource(Counter(10));
assert_eq!(*world.non_send_resource::<Counter>(), Counter(10));
world.run_system_once(non_send_count_down);
world.run_system_once(non_send_count_down).unwrap();
assert_eq!(*world.non_send_resource::<Counter>(), Counter(9));
}

#[test]
fn run_system_once_invalid_params() {
struct T;
impl Resource for T {}
fn system(_: Res<T>) {}

let mut world = World::default();
// This fails because `T` has not been added to the world yet.
let result = world.run_system_once(system);

assert!(matches!(result, Err(RunSystemError::InvalidParams(_))));
}
}
4 changes: 2 additions & 2 deletions crates/bevy_ecs/src/system/system_name.rs
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ mod tests {
let mut world = World::default();
let system =
IntoSystem::into_system(|name: SystemName| name.name().to_owned()).with_name("testing");
let name = world.run_system_once(system);
let name = world.run_system_once(system).unwrap();
assert_eq!(name, "testing");
}

Expand All @@ -151,7 +151,7 @@ mod tests {
let system =
IntoSystem::into_system(|_world: &mut World, name: SystemName| name.name().to_owned())
.with_name("testing");
let name = world.run_system_once(system);
let name = world.run_system_once(system).unwrap();
assert_eq!(name, "testing");
}
}
Loading