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 Android crash on resume with wgpu #3847

Merged
merged 2 commits into from
Jan 22, 2024
Merged

Conversation

Garoven
Copy link
Contributor

@Garoven Garoven commented Jan 19, 2024

Added Viewport reinitialization and Window recreation for Android on resume event.

Closes #3674.

fix.mp4

Copy link
Contributor Author

@Garoven Garoven left a comment

Choose a reason for hiding this comment

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

I think it is the way it should be done

Copy link
Owner

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Great - thanks!

@emilk emilk changed the title Fix android on resume crash Fix Android crash on resume Jan 22, 2024
@emilk emilk added eframe Relates to epi and eframe android labels Jan 22, 2024
@emilk emilk merged commit e46b001 into emilk:master Jan 22, 2024
21 checks passed
@MarijnS95
Copy link
Contributor

@emilk random note: the HTML comment from the PR template ends up in merged commit messages: e46b001

@@ -386,6 +412,8 @@ impl WinitApp for WgpuWinitApp {
log::debug!("Event::Resumed");

let running = if let Some(running) = &self.running {
#[cfg(target_os = "android")]
self.recreate_window(event_loop, running);
Copy link
Contributor

Choose a reason for hiding this comment

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

Fwiw winit is built to emit Resumed on all platforms. Can the initialization be done here, generically?

Copy link
Contributor

Choose a reason for hiding this comment

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

In the same way we're looking to consistently emit Suspended before the loop is destroyed, giving users a consistent place to clean up their swapchain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. On platforms other than Android, Ios and Web Resume event is only send once when loop starts running. So the recreation for other platforms won't happen and for Ios and Web the Suspend/Resume cycle is different.
  2. The same for Suspend event it only occurs for Android, Ios and Web.

Copy link
Contributor

Choose a reason for hiding this comment

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

On other platforms it is not about recreating, it is about initially creating this state in the same place instead of having unnecessary conditionals in eframe.

Destruction only happens in ::Suspended.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay this looks much harder to fix than thought. WgpuWinitRunning contains a lot of state that should easily live outside the Android surface lifecycle.

@m-hugo
Copy link

m-hugo commented Jan 22, 2024

glow might need the same thing but it might be more subtly broken because App->MultiTask->anything->App works but App->Home->App breaks

@MarijnS95
Copy link
Contributor

MarijnS95 commented Jan 22, 2024

@m-hugo that's likely not going to be solvable without a significant rearchitecturing of android-activity (and winit thereafter). Android likely tries to start a second Activity (and terminating the first one some time later) but all Rust crates thus far assume there only exists one at all times.

This might be worked around by changing the launch/taskmode in the manifest, or maybe the shutdown path isn't implemented in eframe (given that the generic path was just implemented in this PR but for Android only...)

@m-hugo
Copy link

m-hugo commented Jan 22, 2024

@m-hugo that's likely not going to be solvable without a significant rearchitecturing of android-activity (and winit thereafter). Android likely tries to start a second Activity (and terminating the first one some time later) but all Rust crates thus far assume there only exists one at all times.

but it works on wgpu and worked on glow before?

@MarijnS95
Copy link
Contributor

but it works on wgpu and worked on glow before?

Then the lifetime cycle is perhaps not implemented correctly? That doesn't seem too surprising given that this PR hides it behind cfg(android) even though it should be is! generic across all backends.

@Garoven
Copy link
Contributor Author

Garoven commented Jan 22, 2024

I actually forgot about glow backend. But it seems fix for it very similar. Sadly Wgpu and Glow differ to much to make it generic.

@MarijnS95
Copy link
Contributor

It looks like egui does not have a canonical example that runs on Android, in the repository?

emilk pushed a commit that referenced this pull request Jan 24, 2024
Addition for <#3847>
In previous one i only fixed crash occurring with Wgpu backend. This
fixes crash with Glow backend as well.
I only tested this change with android so most things i changed are
behind ```#[cfg(target_os = "android")]```.

Both fixes are dirty thought. As
<#3172> says that "The root viewport
is the original viewport, and cannot be closed without closing the
application.". So they break rules i guess? But i can't think about
better solution for now.

Closes <#3861>.
@emilk emilk changed the title Fix Android crash on resume Fix Android crash on resume with wgpu Feb 5, 2024
@emilk emilk added the egui-wgpu label Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
android eframe Relates to epi and eframe egui-wgpu
Projects
None yet
Development

Successfully merging this pull request may close these issues.

eframe's Event::Resumed on android panics since #3172
4 participants