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

Fix window close in example cause panic #17533

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jiangheng90
Copy link

Objective

Fixes #17532

Solution

  • check window valide

@Selene-Amanita
Copy link
Member

Selene-Amanita commented Jan 25, 2025

While your PR fixes the issue from the point of view of the example, I feel like there's something wrong regarding how Bevy handles this that should probably be fixed.

In order, during a Main schedule/frame, Bevy:

  1. in Update, will check if all windows have been closed (aka no entity with a Window component) with exit_on_all_closed and send an AppExit event if that's the case https://docs.rs/bevy/latest/bevy/window/fn.exit_on_all_closed.html
  2. in Last, will despawn Window entities corresponding to a closed Window with despawn_windows
    despawn_windows,
    /
    pub(crate) fn despawn_windows(
  3. at the end of the schedule in bevy_winit will check if an AppExit even has been sent to stop the app

I feel like step 1 should be done between 2 and 3, to avoid having a frame where the Window entity is despawned despite Bevy being configured to exit when that happens. So exit_on_all_closed (and exit_on_primary_closed) should be ordered in Last after despawn_windows (and a flush of the despawns). But maybe I'm missing something? Maybe it's not done that way because it messes with parallelization in the Last schedule, or maybe Bevy does expect to have one frame to deal with the no-window scenario for whatever reason?

I expect the same panic occuring in other examples using the Single<&Window> pattern: https://github.com/search?q=repo%3Abevyengine%2Fbevy+Single%3C%26Window%3E&type=code

@Selene-Amanita Selene-Amanita added C-Bug An unexpected or incorrect behavior C-Examples An addition or correction to our examples A-Windowing Platform-agnostic interface layer to run your app in labels Jan 25, 2025
Copy link
Member

@Selene-Amanita Selene-Amanita left a comment

Choose a reason for hiding this comment

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

Apparently the order in which Bevy does things is to allow people to write a system to react to an AppExit event in Update. I'm not sure this is right, but as long as it stays that way and Single panics, systems should probably never expect that there will always be a Window, and so the Single<&Window> should be reverted back. So, approving this PR. Might be needed for other examples too.

@BenjaminBrienen BenjaminBrienen added S-Needs-Review Needs reviewer attention (from anyone!) to move forward D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Jan 25, 2025
@beholdnec
Copy link

I think this is the same issue I reported here: #16066

@BenjaminBrienen
Copy link
Contributor

This doesn't seem right to me. The problem at hand is a general one. We shouldn't have to validate parameters like this.

@jiangheng90
Copy link
Author

@Selene-Amanita @BenjaminBrienen I have fix this problem in #17543

@BenjaminBrienen
Copy link
Contributor

Closing in favor of the other PR

@mockersf mockersf reopened this Jan 26, 2025
@mockersf
Copy link
Member

mockersf commented Jan 26, 2025

Single is not really usable until we've added back non panicking parameter validation. until then it should be avoided, like this PR does

@jiangheng90 jiangheng90 changed the title Fix parallel query panic Fix window close in example cause panic Jan 26, 2025
@jiangheng90
Copy link
Author

I just have changed Single<&Window>, which will call in update and cause panic. not all of it.

@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 Jan 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Windowing Platform-agnostic interface layer to run your app in C-Bug An unexpected or incorrect behavior C-Examples An addition or correction to our examples D-Straightforward Simple bug fixes and API improvements, docs, test and examples 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.

In example parallel_query close window cause panic
6 participants