Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added peeking to EventReader #13809

Open
wants to merge 36 commits into
base: main
Choose a base branch
from
Open

Added peeking to EventReader #13809

wants to merge 36 commits into from
+1,943 −475

Conversation

BobG1983
Copy link
Contributor

Added peekability to EventReader for when you want to view an event but not mark it as read

Objective

  • Allow the ability to view an event without marking it as read

Solution

  • Copied the existing iterators and implemented them for peeking, ie. they no longer move the "last_read" onwards.
  • I also took this opportunity to improve the documentation on EventReader given I was adding explanations for peeking.

Testing

  • Wrote a number of tests to confirm that the new peek iterators work as expected. Renamed a bunch of the existing tests to specify they test read and wrote peek equivalents. Also wrote tests for combination of reading and peeking to make sure behavior was as expected.

@BobG1983
Copy link
Contributor Author

No idea why the MacOS build fails given I didn't touch those files at all?

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 11, 2024
@BobG1983
Copy link
Contributor Author

The issue is that par_read doesn't mark events as seen like read which I assumed it did, I ended up rewriting the tests to validate that behavior. Which par_read then failed.

It looks like this has always been the behavior of par_read

@BobG1983
Copy link
Contributor Author

BobG1983 commented Jun 13, 2024 •
edited
Loading

Added the fix from #13836 to this pull request too given I'm going to squash the files and tests associated with it.

@BobG1983 BobG1983 mentioned this pull request Jun 13, 2024
Merged
mnmaita
mnmaita reviewed Jun 14, 2024
View reviewed changes
Copy link
Member

@mnmaita mnmaita left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool feature and great additions to documentation! Leaving a few nits and suggestions in doc comments.

crates/bevy_ecs/src/event/reader.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/event/reader.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/event/peek_iterators.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/event/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/event/read_iterators.rs Outdated Show resolved Hide resolved
@BD103 BD103 added the S-Blocked This cannot move forward until something else changes label Jun 15, 2024
@BD103
Copy link
Member

BD103 commented Jun 15, 2024

I'm marking this as blocked by #13836. Once that gets merged and this gets enough reviews, this can be merged :)

kristoff3r
kristoff3r suggested changes Jun 17, 2024
View reviewed changes
crates/bevy_ecs/src/event/reader.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/event/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/event/reader.rs Outdated Show resolved Hide resolved
@BobG1983
Copy link
Contributor Author

@BD103 I wouldn’t say this is blocked. In fact. This has the fix in it already so if this is merged the other PR doesn’t need to be.

@BobG1983
Copy link
Contributor Author

Failed because of a formatting requirement that I can’t really fix on my phone. Will take a look tomorrow.

@BD103
Copy link
Member

BD103 commented Jun 18, 2024

@BD103 I wouldn’t say this is blocked. In fact. This has the fix in it already so if this is merged the other PR doesn’t need to be.

I would prefer we split it up into two different PRs, but I'll defer to maintainers on this. :)

@BobG1983
Copy link
Contributor Author

Only reason I included it in here is because I squash the existing iterator file anyway.

BobG1983 and others added 9 commits June 18, 2024 09:45
@BobG1983
Tidied up multi_threaded feature support
e1d327e
@BobG1983
Fixed formatting of use statements
6c265fd
@BobG1983
Fixed cfg for ParIter
f927c43
@BobG1983 @atornity
Apply suggestions from code review
9061ce3 
Co-authored-by: poopy <gonesbird@gmail.com>
@BobG1983
Updating comment in example.
6802742
@BobG1983
Created EventCursor, replacing ManualEventReader, marked ManualEventR…
de0f0a8 
…eader as deprecated. Updated all uses of ManualEventReader to EventCursor. Deprecated Events::get_reader and replaced it with Events::get_cursor everywhere. Also took the opportunity to tidy up the events tests. Removed unrequried tests of Mutator and Reader and replaced them with tests of EventCursor, also removed unrequried E event and replaced with TestEvent in all tests. Sorted tests so they're easier to follow
@BobG1983
Updated engine and examples to use new EventCursor
becd0c3
@BobG1983
Fixed typo in test
29c4174
@BobG1983
Fixed more typos in test
3ace983
@BobG1983
Copy link
Contributor Author

This branch is now stacked on top of #13818

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mnmaita mnmaita mnmaita left review comments

@kristoff3r kristoff3r kristoff3r approved these changes

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Usability A simple quality-of-life change that makes Bevy easier to use S-Blocked This cannot move forward until something else changes S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@BobG1983 @BD103 @kristoff3r @mnmaita @alice-i-cecile