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

Fix bug where events are not being dropped #11528

Merged
merged 1 commit into from
Feb 2, 2024
Merged

Fix bug where events are not being dropped #11528

merged 1 commit into from
Feb 2, 2024

Conversation

Dig-Doug
Copy link
Contributor

Objective

Fix an issue where events are not being dropped after being read. I believe #10077 introduced this issue. The code currently works as follows:

  1. EventUpdateSignal is shared for all event types
  2. During the fixed update phase, EventUpdateSignal is set to true
  3. event_update_system, unique per event type, runs to update Events
  4. event_update_system reads value of EventUpdateSignal to check if it should update, and then resets the value to false

If there are multiple event types, the first event_update_system run will reset the shared EventUpdateSignal signal, preventing other events from being cleared.

Solution

I've updated the code to have separate signals per event type and added a shared signal to notify all systems that the time plugin is installed.

Changelog

  • Fixed bug where events were not being dropped

@github-actions GitHub Actions
Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@Dig-Doug
Copy link
Contributor Author

Hi @maniwani, would you be able to double check my analysis above?

@BD103
Copy link
Member

BD103 commented Jan 25, 2024

Is this something that we can add a test for, so it doesn't happen again in the future?

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events labels Jan 25, 2024
@JMS55 JMS55 added this to the 0.13 milestone Jan 26, 2024
@Dig-Doug Dig-Doug force-pushed the main-event-drop-fix branch 4 times, most recently from 53717a2 to d8b0428 Compare January 27, 2024 18:05
@Dig-Doug
Copy link
Contributor Author

Added a basic test that sets up 2 different events and checks that they get dropped.

hymm
hymm reviewed Jan 27, 2024
View reviewed changes
Copy link
Contributor

@hymm hymm left a comment

Choose a reason for hiding this comment

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

Your explanation of the bug makes sense. Instead of adding another resource per event can we just clear the EventUpdateSignal after all the event_updaate_systems have run?

@Dig-Doug Dig-Doug force-pushed the main-event-drop-fix branch 4 times, most recently from 2d315b8 to 4f2c401 Compare January 31, 2024 13:42
@Dig-Doug
Copy link
Contributor Author

Done. Updated to use a SystemSet.

@Dig-Doug Dig-Doug force-pushed the main-event-drop-fix branch 2 times, most recently from 4f2c401 to 41a32c0 Compare January 31, 2024 13:46
hymm
hymm approved these changes Feb 1, 2024
View reviewed changes
Copy link
Contributor

@hymm hymm left a comment

Choose a reason for hiding this comment

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

just some nits. Looks good otherwise.

crates/bevy_ecs/src/event.rs Show resolved Hide resolved
crates/bevy_time/src/lib.rs Show resolved Hide resolved
crates/bevy_time/src/lib.rs Outdated Show resolved Hide resolved
@Dig-Doug Dig-Doug force-pushed the main-event-drop-fix branch 5 times, most recently from 8c36764 to 3bd2970 Compare February 1, 2024 12:27
maniwani
maniwani approved these changes Feb 2, 2024
View reviewed changes
Copy link
Contributor

@maniwani maniwani left a comment

Choose a reason for hiding this comment

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

Good catch. Thanks for the clear explanation of the bug! This seems good to me.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 2, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 2, 2024
Merged via the queue into bevyengine:main with commit c859eac Feb 2, 2024
24 checks passed
tjamaan pushed a commit to tjamaan/bevy that referenced this pull request Feb 6, 2024
@Dig-Doug @tjamaan
Fix bug where events are not being dropped (bevyengine#11528)
1b8589f 
# Objective

Fix an issue where events are not being dropped after being read. I
believe bevyengine#10077 introduced this issue. The code currently works as
follows:

1. `EventUpdateSignal` is **shared for all event types**
2. During the fixed update phase, `EventUpdateSignal` is set to true
3. `event_update_system`, **unique per event type**, runs to update
Events<T>
4. `event_update_system` reads value of `EventUpdateSignal` to check if
it should update, and then **resets** the value to false

If there are multiple event types, the first `event_update_system` run
will reset the shared `EventUpdateSignal` signal, preventing other
events from being cleared.

## Solution

I've updated the code to have separate signals per event type and added
a shared signal to notify all systems that the time plugin is installed.

## Changelog

- Fixed bug where events were not being dropped
@maniwani maniwani mentioned this pull request Mar 1, 2024
Closed
@alice-i-cecile alice-i-cecile mentioned this pull request Jun 11, 2024
Merged
6 tasks
github-merge-queue bot pushed a commit that referenced this pull request Jun 12, 2024
@alice-i-cecile @hymm
Ensure that events are updated even when using a bare-bones Bevy App (#…
2cffd14 
…13808)

# Objective

As discovered in
Leafwing-Studios/leafwing-input-manager#538,
there appears to be some real weirdness going on in how event updates
are processed between Bevy 0.13 and Bevy 0.14.

To identify the cause and prevent regression, I've added tests to
validate the intended behavior.
My initial suspicion was that this would be fixed by
#13762, but that doesn't seem to
be the case.

Instead, events appear to never be updated at all when using `bevy_app`
by itself. This is part of the problem resolved by
#11528, and introduced by
#10077.

After some investigation, it appears that `signal_event_update_system`
is never added using a bare-bones `App`, and so event updates are always
skipped.

This can be worked around by adding your own copy to a
later-in-the-frame schedule, but that's not a very good fix.

## Solution

Ensure that if we're not using a `FixedUpdate` schedule, events are
always updated every frame.

To do this, I've modified the logic of `event_update_condition` and
`event_update_system` to clearly and correctly differentiate between the
two cases: where we're waiting for a "you should update now" signal and
where we simply don't care.

To encode this, I've added the `ShouldUpdateEvents` enum, replacing a
simple `bool` in `EventRegistry`'s `needs_update` field.

Now, both tests pass as expected, without having to manually add a
system!

## Testing

I've written two parallel unit tests to cover the intended behavior:

1. Test that `iter_current_update_events` works as expected in
`bevy_ecs`.
2. Test that `iter_current_update_events` works as expected in
`bevy_app`

I've also added a test to verify that event updating works correctly in
the presence of a fixed main schedule, and a second test to verify that
fixed updating works at all to help future authors narrow down failures.

## Outstanding

- [x] figure out why the `bevy_app` version of this test fails but the
`bevy_ecs` version does not
- [x] figure out why `EventRegistry::run_updates` isn't working properly
- [x] figure out why `EventRegistry::run_updates` is never getting
called
- [x] figure out why `event_update_condition` is always returning false
- [x] figure out why `EventRegistry::needs_update` is always false
- [x] verify that the problem is a missing `signal_events_update_system`

---------

Co-authored-by: Mike <mike.hsu@gmail.com>
mnmaita pushed a commit to mnmaita/bevy that referenced this pull request Jun 14, 2024
@alice-i-cecile @hymm @mnmaita
Ensure that events are updated even when using a bare-bones Bevy App (b…
4eaba9a 
…evyengine#13808)

As discovered in
Leafwing-Studios/leafwing-input-manager#538,
there appears to be some real weirdness going on in how event updates
are processed between Bevy 0.13 and Bevy 0.14.

To identify the cause and prevent regression, I've added tests to
validate the intended behavior.
My initial suspicion was that this would be fixed by
bevyengine#13762, but that doesn't seem to
be the case.

Instead, events appear to never be updated at all when using `bevy_app`
by itself. This is part of the problem resolved by
bevyengine#11528, and introduced by
bevyengine#10077.

After some investigation, it appears that `signal_event_update_system`
is never added using a bare-bones `App`, and so event updates are always
skipped.

This can be worked around by adding your own copy to a
later-in-the-frame schedule, but that's not a very good fix.

Ensure that if we're not using a `FixedUpdate` schedule, events are
always updated every frame.

To do this, I've modified the logic of `event_update_condition` and
`event_update_system` to clearly and correctly differentiate between the
two cases: where we're waiting for a "you should update now" signal and
where we simply don't care.

To encode this, I've added the `ShouldUpdateEvents` enum, replacing a
simple `bool` in `EventRegistry`'s `needs_update` field.

Now, both tests pass as expected, without having to manually add a
system!

I've written two parallel unit tests to cover the intended behavior:

1. Test that `iter_current_update_events` works as expected in
`bevy_ecs`.
2. Test that `iter_current_update_events` works as expected in
`bevy_app`

I've also added a test to verify that event updating works correctly in
the presence of a fixed main schedule, and a second test to verify that
fixed updating works at all to help future authors narrow down failures.

- [x] figure out why the `bevy_app` version of this test fails but the
`bevy_ecs` version does not
- [x] figure out why `EventRegistry::run_updates` isn't working properly
- [x] figure out why `EventRegistry::run_updates` is never getting
called
- [x] figure out why `event_update_condition` is always returning false
- [x] figure out why `EventRegistry::needs_update` is always false
- [x] verify that the problem is a missing `signal_events_update_system`

---------

Co-authored-by: Mike <mike.hsu@gmail.com>
@mnmaita mnmaita mentioned this pull request Jun 14, 2024
Merged
6 tasks
mnmaita pushed a commit to mnmaita/bevy that referenced this pull request Jun 14, 2024
@alice-i-cecile @hymm @mnmaita
Ensure that events are updated even when using a bare-bones Bevy App (b…
035d5a8 
…evyengine#13808)

As discovered in
Leafwing-Studios/leafwing-input-manager#538,
there appears to be some real weirdness going on in how event updates
are processed between Bevy 0.13 and Bevy 0.14.

To identify the cause and prevent regression, I've added tests to
validate the intended behavior.
My initial suspicion was that this would be fixed by
bevyengine#13762, but that doesn't seem to
be the case.

Instead, events appear to never be updated at all when using `bevy_app`
by itself. This is part of the problem resolved by
bevyengine#11528, and introduced by
bevyengine#10077.

After some investigation, it appears that `signal_event_update_system`
is never added using a bare-bones `App`, and so event updates are always
skipped.

This can be worked around by adding your own copy to a
later-in-the-frame schedule, but that's not a very good fix.

Ensure that if we're not using a `FixedUpdate` schedule, events are
always updated every frame.

To do this, I've modified the logic of `event_update_condition` and
`event_update_system` to clearly and correctly differentiate between the
two cases: where we're waiting for a "you should update now" signal and
where we simply don't care.

To encode this, I've added the `ShouldUpdateEvents` enum, replacing a
simple `bool` in `EventRegistry`'s `needs_update` field.

Now, both tests pass as expected, without having to manually add a
system!

I've written two parallel unit tests to cover the intended behavior:

1. Test that `iter_current_update_events` works as expected in
`bevy_ecs`.
2. Test that `iter_current_update_events` works as expected in
`bevy_app`

I've also added a test to verify that event updating works correctly in
the presence of a fixed main schedule, and a second test to verify that
fixed updating works at all to help future authors narrow down failures.

- [x] figure out why the `bevy_app` version of this test fails but the
`bevy_ecs` version does not
- [x] figure out why `EventRegistry::run_updates` isn't working properly
- [x] figure out why `EventRegistry::run_updates` is never getting
called
- [x] figure out why `event_update_condition` is always returning false
- [x] figure out why `EventRegistry::needs_update` is always false
- [x] verify that the problem is a missing `signal_events_update_system`

---------

Co-authored-by: Mike <mike.hsu@gmail.com>
mockersf pushed a commit that referenced this pull request Jun 14, 2024
@mnmaita @alice-i-cecile @hymm
Ensure that events are updated even when using a bare-bones Bevy App (#…
84be2b3 
…13808) (#13842)

# Objective

- Related to #13825

## Solution

- Cherry picked the merged PR and performed the necessary changes to
adapt it to the 0.14 release branch.

---------

As discovered in
Leafwing-Studios/leafwing-input-manager#538,
there appears to be some real weirdness going on in how event updates
are processed between Bevy 0.13 and Bevy 0.14.

To identify the cause and prevent regression, I've added tests to
validate the intended behavior.
My initial suspicion was that this would be fixed by
#13762, but that doesn't seem to
be the case.

Instead, events appear to never be updated at all when using `bevy_app`
by itself. This is part of the problem resolved by
#11528, and introduced by
#10077.

After some investigation, it appears that `signal_event_update_system`
is never added using a bare-bones `App`, and so event updates are always
skipped.

This can be worked around by adding your own copy to a
later-in-the-frame schedule, but that's not a very good fix.

Ensure that if we're not using a `FixedUpdate` schedule, events are
always updated every frame.

To do this, I've modified the logic of `event_update_condition` and
`event_update_system` to clearly and correctly differentiate between the
two cases: where we're waiting for a "you should update now" signal and
where we simply don't care.

To encode this, I've added the `ShouldUpdateEvents` enum, replacing a
simple `bool` in `EventRegistry`'s `needs_update` field.

Now, both tests pass as expected, without having to manually add a
system!

I've written two parallel unit tests to cover the intended behavior:

1. Test that `iter_current_update_events` works as expected in
`bevy_ecs`.
2. Test that `iter_current_update_events` works as expected in
`bevy_app`

I've also added a test to verify that event updating works correctly in
the presence of a fixed main schedule, and a second test to verify that
fixed updating works at all to help future authors narrow down failures.

- [x] figure out why the `bevy_app` version of this test fails but the
`bevy_ecs` version does not
- [x] figure out why `EventRegistry::run_updates` isn't working properly
- [x] figure out why `EventRegistry::run_updates` is never getting
called
- [x] figure out why `event_update_condition` is always returning false
- [x] figure out why `EventRegistry::needs_update` is always false
- [x] verify that the problem is a missing `signal_events_update_system`

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: Mike <mike.hsu@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hymm hymm hymm approved these changes

@maniwani maniwani maniwani approved these changes

Assignees
No one assigned
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
Milestone
0.13
Development

Successfully merging this pull request may close these issues.
6 participants
@Dig-Doug @BD103 @hymm @maniwani @alice-i-cecile @JMS55