Skip to content

Commit

Permalink
Remove broken DoubleEndedIterator impls on event iterators (bevyeng…
Browse files Browse the repository at this point in the history
…ine#7469)

The `DoubleEndedIterator` impls produce incorrect results on subsequent calls to `iter()` if the iterator is only partially consumed.

The following code shows what happens
```rust

fn next_back_is_bad() {
    let mut events = Events::<TestEvent>::default();
    events.send(TestEvent { i: 0 });
    events.send(TestEvent { i: 1 });
    events.send(TestEvent { i: 2 });
    let mut reader = events.get_reader();
    let mut iter = reader.iter(&events);
    assert_eq!(iter.next_back(), Some(&TestEvent { i: 2 }));
    assert_eq!(iter.next(), Some(&TestEvent { i: 0 }));
    
    let mut iter = reader.iter(&events);
    // `i: 2` event is returned twice! The `i: 1` event is missed. 
    assert_eq!(iter.next(), Some(&TestEvent { i: 2 }));
    assert_eq!(iter.next(), None);
}
```

I don't think this can be fixed without adding some very convoluted bookkeeping.

## Migration Guide
`ManualEventIterator` and `ManualEventIteratorWithId` are no longer `DoubleEndedIterator`s.



Co-authored-by: devil-ira <[email protected]>
  • Loading branch information
tim-blackbird and tim-blackbird committed Feb 5, 2023
1 parent 386763e commit 32023a5
Show file tree
Hide file tree
Showing 3 changed files with 8 additions and 33 deletions.
31 changes: 4 additions & 27 deletions crates/bevy_ecs/src/event.rs
Original file line number Diff line number Diff line change
Expand Up @@ -390,12 +390,6 @@ impl<'a, E: Event> ExactSizeIterator for ManualEventIterator<'a, E> {
}
}

impl<'a, E: Event> DoubleEndedIterator for ManualEventIterator<'a, E> {
fn next_back(&mut self) -> Option<Self::Item> {
self.iter.next_back().map(|(event, _)| event)
}
}

#[derive(Debug)]
pub struct ManualEventIteratorWithId<'a, E: Event> {
reader: &'a mut ManualEventReader<E>,
Expand Down Expand Up @@ -457,23 +451,6 @@ impl<'a, E: Event> Iterator for ManualEventIteratorWithId<'a, E> {
}
}

impl<'a, E: Event> DoubleEndedIterator for ManualEventIteratorWithId<'a, E> {
fn next_back(&mut self) -> Option<Self::Item> {
match self
.chain
.next_back()
.map(|instance| (&instance.event, instance.event_id))
{
Some(item) => {
event_trace(item.1);
self.unread -= 1;
Some(item)
}
None => None,
}
}
}

impl<'a, E: Event> ExactSizeIterator for ManualEventIteratorWithId<'a, E> {
fn len(&self) -> usize {
self.unread
Expand Down Expand Up @@ -577,9 +554,7 @@ impl<E: Event> Events<E> {
/// between the last `update()` call and your call to `iter_current_update_events`.
/// If events happen outside that window, they will not be handled. For example, any events that
/// happen after this call and before the next `update()` call will be dropped.
pub fn iter_current_update_events(
&self,
) -> impl DoubleEndedIterator<Item = &E> + ExactSizeIterator<Item = &E> {
pub fn iter_current_update_events(&self) -> impl ExactSizeIterator<Item = &E> {
self.events_b.iter().map(|i| &i.event)
}

Expand Down Expand Up @@ -837,8 +812,10 @@ mod tests {
assert_eq!(iter.len(), 3);
iter.next();
assert_eq!(iter.len(), 2);
iter.next_back();
iter.next();
assert_eq!(iter.len(), 1);
iter.next();
assert_eq!(iter.len(), 0);
}

#[test]
Expand Down
7 changes: 2 additions & 5 deletions crates/bevy_ecs/src/world/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -772,7 +772,7 @@ impl World {

/// Returns an iterator of entities that had components of type `T` removed
/// since the last call to [`World::clear_trackers`].
pub fn removed<T: Component>(&self) -> impl DoubleEndedIterator<Item = Entity> + '_ {
pub fn removed<T: Component>(&self) -> impl Iterator<Item = Entity> + '_ {
self.components
.get_id(TypeId::of::<T>())
.map(|component_id| self.removed_with_id(component_id))
Expand All @@ -782,10 +782,7 @@ impl World {

/// Returns an iterator of entities that had components with the given `component_id` removed
/// since the last call to [`World::clear_trackers`].
pub fn removed_with_id(
&self,
component_id: ComponentId,
) -> impl DoubleEndedIterator<Item = Entity> + '_ {
pub fn removed_with_id(&self, component_id: ComponentId) -> impl Iterator<Item = Entity> + '_ {
self.removed_components
.get(component_id)
.map(|removed| removed.iter_current_update_events().cloned())
Expand Down
3 changes: 2 additions & 1 deletion crates/bevy_ui/src/flex/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,8 @@ pub fn flex_node_system(
}
}

if scale_factor_events.iter().next_back().is_some() || ui_scale.is_changed() {
if !scale_factor_events.is_empty() || ui_scale.is_changed() {
scale_factor_events.clear();
update_changed(&mut flex_surface, scale_factor, full_node_query);
} else {
update_changed(&mut flex_surface, scale_factor, node_query);
Expand Down

0 comments on commit 32023a5

Please sign in to comment.