Ensure that events are updated even when using a bare-bones Bevy App (b…

…evyengine#13808)

# Objective

As discovered in
Leafwing-Studios/leafwing-input-manager#538,
there appears to be some real weirdness going on in how event updates
are processed between Bevy 0.13 and Bevy 0.14.

To identify the cause and prevent regression, I've added tests to
validate the intended behavior.
My initial suspicion was that this would be fixed by
bevyengine#13762, but that doesn't seem to
be the case.

Instead, events appear to never be updated at all when using `bevy_app`
by itself. This is part of the problem resolved by
bevyengine#11528, and introduced by
bevyengine#10077.

After some investigation, it appears that `signal_event_update_system`
is never added using a bare-bones `App`, and so event updates are always
skipped.

This can be worked around by adding your own copy to a
later-in-the-frame schedule, but that's not a very good fix.

## Solution

Ensure that if we're not using a `FixedUpdate` schedule, events are
always updated every frame.

To do this, I've modified the logic of `event_update_condition` and
`event_update_system` to clearly and correctly differentiate between the
two cases: where we're waiting for a "you should update now" signal and
where we simply don't care.

To encode this, I've added the `ShouldUpdateEvents` enum, replacing a
simple `bool` in `EventRegistry`'s `needs_update` field.

Now, both tests pass as expected, without having to manually add a
system!

## Testing

I've written two parallel unit tests to cover the intended behavior:

1. Test that `iter_current_update_events` works as expected in
`bevy_ecs`.
2. Test that `iter_current_update_events` works as expected in
`bevy_app`

I've also added a test to verify that event updating works correctly in
the presence of a fixed main schedule, and a second test to verify that
fixed updating works at all to help future authors narrow down failures.

## Outstanding

- [x] figure out why the `bevy_app` version of this test fails but the
`bevy_ecs` version does not
- [x] figure out why `EventRegistry::run_updates` isn't working properly
- [x] figure out why `EventRegistry::run_updates` is never getting
called
- [x] figure out why `event_update_condition` is always returning false
- [x] figure out why `EventRegistry::needs_update` is always false
- [x] verify that the problem is a missing `signal_events_update_system`

---------

Co-authored-by: Mike <[email protected]>

crates/bevy_app/src/app.rs

@@ -925,7 +925,7 @@ mod tests {
         change_detection::{DetectChanges, ResMut},
         component::Component,
         entity::Entity,
-        event::EventWriter,
+        event::{Event, EventWriter, Events},
         query::With,
         removal_detection::RemovedComponents,
         schedule::{IntoSystemConfigs, ScheduleLabel},
@@ -1274,4 +1274,41 @@ mod tests {
             .init_non_send_resource::<NonSendTestResource>()
             .init_resource::<TestResource>();
     }
+
+    #[test]
+    fn events_should_be_updated_once_per_update() {
+        #[derive(Event, Clone)]
+        struct TestEvent;
+
+        let mut app = App::new();
+        app.add_event::<TestEvent>();
+
+        // Starts empty
+        let test_events = app.world().resource::<Events<TestEvent>>();
+        assert_eq!(test_events.len(), 0);
+        assert_eq!(test_events.iter_current_update_events().count(), 0);
+        app.update();
+
+        // Sending one event
+        app.world_mut().send_event(TestEvent);
+
+        let test_events = app.world().resource::<Events<TestEvent>>();
+        assert_eq!(test_events.len(), 1);
+        assert_eq!(test_events.iter_current_update_events().count(), 1);
+        app.update();
+
+        // Sending two events on the next frame
+        app.world_mut().send_event(TestEvent);
+        app.world_mut().send_event(TestEvent);
+
+        let test_events = app.world().resource::<Events<TestEvent>>();
+        assert_eq!(test_events.len(), 3); // Events are double-buffered, so we see 1 + 2 = 3
+        assert_eq!(test_events.iter_current_update_events().count(), 2);
+        app.update();
+
+        // Sending zero events
+        let test_events = app.world().resource::<Events<TestEvent>>();
+        assert_eq!(test_events.len(), 2); // Events are double-buffered, so we see 2 + 0 = 2
+        assert_eq!(test_events.iter_current_update_events().count(), 0);
+    }
 }

crates/bevy_ecs/src/event/collections.rs

@@ -366,3 +366,41 @@ impl<E: Event> ExactSizeIterator for SendBatchIds<E> {
         self.event_count.saturating_sub(self.last_count)
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use crate::{self as bevy_ecs, event::Events};
+    use bevy_ecs_macros::Event;
+
+    #[test]
+    fn iter_current_update_events_iterates_over_current_events() {
+        #[derive(Event, Clone)]
+        struct TestEvent;
+
+        let mut test_events = Events::<TestEvent>::default();
+
+        // Starting empty
+        assert_eq!(test_events.len(), 0);
+        assert_eq!(test_events.iter_current_update_events().count(), 0);
+        test_events.update();
+
+        // Sending one event
+        test_events.send(TestEvent);
+
+        assert_eq!(test_events.len(), 1);
+        assert_eq!(test_events.iter_current_update_events().count(), 1);
+        test_events.update();
+
+        // Sending two events on the next frame
+        test_events.send(TestEvent);
+        test_events.send(TestEvent);
+
+        assert_eq!(test_events.len(), 3); // Events are double-buffered, so we see 1 + 2 = 3
+        assert_eq!(test_events.iter_current_update_events().count(), 2);
+        test_events.update();
+
+        // Sending zero events
+        assert_eq!(test_events.len(), 2); // Events are double-buffered, so we see 2 + 0 = 2
+        assert_eq!(test_events.iter_current_update_events().count(), 0);
+    }
+}

crates/bevy_ecs/src/event/mod.rs

@@ -13,7 +13,7 @@ pub use bevy_ecs_macros::Event;
 pub use collections::{Events, SendBatchIds};
 pub use iterators::{EventIterator, EventIteratorWithId, EventParIter};
 pub use reader::{EventReader, ManualEventReader};
-pub use registry::EventRegistry;
+pub use registry::{EventRegistry, ShouldUpdateEvents};
 pub use update::{
     event_update_condition, event_update_system, signal_event_update_system, EventUpdates,
 };

crates/bevy_ecs/src/event/registry.rs

@@ -17,14 +17,29 @@ struct RegisteredEvent {
     update: unsafe fn(MutUntyped),
 }
 
-/// A registry of all of the [`Events`] in the [`World`], used by [`event_update_system`]
+/// A registry of all of the [`Events`] in the [`World`], used by [`event_update_system`](crate::event::update::event_update_system)
 /// to update all of the events.
 #[derive(Resource, Default)]
 pub struct EventRegistry {
-    pub(super) needs_update: bool,
+    /// Should the events be updated?
+    ///
+    /// This field is generally automatically updated by the [`signal_event_update_system`](crate::event::update::signal_event_update_system).
+    pub should_update: ShouldUpdateEvents,
     event_updates: Vec<RegisteredEvent>,
 }
 
+/// Controls whether or not the events in an [`EventRegistry`] should be updated.
+#[derive(Default, Debug, Clone, Copy, PartialEq, Eq)]
+pub enum ShouldUpdateEvents {
+    /// Without any fixed timestep, events should always be updated each frame.
+    #[default]
+    Always,
+    /// We need to wait until at least one pass of the fixed update schedules to update the events.
+    Waiting,
+    /// At least one pass of the fixed update schedules has occurred, and the events are ready to be updated.
+    Ready,
+}
+
 impl EventRegistry {
     /// Registers an event type to be updated.
     pub fn register_event<T: Event>(world: &mut World) {

crates/bevy_ecs/src/event/update.rs

@@ -10,14 +10,19 @@ use bevy_ecs_macros::SystemSet;
 #[cfg(feature = "bevy_reflect")]
 use std::hash::Hash;
 
+use super::registry::ShouldUpdateEvents;
+
 #[doc(hidden)]
 #[derive(SystemSet, Clone, Debug, PartialEq, Eq, Hash)]
 pub struct EventUpdates;
 
 /// Signals the [`event_update_system`] to run after `FixedUpdate` systems.
+///
+/// This will change the behavior of the [`EventRegistry`] to only run after a fixed update cycle has passed.
+/// Normally, this will simply run every frame.
 pub fn signal_event_update_system(signal: Option<ResMut<EventRegistry>>) {
     if let Some(mut registry) = signal {
-        registry.needs_update = true;
+        registry.should_update = ShouldUpdateEvents::Ready;
     }
 }
 
@@ -26,16 +31,32 @@ pub fn event_update_system(world: &mut World, mut last_change_tick: Local<Tick>)
     if world.contains_resource::<EventRegistry>() {
         world.resource_scope(|world, mut registry: Mut<EventRegistry>| {
             registry.run_updates(world, *last_change_tick);
-            // Disable the system until signal_event_update_system runs again.
-            registry.needs_update = false;
+
+            registry.should_update = match registry.should_update {
+                // If we're always updating, keep doing so.
+                ShouldUpdateEvents::Always => ShouldUpdateEvents::Always,
+                // Disable the system until signal_event_update_system runs again.
+                ShouldUpdateEvents::Waiting | ShouldUpdateEvents::Ready => {
+                    ShouldUpdateEvents::Waiting
+                }
+            };
         });
     }
     *last_change_tick = world.change_tick();
 }
 
 /// A run condition for [`event_update_system`].
-pub fn event_update_condition(signal: Option<Res<EventRegistry>>) -> bool {
-    // If we haven't got a signal to update the events, but we *could* get such a signal
-    // return early and update the events later.
-    signal.map_or(false, |signal| signal.needs_update)
+///
+/// If [`signal_event_update_system`] has been run at least once,
+/// we will wait for it to be run again before updating the events.
+///
+/// Otherwise, we will always update the events.
+pub fn event_update_condition(maybe_signal: Option<Res<EventRegistry>>) -> bool {
+    match maybe_signal {
+        Some(signal) => match signal.should_update {
+            ShouldUpdateEvents::Always | ShouldUpdateEvents::Ready => true,
+            ShouldUpdateEvents::Waiting => false,
+        },
+        None => true,
+    }
 }