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.12 #33

Merged
merged 3 commits into from
Jan 18, 2024
Merged

Conversation

snendev
Copy link
Collaborator

@snendev snendev commented Jan 13, 2024

Changes

This PR upgrades Bevy to version 0.12.

Most changes are required by updates to Bevy's API:

  • add_plugin -> add_plugins
  • add_system -> add_systems and removing in_base_set in favor of Schedule
  • EventReader::iter -> EventReader::read
  • adding Entity::PLACEHOLDER as the window entity for input events in tests

TODOs

Some tests are currently failing in tests/input_playback.rs:

  • repeated_playback
  • playback_strategy_frame_range_once
  • playback_strategy_frame_range_loop

This is definitely due to changes to Events in the last two versions, but I haven't figured out the correct solution yet. I'll take another look soon to try to figure it out if no one else gets to it before me.

Misc Notes

@snendev
Copy link
Collaborator Author

snendev commented Jan 17, 2024

The issue seems to be that no EventUpdateSignal is fired when update is called in tests.

In the failing tests (playback_strategy_frame_range_loop at least, the one I was playing with), I observed the following behavior:

  • Attaching a system with an EventReader (just logging the events) does report the correct events during the expected frames
  • During playback, the Events<KeyboardInput> resource executes calls to event_update_system but does not reach Events::update. In short, events are never moved from events_b to events_a during this playback.
  • In a new test that calls App::run and runs for a while (I attached a system that calls an AppExit after a certain number of frames), Events<KeyboardInput> does update every few frames after EventUpdateSignals are processed.

Since EventUpdateSignal is tied to FixedUpdate, it seems like the simultaneity of EventUpdateSignal and the playback period ending is only a coincidence, and this is just when enough time has passed that FixedUpdate performs some default behavior.

So, ultimately, I am not sure whether this PR needs to (1) update the tests to execute FixedUpdates at the appropriate times OR (2) update the actual PlaybackPlugin to account for FixedUpdate in some additional way

Here is the Bevy commit that introduces EventUpdateSignal. I wonder whether other testing environments have run into this issue. There's also a lot of comments on that PR (which I have not yet read) but I wonder whether this is addressed in there at all.

Anwyay -- I'd be happy to implement a solution here but I'm afraid I can't tell what direction would be "correct":

  • If PlaybackPlugin should be updated, I do not know what to do to it.
  • Otherwise:
    • Should testing environments have FixedTime's timestep set to 0? (and if so, it may be wise to document this behavior/requirement about testing environments somewhere, if it isn't already)
    • Should tests directly manipulate the EventUpdateSystem between updates?
    • Should the tests instead execute get_reader_current in some way (if I can get the borrowing to work, I haven't tried)?

(It seems at first glance that, in some game repository writing tests related to input events, the choice between the latter three questions would depend heavily on how the game is structured, which is interesting to me.)

I should finally state that have not tried this on bevy's main branch, only 0.12.1. But I didn't see any relevant changes when scouring the filesystem/git blame on Github.

@snendev
Copy link
Collaborator Author

snendev commented Jan 17, 2024

Ah -- perhaps we should remove the resource in tests?

@snendev
Copy link
Collaborator Author

snendev commented Jan 17, 2024

Ah -- perhaps we should remove the resource in tests?

I have implemented this solution for the time being. Seems like we will have to revisit this in 0.13 anyway. Please let me know if you prefer another intermediate solution, and I can make the change.

(also, apologies for the spam)

@snendev snendev marked this pull request as ready for review January 17, 2024 01:44
@alice-i-cecile alice-i-cecile merged commit 005d558 into Leafwing-Studios:main Jan 18, 2024
4 checks passed
@alice-i-cecile
Copy link
Contributor

Awesome, thank you <3

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