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

Upgrade to bevy 0.14 #37

Merged
merged 5 commits into from
Aug 22, 2024
Merged

Conversation

snendev
Copy link
Collaborator

@snendev snendev commented Aug 21, 2024

Issue

This upgrades Bevy to 0.14.

Changes

Most changes relate to color APIs, app.world() and app.world_mut(). Other changes include:

  • Returning AppExit from example main functions
  • Removing the test code that removes EventUpdateSignal, which no longer exists
  • Set ShouldUpdateEvents::Always on EventRegistry for playback tests and updated test assertions in FrameCount-related tests not to expect double buffering. configure InputPlaybackPlugin systems in First to occur after events flush to fix tests and guarantee correct system ordering.

Questions / TODOs Solved! Skip this :)

This last bit is potentially controversial but I didn't want to submit a PR with an incomplete solution even if I wasn't entirely confident in the more complete one.

Without this last change (feel free to try the commit, it's 432c5a7), there are issues with the input-playback.rs tests using PlaybackStrategy::FrameCount. Specifically, the Event buffers in these tests are not flushing each update unless ShouldUpdateEvents::Always is set on the EventRegistry resource. Then, if ShouldUpdateEvents::Always is set, the buffer flushes at the end of the update call, so the test code that follows app.update() sees an empty "front" buffer (to accept input in the next update call) and the events of the last update in the "back" buffer. This makes sense (that is the "last frame" now), but does imply that something needs to change.

To provide some concrete example behaviors, this creates issues in the frame_range_once and frame_range_loop tests, which specifically check for double-buffering behavior. Without ShouldUpdateEvents::Always, event counts simply accumulate, but with it included, the tests still fail because they expect double-buffered event counts. Additionally, the comment at tests/input_playback.rs#87 is now misleading: that input_events variable originally has both events in the events_a buffer, and if we set ShouldUpdateEvents::Always, it is the same but for the events_b buffer. This is not representative of double-buffering.

This is also true if we read the input events during playback_strategy_frame and other tests. The one test that does not have this behavior is repeated_playback which sets a TimeUpdateStrategy::ManualDuration and probably triggers FixedUpdate, which would set ShouldUpdateEvents::Ready and execute a flush.

It didn't seem as appropriate to wrangle FixedUpdate or manually trigger event flushes, nor to make assertions inside Update or PostUpdate somehow so that we could observe the double-buffering, which is why I took the approach of not expecting double-buffered events in these tests.

Seems like Events have gone through a few changes in the past few versions and I'm not an expert on all them, so please let me know if there is a preferable approach!

@snendev
Copy link
Collaborator Author

snendev commented Aug 21, 2024

ah, actually, I am wrong -- event updates should be flushed in First, not Last. I went back to double check because I was confused by that!

https://github.com/bevyengine/bevy/blob/main/crates%2Fbevy_app%2Fsrc%2Fapp.rs#L105

sorry, I'm out now but I'll try again / fix this PR later tonight

final edit: seems like it's a scheduling problem of the input playback system and the FlushEvents system set! FlushEvents should happen first. Will fix!

@snendev
Copy link
Collaborator Author

snendev commented Aug 21, 2024

Looks like everything's good now!

@alice-i-cecile alice-i-cecile merged commit e779860 into Leafwing-Studios:main Aug 22, 2024
4 checks passed
@snendev snendev deleted the bevy-0.14 branch August 22, 2024 20:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants