Update To GamesActivity 2.0.2 #88
Conversation
|
Cool, thanks for looking at this. There were a number of patches on top of upstream we need to keep so I can try and check this. E.g. we added support for tracking keyboard scan codes for consistency with NativeActivity and the ability to wake up the event loop - which is what the |
|
I did initially follow a convention of putting 'PATCH' in the commit but probably need to have a better way of tracking changes against the upstream GameActivity code |
|
If you run |
|
Since the code moved around quite a bit since I originally wrote those patches then they no longer show up with |
| @@ -215,7 +233,7 @@ static void* android_app_entry(void* param) { | |||
| pthread_cond_broadcast(&android_app->cond); | |||
| pthread_mutex_unlock(&android_app->mutex); | |||
|
|
|||
| _rust_glue_entry(android_app); | |||
| android_main(android_app); | |||
There was a problem hiding this comment.
I think I'd expect a crash here atm, so I'm a bit surprised this is somehow working for you.
This should be calling _rust_glue_entry defined in src/game-activity/mod.rs which will then call the application's android_main Rust symbol.
Technically the android_main for the app could be a mangled symbol since it a "Rust" ABI symbol so I'm not quite sure what this is going to call currently.
It doesn't look like _rust_glue_entry was renamed to android_main (and I guess the compiler wouldn't let us have a Rust android_main at the same time as a "C" android_main even though they may technically map to different symbol names.)
There was a problem hiding this comment.
I will say, one thing I did notice between my fix pre-patches and post-patches. Pre-patches had the app visually stop displaying on the splash screen. I don't currently have any rendering code at all, so nothing is going to be drawn regardless. Post-patches, my screen actually shows up as a black screen where the only visible feature is my engine's name in the top left of the app.
I suspect this may be why, but I have not explicitly tested this
|
For reference here I've just opened #89 which digs up a few older notes I made about synchronizing with game-activity releases and I've tried to collate a set of patches that we should review when looking to pull in a new version of AGDK |
|
I'll be working on having my updates re-add the patches and fixes as per what you've mentioned to check. I'll also be working on making sure that the code passes the format PR check too. As for future work on updating GameActivity, generally what I see people do (usually due to necessity with licensing requirements), is they'll generate patch files with something like git and then have the build script download the original code and then apply those patch files during build. This'll allow only storing the changes made to the code, however, it'll make editing the code a little bit more complicated. |
|
I've gone ahead and manually reapplied the patches I still could. I'm not the world's biggest fan of these messages such as ALooper_pollAll and so on being visible and using my tag for my game engine. This makes it much harder for me too see my own log messages, so this is definitely something we want to move to a different tag if we can. I'll be restructuring my commits so that way we can more easily see the changes I've made pre-patches as well as the patches I've manually applied Edit: I guess I can filter on Edit 2: Yep, I can filter it out. While it's still a bit noisy, but it looks like you can filter it out
|
|
I finished creating the commits. The first commit is the pure unmodified Games-Activity source 2.0.2. This commit exists purely so one can see the difference between it and commit 2. The second commit are the changes I made to make android-activity work plus |
All these logs also go through the Rust log API and can be filtered at that level too. E.g. I use android_logger in these examples: https://github.com/rust-mobile/rust-android-examples/blob/3ad2b59852159fa98a33e1fd2575f0e89540ac6a/agdk-winit-wgpu/src/lib.rs#L302 and it's possible to filter out specific modules you're interested in for your game engine without necessarily needing to do that via logcat: https://docs.rs/android_logger/latest/android_logger/ The events like "Calling ALooper_pollAll, timeout =" are all emitted via |
|
Cool, thanks for taking another pass over this. Sounds good that you broke it down in terms of first adding the pristine upstream code so then we can easily see the changes we need. I don't think we need to worry about patching dynamically at build time. Although it's a shame that Google's AGDK is not dual MIT licensed, at least the Apache 2 license is OK. The top-level For the historical events I'm hopeful that should be redundant now - I'm fairly sure that upstream added support for historical events after I initially shared a proposal based on what I initially implemented for android-activity. It also looks like include scan codes upstream so we don't need a patch for that either. |
|
Cool! I'll definitely have to take a look into disabling that logging from my engine's end. If there's anything else you need me to do to the PR, let me know. Edit: For those wanting to limit, but not completely filter out the log messages from other crates such as android_activity and winit, I posted my code I tested below. A stronger filter is if cfg!(target_os = "android") {
#[cfg(target_os = "android")]
android_logger::init_once(
android_logger::Config::default()
.with_max_level(log::LevelFilter::Trace)
.with_tag("CatgirlEngine")
.with_filter(android_logger::FilterBuilder::new()
.parse("trace,android_activity=debug,winit=debug").build()),
);
} |
|
Is this good? I'm asking as I would like to be able to publish updates to my engine's crate and they have some kind of restriction which requires all sources to be other crates published with them |
|
Hey @alexisart - I'd like to take a bit of a closer look I think, so sorry if I don't manage to land this asap but I'm also interested in updating this. My initial thoughts are:
|
|
Hey @lexi-the-cute, I took another pass over this based on your patch to import the 2.0.2 source. Reviewing the previous changes, we:
The main feature change we still need is the support for waking up the Looper when new input events arrive. It's a bit awkward to decide what policy to follow when it comes to the semver of the I've tested this against several of the agdk examples here https://github.com/rust-mobile/rust-android-examples (including agdk-egui - though I have local changes to hide the titlebar that messes up the coordinate mapping) |
|
It would be good to hear if this works for you @lexi-the-cute and maybe you might also be able to give it a test @tkkcc? |
|
tested agdk-eframe using glow backend, works as before on genymotion android 11 emulator. |
|
@tkkcc I wonder if you have any opinion on whether we should consider this a semver breaking change for this crate - not sure if you're actively using the game-activity backend? Although it needs to be synchronized with a new version of GameActivity on the Java side, it's still tempting to be able to land this without needing a semver break for the android-activity crate. I'd e.g. like to start playing more with using the game-activity backend at Embark and that would be slightly easier if it remains compatible with the current release of Winit. |
|
i am not actively using the game-activity. |
|
Opinions don't mean much when there are standards that determine how to bump versions based on whether a release involves breaking changes. (which seems to be hard here given that the breaking change is outside of Rust code... but still necessary to make said Rust code work?) I'd say if users that run |
oh, I should clarify that I technically mean a 'minor' version bump since this crate hasn't made a 1.0 release. semver has special rules for versions below 1.0 whereby the significance of the version components effectively shift right and so a "minor" version bump is effectively for major/breaking changes. |
|
I like to use the term "most-significant version/component" for that but it's not in the official docs :) |
yeah, it's just that there's a bit of an ambiguity here because this doesn't break the API for the Rust crate. especially if we figure that we don't currently have many people actively using the game-activity backend then it might be a reasonable trade off to only look at the rust API in deciding what's semver compatible. In some ways it could be considered a bit like an MSRV bump where you have an orthogonal component that may need to be updated to continue building but your Rust code isn't going to need changing. |
This rule of thumb wouldn't e.g. hold for an MSRV change so it may still be reasonable that a more significant build change is required that's separate from the Rust API compatibility. Although technically on the Java side Google have bumped the semver for GameActivity I don't currently know why exactly, since it looks API compatible on the Java side from what I've seen so far. Again considering that I guess there aren't many people currently depending on this I would guess that current usage of GameActivity (in combo with android-activity) is also API compatible between these releases on the Java side. |
|
I gave this a try to see if it fixes some weirdness I have been experiencing with the text input I implemented, but it wouldn't run on my older tablet with android 9, I got the following error: Commenting these lines in GameActivity.cpp makes it run successfully: I guess the Now that it's running the update does actually seem to make the text input a bit more reliable. |
|
ah curious. It looks like before android api level 29 (i.e. Android 10) then Are you using If so what "platform" version are you specifying? It might be that you need to ensure you're configuring your ndk toolchain to use a lower API level (e.g. pass |
|
Yes, I'm using cargo ndk. I didn't specify a platform version beforehand, but when I try with -p 28 I get the same error. |
|
Can you maybe try adding a snippet of code like below to to try and double check that the |
|
oh and if you're able to try that out it looks like you'd need to put that check at the very top of |
|
Looking at so it looks like it should end up inlining the code to get the api version (and then that should mean that it doesn't try and resolve an exported symbol named "android_get_device_api_level" at runtime). |
|
Putting it above So I guess the -p flag doesn't really work as expected for me? Unfortunately I have no experience with c / cpp builds so sorry for being so clueless here. |
Although the changes in d6cdbf4 set up a `cflags_key` + `cxxflags_key` and values that would pass `--target=<triple><api-level>` to the compiler; these didn't actually get passed via `.env()` when building the command to run cargo. This means that the `cc` crate doesn't use the right api-level when compiling C/C++ code since it doesn't find the `CFLAGS_` and `CXXFLAGS_` that we intended to export. This recently caused an issue while testing the game-activity backend for android-activity on older versions of android because when targeting levels < 29 then any use of the `android_get_device_api_level` API needs to be inlined and that isn't currently happening - which leads to a runtime failure to lookup the symbol. Ref: rust-mobile/android-activity#88 (comment)
|
Argh, it's a @lucasmerlin assuming you're building for
Or you could clone the branch of It would be good if you can confirm that fixes this issue for you. |
|
I tried installing cargo ndk from your branch and this does indeed fix the issue! I don't even have to add the -p flag to the cargo ndk build (I assume because of the minSdk configuration in my build.gradle?). |
|
Cool, thanks for testing. cargo-ndk doesn't know anything about your |
Give C symbols that need to be exported a `_C` suffix so that they can be linked into a Rust symbol with the correct name (Since we can't directly export from C/C++ with Rust+Cargo) See: rust-lang/rfcs#2771
The real `android_main` is going to be written in Rust and android-activity needs to handle its own initialization before calling the application's `android_main` and so the C/C++ code calls an intermediate `_rust_glue_entry` function.
This makes a small change to the C glue code for GameActivity to send looper wake ups when new input is received (only sending a single wake up, until the application next handles input). This makes it possible to recognise that new input is available and send an `InputAvailable` event to the application - consistent with how NativeActivity can deliver `InputAvailable` events. This addresses a significant feature disparity between GameActivity and NativeActivity that meant GameActivity was not practically usable for GUI applications that wouldn't want to render continuously like a game.
This ensures that any java Activity callbacks take into account the possibility that the `android_app` may have already been marked destroyed if `android_main` has returned - and so they mustn't block and wait for a thread that is no longer running.
Although the changes in d6cdbf4 set up a `cflags_key` + `cxxflags_key` and values that would pass `--target=<triple><api-level>` to the compiler; these didn't actually get passed via `.env()` when building the command to run cargo. This means that the `cc` crate doesn't use the right api-level when compiling C/C++ code since it doesn't find the `CFLAGS_` and `CXXFLAGS_` that we intended to export. This recently caused an issue while testing the game-activity backend for android-activity on older versions of android because when targeting levels < 29 then any use of the `android_get_device_api_level` API needs to be inlined and that isn't currently happening - which leads to a runtime failure to lookup the symbol. Ref: rust-mobile/android-activity#88 (comment)
* This is needed after bumping egui to 0.25 as android-activity 0.5 needs newer games-activity: rust-mobile/android-activity#88
I updated to Games-Activity 2.0.2. I also removed
android_app_input_available_wake_upas I could not find what the function was renamed too if it wasn't removed entirely.I made some modifications such as adding
_Cat the end of a couple of functions to match what the previous version of android-activity did. This is just in case rust-lang/rfcs#2771 still held true.I tested this by loading my engine I'm currently working on and checking to see if it showed up in the ADB log. It did as per the below screenshot. I tested this through winit.