From e326bdd6ceee001b8bb43e917010379023531bd2 Mon Sep 17 00:00:00 2001 From: Daniel McNab <36049421+DJMcNab@users.noreply.github.com> Date: Mon, 9 May 2022 22:40:29 +0100 Subject: [PATCH 1/7] Internal cleanup of events --- crates/bevy_ecs/src/event.rs | 139 +++++++++++++++-------------------- 1 file changed, 59 insertions(+), 80 deletions(-) diff --git a/crates/bevy_ecs/src/event.rs b/crates/bevy_ecs/src/event.rs index 0a4a459073a5d..f1ddf2b97dfbd 100644 --- a/crates/bevy_ecs/src/event.rs +++ b/crates/bevy_ecs/src/event.rs @@ -3,6 +3,7 @@ use crate as bevy_ecs; use crate::system::{Local, Res, ResMut, SystemParam}; use bevy_utils::tracing::trace; +use std::ops::{Deref, DerefMut}; use std::{ fmt::{self}, hash::Hash, @@ -56,12 +57,6 @@ struct EventInstance { pub event: E, } -#[derive(Debug)] -enum State { - A, - B, -} - /// An event collection that represents the events that occurred within the last two /// [`Events::update`] calls. /// Events can be written to using an [`EventWriter`] @@ -135,27 +130,52 @@ enum State { /// #[derive(Debug)] pub struct Events { - events_a: Vec>, - events_b: Vec>, - a_start_event_count: usize, - b_start_event_count: usize, + events_a: EventSequence, + events_b: EventSequence, event_count: usize, - state: State, } +// Derived Default impl would incorrectly require E: Default impl Default for Events { fn default() -> Self { - Events { - a_start_event_count: 0, - b_start_event_count: 0, - event_count: 0, - events_a: Vec::new(), - events_b: Vec::new(), - state: State::A, + Self { + events_a: Default::default(), + events_b: Default::default(), + event_count: Default::default(), } } } +#[derive(Debug)] +struct EventSequence { + events: Vec>, + count: usize, +} + +// Derived Default impl would incorrectly require E: Default +impl Default for EventSequence { + fn default() -> Self { + Self { + events: Default::default(), + count: Default::default(), + } + } +} + +impl Deref for EventSequence { + type Target = Vec>; + + fn deref(&self) -> &Self::Target { + &self.events + } +} + +impl DerefMut for EventSequence { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.events + } +} + fn map_instance_event_with_id(event_instance: &EventInstance) -> (&E, EventId) { (&event_instance.event, event_instance.event_id) } @@ -167,7 +187,7 @@ fn map_instance_event(event_instance: &EventInstance) -> &E { /// Reads events of type `T` in order and tracks which events have already been read. #[derive(SystemParam)] pub struct EventReader<'w, 's, E: Event> { - last_event_count: Local<'s, (usize, PhantomData)>, + reader: Local<'s, ManualEventReader>, events: Res<'w, Events>, } @@ -252,24 +272,13 @@ fn internal_event_reader<'a, E: Event>( { // if the reader has seen some of the events in a buffer, find the proper index offset. // otherwise read all events in the buffer - let a_index = if *last_event_count > events.a_start_event_count { - *last_event_count - events.a_start_event_count - } else { - 0 - }; - let b_index = if *last_event_count > events.b_start_event_count { - *last_event_count - events.b_start_event_count - } else { - 0 - }; + let a_index = (*last_event_count).saturating_sub(events.events_a.count); + let b_index = (*last_event_count).saturating_sub(events.events_b.count); let a = events.events_a.get(a_index..).unwrap_or_default(); let b = events.events_b.get(b_index..).unwrap_or_default(); let unread_count = a.len() + b.len(); *last_event_count = events.event_count - unread_count; - let iterator = match events.state { - State::A => b.iter().chain(a.iter()), - State::B => a.iter().chain(b.iter()), - }; + let iterator = a.iter().chain(b.iter()); iterator .map(map_instance_event_with_id) .with_exact_size(unread_count) @@ -343,15 +352,15 @@ impl<'w, 's, E: Event> EventReader<'w, 's, E> { &mut self, ) -> impl DoubleEndedIterator)> + ExactSizeIterator)> { - internal_event_reader(&mut self.last_event_count.0, &self.events).map(|(event, id)| { + self.reader.iter_with_id(&self.events).map(|r @ (_, id)| { trace!("EventReader::iter() -> {}", id); - (event, id) + r }) } /// Determines the number of events available to be read from this [`EventReader`] without consuming any. pub fn len(&self) -> usize { - internal_event_reader(&mut self.last_event_count.0.clone(), &self.events).len() + self.reader.len(&self.events) } /// Determines if are any events available to be read without consuming any. @@ -372,11 +381,7 @@ impl Events { let event_instance = EventInstance { event_id, event }; - match self.state { - State::A => self.events_a.push(event_instance), - State::B => self.events_b.push(event_instance), - } - + self.events_b.push(event_instance); self.event_count += 1; } @@ -390,10 +395,7 @@ impl Events { /// Gets a new [`ManualEventReader`]. This will include all events already in the event buffers. pub fn get_reader(&self) -> ManualEventReader { - ManualEventReader { - last_event_count: 0, - _marker: PhantomData, - } + ManualEventReader::default() } /// Gets a new [`ManualEventReader`]. This will ignore all events already in the event buffers. @@ -401,25 +403,16 @@ impl Events { pub fn get_reader_current(&self) -> ManualEventReader { ManualEventReader { last_event_count: self.event_count, - _marker: PhantomData, + ..Default::default() } } /// Swaps the event buffers and clears the oldest event buffer. In general, this should be /// called once per frame/update. pub fn update(&mut self) { - match self.state { - State::A => { - self.events_b.clear(); - self.state = State::B; - self.b_start_event_count = self.event_count; - } - State::B => { - self.events_a.clear(); - self.state = State::A; - self.a_start_event_count = self.event_count; - } - } + std::mem::swap(&mut self.events_a, &mut self.events_b); + self.events_b.clear(); + self.events_b.count = self.event_count; } /// A system that calls [`Events::update`] once per frame. @@ -429,8 +422,8 @@ impl Events { #[inline] fn reset_start_event_count(&mut self) { - self.a_start_event_count = self.event_count; - self.b_start_event_count = self.event_count; + self.events_a.count = self.event_count; + self.events_b.count = self.event_count; } /// Removes all events. @@ -452,18 +445,10 @@ impl Events { self.reset_start_event_count(); let map = |i: EventInstance| i.event; - match self.state { - State::A => self - .events_b - .drain(..) - .map(map) - .chain(self.events_a.drain(..).map(map)), - State::B => self - .events_a - .drain(..) - .map(map) - .chain(self.events_b.drain(..).map(map)), - } + self.events_a + .drain(..) + .map(map) + .chain(self.events_b.drain(..).map(map)) } /// Iterates over events that happened since the last "update" call. @@ -475,10 +460,7 @@ impl Events { pub fn iter_current_update_events( &self, ) -> impl DoubleEndedIterator + ExactSizeIterator { - match self.state { - State::A => self.events_a.iter().map(map_instance_event), - State::B => self.events_b.iter().map(map_instance_event), - } + self.events_b.iter().map(map_instance_event) } } @@ -497,10 +479,7 @@ impl std::iter::Extend for Events { EventInstance { event_id, event } }); - match self.state { - State::A => self.events_a.extend(events), - State::B => self.events_b.extend(events), - } + self.events_b.extend(events); trace!( "Events::extend() -> ids: ({}..{})", From 45b720b77d9396bdca9b5fa57437f91cf6890327 Mon Sep 17 00:00:00 2001 From: Daniel McNab <36049421+DJMcNab@users.noreply.github.com> Date: Mon, 9 May 2022 22:40:33 +0100 Subject: [PATCH 2/7] Fix warning --- benches/benches/bevy_ecs/run_criteria.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/benches/benches/bevy_ecs/run_criteria.rs b/benches/benches/bevy_ecs/run_criteria.rs index 7d842d01ea959..26a9ef1984e72 100644 --- a/benches/benches/bevy_ecs/run_criteria.rs +++ b/benches/benches/bevy_ecs/run_criteria.rs @@ -2,7 +2,7 @@ use bevy_ecs::{ component::Component, prelude::{ParallelSystemDescriptorCoercion, Res, RunCriteriaDescriptorCoercion}, schedule::{ShouldRun, Stage, SystemStage}, - system::{IntoSystem, Query}, + system::Query, world::World, }; use criterion::{criterion_group, criterion_main, Criterion}; From 8ab5736b7c8a1de97b73759fa4ed841c999d8758 Mon Sep 17 00:00:00 2001 From: Daniel McNab <36049421+DJMcNab@users.noreply.github.com> Date: Tue, 10 May 2022 08:51:19 +0100 Subject: [PATCH 3/7] Clean up events --- crates/bevy_ecs/src/event.rs | 73 +++++++++++++++++++++--------------- 1 file changed, 42 insertions(+), 31 deletions(-) diff --git a/crates/bevy_ecs/src/event.rs b/crates/bevy_ecs/src/event.rs index f1ddf2b97dfbd..b3f1fc2f7a60d 100644 --- a/crates/bevy_ecs/src/event.rs +++ b/crates/bevy_ecs/src/event.rs @@ -130,7 +130,9 @@ struct EventInstance { /// #[derive(Debug)] pub struct Events { + // Holds the oldest still active events events_a: EventSequence, + // Holds the newer events events_b: EventSequence, event_count: usize, } @@ -149,7 +151,7 @@ impl Default for Events { #[derive(Debug)] struct EventSequence { events: Vec>, - count: usize, + start_event_count: usize, } // Derived Default impl would incorrectly require E: Default @@ -157,7 +159,7 @@ impl Default for EventSequence { fn default() -> Self { Self { events: Default::default(), - count: Default::default(), + start_event_count: Default::default(), } } } @@ -219,6 +221,7 @@ impl<'w, 's, E: Event> EventWriter<'w, 's, E> { } } +#[derive(Debug)] pub struct ManualEventReader { last_event_count: usize, _marker: PhantomData, @@ -240,7 +243,7 @@ impl ManualEventReader { &'a mut self, events: &'a Events, ) -> impl DoubleEndedIterator + ExactSizeIterator { - internal_event_reader(&mut self.last_event_count, events).map(|(e, _)| e) + self.iter_with_id(events).map(|(e, _)| e) } /// See [`EventReader::iter_with_id`] @@ -249,12 +252,34 @@ impl ManualEventReader { events: &'a Events, ) -> impl DoubleEndedIterator)> + ExactSizeIterator)> { - internal_event_reader(&mut self.last_event_count, events) + // if the reader has seen some of the events in a buffer, find the proper index offset. + // otherwise read all events in the buffer + let a_index = (self.last_event_count).saturating_sub(events.events_a.start_event_count); + let b_index = (self.last_event_count).saturating_sub(events.events_b.start_event_count); + let a = events.events_a.get(a_index..).unwrap_or_default(); + let b = events.events_b.get(b_index..).unwrap_or_default(); + let unread_count = a.len() + b.len(); + // Ensure `len` is implemented correctly + debug_assert_eq!(unread_count, self.len(events)); + self.last_event_count = events.event_count - unread_count; + // Iterate the oldest first, then the newer events + let iterator = a.iter().chain(b.iter()); + iterator + .map(map_instance_event_with_id) + .with_exact_size(unread_count) + .inspect(move |(_, id)| self.last_event_count = (id.id + 1).max(self.last_event_count)) } /// See [`EventReader::len`] pub fn len(&self, events: &Events) -> usize { - internal_event_reader(&mut self.last_event_count.clone(), events).len() + // The number of events in this reader is the difference between the most recent event + // and the last event seen by it. This will be at most the number of events contained + // with the events (any others have already been dropped) + // TODO: Warn when there are dropped events, or return e.g. a `Result` + events + .event_count + .saturating_sub(self.last_event_count) + .min(events.len()) } /// See [`EventReader::is_empty`] @@ -263,28 +288,6 @@ impl ManualEventReader { } } -/// Like [`iter_with_id`](EventReader::iter_with_id) except not emitting any traces for read -/// messages. -fn internal_event_reader<'a, E: Event>( - last_event_count: &'a mut usize, - events: &'a Events, -) -> impl DoubleEndedIterator)> + ExactSizeIterator)> -{ - // if the reader has seen some of the events in a buffer, find the proper index offset. - // otherwise read all events in the buffer - let a_index = (*last_event_count).saturating_sub(events.events_a.count); - let b_index = (*last_event_count).saturating_sub(events.events_b.count); - let a = events.events_a.get(a_index..).unwrap_or_default(); - let b = events.events_b.get(b_index..).unwrap_or_default(); - let unread_count = a.len() + b.len(); - *last_event_count = events.event_count - unread_count; - let iterator = a.iter().chain(b.iter()); - iterator - .map(map_instance_event_with_id) - .with_exact_size(unread_count) - .inspect(move |(_, id)| *last_event_count = (id.id + 1).max(*last_event_count)) -} - trait IteratorExt { fn with_exact_size(self, len: usize) -> ExactSize where @@ -412,7 +415,7 @@ impl Events { pub fn update(&mut self) { std::mem::swap(&mut self.events_a, &mut self.events_b); self.events_b.clear(); - self.events_b.count = self.event_count; + self.events_b.start_event_count = self.event_count; } /// A system that calls [`Events::update`] once per frame. @@ -422,8 +425,8 @@ impl Events { #[inline] fn reset_start_event_count(&mut self) { - self.events_a.count = self.event_count; - self.events_b.count = self.event_count; + self.events_a.start_event_count = self.event_count; + self.events_b.start_event_count = self.event_count; } /// Removes all events. @@ -434,10 +437,15 @@ impl Events { self.events_b.clear(); } + #[inline] + pub fn len(&self) -> usize { + self.events_a.len() + self.events_b.len() + } + /// Returns true if there are no events in this collection. #[inline] pub fn is_empty(&self) -> bool { - self.events_a.is_empty() && self.events_b.is_empty() + self.len() == 0 } /// Creates a draining iterator that removes all events. @@ -445,6 +453,7 @@ impl Events { self.reset_start_event_count(); let map = |i: EventInstance| i.event; + // Drain the oldest events first, then the newest self.events_a .drain(..) .map(map) @@ -698,6 +707,8 @@ mod tests { let mut events = Events::::default(); events.send(TestEvent { i: 0 }); let reader = events.get_reader_current(); + dbg!(&reader); + dbg!(&events); assert!(reader.is_empty(&events)); events.send(TestEvent { i: 0 }); assert_eq!(reader.len(&events), 1); From 533470bb7fc75a7595982914af09298958c9b232 Mon Sep 17 00:00:00 2001 From: Daniel McNab <36049421+DJMcNab@users.noreply.github.com> Date: Tue, 10 May 2022 08:56:21 +0100 Subject: [PATCH 4/7] Add some comments and debug assertions --- crates/bevy_ecs/src/event.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/crates/bevy_ecs/src/event.rs b/crates/bevy_ecs/src/event.rs index b3f1fc2f7a60d..3762cf15e689f 100644 --- a/crates/bevy_ecs/src/event.rs +++ b/crates/bevy_ecs/src/event.rs @@ -131,6 +131,7 @@ struct EventInstance { #[derive(Debug)] pub struct Events { // Holds the oldest still active events + // Note that a.start_event_count + a.len() should always === events_b.start_event_count events_a: EventSequence, // Holds the newer events events_b: EventSequence, @@ -416,6 +417,10 @@ impl Events { std::mem::swap(&mut self.events_a, &mut self.events_b); self.events_b.clear(); self.events_b.start_event_count = self.event_count; + debug_assert_eq!( + self.events_a.start_event_count + self.events_a.len(), + self.events_b.start_event_count + ); } /// A system that calls [`Events::update`] once per frame. From 40505512f6f027dfd127f50aa8a251fd4e18f872 Mon Sep 17 00:00:00 2001 From: Daniel McNab <36049421+DJMcNab@users.noreply.github.com> Date: Tue, 10 May 2022 10:48:15 +0100 Subject: [PATCH 5/7] Update crates/bevy_ecs/src/event.rs Co-authored-by: Federico Rinaldi --- crates/bevy_ecs/src/event.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/bevy_ecs/src/event.rs b/crates/bevy_ecs/src/event.rs index 3762cf15e689f..f87e5ff5dab6a 100644 --- a/crates/bevy_ecs/src/event.rs +++ b/crates/bevy_ecs/src/event.rs @@ -130,10 +130,10 @@ struct EventInstance { /// #[derive(Debug)] pub struct Events { - // Holds the oldest still active events - // Note that a.start_event_count + a.len() should always === events_b.start_event_count + /// Holds the oldest still active events. + /// Note that a.start_event_count + a.len() should always === events_b.start_event_count. events_a: EventSequence, - // Holds the newer events + /// Holds the newer events. events_b: EventSequence, event_count: usize, } From 179c26365e3519405517be812f9effb69d394042 Mon Sep 17 00:00:00 2001 From: Daniel McNab <36049421+DJMcNab@users.noreply.github.com> Date: Tue, 10 May 2022 14:47:34 +0100 Subject: [PATCH 6/7] Cleanup maps --- crates/bevy_ecs/src/event.rs | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/crates/bevy_ecs/src/event.rs b/crates/bevy_ecs/src/event.rs index 3762cf15e689f..36d3f81fb508f 100644 --- a/crates/bevy_ecs/src/event.rs +++ b/crates/bevy_ecs/src/event.rs @@ -179,14 +179,6 @@ impl DerefMut for EventSequence { } } -fn map_instance_event_with_id(event_instance: &EventInstance) -> (&E, EventId) { - (&event_instance.event, event_instance.event_id) -} - -fn map_instance_event(event_instance: &EventInstance) -> &E { - &event_instance.event -} - /// Reads events of type `T` in order and tracks which events have already been read. #[derive(SystemParam)] pub struct EventReader<'w, 's, E: Event> { @@ -266,7 +258,7 @@ impl ManualEventReader { // Iterate the oldest first, then the newer events let iterator = a.iter().chain(b.iter()); iterator - .map(map_instance_event_with_id) + .map(|e| (&e.event, e.event_id)) .with_exact_size(unread_count) .inspect(move |(_, id)| self.last_event_count = (id.id + 1).max(self.last_event_count)) } @@ -457,12 +449,11 @@ impl Events { pub fn drain(&mut self) -> impl Iterator + '_ { self.reset_start_event_count(); - let map = |i: EventInstance| i.event; // Drain the oldest events first, then the newest self.events_a .drain(..) - .map(map) - .chain(self.events_b.drain(..).map(map)) + .chain(self.events_b.drain(..)) + .map(|i| i.event) } /// Iterates over events that happened since the last "update" call. @@ -474,7 +465,7 @@ impl Events { pub fn iter_current_update_events( &self, ) -> impl DoubleEndedIterator + ExactSizeIterator { - self.events_b.iter().map(map_instance_event) + self.events_b.iter().map(|i| &i.event) } } From f858250b945853cfa5565e4283d127d4afa5adcf Mon Sep 17 00:00:00 2001 From: Daniel McNab <36049421+DJMcNab@users.noreply.github.com> Date: Tue, 10 May 2022 16:44:16 +0100 Subject: [PATCH 7/7] Move impl block --- crates/bevy_ecs/src/event.rs | 60 ++++++++++++++++++------------------ 1 file changed, 30 insertions(+), 30 deletions(-) diff --git a/crates/bevy_ecs/src/event.rs b/crates/bevy_ecs/src/event.rs index 1d69a5f43751e..00e0c6b74607e 100644 --- a/crates/bevy_ecs/src/event.rs +++ b/crates/bevy_ecs/src/event.rs @@ -186,6 +186,36 @@ pub struct EventReader<'w, 's, E: Event> { events: Res<'w, Events>, } +impl<'w, 's, E: Event> EventReader<'w, 's, E> { + /// Iterates over the events this [`EventReader`] has not seen yet. This updates the + /// [`EventReader`]'s event counter, which means subsequent event reads will not include events + /// that happened before now. + pub fn iter(&mut self) -> impl DoubleEndedIterator + ExactSizeIterator { + self.iter_with_id().map(|(event, _id)| event) + } + + /// Like [`iter`](Self::iter), except also returning the [`EventId`] of the events. + pub fn iter_with_id( + &mut self, + ) -> impl DoubleEndedIterator)> + ExactSizeIterator)> + { + self.reader.iter_with_id(&self.events).map(|r @ (_, id)| { + trace!("EventReader::iter() -> {}", id); + r + }) + } + + /// Determines the number of events available to be read from this [`EventReader`] without consuming any. + pub fn len(&self) -> usize { + self.reader.len(&self.events) + } + + /// Determines if are any events available to be read without consuming any. + pub fn is_empty(&self) -> bool { + self.len() == 0 + } +} + /// Sends events of type `T`. #[derive(SystemParam)] pub struct EventWriter<'w, 's, E: Event> { @@ -335,36 +365,6 @@ impl ExactSizeIterator for ExactSize { } } -impl<'w, 's, E: Event> EventReader<'w, 's, E> { - /// Iterates over the events this [`EventReader`] has not seen yet. This updates the - /// [`EventReader`]'s event counter, which means subsequent event reads will not include events - /// that happened before now. - pub fn iter(&mut self) -> impl DoubleEndedIterator + ExactSizeIterator { - self.iter_with_id().map(|(event, _id)| event) - } - - /// Like [`iter`](Self::iter), except also returning the [`EventId`] of the events. - pub fn iter_with_id( - &mut self, - ) -> impl DoubleEndedIterator)> + ExactSizeIterator)> - { - self.reader.iter_with_id(&self.events).map(|r @ (_, id)| { - trace!("EventReader::iter() -> {}", id); - r - }) - } - - /// Determines the number of events available to be read from this [`EventReader`] without consuming any. - pub fn len(&self) -> usize { - self.reader.len(&self.events) - } - - /// Determines if are any events available to be read without consuming any. - pub fn is_empty(&self) -> bool { - self.len() == 0 - } -} - impl Events { /// "Sends" an `event` by writing it to the current event buffer. [`EventReader`]s can then read /// the event.