-
Notifications
You must be signed in to change notification settings - Fork 903
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
Android: rework backend to use android_activity crate #2444
Conversation
I'm not sure at the moment what the best way of linking to android specific API from common docs would be (currently causing CI failures) |
b464119
to
4fc8c04
Compare
I wasn't aware that Winit supported compiler version 1.57.0 (the cause of the last CI failure) and although I've now updated android-activity to support being compiled with 1.57.0 I also now need to upstream patches for I'm wondering if we could bring the MSRV for Winit forwards a little bit? - at least to |
This is a breaking change, right? In that case, we may be able to bump MSRV, since it'll probably take us 2-3 months before we'll release a new version. (Also, remember to add a CHANGELOG entry) |
With the MSRP bump now in, is this unblocked? I'm super stoked to be able to put the |
We should be able to unblock this once I get a chance to do a 0.3 release. I was also taking a pass at trying to add Rust bindings for GameTextInput to the API to support input methods (soft keyboards) but that hasn't gone as smoothly as I was hoping and so I may have to skip that for now (I had been thinking it would be good to cram this into 0.3 ideally) Off the top of my head I'm not actually sure if you need GameActivity to be able to put the view of your activity in a layout. I would guess that's possible with NativeActivity too. GameActivity derives from AppCompatActivity which NativeActivity does not and there might be something about that that helps with Layout integration perhaps? I'll try and get a chance soon to update / unblock this PR. |
I tried to do so really hard, but haven't succeeded. This still doesn't rule out that it's possible, but it didn't seem easy at all.
Yes, this is also the conclusion I ended up settling on so far.
Yay! |
7f720dc
to
9b3b3b0
Compare
Okey, so I had a chance to update this yesterday - based on a new release of I started looking at exposing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for looking into this topic - allowing a broader set of activities seems interesting while keeping compatibility with the base NativeActivity
. Tested it yesterday a bit with simpler applications.
One point to discuss would probably be organization in general under the light that we would effectively offload larger implementation details into an external dependency requiring syncing for issues and releases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This updates the Android backend to use the android_activity crate instead
of ndk-glue. This solves a few issues:
I have previously requested to keep ndk-glue
as a pure-Rust glue implementation atop NativeActivity
and even offered to finalize a rework branch to match the new API (effectively redoing some of the code that currently lives in winit
, solving exactly the issues that were reported later) as long as we could set up a shared place to define this interface (or toggle it purely at compile time) yet nothing seems to have been done in this regard, making this an instant no-go from me.
Thanks @msiglreith for making me aware, as I hadn't seen/expected this to make it into a PR yet.
I recall we had some discussion around wanting a pure rust glue layer which I agree would be nice to have eventually but I think I tried to say I saw that as an implementation detail that we can hopefully tackle separately. I think we are hopefully also in a better position with I tried to say that I believe that the Even though the In terms of the comments about offering to finalize a rework branch to match the new API I'm not sure what this is referring too, so sorry if we had different ideas in some earlier discussion. I tried to look over #2293 but I don't see this. I see these initial questions from you, which might be what you're referring to?:
and after my first pass at implementing At this point In particular, in July I answered your question "Who'll make winit use the new API" with:
and I didn't see that there was any objection to my proposal or push back - you even sounded positive about some of the issues that I had tackled with its design, such as removing all global static state? |
9b3b3b0
to
dbd1d07
Compare
It can be tackled separately (i.e. not conflating this PR) without temporarily forcing your C-only backend on Android +
Again, no need to rewrite - it is mostly finalizing an in-progress refactor of
Fair enough, let's create a repo under
I was positive about an independent contributor reporting the exact same issues I had on my TODO list, giving some extra incentive to solve them at once by a big |
Fwiw it looks like @msiglreith shares similar concerns about offloading larger chunks of the backend to external crates (outside |
Sorry but this seems like a disingenuous criticism at this point. The backend isn't "C-only" by a long shot but it does currently reuse some very well tested C code. In the context of a low-level piece of glue for an operating system, that inherently going to involve some unsafe code (to interact with JNI at the very least) I really don't understand the hang up here. It seems pedantic to point out but it's perhaps also worth noting that APIs like Using this C code just saved a small amount of time because the code already existed and has been widely used for a long time. The C code also naturally addressed at least one specific design issue that was noted after I initially looked at using ndk-glue - namely it encapsulates how it synchronizes with Java for saving state and creating/destroying window surfaces. As I've noted before - this code can be re-written in Rust if we want and since it's fully encapsulated it also won't affect the public API. As a litmus test for this issue; do you also plan on re-implementing existing NDK libraries that are written in C/C++ out of some concern that all C/C++ code should be avoided on Android? It would technically be possible, and even relatively straight forward, but probably also quite a chore.
As far as I can tell, you're apparently upset that I either didn't submit all my work as a PR against the ndk-glue project or else upset that I didn't wait for you to do the work instead. Fwiw I originally looked at building on ndk-glue and as I did this work incrementally in the open I've also repeatedly documented various issues I hit. I also reached out to the ndk-glue project in April (rust-mobile/ndk#266) explaining my interest in using GameActivity and the brief feedback at the time was that there was more interest in creating a vendored RustActivity at some point, though there wasn't much of a clear plan to implement that yet.
Sorry but this doesn't seem reasonable at this point. I made and shared the android-activity repo specifically to be able to create a common API that could be Activity agnostic and I invited you to share feedback and contribute. (which you are still welcome to do) I didn't just come up with a new API and then decide to write an implementation with an intention to make ndk-glue redundant. The incremental process of designing the common API happened as result of building this project which supports both GameActivity and NativeActivity and I currently see practical benefits to supporting multiple Activities via backends in a single repo instead of prioritizing external glue crates (comparable to in-tree Winit backends vs out-of tree Winit backends). It's quite frustrating that you've become focused on the fact that the
I rather wish you could see the positive of finding someone else facing the same issues and being willing to work towards solving them - those people are potential collaborators. It makes sense that android-activity ended up as a separate repo from ndk-glue on multiple levels:
|
For reference here I recently merged this PR for android-activity which fully ports the native-activity backend to Rust: https://github.com/rib/android-activity/pull/35. It has no affect on the public API / design etc but since multiple people have taken some issue with the fact that the backend wasn't pure Rust I figured I may as well just do the port to Rust now to avoid it being a distraction. |
Thanks @msiglreith for taking the time to test this out and esp on the constructive feedback that led to the input API fix, as well as your suggested ergonomic improvement to re-export Regarding organization in general: Although I'm open to e.g. moving the repo under the rust-windowing organisation I think my preference would be to wait and see if that would be helpful because I'd prefer to consider organisational changes in the context of a specific challenge/problem. One minor reservation is that I'd like Github makes it easy to add contributors to the repo without it needing to belong to an organisation so if there are others that are interested in contributing regularly that's easy to address. My preference is to add developers I know based on their PRs/issues/discussion etc. If there were some reason why I wouldn't be able to contribute to / maintain the project in the future then I'd of course look for a new maintainer or e.g. agree to moving the repo to a neutral organisation such as In terms of release coordination my hope is that it shouldn't usually be a concern if winit sticks to stable features and release (unless we've specifically discussed coordinating / synchronizing something). I prefer to always keep the main branch of a project stable such that a release can always be cut from If there is some feature that's critical for Winit specifically then I would expect to only land that feature based on a branch of Winit that would validate the design + implementation. That means that by the time the feature would land in android-activity we should already know that it's stable as far as winit is concerned and that feature can immediately be rolled into a new stable release which Winit can depend on. Does that sound reasonable? |
I think your points wrt. maintenance are valid, and I doubt it'll be a problem in practice - it's essentially the same thing I've done with I think you've made some good points in general, but I lack the technical knowledge to actually determine which approach ( |
caff6b7
to
a256423
Compare
For reference here; the change I pushed today was to implement @msiglreith's idea of re-exporting the android-activity API so that it's possible for applications to avoid explicitly depending on This also adds "android-native-activity" and "android-game-activity" features for Winit that set the corresponding feature in I feel like at this point hopefully all feedback has been addressed? |
a256423
to
9c0c690
Compare
just rebased, updated the features table and removed an out-of-date comment |
Since this PR is currently blocked due to requested changes from @MarijnS95 I've re-requested review while I hope all issue have been resolved now. |
6cfaea4
to
713bb4e
Compare
Hey all, sorry to poke here but since there hasn't been any follow up here in the last week I wonder if we can agree whether this PR is ready to land? @msiglreith since you're currently listed as the maintainer for the Android backend maybe you're able to comment? I think that landing this could help with being able to progress other Android features as well as other cross-cutting features that affect all backends but are awkward to deal with while this big change for the Android backend is pending. fwiw @dvc94ch who was the original author of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested this PR and it works great. It enables some cool new stuff like building apps with custom and/or multiple activities and I think it's a pragmatic way forward for the rust on android ecosystem.
sorry for taking so long! I have a week off where I'll try to work on tha backlog. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I believe this goes in the right direction. I would merge this once the remaining parts are addressed and there are no further arguments against this step. Given the large change my general opinion on this topic:
pro:
- Compatible with NativeActivity -> no hard requirement on Java
- Reduce user-level API - easier to upgrade versions and less dependencies for the users
- Appears strong demand from users to support more sophisticated features
- C parts reimplemented in Rust based on reference impl, doesn't regress in this place
con:
- Increases fragmentation of the Rust/Android ecosystem (build systems, windowing and activity handling). Stays compatible with ndk and cargo-apk, which is positive.
- Moving parts of the backend out of the organization causing syncing overhead, as well as larger changes in general comes with some risk 👀
README.md
Outdated
|
||
| Base Class | Feature Flag | Notes | | ||
| :--------------: | :---------------: | :-----: | | ||
| `NativeActivity` | `android-native-activity` | Built-in to Android - making it possible to build some tests/demos without needing to compile any JVM code. Can give a false sense of convenience because it's often not really possible to avoid needing a build system that can compile some JVM code, to at least subclass `NativeActivity` | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's write this is a bit more neutral:
- builtin without need for compiling Java
- therefore, limited functionality and extensibility
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've taken a pass at tweaking the wording here.
I've tried to remove the language that implies it's only "tests/demos" that can use NativeActivity or assuming anything about the false sense of convenience it has.
The general intent here was to try and highlight a few of the pros/cons of each option that can help someone make a choice.
I didn't say anything about lack of extensibility, from the pov that developers can subclass NativeActivity
if they need to (so GameActivity / NativeActivity are both similarly extensible) - but did highlight that it may be necessary to subclass NativeActivity
to access some platform features.
So although it's notably possible to use NativeActivity
without compiling any Java/Kotlin code, it's hopefully also clear that it may sometimes be necessary to subclass NativeActivity
.
I'm not super happy with how these notes are currently packed in this table though, since it squashes the feature name and it's awkward to format the notes. Maybe the notes should be split out into some pros/cons afterwards - not quite sure atm.
@@ -60,7 +62,7 @@ simple_logger = { version = "2.1.0", default_features = false } | |||
[target.'cfg(target_os = "android")'.dependencies] | |||
# Coordinate the next winit release with android-ndk-rs: https://github.com/rust-windowing/winit/issues/1995 | |||
ndk = "0.7.0" | |||
ndk-glue = "0.7.0" | |||
android-activity = "0.4.0-beta.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is a stable release here expectable soon-ish (in terms of a few weeks)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only thing I've generally been waiting for before releasing 0.4 is the green light that we're good to go here.
My original PR was based on a stable release but with the review here we picked up on the need to iterate the input API.
So just in case we spotted some other last-minute issue I went with a beta release.
I'll probably just roll the 0.4 release today while I'm not aware of any issue with the current API.
As far as ecosystem fragmentation goes, xbuild is compatible with both native activity and custom kotlin activities (manifest.yaml): android:
gradle: true
dependencies:
- 'androidx.appcompat:appcompat:1.4.1'
- 'androidx.games:games-activity:1.1.0'
manifest:
package: co.realfit.agdkegui
application:
has_code: true
theme: '@style/Theme.AppCompat.Light.NoActionBar'
activities:
- name: .MainActivity any code in the |
One remaining question would be, since the effort to port the native-app-glue to rust has been made, why does the game activity not make use of it? |
The For example There would be code that could be borrowed from the port of the glue code for
For a similar end-result my gut feeling is that we should instead plan to look at vendoring / forking |
About build system compatibility in general - I'm not quite sure how come that was listed as a con really. As far as I know this backend + There will be additional build system requirements for any native app that doesn't use It's a notable consideration when deciding whether to use |
This updates the Android backend to use the android-activity crate instead of ndk-glue. This solves a few issues: 1. The backend is agnostic of the application's choice of Activity base class 2. Winit is no longer responsible for handling any Java synchronization details, since these are encapsulated by the design of android_activity 3. The backend no longer depends on global / static getters for state such as the native_window() which puts it in a better position to support running multiple activities within a single Android process. 4. Redraw requests are flagged, not queued, in a way that avoids taking priority over user events (resolves rust-windowing#2299) To make it possible for application crates to avoid explicitly depending on the `android-activity` crate (and avoid version conflicts) this re-exports the android-activity crate under: `winit::platform::android::activity::*` This also adds `android-native-activity` and `android-game-activity` features that set the corresponding android-activity features. Addresses: PR rust-windowing#1892 Addresses: PR rust-windowing#2307 Addresses: PR rust-windowing#2343 Addresses: rust-windowing#2293 Resolves: rust-windowing#2299 Co-authored-by: Markus Siglreithmaier <m.siglreith@gmail.com>
83102af
to
c0888fe
Compare
Ah, I just meant fragmentation in general not specific to this PR as there is like |
Whoop! Thanks for merging @msiglreith! (and also the feedback given) Although I didn't end up making the stable release for android-activity the other day as I had suggested I'll look at that now and open up a PR to bump the dep to a stable 0.4 version. Hopefully can also start looking at follow up changes / PRs that I've been a bit reluctant to look at while this PR was pending. |
Just for reference here I made the 0.4.0 release today. Of course in the process there was a bit of a last minute issue I hit which essentially got highlighted by Clippy which took a bit of work to address and thankfully didn't affect the public API. |
This is a follow up on the discussion here: #2293
This updates the Android backend to use the android_activity crate instead
of ndk-glue. This solves a few issues:
GameActivity
also implies support forAppCompatActivity
based applications (not possible withNativeActivity
)request_redraw()
blockssend_event()
#2299)Probably the most notable change for applications is that this backend requires the use of the
EventLoopBuilderExtAndroid
trait to be able to associate anAndroidApp
with an event loop that is being built. This is dueandroid-activity
and this backend strictly avoiding the need for any global, static state. E.g. an application's entry point for Android may look something like:I feel that it's reasonable to have some platform specific requirements at this point, similar to how there are special requirements for web applications. Theoretically we could consider introducing some thread local state that could still allow for multiple activities running in a single process but it seems good to avoid that if not strictly necessary.
For reference work was recently done to support building Egui, EFrame applications based on
android-activity
and for that we added a general purpose mechanism for applications to configure platform specific EventLoopBuilder options: ref: emilk/egui@fb92434 and https://github.com/rib/android-activity/blob/5dab74466c53a764c9921c588f01a38edb2f18bc/examples/agdk-eframe/src/lib.rs#L25 (so I think in practice this requirement is manageable)Addresses: PR #1892
Addresses: PR #2307
Addresses: PR #2343
Addresses: #2293
Resolves: #2299
CHANGELOG.md
if knowledge of this change could be valuable to usersandroid-activity
crate itself has several examplesP.S Apologies that the patch does combine a few inter-related changes while re-working the backend (such as switching to a
sticky_exit_callback
utility function consistent with other backends) and these could potentially be split out if it's making the patch tricky to review for anyone, but otherwise I'm hoping to avoid the extra work at this point to split the patch up while it still seems reasonably OK to review 🤞