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

Merged
merged 20 commits into from
Jun 12, 2024

Conversation

alice-i-cecile
Copy link
Member

@alice-i-cecile alice-i-cecile commented Jun 11, 2024

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

  • 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

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-ECS Entities, components, systems, and events S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jun 11, 2024
@alice-i-cecile alice-i-cecile added this to the 0.14 milestone Jun 11, 2024
@rparrett
Copy link
Contributor

Maybe I also ran across this while updating bevy-ui-navigation as a couple failing tests. At the time I just assumed there was something breaking change I wasn't understanding or the tests themselves were broken so I worked around it.

rparrett/bevy-alt-ui-navigation-lite@d742bf2

@hymm
Copy link
Contributor

hymm commented Jun 11, 2024

looks like you have to add the event_update_system manually now if you're not using the default app https://github.com/bevyengine/bevy/pull/12936/files#diff-b2fba3a0c86e496085ce7f0e3f1de5960cb754c7d215ed0f087aa556e529f97fR94

@alice-i-cecile
Copy link
Member Author

looks like you have to add the event_update_system manually now if you're not using the default app

The event update system is being added already, but event_update_condition is always returning false.

@alice-i-cecile alice-i-cecile changed the title Fix broken app-level event updating Add tests to validate event updating behavior Jun 11, 2024
@alice-i-cecile alice-i-cecile changed the title Add tests to validate event updating behavior Ensure that events are updated even when using a bare-bones Bevy App Jun 11, 2024
@alice-i-cecile alice-i-cecile marked this pull request as ready for review June 11, 2024 17:13
@alice-i-cecile
Copy link
Member Author

@mockersf once this is merged, please bother me to replicate it on the 0.14 branch. I know this is non-trivial to cherrypick due to #13801.

@rparrett
Copy link
Contributor

rparrett commented Jun 11, 2024

After reverting my workaround and pointing at this branch, bevy-alt-ui-navigation-lite's tests are green again, for what it's worth. My experience with that crate is just ripping a bunch of stuff out of it, so I don't know much about the tests other than that they worked with Bevy 0.13.

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.

Fix seems good. Are there any tests for events with FixedTimestep active?

crates/bevy_ecs/src/event/registry.rs Outdated Show resolved Hide resolved
Co-authored-by: Mike <mike.hsu@gmail.com>
@alice-i-cecile
Copy link
Member Author

Not currently. I'll think about how to write those 🤔

@alice-i-cecile alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jun 11, 2024
@alice-i-cecile
Copy link
Member Author

The FixedUpdate logic is working as expected: testing that was substantially harder than I expected but I'm quite happy with the net result.

@alice-i-cecile alice-i-cecile added D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes A-Time Involves time keeping and reporting labels Jun 11, 2024
Copy link
Contributor

@rparrett rparrett left a comment

Choose a reason for hiding this comment

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

Approving with the caveat that I don't particularly understand the fixed update schedule / event machinery.

But this code seems to be very-high quality, the new tests look great, and it fixes existing tests in a crate I'm looking after.

@alice-i-cecile alice-i-cecile 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 12, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 12, 2024
Merged via the queue into bevyengine:main with commit 2cffd14 Jun 12, 2024
31 checks passed
mnmaita pushed a commit to mnmaita/bevy that referenced this pull request Jun 14, 2024
…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 pushed a commit to mnmaita/bevy that referenced this pull request Jun 14, 2024
…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
…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
Labels
A-ECS Entities, components, systems, and events A-Time Involves time keeping and reporting C-Bug An unexpected or incorrect behavior D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes 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