[Merged by Bors] - Add a clear() method to the EventReader that consumes the iterator #4693

IceSentry · 2022-05-07T20:55:17Z

Objective

It's pretty common to want to check if an EventReader has received one or multiple events while also needing to consume the iterator to "clear" the EventReader.
The current approach is to do something like events.iter().count() > 0 or events.iter().last().is_some(). It's not immediately obvious that the purpose of that is to consume the events and check if there were any events. My solution doesn't really solve that part, but it encapsulates the pattern.

Solution

Add a .clear() method that consumes the iterator.
- It takes the EventReader by value to make sure it isn't used again after it has been called.

Migration Guide

Not a breaking change, but if you ever found yourself in a situation where you needed to consume the EventReader and check if there was any events you can now use

fn system(events: EventReader<MyEvent>) {
	if !events.is_empty {
		events.clear();
		// Process the fact that one or more event was received
	}
}

DJMcNab

Code looks good

We should link to any from is_empty. I'd say the docs for is_empty seem a bit backwarrds too, since e.any()===!e.is_empty(), but they both have the same docs.

I'm not sure I like how hazardous this is, but this makes it better

IceSentry · 2022-05-07T21:07:04Z

Yeah, that's why I would like a better name, but I couldn't find anything. Or at least, anything that isn't overly verbose.

I thought of something like is_empty_consume() but it feels a bit weird.

I added a bit more docs to clarify the differences

Nilirad · 2022-05-08T06:27:38Z

It's hard to find a non verbose name since we want to get two outcomes from the function:

yields a bool whether or not the event queue contains any event.
clears the event queue.

As there is no word that encompasses those two actions, I'm OK with making the function name verbose.

vacuus · 2022-05-09T02:02:36Z

I'm not entirely satisfied with this name, but perhaps something like query_new_unread? My main concern is that it overlaps with how "query" is used in Bevy.
/bikeshed

Nilirad

Honestly, any doesn't give the idea of what the function does.

I think we can do of the following things:

keep using .iter().last().is_some() as it is the most idiomatic method and is quite understandable, especially if one knows that last consumes the iterator.
renaming any to something else, like consume_and_report, which is not 100% clear, but it's something that can be searched up and invites the user to read the docs, while any is obscure and completely undiscoverable.

crates/bevy_ecs/src/event.rs

tim-blackbird · 2022-05-09T12:42:52Z

It seems difficult to convey the semantics of .iter().last().is_some() in a single name, even a verbose one.
How about adding a clear() method to EventReader that consumes the iterator(i.e. .iter().last()), and using it together with is_empty().

system_a should be easy to read, and familiar with anyone that has used a Vec.

fn system_a(mut events: EventReader<Event>) {
    if !events.is_empty() {
        events.clear();
        // Do the thing!
    }
}

system_b might be easy to read if you're familiar with this pattern and it saves one line, but is not any less verbose than system_a.

fn system_b(mut events: EventReader<Event>) {
    if events.iter().last().is_some() {
        // Do the thing!
    }
}

alice-i-cecile · 2022-05-09T13:14:43Z

I really like @devil-ira's suggestion here. I like that it's so much more explicit, and the boilerplate isn't too bad.

DJMcNab · 2022-05-09T13:24:58Z

Should EventReader panic (on drop?) if it still has events when the system ends?

That is, recognise that it's a logic error to not clear it? Note that people could still use ManualEventReader (which EventReader really should be created in terms - that's on the todo list) for a version which doesn't have that behaviour

alice-i-cecile · 2022-05-09T13:45:09Z

Should EventReader panic (on drop?) if it still has events when the system ends?

Hmm. I like this idea: it's a good additional layer of safety, and I agree with you that this is almost always a logic error. That said, I'm nervous about the mechanism: the addition of panics in edge cases is not fun.

What about a debug assert on drop?

crates/bevy_ecs/src/event.rs

Nilirad · 2022-05-09T14:51:47Z

I wonder if we could also change the breakout example to use the new approach.

IceSentry · 2022-05-09T17:21:46Z

I decided to use @devil-ira suggestion of using a clear method with no returns. It's a bit verbose but it makes the pattern a lot clearer.

I also used it in the breakout example as @Nilirad suggested.

I also added a simple test.

Is it possible to reset the reviews? I wouldn't consider them valid after this change.

crates/bevy_ecs/src/event.rs

examples/games/breakout.rs

alice-i-cecile

I like this approach much better.

IMO this is ready to merge once:

Nits are addressed.
A doc example is added.
PR title and description are updated.

Nilirad · 2022-05-09T17:58:58Z

The API doc example can be easily added by copying it from the test.

IceSentry · 2022-05-09T18:01:01Z

Should I just use the test directly in the doc example and remove the test?

Nilirad · 2022-05-09T18:39:33Z

I would prefer the example being under a level 2 “Example” header. The example code should also be inside a common system function and boilerplate lines (arguably everything except the system function) should be hidden with #. The function can be tested as a system with assert_is_system.

SUPERCILEX · 2022-05-09T20:47:14Z

Can we have clear return true if there was anything to clear? I've been doing something similar:

    if level_unloaded.iter().count() > 0 {
        **selected_piece = None;
    }

It might be a little weird, but it's not too different from HashSet::remove for example:

    if level_unloaded.clear() {
        **selected_piece = None;
    }

Co-authored-by: Federico Rinaldi <[email protected]>

alice-i-cecile · 2022-05-12T22:14:11Z

bors r+

…4693) # Objective - It's pretty common to want to check if an EventReader has received one or multiple events while also needing to consume the iterator to "clear" the EventReader. - The current approach is to do something like `events.iter().count() > 0` or `events.iter().last().is_some()`. It's not immediately obvious that the purpose of that is to consume the events and check if there were any events. My solution doesn't really solve that part, but it encapsulates the pattern. ## Solution - Add a `.clear()` method that consumes the iterator. - It takes the EventReader by value to make sure it isn't used again after it has been called. --- ## Migration Guide Not a breaking change, but if you ever found yourself in a situation where you needed to consume the EventReader and check if there was any events you can now use ```rust fn system(events: EventReader<MyEvent>) { if !events.is_empty { events.clear(); // Process the fact that one or more event was received } } ``` Co-authored-by: Charles <[email protected]>

bors · 2022-05-12T22:20:37Z

Build failed:

run-examples

IceSentry · 2022-05-13T00:56:48Z

bors retry

…4693) # Objective - It's pretty common to want to check if an EventReader has received one or multiple events while also needing to consume the iterator to "clear" the EventReader. - The current approach is to do something like `events.iter().count() > 0` or `events.iter().last().is_some()`. It's not immediately obvious that the purpose of that is to consume the events and check if there were any events. My solution doesn't really solve that part, but it encapsulates the pattern. ## Solution - Add a `.clear()` method that consumes the iterator. - It takes the EventReader by value to make sure it isn't used again after it has been called. --- ## Migration Guide Not a breaking change, but if you ever found yourself in a situation where you needed to consume the EventReader and check if there was any events you can now use ```rust fn system(events: EventReader<MyEvent>) { if !events.is_empty { events.clear(); // Process the fact that one or more event was received } } ``` Co-authored-by: Charles <[email protected]>

bors · 2022-05-13T01:13:24Z

Pull request successfully merged into main.

Build succeeded:

…evyengine#4693) # Objective - It's pretty common to want to check if an EventReader has received one or multiple events while also needing to consume the iterator to "clear" the EventReader. - The current approach is to do something like `events.iter().count() > 0` or `events.iter().last().is_some()`. It's not immediately obvious that the purpose of that is to consume the events and check if there were any events. My solution doesn't really solve that part, but it encapsulates the pattern. ## Solution - Add a `.clear()` method that consumes the iterator. - It takes the EventReader by value to make sure it isn't used again after it has been called. --- ## Migration Guide Not a breaking change, but if you ever found yourself in a situation where you needed to consume the EventReader and check if there was any events you can now use ```rust fn system(events: EventReader<MyEvent>) { if !events.is_empty { events.clear(); // Process the fact that one or more event was received } } ``` Co-authored-by: Charles <[email protected]>

IceSentry added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use labels May 7, 2022

DJMcNab approved these changes May 7, 2022

View reviewed changes

mockersf approved these changes May 8, 2022

View reviewed changes

Nilirad suggested changes May 9, 2022

View reviewed changes

crates/bevy_ecs/src/event.rs Outdated Show resolved Hide resolved

crates/bevy_ecs/src/event.rs Outdated Show resolved Hide resolved

alice-i-cecile added X-Controversial There is active debate or serious implications around merging this PR and removed C-Feature A new feature, making something new possible X-Controversial There is active debate or serious implications around merging this PR labels May 9, 2022

bjorn3 reviewed May 9, 2022

View reviewed changes

crates/bevy_ecs/src/event.rs Outdated Show resolved Hide resolved

IceSentry force-pushed the event-reader-consume-check branch from c7545e6 to 70a7467 Compare May 9, 2022 17:20

alice-i-cecile reviewed May 9, 2022

View reviewed changes

crates/bevy_ecs/src/event.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed May 9, 2022

View reviewed changes

crates/bevy_ecs/src/event.rs Outdated Show resolved Hide resolved

Nilirad suggested changes May 9, 2022

View reviewed changes

crates/bevy_ecs/src/event.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed May 9, 2022

View reviewed changes

examples/games/breakout.rs Show resolved Hide resolved

alice-i-cecile reviewed May 9, 2022

View reviewed changes

IceSentry changed the title ~~Add a check to the EventReader that consumes the iterator~~ Add a clear() method to the EventReader that consumes the iterator May 9, 2022

IceSentry and others added 15 commits May 12, 2022 13:34

add any() check to event reader that consumes the iterator

87afcbf

update doc

cbbdd2f

add warning to is_empty

4a69efd

rephrase is_empty docs

39fd84b

use clear instead of any

228a4a8

use clear in breakout example

87262e5

fix clippy

de70598

actually fix clippy

eba3325

improve docs

f52c247

fix doc tests

84e1f64

clean up doc example

98c7986

align code correctly

67ea749

make example a bit more concrete

f3e0957

Update crates/bevy_ecs/src/event.rs

3df8d70

Co-authored-by: Federico Rinaldi <[email protected]>

fix conflict

f399c4a

IceSentry force-pushed the event-reader-consume-check branch from 92907db to f399c4a Compare May 12, 2022 17:38

alice-i-cecile approved these changes May 12, 2022

View reviewed changes

Nilirad approved these changes May 12, 2022

View reviewed changes

bors bot changed the title ~~Add a clear() method to the EventReader that consumes the iterator~~ [Merged by Bors] - Add a clear() method to the EventReader that consumes the iterator May 13, 2022

bors bot closed this May 13, 2022

IceSentry deleted the event-reader-consume-check branch May 13, 2022 05:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Add a clear() method to the EventReader that consumes the iterator #4693

[Merged by Bors] - Add a clear() method to the EventReader that consumes the iterator #4693

IceSentry commented May 7, 2022 •

edited

Loading

DJMcNab left a comment •

edited

Loading

IceSentry commented May 7, 2022

Nilirad commented May 8, 2022

vacuus commented May 9, 2022

Nilirad left a comment

tim-blackbird commented May 9, 2022 •

edited

Loading

alice-i-cecile commented May 9, 2022

DJMcNab commented May 9, 2022 •

edited

Loading

alice-i-cecile commented May 9, 2022

Nilirad commented May 9, 2022

IceSentry commented May 9, 2022 •

edited

Loading

alice-i-cecile left a comment

Nilirad commented May 9, 2022 •

edited

Loading

IceSentry commented May 9, 2022 •

edited

Loading

Nilirad commented May 9, 2022

SUPERCILEX commented May 9, 2022

alice-i-cecile commented May 12, 2022

bors bot commented May 12, 2022

IceSentry commented May 13, 2022

bors bot commented May 13, 2022

[Merged by Bors] - Add a clear() method to the EventReader that consumes the iterator #4693

[Merged by Bors] - Add a clear() method to the EventReader that consumes the iterator #4693

Conversation

IceSentry commented May 7, 2022 • edited Loading

Objective

Solution

Migration Guide

DJMcNab left a comment • edited Loading

Choose a reason for hiding this comment

IceSentry commented May 7, 2022

Nilirad commented May 8, 2022

vacuus commented May 9, 2022

Nilirad left a comment

Choose a reason for hiding this comment

tim-blackbird commented May 9, 2022 • edited Loading

alice-i-cecile commented May 9, 2022

DJMcNab commented May 9, 2022 • edited Loading

alice-i-cecile commented May 9, 2022

Nilirad commented May 9, 2022

IceSentry commented May 9, 2022 • edited Loading

alice-i-cecile left a comment

Choose a reason for hiding this comment

Nilirad commented May 9, 2022 • edited Loading

IceSentry commented May 9, 2022 • edited Loading

Nilirad commented May 9, 2022

SUPERCILEX commented May 9, 2022

alice-i-cecile commented May 12, 2022

bors bot commented May 12, 2022

IceSentry commented May 13, 2022

bors bot commented May 13, 2022

IceSentry commented May 7, 2022 •

edited

Loading

DJMcNab left a comment •

edited

Loading

tim-blackbird commented May 9, 2022 •

edited

Loading

DJMcNab commented May 9, 2022 •

edited

Loading

IceSentry commented May 9, 2022 •

edited

Loading

Nilirad commented May 9, 2022 •

edited

Loading

IceSentry commented May 9, 2022 •

edited

Loading