feat(ecs): implement fallible observer systems (#17731)

This commit builds on top of the work done in #16589 and #17051, by
adding support for fallible observer systems.

As with the previous work, the actual results of the observer system are
suppressed for now, but the intention is to provide a way to handle
errors in a global way.

Until then, you can use a `PipeSystem` to manually handle results.

---------

Signed-off-by: Jean Mertz <git@jeanmertz.com>

Author: JeanMertz

Cargo.toml

@@ -2165,6 +2165,7 @@ name = "Fallible Systems"
 description = "Systems that return results to handle errors"
 category = "ECS (Entity Component System)"
 wasm = false
+required-features = ["bevy_mesh_picking_backend"]
 
 [[example]]
 name = "startup_system"

crates/bevy_ecs/src/observer/runner.rs

@@ -6,6 +6,7 @@ use crate::{
     observer::{ObserverDescriptor, ObserverTrigger},
     prelude::*,
     query::DebugCheckedUnwrap,
+    result::{DefaultSystemErrorHandler, SystemErrorContext},
     system::{IntoObserverSystem, ObserverSystem},
     world::DeferredWorld,
 };
@@ -272,6 +273,7 @@ pub struct Observer {
     system: Box<dyn Any + Send + Sync + 'static>,
     descriptor: ObserverDescriptor,
     hook_on_add: ComponentHook,
+    error_handler: Option<fn(Error, SystemErrorContext)>,
 }
 
 impl Observer {
@@ -282,6 +284,7 @@ impl Observer {
             system: Box::new(IntoObserverSystem::into_system(system)),
             descriptor: Default::default(),
             hook_on_add: hook_on_add::<E, B, I::System>,
+            error_handler: None,
         }
     }
 
@@ -316,6 +319,14 @@ impl Observer {
         self
     }
 
+    /// Set the error handler to use for this observer.
+    ///
+    /// See the [`result` module-level documentation](crate::result) for more information.
+    pub fn with_error_handler(mut self, error_handler: fn(Error, SystemErrorContext)) -> Self {
+        self.error_handler = Some(error_handler);
+        self
+    }
+
     /// Returns the [`ObserverDescriptor`] for this [`Observer`].
     pub fn descriptor(&self) -> &ObserverDescriptor {
         &self.descriptor
@@ -363,6 +374,15 @@ fn observer_system_runner<E: Event, B: Bundle, S: ObserverSystem<E, B>>(
     }
     state.last_trigger_id = last_trigger;
 
+    // SAFETY: Observer was triggered so must have an `Observer` component.
+    let error_handler = unsafe {
+        observer_cell
+            .get::<Observer>()
+            .debug_checked_unwrap()
+            .error_handler
+            .debug_checked_unwrap()
+    };
+
     let trigger: Trigger<E, B> = Trigger::new(
         // SAFETY: Caller ensures `ptr` is castable to `&mut T`
         unsafe { ptr.deref_mut() },
@@ -386,7 +406,15 @@ fn observer_system_runner<E: Event, B: Bundle, S: ObserverSystem<E, B>>(
     unsafe {
         (*system).update_archetype_component_access(world);
         if (*system).validate_param_unsafe(world) {
-            (*system).run_unsafe(trigger, world);
+            if let Err(err) = (*system).run_unsafe(trigger, world) {
+                error_handler(
+                    err,
+                    SystemErrorContext {
+                        name: (*system).name(),
+                        last_run: (*system).get_last_run(),
+                    },
+                );
+            };
             (*system).queue_deferred(world.into_deferred());
         }
     }
@@ -416,10 +444,15 @@ fn hook_on_add<E: Event, B: Bundle, S: ObserverSystem<E, B>>(
             ..Default::default()
         };
 
+        let error_handler = world.get_resource_or_init::<DefaultSystemErrorHandler>().0;
+
         // Initialize System
         let system: *mut dyn ObserverSystem<E, B> =
             if let Some(mut observe) = world.get_mut::<Observer>(entity) {
                 descriptor.merge(&observe.descriptor);
+                if observe.error_handler.is_none() {
+                    observe.error_handler = Some(error_handler);
+                }
                 let system = observe.system.downcast_mut::<S>().unwrap();
                 &mut *system
             } else {
@@ -442,3 +475,44 @@ fn hook_on_add<E: Event, B: Bundle, S: ObserverSystem<E, B>>(
         }
     });
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::{event::Event, observer::Trigger};
+
+    #[derive(Event)]
+    struct TriggerEvent;
+
+    #[test]
+    #[should_panic(expected = "I failed!")]
+    fn test_fallible_observer() {
+        fn system(_: Trigger<TriggerEvent>) -> Result {
+            Err("I failed!".into())
+        }
+
+        let mut world = World::default();
+        world.add_observer(system);
+        Schedule::default().run(&mut world);
+        world.trigger(TriggerEvent);
+    }
+
+    #[test]
+    fn test_fallible_observer_ignored_errors() {
+        #[derive(Resource, Default)]
+        struct Ran(bool);
+
+        fn system(_: Trigger<TriggerEvent>, mut ran: ResMut<Ran>) -> Result {
+            ran.0 = true;
+            Err("I failed!".into())
+        }
+
+        let mut world = World::default();
+        world.init_resource::<Ran>();
+        let observer = Observer::new(system).with_error_handler(crate::result::ignore);
+        world.spawn(observer);
+        Schedule::default().run(&mut world);
+        world.trigger(TriggerEvent);
+        assert!(world.resource::<Ran>().0);
+    }
+}

crates/bevy_ecs/src/system/observer_system.rs

@@ -1,22 +1,27 @@
+use alloc::{borrow::Cow, vec::Vec};
+use core::marker::PhantomData;
+
 use crate::{
+    archetype::ArchetypeComponentId,
+    component::{ComponentId, Tick},
     prelude::{Bundle, Trigger},
-    system::System,
+    query::Access,
+    result::Result,
+    schedule::{Fallible, Infallible},
+    system::{input::SystemIn, System},
+    world::{unsafe_world_cell::UnsafeWorldCell, DeferredWorld, World},
 };
 
 use super::IntoSystem;
 
 /// Implemented for [`System`]s that have a [`Trigger`] as the first argument.
-pub trait ObserverSystem<E: 'static, B: Bundle, Out = ()>:
+pub trait ObserverSystem<E: 'static, B: Bundle, Out = Result>:
     System<In = Trigger<'static, E, B>, Out = Out> + Send + 'static
 {
 }
 
-impl<
-        E: 'static,
-        B: Bundle,
-        Out,
-        T: System<In = Trigger<'static, E, B>, Out = Out> + Send + 'static,
-    > ObserverSystem<E, B, Out> for T
+impl<E: 'static, B: Bundle, Out, T> ObserverSystem<E, B, Out> for T where
+    T: System<In = Trigger<'static, E, B>, Out = Out> + Send + 'static
 {
 }
 
@@ -32,31 +37,158 @@ impl<
     label = "the trait `IntoObserverSystem` is not implemented",
     note = "for function `ObserverSystem`s, ensure the first argument is a `Trigger<T>` and any subsequent ones are `SystemParam`"
 )]
-pub trait IntoObserverSystem<E: 'static, B: Bundle, M, Out = ()>: Send + 'static {
+pub trait IntoObserverSystem<E: 'static, B: Bundle, M, Out = Result>: Send + 'static {
     /// The type of [`System`] that this instance converts into.
     type System: ObserverSystem<E, B, Out>;
 
     /// Turns this value into its corresponding [`System`].
     fn into_system(this: Self) -> Self::System;
 }
 
-impl<
-        S: IntoSystem<Trigger<'static, E, B>, Out, M> + Send + 'static,
-        M,
-        Out,
-        E: 'static,
-        B: Bundle,
-    > IntoObserverSystem<E, B, M, Out> for S
+impl<E, B, M, Out, S> IntoObserverSystem<E, B, (Fallible, M), Out> for S
 where
+    S: IntoSystem<Trigger<'static, E, B>, Out, M> + Send + 'static,
     S::System: ObserverSystem<E, B, Out>,
+    E: 'static,
+    B: Bundle,
 {
-    type System = <S as IntoSystem<Trigger<'static, E, B>, Out, M>>::System;
+    type System = S::System;
 
     fn into_system(this: Self) -> Self::System {
         IntoSystem::into_system(this)
     }
 }
 
+impl<E, B, M, S> IntoObserverSystem<E, B, (Infallible, M), Result> for S
+where
+    S: IntoSystem<Trigger<'static, E, B>, (), M> + Send + 'static,
+    S::System: ObserverSystem<E, B, ()>,
+    E: Send + Sync + 'static,
+    B: Bundle,
+{
+    type System = InfallibleObserverWrapper<E, B, S::System>;
+
+    fn into_system(this: Self) -> Self::System {
+        InfallibleObserverWrapper::new(IntoSystem::into_system(this))
+    }
+}
+
+/// A wrapper that converts an observer system that returns `()` into one that returns `Ok(())`.
+pub struct InfallibleObserverWrapper<E, B, S> {
+    observer: S,
+    _marker: PhantomData<(E, B)>,
+}
+
+impl<E, B, S> InfallibleObserverWrapper<E, B, S> {
+    /// Create a new `InfallibleObserverWrapper`.
+    pub fn new(observer: S) -> Self {
+        Self {
+            observer,
+            _marker: PhantomData,
+        }
+    }
+}
+
+impl<E, B, S> System for InfallibleObserverWrapper<E, B, S>
+where
+    S: ObserverSystem<E, B, ()>,
+    E: Send + Sync + 'static,
+    B: Bundle,
+{
+    type In = Trigger<'static, E, B>;
+    type Out = Result;
+
+    #[inline]
+    fn name(&self) -> Cow<'static, str> {
+        self.observer.name()
+    }
+
+    #[inline]
+    fn component_access(&self) -> &Access<ComponentId> {
+        self.observer.component_access()
+    }
+
+    #[inline]
+    fn archetype_component_access(&self) -> &Access<ArchetypeComponentId> {
+        self.observer.archetype_component_access()
+    }
+
+    #[inline]
+    fn is_send(&self) -> bool {
+        self.observer.is_send()
+    }
+
+    #[inline]
+    fn is_exclusive(&self) -> bool {
+        self.observer.is_exclusive()
+    }
+
+    #[inline]
+    fn has_deferred(&self) -> bool {
+        self.observer.has_deferred()
+    }
+
+    #[inline]
+    unsafe fn run_unsafe(
+        &mut self,
+        input: SystemIn<'_, Self>,
+        world: UnsafeWorldCell,
+    ) -> Self::Out {
+        self.observer.run_unsafe(input, world);
+        Ok(())
+    }
+
+    #[inline]
+    fn run(&mut self, input: SystemIn<'_, Self>, world: &mut World) -> Self::Out {
+        self.observer.run(input, world);
+        Ok(())
+    }
+
+    #[inline]
+    fn apply_deferred(&mut self, world: &mut World) {
+        self.observer.apply_deferred(world);
+    }
+
+    #[inline]
+    fn queue_deferred(&mut self, world: DeferredWorld) {
+        self.observer.queue_deferred(world);
+    }
+
+    #[inline]
+    unsafe fn validate_param_unsafe(&mut self, world: UnsafeWorldCell) -> bool {
+        self.observer.validate_param_unsafe(world)
+    }
+
+    #[inline]
+    fn initialize(&mut self, world: &mut World) {
+        self.observer.initialize(world);
+    }
+
+    #[inline]
+    fn update_archetype_component_access(&mut self, world: UnsafeWorldCell) {
+        self.observer.update_archetype_component_access(world);
+    }
+
+    #[inline]
+    fn check_change_tick(&mut self, change_tick: Tick) {
+        self.observer.check_change_tick(change_tick);
+    }
+
+    #[inline]
+    fn get_last_run(&self) -> Tick {
+        self.observer.get_last_run()
+    }
+
+    #[inline]
+    fn set_last_run(&mut self, last_run: Tick) {
+        self.observer.set_last_run(last_run);
+    }
+
+    fn default_system_sets(&self) -> Vec<crate::schedule::InternedSystemSet> {
+        self.observer.default_system_sets()
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use crate::{