Skip to content

Prevent an additional world update after all windows have closed on Windows#18175

Merged
alice-i-cecile merged 1 commit intobevyengine:mainfrom
aloucks:windows-emit-app-exit-only-once
Mar 6, 2025
Merged

Prevent an additional world update after all windows have closed on Windows#18175
alice-i-cecile merged 1 commit intobevyengine:mainfrom
aloucks:windows-emit-app-exit-only-once

Conversation

@aloucks
Copy link
Copy Markdown
Contributor

@aloucks aloucks commented Mar 6, 2025 •
edited
Loading

Objective

The fix in #18105 includes a check for running headless, but that allows for an extra world update during shutdown.

This commit checks if the AppExit event has been recorded and prevents the additional world update.

Before

2025-03-06T03:11:59.999679Z  INFO bevy_window::system: No windows are open, exiting    
2025-03-06T03:12:00.001942Z  INFO bevy_winit::system: Closing window 0v1
2025-03-06T03:12:00.012691Z  INFO bevy_window::system: No windows are open, exiting

After

2025-03-06T03:18:45.552243Z  INFO bevy_window::system: No windows are open, exiting    
2025-03-06T03:18:45.554119Z  INFO bevy_winit::system: Closing window 0v1

Testing

Ran window examples

  • monitor_info continues to run after all windows are closed (it has ExitCondition::DontExit)
  • window_settings invisible window creation works as expected
  • multiple_windows exits after both windows are closed with a single exit message

@aloucks
Prevent an additional world update after all windows have closed on W…
58982ad 
…indows

The fix in bevyengine#18105 includes a check for running headless, but this allows
for an extra world update during shutdown.

This commit checks if the AppExit event has been recorded and prevents
the additional world update.
@aloucks aloucks force-pushed the windows-emit-app-exit-only-once branch from fa155ad to 58982ad Compare March 6, 2025 03:45
greeble-dev
greeble-dev approved these changes Mar 6, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@greeble-dev greeble-dev left a comment

Choose a reason for hiding this comment

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

Tested on Win10/Nvidia. Tried the mentioned examples and a few others.

Side note: Is there a reason the monitor_info example is set to not close when all windows are closed? I assumed this was a bug at first. I tried removing the ExitCondition::DontExit and it worked fine, but I don't have multiple monitors.

@aloucks
Copy link
Copy Markdown
Contributor Author

aloucks commented Mar 6, 2025

Side note: Is there a reason the monitor_info example is set to not close when all windows are closed? I assumed this was a bug at first. I tried removing the ExitCondition::DontExit and it worked fine, but I don't have multiple monitors.

That confused me on a previous PR. I also thought it was a bug at first. I think there needs to be an example that exercises that behavior, but I'm not sure that monitor_info is the right place. I didn't want to change the examples in this PR though.

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Windowing Platform-agnostic interface layer to run your app in labels Mar 6, 2025
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 6, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 6, 2025
Merged via the queue into bevyengine:main with commit 19ee692 Mar 6, 2025
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

+2 more reviewers

@JMS55 JMS55 JMS55 approved these changes

@greeble-dev greeble-dev greeble-dev approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-Windowing Platform-agnostic interface layer to run your app in C-Bug An unexpected or incorrect behavior 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@aloucks @alice-i-cecile @JMS55 @greeble-dev