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.

Sign up for GitHub

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

Jump to bottom

Ensure clean exit #13236

Merged
merged 2 commits into from May 12, 2024
Merged

Ensure clean exit #13236

merged 2 commits into from May 12, 2024
+55 −12

Conversation

tychedelia
Copy link
Contributor

@tychedelia tychedelia commented May 4, 2024 •
edited by alice-i-cecile

Objective

Fixes two issues related to #13208.

First, we ensure render resources for a window are always dropped first to ensure that the winit::Window always drops on the main thread when it is removed from WinitWindows. Previously, changes in #12978 caused the window to drop in the render world, causing issues.

We accomplish this by delaying despawning the window by a frame by inserting a marker component ClosingWindow that indicates the window has been requested to close and is in the process of closing. The render world now responds to the equivalent WindowClosing event rather than WindowCloseed which now fires after the render resources are guarunteed to be cleaned up.

Secondly, fixing the above caused (revealed?) that additional events were being delivered to the the event loop handler after exit had already been requested: in my testing RedrawRequested and LoopExiting. This caused errors to be reported try to send an exit event on the close channel. There are two options here:

  • Guard the handler so no additional events are delivered once the app is exiting. I considered this but worried it might be confusing or bug prone if in the future someone wants to handle LoopExiting or some other event to clean-up while exiting. We are now taking this approach.
  • Only send an exit signal if we are not already exiting. It doesn't appear to cause any problems to handle the extra events so this seems safer.

Fixing this also appears to have fixed #13231.

Fixes #10260.

Testing

Tested on mac only.

Changelog

Added

  • A WindowClosing event has been added that indicates the window will be despawned on the next frame.

Changed

  • Windows now close a frame after their exit has been requested.

Migration Guide

  • Ensure custom exit logic does not rely on the app exiting the same frame as a window is closed.

@tychedelia
Ensure clean exit
a8acdb3 
Fixes two issues related to bevyengine#13208.

First, we ensure render resources for a window are always dropped first
to ensure that the `winit::Window` always drops on the main thread when
it is removed from `WinitWindows`. Previously, changes in bevyengine#12978 caused
the window to drop in the render world, causing issues.

We accomplish this by delaying despawning the window by a frame by
inserting a marker component `ClosingWindow` that indicates the window
has been requested to close and is in the process of closing. The
render world now responds to the equivalent `WindowClosing` event
rather than `WindowCloseed` which now fires after the render resources
are guarunteed to be cleaned up.

Secondly, fixing the above caused (revealed?) that additional events
were being delivered to the the event loop handler after exit had
already been requested: in my testing `RedrawRequested` and
`LoopExiting`. This caused errors to be reported try to send an
exit event on the close channel. There are two options here:
    - Guard the handler so no additional events are delivered once the
    app is exiting. I considered this but worried it might be
    confusing or bug prone if in the future someone wants to handle
   `LoopExiting` or some other event to clean-up while exiting.
    - Only send an exit signal if we are not already exiting. It
    doesn't appear to cause any problems to handle the extra events
    so this seems safer.
Fixing this also appears to have fixed bevyengine#13231.
@alice-i-cecile alice-i-cecile added this to the 0.14 milestone May 4, 2024
@JMS55
Copy link
Contributor

JMS55 commented May 4, 2024 •
edited

Is the latter issue with events firing after window close related to #13203?

@tychedelia
Copy link
Contributor Author

Is the latter related to #13203?

Yup, this looks like it's trying to fix the same issue. Happy to remove in favor of that PR.

@BD103 BD103 added C-Bug An unexpected or incorrect behavior A-Windowing Platform-agnostic interface layer to run your app in O-MacOS Specific to the MacOS (Apple) desktop operating system C-Regression Functionality that used to work but no longer does. Add a test for this! S-Needs-Testing Testing must be done before this is safe to merge labels May 4, 2024
@BD103
Copy link
Member

BD103 commented May 4, 2024

This works on my MacBook, but it will probably need testing on other platforms just in case.

@tychedelia tychedelia mentioned this pull request May 4, 2024
Closed
@mockersf
Copy link
Member

mockersf commented May 5, 2024

works for me, but there may be an issue with ordering in the logs:

2024-05-05T21:46:49.749344Z  INFO bevy_window::system: No windows are open, exiting
2024-05-05T21:46:49.750662Z  INFO bevy_winit::system: Closing window Entity { index: 0, generation: 1 }

I don't think this is sensitive for wasm or mobile, but testing on windows and linux would be good

@mockersf mockersf mentioned this pull request May 5, 2024
Closed
@tychedelia
Copy link
Contributor Author

works for me, but there may be an issue with ordering in the logs:

I think this is the same on main: exit_on_all_closed runs on PostUpdate and despawn_windows which releases the actual winit window runs on Last. They both respond to the Window component being de-spawned. But agree it could be more clear, it's really more like "No window components detected".

I don't think this is sensitive for wasm or mobile, but testing on windows and linux would be good

I'll test on windows later.

@Friz64
Copy link
Contributor

Friz64 commented May 6, 2024

Works fine on both Wayland and XWayland for me.

@tychedelia
Copy link
Contributor Author

Looks like we have some warnings to clean up:

2024-05-06T21:08:35.419078Z  INFO bevy_window::system: No windows are open, exiting
2024-05-06T21:08:35.420490Z  INFO bevy_winit::system: Closing window Entity { index: 0, generation: 1 }
2024-05-06T21:08:35.424675Z  WARN bevy_winit: Skipped event ModifiersChanged(Modifiers { state: ModifiersState(0x0), pressed_mods: ModifiersKeys(0x0) }) for unknown winit Window Id WindowId(WindowId(1182052))
2024-05-06T21:08:35.424813Z  WARN bevy_winit: Skipped event Focused(false) for unknown winit Window Id WindowId(WindowId(1182052))
2024-05-06T21:08:35.429418Z  WARN bevy_winit: Skipped event Destroyed for unknown winit Window Id WindowId(WindowId(1182052))
2024-05-06T21:08:35.431869Z  INFO bevy_window::system: No windows are open, exiting

I think we need to prevent any events being delivered post exit. Right now the PR just skips sending a second exit signal.

@tychedelia
Copy link
Contributor Author

This should be good now on windows. I changed the behavior to never deliver new events once we start exiting. #13203 does something similar, but I really think the simple approach is honestly fine. Happy to go either way.

@Brezak
Copy link
Contributor

Brezak commented May 7, 2024

I think this PR should be merged instead of #13203. The only thing this PR is (debatably) missing is logging the winit events we have skipped.

@MiniaczQ
Copy link
Contributor

MiniaczQ commented May 7, 2024

Windows 11, double closing is fixed, no warnings about unread events.
Not sure if the order is considered a problem

image

@Friz64
Copy link
Contributor

Friz64 commented May 9, 2024

This fixes #10260.

@Friz64 Friz64 mentioned this pull request May 9, 2024
Closed
@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-Testing Testing must be done before this is safe to merge labels May 12, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 12, 2024
Merged via the queue into bevyengine:main with commit dc0fdd6 May 12, 2024
28 checks passed
@Friz64 Friz64 mentioned this pull request May 12, 2024
Closed
This was referenced May 12, 2024
Closed
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mockersf mockersf mockersf approved these changes

@Brezak Brezak Brezak approved these changes

Assignees
No one assigned
Labels
A-Windowing Platform-agnostic interface layer to run your app in C-Bug An unexpected or incorrect behavior C-Regression Functionality that used to work but no longer does. Add a test for this! O-MacOS Specific to the MacOS (Apple) desktop operating system 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
0.14
8 participants
@tychedelia @JMS55 @BD103 @mockersf @Friz64 @Brezak @MiniaczQ @alice-i-cecile