-
Notifications
You must be signed in to change notification settings - Fork 65
Import android-games-sdk changes for GameActivity 4.0.0 #214
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
base: main
Are you sure you want to change the base?
Conversation
|
@jb55 and @nicoburns I wonder if either of you could ACK these initial patches to GameActivity related to IME support, considering you've both been looking at Android IME support recently. In particular, we will be able to build on this to expose the editor actions as InputEvents. (I've tested this and will open up a follow up PR for that which I can link here in a bit) |
Would it not be better to just handle this on the Rust side (even if they handler is just an empty match arm)? That would avoid having the fork the SDK for this? |
I wouldn't block on this, but I don't really like this. I feel like by the biggest problem with Rust's windowing libs atm is trying to abstract everything rather than just exposing the underlying platform functionality. Would it not be be better to just expose everything available, document any caveats and then let it be the users decision if something works for their use case or not? |
hmm, yeah one patch fewer could be good and we can just as well ignore it if we want on the Rust side for the sake of minimizing changes to upstream GameActivity code. |
I'm not so sure about this take. GameActivity is a bit of a special-case abstraction that's geared towards fullscreen games that I don't really consider it to be authoritative of underlying platform functionality. soft keyboard visibility is not something that the underlying Android SDK supports tracking and I'd say GameActivity is essentially doing exactly the thing you're framing as a problem here (trying to abstract instead of expose underlying functionality). In this case GameActivity is listening to inset changes and using that as a heuristic to try and infer when a soft keyboard is visible which is not something that the underlying platform supports tracking. So I suppose another way of putting it is that I kind of agree with you on not wanting extra abstraction and by not exposing this in the Rust API then we avoid exposing a flawed (imo) abstraction from GameActivity. Unfortunately GameActivity / NativeActivity make a bunch of trade offs to try and abstract in ways that may be convenient for fullscreen games but more generally speaking they also end up hiding platform functionality. The most notable thing probably being the assumption that your app only has one surface (aka ANativeWindow). The vague pipe dream here has been that ideally we'd create our own RustActivity that wouldn't necessarily make the same assumption that you're developing a fullscreen game and it could try and provide a thinner abstraction. |
I think maybe keep in mind here that this functionality was only added in GameActivity 4.0 and in the process of updating the game-activity backend to GameActivity 4.0, no real effort was put into exposing new public API for android_activity - the focus would have just been on updating the backend to the newer version. Maybe also keep in mind that even if it would be nice to expose everything (broadly speaking I probably do agree with your sentiment of just wanting there to be a minimal binding that doesn't add needless abstraction) it's still extra typing to actually expose new features and in this context we have to pick some way of reconciling how a feature can be exposed consistently (so if you build with the native-activity backend we need some graceful fallback or some feature needs to be optional etc) If there's a use case for exposing an android-activity event that represents a change in insets that may indicate a change in soft keyboard visibility then I don't have a particularly strong objection to exposing it via android-activity. So while there's currently no public android-activity API then the two easy options are 1) comment out the functionality in the GameActivity code or ignore it in the Rust code. Neither of these options preclude exposing an event later. For reference, #212 added an event (except that there's still no api to query the visibility value itself) and we could optionally go with that, but my concern with this is that I think this GameActivity abstraction is misleading. My inclination for now is to continue to ignore the |
How closely have you looked at https://github.com/rust-mobile/android-view ? (I see you posted an issue there, so I guess at least a little). I took a look last week, and got the examples in the repo running on my device. And while it definitely needs some polish, I'm impressed. It looks like it has pretty much everthing you'd need to build a Winit backend (I didn't go through with a fine tooth comb, but there wasn't anything obviously mising) + it allows users to own the Activity (even use their own subclass) + it is View-centric rather than Activity-centric, so there would be the possibility of using multiple Views within the same app + it has most-if-not-all of InputConnection exposed into user code, and the experience of typing in their demo is fantastic (auto-complete and such is all working perfectly). If you're having trouble navigating that repo (I was):
You can pretty much ignore the masonry bits, and just look at |
I did look over it a bit and initially was optimistic but I think my initial take away was that the idea of being a library (that is in some way closer to the metal) may not really deliver everything in the brochure if it means you end up having to manually implement the same kind of thread boundary that NativeActivity/GameActivity provides plus IPC for ANativeWindow updates if you want to write a native app that uses the GPU - as opposed to rendering with Android's UI framework. The general library + JNI approach is a perfectly good idea for integrating Rust into Android applications that makes a lot of sense when you just want to integrate business logic but things get more fiddly when you want to render with a GPU. There's a perf issue open for android-view which my gut tells me is very likely just going to come down to trying to render from Android's UI thread: rust-mobile/android-view#9 (but with the caveat that I haven't really looked closely enough at their architecture to confirm that this is what they're doing) Rendering on a GPU is essentially IO and you really don't want to try and render from Android's main/UI thread. Normally if you have a vanilla Android app then there is a separate Render thread provided by the system for rendering the UI, but if you're rendering with GLES or Vulkan it's your responsibility to spawn a separate thread for that. If you boil it down, the android_main thread that NativeActivity and GameActivity spawn can be thought of as a render thread and which pretty much any winit application would need to have. (that's over simplifying). So I guess that once you start introducing a render thread then you'll end up working back towards the NativeActivity/GameActivity design. Of course it would be solvable to spawn your own thread for rendering and come up with a way of forwarding ANativeWindow updates but one of the benefits of the NativeActivity approach is that your native app is similar to a native app on other OSs with a main() function, which can make it easier to port code designed to run across multiple platforms. That said I do also think NativeActivity + GameActivity have limited designs too - they were both designed with fullscreen games in mind and designed to facilitate porting code from platforms that have a main function that is responsible for its own blocking main loop. E.g It would be good if we had the means to create or bind to multiple views and then get a per-view channel for ANativeWindow updates, instead of assuming you only have one fullscreen view. It would also be good if native Rust applications could implement more than one Activity. Technically android-activity doesn't directly preclude this but it wouldn't be ergonomic to try and do and NativeActivity + GameActivity are not designed with that in mind. NativeActivity is also a special case that makes sense to support because it's the only solution for native applications that's shipped as part of Android SDK. In this context I'd also like a consistent solution for writing native Services in addition to Activities. Similar to how NativeActivity + GameActivity are geared towards fullscreen games - the way that android-view is focused around Views doesn't entirely feel like it's taking stock of how Android applications are generally based around Activities and Services and so, even with a library design that would try to support keeping more native code running in the UI thread (to make JNI easier) I think I would still imagine that it should revolve around Activity + Service classes. But, yeah all caveated with the fact that I haven't really looked super closely at android-view so there's definitely some chance I've jumped to some unfair conclusions. |
2551bb1 to
694bb4a
Compare
This imports the SDK from commit 8fa58b0e145ec28e726fa2b1c7e7a52af925ca35, from: https://github.com/rust-mobile/android-games-sdk/commits/android-activity-4.0.0 This includes one "notify android_main of editor actions" patch which will make it possible to forward editor actions and support IME Commit events in Winit) # notify android_main of editor actions This adds a pendingEditorActions member to android_app that is set via onEditorAction and the android_main thread is notified via notifyInput instead of re-instating APP_CMD_EDITOR_ACTION. The idea is that the android_main thread should check for android_app->pendingEditorActions whenever input events are polled/iterated. # FFI bindings update Also updates the FFI bindings via generate-bindings.sh
694bb4a to
8912aab
Compare
|
Okey, for reference, I've dropped the patch that removes the KB_VIS_CHANGED cmd. This was a bit more fiddly than initially expected because ignoring the cmd in Rust broke the invariant we had in poll_events where every cmd mapped to a MainEvent (and we have to be careful to still call android_app_pre_exec_cmd and android_app_post_exec_cmd when handling the cmd even if it doesn't invoke the given callback). I'll open a separate PR for this change. |
|
(didn't mean to send that yet) |
This imports the SDK from commit 8fa58b0e145ec28e726fa2b1c7e7a52af925ca35, from: https://github.com/rust-mobile/android-games-sdk/commits/android-activity-4.0.0
This includes one "notify android_main of editor actions" patch which will make it possible to forward editor actions and support IME Commit events in Winit)
notify android_main of editor actions
This adds a pendingEditorActions member to android_app that is set via onEditorAction and the android_main thread is notified via notifyInput instead of re-instating APP_CMD_EDITOR_ACTION.
The idea is that the android_main thread should check for android_app->pendingEditorActions whenever input events are polled/iterated.
FFI bindings update
Also updates the FFI bindings via generate-bindings.sh
Note: This is an internal-only change, it just pulls in a patche for the upstream GameActivity code that will enable us to expose IME editor actions.