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

Removed window unwrap()s #20

Merged
merged 2 commits into from Aug 3, 2022
Merged

Conversation

JonahPlusPlus
Copy link
Contributor

@JonahPlusPlus JonahPlusPlus commented Aug 3, 2022

Fixes #19

What is the problem?

When closing a Bevy app, the window may be closed but the application is still running. Some bevy_flycam systems try to access the window, and in the process unwrap an Option<&mut Window>. If that Option is None, the application crashes.

What is the solution?

This PR changes those unwrap()s to matching patterns. Now, when a window isn't found, a warning is given instead.

What is the alternative?

This is still proper error handling, but gives unnecessary warnings when closing. There may be a way to use run criteria to stop the systems when AppExit events are sent, but I was unable to get this to work (presumably because of event delays). It would probably be better if Bevy provided an AppState resource from which the global state of the application could be checked and then use that for a run criteria. (Now that I think about it, explicit system ordering may be the solution)

@JonahPlusPlus
Copy link
Contributor Author

Now exiting looks like this:
image

@JonahPlusPlus
Copy link
Contributor Author

Also, I would recommend yanking the current version since I consider this type of behavior broken (see reasons for yanking). If not that, at least a patch update.

@BlackPhlox BlackPhlox merged commit a39c7af into sburris0:master Aug 3, 2022
@BlackPhlox
Copy link
Collaborator

@JonahPlusPlus Yeah no you are correct, due to the camera-driven update, windowing is handled differently which wasn't caught on the initial update to 0.8, maybe there should be some test that checks for this case? Either way, I've yanked 0.8.0 and published 0.8.1. Thanks for the effort! Really appreaciate it! 🚀

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.

Panics when window closes
2 participants