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

WIP – Android EventLoop 2.0 #1001

Closed
wants to merge 1 commit into from

Conversation

mb64
Copy link

@mb64 mb64 commented Jul 1, 2019

Depends on rust-mobile/android-rs-glue#220

Starts to help out with #948.

This implementation is not great, undocumented, unfinished, and in some cases, unsound or incorrect. It's intended just to have the overall structure down, to be able to make gradual progress as different pieces of it are switched out for actual, better implementations.

It does, however, minimally work; I am able to log events on my phone.

These are the different "sub-parts" that need work:

The event loop

android-rs-glue has known issues, as well as hard-to-maintain cross-ffi datastructures and I think a general overhaul is warranted. (Of the
glue code – this is (mostly) independant of the build system.)

Currently, android-rs-glue starts main in a separate thread, then polls events, parses them, and sends them on a mpsc channel. I would like to transition the glue code away from this, instead giving main direct access to the ALooper, likely through a nice API that wraps the FFI calls. This gives Winit the ability to parse the AInputEvents directly into Winit events, avoiding intermediaries, and also removes a layer (one of many) of glue.

This might mean you can't run the event loop in threads other than the main one; I'm not certain/haven't done any tests. (iOS already sets a precedent for only allowing things in the main thread, though.)

When an activity is destroyed, Android asks it to save its state as a &[u8], then gives this data back when it restarts it later. This is somewhat incompatible with the normal C entrypoint of int main(int argc, char **argv). Here's my idea to deal with this:

  • An extention trait allows users to save state, which must be a String.
  • On Android, the supplied "command-line arguments" are "android", then the saved state.

This part is a fairly major change for android-rs-glue, so I'd like to discuss this and get an OK before I work on its implementation.

Support for multi-window

Multi-window is Android's term for things like split screen and Samsung's pop-up view floating apps. While it's a nice feature, it does make life more complicated when implementing Window stuff. I mostly ignored it in this initial implementation.

Multiple displays

It is possible to use multiple displays with Android phones. (For example, the Motorola Lapdock.) This implementation assumes only one monitor. This should be fixed.

Hidpi, window attributes, etc.

The hardcoded hidpi factor of 1 should be replaced. Information about display density can be found in the AConfiguration associated with the android_app, and can be mapped to hidpi factors using the table here.

Currently it ignores window attributes; this needs to be implemented.

There are various other details that need attending to, such as:

  • Figure out how (if possible) to set the title
  • Legit video modes
  • I'm sure there's more that I'm forgetting

Docs and tests

Last but not least, the documentation and tests should be updated for Android.


Some of these things are fairly straightforward. Hidpi support is probably the easiest of these items. If my ideas are OK'd, updating android-rs-glue will take some work, but seems doable.

However, I'm not at all well-versed in API's for multi-window or multiple displays. So if there's an Android guru reading this with free time, consider helping out.

A couple of questions I had along the way:

  • What is set_ime_position supposed to do?
  • What should run do when the app is closed?
  • What should run do if the event handler sets it to ControlFlow::Exit?

@felixrabe
Copy link
Contributor

  • An extention trait allows users to save state, which must be a String.
  • On Android, the supplied "command-line arguments" are "android", then the saved state.

I don't like abusing command-line arguments for state loading. As you already are (proposing to be) using an ext. trait for saving, why not also use that trait for loading?

@mb64
Copy link
Author

mb64 commented Jul 2, 2019

why not also use that trait for loading?

Haha, that's a much better idea! You don't really need an event loop to load state, though, so my recommendation is just a free function, called winit::platform::android::saved_state or something.

@mb64 mb64 changed the title Initial (bad) Android eventloop WIP – Android EventLoop 2.0 Jul 2, 2019
@Osspial
Copy link
Contributor

Osspial commented Jul 2, 2019

I'm totally down with you reworking android-glue as much as you deem necessary. I've given you write access to that repository since there isn't anyone to review it, so go nuts!

Anyhow, you had questions:

  • What is set_ime_position supposed to do?

set_ime_position is used for Input Method Editors to specify where exactly on the screen the IME should go. For instance, it would cause the japanese IME to appear below the cursor in the screenshot below.

image

We need to put more thought into the IME API and I'm not sure it's applicable on Android, so you should be able to safely ignore it for now.

  • What should run do when the app is closed?

Ya'know, there's been some discussion on having an "application quit" event in #41, and that may be applicable here. As for now, I'd suggest using Suspended/Resumed.

  • What should run do if the event handler sets it to ControlFlow::Exit?

I believe the iOS backend currently panics, but there isn't really a good option here.

@mtak-
Copy link
Contributor

mtak- commented Jul 3, 2019

Thanks for working on this!

Display cutout support would also be wonderful! On iOS, get_inner_* returns the coordinates/size of the safe area. get_outer_* returns the coordinates/size of the full window. Generally you render some background covering the entire outer size, and position UI elements within the inner size.

What should run do if the event handler sets it to ControlFlow::Exit?

I suspect panicking makes sense on android too. I'd have to read a lot more docs to be sure. Neither OS really wants an app to close itself.

What should run do when the app is closed?

Again, I can only speak to what the iOS backend does, but it simply transitions the AppState of the backend to a terminated state. Attempts to do things like process more events causes a panic. iOS calls UIApplicationMain from run which never returns, even on app exit. I dunno if android is the same.

@Osspial
Copy link
Contributor

Osspial commented Jul 11, 2019

One potential option for Exit is to tag it with #[cfg(/*desktop platforms*/)] so that it's just not possible to use on mobile platforms. That's got cross-platform compatibility issues of course, but we've got those issues already - this just moves the error to compile-time, not run-time.

@philip-alldredge
Copy link

@mb64 can you clarify how many layers of glue you are looking at reworking?

This is my understanding of the current pieces:

  • android_native_glue - Provided by NDK. Provides the function that Android calls "ANativeActivity_onCreate". This creates a thread that sets up a looper and calls android_main.
  • injected-glue - Injected by Cargo APK. Does additional setup. Creates a logging and a "main" thread.
  • glue - Provides API based on what cargo_apk provides.

I think the following approach would be nicer:

  • cargo-apk - Provides ANativeActivity_onCreate which stores the native activity and saved state. Provides an unsafe API for accessing it. Resets pointers after main finishes since the saved state is freed after ANativeActivity_onCreate finishes.
  • glue (appropriate name? android-sys)
    • Provide (un)safe APIs that allows applications to do the setup that injected-glue and android_native_glue is currently doing. This would allow other cates to implement things like requesting permissions, which is required for working with newer versions of android, or accessing the NDK camera API.
    • Provides an unsafe API for accessing the JNIEnv* when running in the main thread.
  • winit
    • Provide the cross-platform API using glue. Makes use of glue.

The approach seems much cleaner than the current approach and would reduce coupling between glue and cargo-apk and make it much easier to use the glue crate outside without cargo-apk.

I couldn't tell if what you were thinking was similar to this or if you had other thoughts.

@mb64
Copy link
Author

mb64 commented Jul 27, 2019

@philip-alldredge This is pretty much what I'm thinking, though with slightly different specifics.

  • cargo-apk
    • uses android_native_glue (a vendored, modified version) to provide ANativeActivity_onCreate as well as handling the various lifecycle events. This is easier than essentially re-implementing something similar in order to provide the same API. Its main job, though, is to handle the build process (for which I haven't looked at your PR yet, but will shortly)
  • (injected-glue and glue_obj are removed)
  • glue (or whatever you want to call it, I have no qualms with renaming but other people might have opinions) sort of has three related goals:
    • Provide unsafe FFI "headers" for the NDK
    • Provide unsafe FFI "headers" for the android_native_glue statically linked by cargo-apk
    • Provide (sometimes un)safe nicer Rust-y API wrappers over these libraries
  • winit
    • uses the APIs exposed by glue to implement the cross-platform event loop abstraction

I don't have any attachments to this hierarchy; if it makes more sense to split glue into android-ndk-sys and android-ndk, or any other substantial change, please suggest them. (Sooner is better, so I can take them into account while working on the first-draft glue-overaul event loop.)

The order of operations seems to me to be:

  • Updated build system
  • Minimal glue necessary for Winit (for which I have the WIP eventloop-2.0-overhaul branch)
  • Minimal working Winit event loop using this glue
  • The "necessary" additions to glue/winit: asset loading, permission requests, ...
  • The "nice" additions to Winit: saved state, device IDs (which might only be accessibly via JNI?), multi window, ...

@philip-alldredge
Copy link

@mb64
I have no strong objection to keeping android_native_glue around. My primary thinking was to reduce the amount of magic code that was injected by cargo APK and allow glue to control whether to create threads and event queues. In the end it is an implementation detail that won't affect much unless a user wants to use glue without cargo-apk. I'm personally not interested in that use case so I have no objections to vendoring it. The new build system doesn't build any C++ code itself but it should be trivial to add it to the makefile.

I don't have strong enough feelings about the name to push for renaming.

The ordering of things looks reasonable to me. The permission request will require JNI.

@philip-alldredge
Copy link

@mb64 after some additional thought, I've reconsidered my comment about renaming and organization of things. Personally, I think it would be best to have android_glue only contain the APIs required to work with cargo-apk and the injected-glue. The FFI definitions for the NDK and the safe wrappers should be in separate android-ndk-sys and/or android-ndk crates. The split would provide a clean separation, improve visibility, and encourage other rust android developers which do not use cargo-apk to use the library as opposed re-implementing the FFI and safe APIs.

That's just my opinion. You are the one putting in the effort to improve things so ultimately, it's up to you.

@goddessfreya
Copy link
Contributor

I've noticed a lot of traffic on https://github.com/rust-windowing/android-rs-glue. What's the status of this?

@Osspial
Copy link
Contributor

Osspial commented Sep 10, 2019

@zegentzy iirc android-glue needed to be reworked fairly significantly, so @mb64 and company have been reworking it before doing the work here.

@nox
Copy link
Contributor

nox commented Oct 24, 2019

What is blocking this and how can we help?

@mb64
Copy link
Author

mb64 commented Nov 2, 2019

Here's the progress so far, and what needs to be done:

android-rs-glue:

  • The build system is functional
  • I have a local copy with most of the changes to remove the injected-glue; I'll try to make a PR sometime early this week.
  • It also needs some assistance from those in the rust-windowing squad with publishing authority, to set up a rust-windowing Docker and also to push it to Crates.io.

android-ndk:

  • It's still currently in the 0.0.* stage because I keep finding changes that I think should be made, and I don't want to start out with the first "real" release being like 0.8.0. However, I think it's pretty close, so I'll call it 0.1 soon. This is relevant because I don't want Winit to depend on a not-yet-stable version.
  • In terms of features that I think should be there before 0.1, the main one on my list is requesting permissions, since this is a common, non-trivial task.
  • Docs, examples, #[inline] attributes, etc are still to do.

Finally, winit:

  • This PR is outdated, so I'll close it.
  • I have probably half a new implementation; I'll make a new PR once it's complete enough to be able to run an app that just logs Winit events. I've been more busy than anticipated but my conservative expectation is to have this within a month.

If you want to help:

  • At the moment, there's not much actionable that's not held up by me (sorry).
  • Once I push my local android-rs-glue with the injected glue gone (hopefully within the next few days), you can use android-ndk and find out where it's lacking/what would make it better – both in terms of features and in terms of its general architecture. Here are the docs. Some things are still undocumented, but since it's mostly direct wrappers over the NDK it at least shouldn't be completely opaque. Feel free to open an issue or a PR.

@mb64 mb64 closed this Nov 2, 2019
@dvc94ch
Copy link
Member

dvc94ch commented Dec 4, 2019

Where the changes to android-rs-glue pushed?

@dvc94ch
Copy link
Member

dvc94ch commented Dec 9, 2019

Made some progress, enough to get my port of flutter-rs to winit to display something on android. Still need to figure out how to get the screen density and handle orientation changes correctly and a couple of other issues.

screenshot

@simlay
Copy link
Contributor

simlay commented Dec 18, 2019

@dvc94ch Want to submit a PR?

@dvc94ch
Copy link
Member

dvc94ch commented Dec 19, 2019

There's a couple of kinks that need to be worked out. And the android-ndk-rs repo isn't accepting PR's at the moment. Maybe we can move it to the winit org? @mb64

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

9 participants