Skip to content

Mark events as read during EventReader::par_read#13836

Merged
alice-i-cecile merged 8 commits intobevyengine:mainfrom
BobG1983:par_read_fix_13821
Jun 25, 2024
Merged

Mark events as read during EventReader::par_read#13836
alice-i-cecile merged 8 commits intobevyengine:mainfrom
BobG1983:par_read_fix_13821

Conversation

@BobG1983
Copy link
Contributor

Objective

Solution

  • Rewrote the test to ensure that it actually tests the functionality correctly. Then made the par_read function correctly change the values of self.reader.last_event_count.

Testing

  • Rewrote the test for par_read to run the system schedule twice, checking the output each time

@alice-i-cecile alice-i-cecile added this to the 0.14 milestone Jun 13, 2024
@janhohenheim janhohenheim added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled A-ECS Entities, components, systems, and events S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Needs-Triage This issue needs to be labelled labels Jun 14, 2024
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.

Nice catch! :)

Co-authored-by: Martín Maita <47983254+mnmaita@users.noreply.github.com>
Copy link
Contributor

@kristoff3r kristoff3r left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@komadori komadori left a comment

Choose a reason for hiding this comment

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

LGTM.

@BD103 BD103 added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 19, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 25, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 25, 2024
@alice-i-cecile
Copy link
Member

@BobG1983 I'd like to include this in 0.14 due to the severity of the bug, but CI is failing in a way that seems real. See https://github.com/bevyengine/bevy/actions/runs/9663589923/job/26656115871#step:5:208 for the error.

@BobG1983
Copy link
Contributor Author

So, I think this is a seperate bug. This should have failed previously, it's caused by the pariter iterator being included without multi_threading. Previously it did nothing (par_read made an iterator, but the iterators body was blank). This was fine until I added mut to the function signature, now its making clippy complain.

To fix it I should remove ParIter entirely in non-muilti_threaded builds.

@BobG1983
Copy link
Contributor Author

Nearly done

@BobG1983
Copy link
Contributor Author

Done

@alice-i-cecile alice-i-cecile changed the title Fix to events par_read bug 13821 Mark events as read during EventReader::par_read Jun 25, 2024
@alice-i-cecile alice-i-cecile enabled auto-merge June 25, 2024 15:30
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 25, 2024
Merged via the queue into bevyengine:main with commit 4c3b4a4 Jun 25, 2024
@alice-i-cecile alice-i-cecile removed this from the 0.14 milestone Jun 25, 2024
@BobG1983 BobG1983 deleted the par_read_fix_13821 branch June 25, 2024 17:52
cBournhonesque pushed a commit to cBournhonesque/bevy that referenced this pull request Jun 25, 2024
# Objective

- Fix issue bevyengine#13821  

## Solution

- Rewrote the test to ensure that it actually tests the functionality
correctly. Then made the par_read function correctly change the values
of self.reader.last_event_count.

## Testing

- Rewrote the test for par_read to run the system schedule twice,
checking the output each time

---------

Co-authored-by: Martín Maita <47983254+mnmaita@users.noreply.github.com>
mockersf pushed a commit that referenced this pull request Jun 25, 2024
Fix for par_read bugs retargetting 0.14.

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

Labels

A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants