Winit 0.28 and android #6830

slyedoc · 2022-12-03T03:33:43Z

Objective

Currently Android support has a few issues. While learning winit I noticed 0.28 was going to change quite a bit for android thanks to @rib and this is an attempt to get ready for those and hopefully improve bevy android support. This is not complete, but I have enough working that a few more eyes would be welcome.

Update to winit 0.8 when released
Fixes Pipelined Rendering on Android crashing due to windows initialisation too early #3249
Addresses Resume/Suspend on Android

Solution

Two main changes changes with 0.28:

Android: rework backend to use android_activity crate, this removes the use ndk-glue in an attempt to support both NativeActivity and GameActivity. This PR is large though while only targeting a NativeActivity example for now, though that is an goal.
Removed Window::set_always_on_top and related APIs in favor of Window::set_window_level, created WindowLevel settings to follow the bevy style and updated examples

Workaround for Resume/Suspend on Android

I spend over a week working on trying to fix this the "ideal" way, but I am now of the option its not possible due to fact that it would be such burden to the rest of the bevy community it wouldn't be welcome. "After being Suspended on Android applications must drop all render surfaces before the event callback completes, which may be re-created when the application is next Resumed." For bevy though this is a lot worse then it sounds, its not just window surfaces, its every render resource, basically anything wgpu had a hand in. We have no way to reload plugins much less any resource that's been added from systems or FromWorld impls and since some of these resources are dependent on each other the order they would be reloaded matters. It get even worse when you consider we don't know what to drop either.
Simple but stupid work around, restart. If you exit the eventloop on suspend, android will restart you on resume, its not ideal, but its better than a blank screen or crashing.

Todo:

Fix Audio - See Android Support #86 notes
Provide example to save on exit and load on start
Test that I didn't break anything

Tried to break this down, but its all winit and android releated

alice-i-cecile · 2022-12-03T03:40:23Z

Awesome, thanks for putting the work in. @rib, if you're able, I'd love your review here.

We have no way to reload plugins

This is useful context that will help shape redesigns of plugins, thanks for raising it.

much less any resource that's been added from systems or FromWorld impls and since some of these resources are dependent on each other the order they would be reloaded matters. It get even worse when you consider we don't know what to drop either.

Eventually we might be able to get away with something involving a dedicated schedule for this. For now though I think the dumb-but-vaguely-functional solution is the right one.

rib

Thanks for taking a look at this - I've taken a quick look over it, looking at the android-activity bits and added a few comments

crates/bevy_derive/src/bevy_main.rs

rib · 2022-12-03T12:24:44Z

crates/bevy_android/Cargo.toml

+bevy_log = { path = "../bevy_log", version = "0.9.0" }
+once_cell = "1.16"
+
+ndk = "0.7" # This version *must* be the same as the version used by android-activity


I'm not sure this is true, but might be overlooking something.

The ndk crate shouldn't have any static state, though there are some ndk types exposed by the android-activity crate, e.g. for input.

I would have expected there wouldn't be any conflict with using other versions, but maybe it's an issue considering that the only use of the ndk crate in Bevy is in conjunction with an android-activity API and some types don't match without picking the same version here.

Maybe android-activity and Winit could re-export the ndk API similar to how the Winit crate re-exports the android-activity crate. That way you wouldn't have to worry about synchronizing this if you're only interested in using the same version that Winit and android-activity use.

This isn't the same as the previous requirement for the ndk-glue crate version needing to match the version used by Winit. There is a similar requirement that the android-activity crate must match watch Winit expects (for the same reason todo with how these glue crates are responsible for the main entrypoint of the application)

rib · 2022-12-03T12:38:35Z

examples/android/Cargo.toml

-assets = "../../assets"
-resources = "../../assets/android-res"
-build_targets = ["aarch64-linux-android", "armv7-linux-androideabi"]
+#package = "org.bevyengine.example"


Is this stuff not still needed for building with cargo apk? (which I guess can continue working while Bevy is using NativeActivity)

rib · 2022-12-03T12:40:03Z

examples/android/Cargo.toml

@@ -7,23 +7,38 @@ publish = false
 license = "MIT OR Apache-2.0"

 [lib]
-name = "bevy_android_example"
-crate-type = ["cdylib"]
+crate-type = ["cdylib", "rlib"]


not sure what the rlib is for here?

rlib is for the main.rs, so its easy to run on desktop. If I remove it change [[bin]] src to lib.rs like you did in android-activity examples, they work but then you get these warnings:

warning: ~/bevy/examples/android/Cargo.toml: file found to be present in multiple build targets: ~/bevy/examples/android/src/lib.rs

crates/bevy_android/src/lib.rs

crates/bevy_asset/src/io/android_asset_io.rs

rib · 2022-12-03T13:25:55Z

About the suspend and resume stuff, I'm not sure it's quite as bad as it may seem.

The only thing that needs to be re-created is the top-level render target/surface - not all wgpu rendering resources.

I think there's often a bit of a confusion about this topic, perhaps in part because it used to be true a long time ago that Android required a full GPU context tear down on some devices when they suspended and resumed (there was notion that a context would be "lost")

These days it's just the SurfaceView that is effectively dropped when the application suspends and the application can create a new render target and continue using it's pre-existing context and all gpu state (such as loaded textures etc).

I think a good first step for supporting suspend / resume would probably be to update this branch based on the last comments (unfortunately t ran out of time to follow up on that): #4913

Enabling suspend should then mainly just be a question of getting Bevy to re-create the render target surface for each resume on Android. This android-activity example tries to illustrate how that can work with wgpu + winit applications: https://github.com/rib/android-activity/blob/main/examples/agdk-winit-wgpu/src/lib.rs

In general though enabling suspend/resume support for Android can probably be treated as an orthogonal issue that shouldn't hold up an initial port to winit 0.28 + android-activity.

slyedoc · 2022-12-03T15:02:36Z

Switch from using global to resource, AndroidResource (happy to change the name), and pulled fn android_main from bevymain macro. Without the global, main has to receive the app, a resource, or the android app its self, or an option wrapping the later two. Only the first option keeps non android builds from needing to know about android related crates. This breaks the intended use of bevymain, but there is no magic global state.

slyedoc · 2022-12-03T15:54:16Z

I hadn't seen all the work in #4913, I got as far as recreating RenderApp resources and surfaces then threw in the towel once I started seeing the scope of the issue. You are far more knowledgeable in this area.

rib · 2022-12-04T13:13:59Z

Switch from using global to resource, AndroidResource (happy to change the name), and pulled fn android_main from bevymain macro. Without the global, main has to receive the app, a resource, or the android app its self, or an option wrapping the later two. Only the first option keeps non android builds from needing to know about android related crates. This breaks the intended use of bevymain, but there is no magic global state.

Seems reasonable to me. Name seems ok, if a bit general maybe? Maybe it'd be good to have 'App' in the name?

Personally I tend to think it's fine to not use the bevymain macro, but I'm not generally a huge fan of magic main function macros as a solution for platform portability vs having some tiny amount of explicit per-platform code. (I'm not a Bevy maintainer though so not sure what preference others will have)

Maybe if there would be strong interest in making the bevymain macro work with a main() that has no arguments on Android then it could at least somehow use a hidden thread-local instead of a global static, so that way it shouldn't conflict with the possibility of spawning multiple android_main() threads for separate Activities in the future.

mockersf · 2022-12-04T23:16:57Z

crates/bevy_android/src/lib.rs

+#[cfg(target_os = "android")]
+use bevy_ecs::system::Resource;
+
+#[cfg(target_os = "android")]
+pub use ndk::asset::AssetManager;
+#[cfg(target_os = "android")]
+pub use winit::platform::android::activity::*;
+
+/// Resource containing AndroidApp
+#[cfg(target_os = "android")]
+pub struct AndroidResource {
+    pub android_app: AndroidApp
+}
+
+#[cfg(target_os = "android")]
+impl Resource for AndroidResource {}


I don't think this justify creating a new crate for it. Also, I would rather avoid having a bevy_android crate where everything is behind a check for android.

Could it be placed in an existing crate?

mockersf · 2022-12-05T02:02:08Z

examples/android/Cargo.toml

-assets = "../../assets"
-resources = "../../assets/android-res"


is it possible to keep referencing the same asset folder?

# Objective - Update winit to 0.28 ## Solution - Small API change - A security advisory has been added for a unmaintained crate used by a dependency of winit build script for wayland I didn't do anything for Android support in this PR though it should be fixable, it should be done in a separate one, maybe #6830 --- ## Changelog - `window.always_on_top` has been removed, you can now use `window.window_level` ## Migration Guide before: ```rust app.new() .add_plugins(DefaultPlugins.set(WindowPlugin { primary_window: Some(Window { always_on_top: true, ..default() }), ..default() })); ``` after: ```rust app.new() .add_plugins(DefaultPlugins.set(WindowPlugin { primary_window: Some(Window { window_level: bevy::window::WindowLevel::AlwaysOnTop, ..default() }), ..default() })); ```

alice-i-cecile · 2023-02-03T21:53:14Z

Once #7493 is merged, please rebase (or redo) to capture the remaining fixes needed.

alice-i-cecile · 2023-02-04T16:49:50Z

They were already done in the PR to update winit (#7480)

The other work from this PR has already been merged, so closing this out.

# Objective - Merge the examples on iOS and Android - Make sure they both work from the same code ## Solution - don't create window when not in an active state (from #6830) - exit on suspend on Android (from #6830) - automatically enable dependency feature of bevy_audio on android so that it works out of the box - don't inverse y position of touch events - reuse the same example for both Android and iOS Fixes #4616 Fixes #4103 Fixes #3648 Fixes #3458 Fixes #3249 Fixes #86

# Objective - Update winit to 0.28 ## Solution - Small API change - A security advisory has been added for a unmaintained crate used by a dependency of winit build script for wayland I didn't do anything for Android support in this PR though it should be fixable, it should be done in a separate one, maybe bevyengine/bevy#6830 --- ## Changelog - `window.always_on_top` has been removed, you can now use `window.window_level` ## Migration Guide before: ```rust app.new() .add_plugins(DefaultPlugins.set(WindowPlugin { primary_window: Some(Window { always_on_top: true, ..default() }), ..default() })); ``` after: ```rust app.new() .add_plugins(DefaultPlugins.set(WindowPlugin { primary_window: Some(Window { window_level: bevy::window::WindowLevel::AlwaysOnTop, ..default() }), ..default() })); ```

alice-i-cecile requested a review from mockersf December 3, 2022 03:35

alice-i-cecile added C-Bug An unexpected or incorrect behavior P-High This is particularly urgent, and deserves immediate attention O-Android Specific to the Android mobile operating system labels Dec 3, 2022

alice-i-cecile requested a review from hymm December 3, 2022 03:36

rib reviewed Dec 3, 2022

View reviewed changes

rib mentioned this pull request Dec 4, 2022

Consider either re-exporting ndk crate or encapsulating the ndk crate API rust-mobile/android-activity#45

Open

mockersf reviewed Dec 4, 2022

View reviewed changes

mockersf reviewed Dec 5, 2022

View reviewed changes

winit update and better support for android resume suspend

febb8fe

slyedoc force-pushed the winit-0.28-and-android branch from 5d706a4 to febb8fe Compare December 15, 2022 04:06

This was referenced Dec 25, 2022

ndk-glue looks to be archived and marked unmaintained #7028

Closed

Make asset_manager configurable #7029

Closed

inodentry added this to the 0.10 milestone Jan 15, 2023

mockersf mentioned this pull request Feb 3, 2023

[Merged by Bors] - update winit to 0.28 #7480

Closed

mockersf mentioned this pull request Feb 3, 2023

[Merged by Bors] - IOS, Android... same thing #7493

Closed

alice-i-cecile closed this Feb 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Winit 0.28 and android #6830

Winit 0.28 and android #6830

slyedoc commented Dec 3, 2022 •

edited

Loading

alice-i-cecile commented Dec 3, 2022

rib left a comment

rib Dec 3, 2022

rib Dec 3, 2022

rib Dec 3, 2022

slyedoc Dec 3, 2022

rib commented Dec 3, 2022

slyedoc commented Dec 3, 2022

slyedoc commented Dec 3, 2022

rib commented Dec 4, 2022

mockersf Dec 4, 2022

mockersf Dec 5, 2022

alice-i-cecile commented Feb 3, 2023 •

edited

Loading

alice-i-cecile commented Feb 4, 2023

		assets = "../../assets"
		resources = "../../assets/android-res"

Winit 0.28 and android #6830

Winit 0.28 and android #6830

Conversation

slyedoc commented Dec 3, 2022 • edited Loading

Objective

Solution

Todo:

alice-i-cecile commented Dec 3, 2022

rib left a comment

Choose a reason for hiding this comment

rib Dec 3, 2022

Choose a reason for hiding this comment

rib Dec 3, 2022

Choose a reason for hiding this comment

rib Dec 3, 2022

Choose a reason for hiding this comment

slyedoc Dec 3, 2022

Choose a reason for hiding this comment

rib commented Dec 3, 2022

slyedoc commented Dec 3, 2022

slyedoc commented Dec 3, 2022

rib commented Dec 4, 2022

mockersf Dec 4, 2022

Choose a reason for hiding this comment

mockersf Dec 5, 2022

Choose a reason for hiding this comment

alice-i-cecile commented Feb 3, 2023 • edited Loading

alice-i-cecile commented Feb 4, 2023

slyedoc commented Dec 3, 2022 •

edited

Loading

alice-i-cecile commented Feb 3, 2023 •

edited

Loading