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.

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

Ensure that events are updated even when using a bare-bones Bevy App (#13808) #13842

Merged

Conversation

mnmaita
Copy link
Member

@mnmaita mnmaita commented Jun 14, 2024

Objective

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.

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

@mnmaita
Copy link
Member Author

mnmaita commented Jun 14, 2024

@alice-i-cecile would you be able to check if I've cleanly applied the changes to the events.rs module?

Feel free to edit the PR title and description in any way, it's 99% copypasta.

…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>
@alice-i-cecile alice-i-cecile added this to the 0.14 milestone Jun 14, 2024
@alice-i-cecile alice-i-cecile added A-App Bevy apps and plugins C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events labels Jun 14, 2024
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Nice work, and the tests are passing. Thank you very much for tackling this!

@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 Jun 14, 2024
@mockersf mockersf merged commit 84be2b3 into bevyengine:release-0.14.0 Jun 14, 2024
31 checks passed
@mnmaita mnmaita deleted the mnmaita/cherry-pick-13808 branch June 15, 2024 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-App Bevy apps and plugins 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.

None yet

3 participants