Update To GamesActivity 2.0.2

[lexi-the-cute]

I updated to Games-Activity 2.0.2. I also removed android_app_input_available_wake_up as I could not find what the function was renamed too if it wasn't removed entirely.

I made some modifications such as adding _C at the end of a couple of functions to match what the previous version of android-activity did. This is just in case https://github.com/rust-lang/rfcs/issues/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.

[rib]

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 android_app_input_available_wake_up would have been for.

[rib]

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

[rib]

If you run git log and look for GameActivity PATCH you can find some of the changes against upstream which we need to check. Some of them related to things that I opened upstream issues for so might not be needed anyone but others where about being able to provide a consistent API between the native-activity and game-activity backends.

[rib]

Since the code moved around quite a bit since I originally wrote those patches then they no longer show up with git log --follow android-activity/game-activity-csrc which is unfortunate.

[rib]

For reference here I've just opened https://github.com/rust-mobile/android-activity/pull/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

[lexi-the-cute]

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.

[lexi-the-cute]

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 android_activity::game_activity and winit::platform_impl::platform.

Edit 2: Yep, I can filter it out. While it's still a bit noisy, but it looks like you can filter it out

[lexi-the-cute]

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 cargo fmt .... The third and last commit contains the patches I dug out of the previous commits. I had to pull some patches out of GameActivity.cpp and put them in GameActivityEvents.cpp and some patches could no longer be added either because the code that needed the patches no longer existed or the patches were already available in upstream. This just leaves the historical axes as the main patches as well as a few other things like enabling verbose logging. I have not checked that I sanely applied the patches. I've only checked to make sure my engine itself could run without crash and output the log messages I expected.

[rib]

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.

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 trace!() which is also the most verbose level - yeah you almost certainly don't want to be tracing crates outside your game engine by default - maybe not even for your engine by default.

[rib]

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 LICENSE file does highlight that the third-party AGDK code is Apache 2 only.

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.

[lexi-the-cute]

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 trace,android_activity=off,winit=off. I tried off,catgirl_engine=trace and variations to completely disable all logging other than my engine, but it just disables logging completely.

    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()),
        );
    }

 ✘ alexis@laptop  ~/Desktop/game/android   rewrite ±  adb logcat -v color | grep CatgirlEngine
06-26 14:58:10.929  3104  3129 D CatgirlEngine: main: Launched as Android app...
06-26 14:58:10.929  3104  3129 I CatgirlEngine: main::game: Starting Game...
06-26 14:58:10.929  3104  3129 V CatgirlEngine: main::game: Testing Trace...
06-26 14:58:10.930  3104  3129 D CatgirlEngine: main::game: Starting Main Loop...
06-26 14:58:10.934  3104  3129 W CatgirlEngine: winit::platform_impl::platform: TODO: forward onStart notification to application
06-26 14:58:10.936  3104  3129 D CatgirlEngine: winit::platform_impl::platform: App Resumed - is running
06-26 14:58:10.997  3104  3129 W CatgirlEngine: winit::platform_impl::platform: TODO: handle Android InsetsChanged notification
06-26 14:58:11.034  3104  3129 W CatgirlEngine: winit::platform_impl::platform: TODO: find a way to notify application of content rect change
06-26 14:58:11.265  3104  3129 W CatgirlEngine: winit::platform_impl::platform: TODO: handle Android InsetsChanged notification
06-26 14:58:11.288  3104  3129 W CatgirlEngine: winit::platform_impl::platform: TODO: handle Android InsetsChanged notification
06-26 14:58:11.291  3104  3129 W CatgirlEngine: winit::platform_impl::platform: TODO: find a way to notify application of content rect change
06-26 14:58:42.010  3104  3129 D CatgirlEngine: winit::platform_impl::platform: App Paused - stopped running
06-26 14:58:42.461  3104  3129 W CatgirlEngine: winit::platform_impl::platform: TODO: handle Android LowMemory notification
06-26 14:58:42.463  3104  3129 W CatgirlEngine: winit::platform_impl::platform: TODO: forward onStop notification to application
06-26 14:58:42.464  3104  3129 W CatgirlEngine: winit::platform_impl::platform: TODO: forward saveState notification to application
06-26 14:58:49.045  3104  3129 W CatgirlEngine: winit::platform_impl::platform: TODO: forward onStart notification to application
06-26 14:58:49.048  3104  3129 D CatgirlEngine: winit::platform_impl::platform: App Resumed - is running
06-26 14:58:49.062  3104  3129 W CatgirlEngine: winit::platform_impl::platform: TODO: handle Android InsetsChanged notification
06-26 14:58:49.157  3104  3129 W CatgirlEngine: winit::platform_impl::platform: TODO: handle Android InsetsChanged notification

[lexi-the-cute]

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

[rib]

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:

1. I'd like to double check the situation with regards to historic events - I had got the impression that upstream now has support for tracking historic touch events so I'd like to double check that we aren't adding duplicate support for history event tracking and I'm hoping we can simplify the changes we need.
2. It looked like this update doesn't currently port the InputAvailable changes that enable the GameActivity backend to deliver an event to the app for new, un-read input events, re: https://github.com/rust-mobile/android-activity/commit/3d1b1c5cb9ccfb47d8a067b78bea45c4b72e2033

[rib]

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:

• no longer need the various compilation fixes
• no longer need the LOG_TRACE change
• no longer need the scanCode change
• no longer need the historic motion event changes

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 android-activity crate. Technically from the Rust API pov this isn't a breaking change but it does require applications to pull in the corresponding 2.0.2 .aar release from Google. Jetpack libraries for Android are supposed to follow semver and so this represents a semver break on the Java side. We should probably treat this as a semver break for this crate.

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)

[rib]

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?

[tkkcc]

tested agdk-eframe using glow backend, works as before on genymotion android 11 emulator.

[rib]

@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.

[tkkcc]

i am not actively using the game-activity. i vote for minor version change. in my experience, a minor version change in any library could break my code. :) and i think this upgrade is not big enough for a major version change. and it's 0.x, developers know it's not stable. just my option.

[MarijnS95]

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 cargo update (or don't have Cargo.lock checked in in the first place) get a broken build until they update some external Java component, consider this as breaking?

[rib]

in my experience, a minor version change in any library could break my code. :) and i think this upgrade is not big enough for a major version change. and it's 0.x, developers know it's not stable.

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.

[MarijnS95]

I like to use the term "most-significant version/component" for that but it's not in the official docs :)

[rib]

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?)

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.

[rib]

I'd say if users that run cargo update (or don't have Cargo.lock checked in in the first place) get a broken build until they update some external Java component, consider this as breaking?

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.

[lucasmerlin]

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:

2023-07-29 16:58:10.166  2915-2915  AndroidRuntime          io.hellopaint.app                    E  FATAL EXCEPTION: main
                                                                                                    Process: io.hellopaint.app, PID: 2915
                                                                                                    java.lang.UnsatisfiedLinkError: dlopen failed: cannot locate symbol "android_get_device_api_level" referenced by "/data/app/io.hellopaint.app-Jyg5CDK2l469VHWuqRXXfw==/base.apk!/lib/arm64-v8a/libmain.so"...
                                                                                                        at java.lang.Runtime.loadLibrary0(Runtime.java:1016)

Commenting these lines in GameActivity.cpp makes it run successfully:

//    if (android_get_device_api_level() >= 26) {
//        GET_FIELD_ID(gConfigurationClassInfo.colorMode, configuration_class,
//                     "colorMode", "I");
//    }

    GET_FIELD_ID(gConfigurationClassInfo.densityDpi, configuration_class,
                 "densityDpi", "I");
    GET_FIELD_ID(gConfigurationClassInfo.fontScale, configuration_class,
                 "fontScale", "F");

//    if (android_get_device_api_level() >= 31) {
//        GET_FIELD_ID(gConfigurationClassInfo.fontWeightAdjustment,
//                     configuration_class, "fontWeightAdjustment", "I");
//    }

I guess the android_get_device_api_level function is not available on older android versions?

Now that it's running the update does actually seem to make the text input a bit more reliable.

[rib]

ah curious.

It looks like before android api level 29 (i.e. Android 10) then android_get_device_api_level is implemented as a static inline:

#if __ANDROID_API__ < 29

  /* android_get_device_api_level is a static inline before API level 29. */
  #define __BIONIC_GET_DEVICE_API_LEVEL_INLINE static __inline
  #include <bits/get_device_api_level_inlines.h>
  #undef __BIONIC_GET_DEVICE_API_LEVEL_INLINE
   
#else

Are you using cargo ndk to compile your native cdylib library?

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 -p 28 if building with cargo ndk)

[lucasmerlin]

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. Building with -p 20 or lower makes the build fail with error: cannot open crtbegin_so.o: No such file or directory The version of the ndk I'm using is 25.2.9519653, in case that is important.

[rib]

Can you maybe try adding a snippet of code like below to GameActivity.cpp:

#if __ANDROID_API__ < 29
#error "yes the toolchain is using an API level below 29"
#endif

to try and double check that the cc crate is ending up compiling the GameActivity C/C++ code with the required __ANDROID_API__ level

[rib]

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 GameActivity.cpp before api-level.h is included because that header will actually define __ANDROID_API__ if it's not already defined.

[rib]

Looking at api-level.h in the r25 NDK it still has the same guard like:

#if __ANDROID_API__ < 29

/* android_get_device_api_level is a static inline before API level 29. */
#define __BIONIC_GET_DEVICE_API_LEVEL_INLINE static __inline
#include <bits/get_device_api_level_inlines.h>
#undef __BIONIC_GET_DEVICE_API_LEVEL_INLINE

#else

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).

[lucasmerlin]

Putting it above #include GameActivity.h makes the build fail, but it fails even when I use -p 29 or don't specify it. If I put it between #include GameActivity.h and #include <android/api-level.h> the build is always successful.

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.

[rib]

Argh, it's a cargo-ndk bug, which is also my bad oops :/ https://github.com/bbqsrc/cargo-ndk/pull/115 🤦

@lucasmerlin assuming you're building for aarch64-linux-android then you should be able to workaround this by running cargo like:

CXXFLAGS=--target=aarch64-linux-android28 CFLAGS=--target=aarch64-linux-android28 cargo ndk ...

Or you could clone the branch of cargo-ndk from the PR linked above and run cargo install --path . to get a fix.

It would be good if you can confirm that fixes this issue for you.

[lucasmerlin]

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?).

[rib]

Cool, thanks for testing.

cargo-ndk doesn't know anything about your build.gradle config but the default just happens to be lower than 29 (21): https://github.com/bbqsrc/cargo-ndk/blob/main/src/meta.rs#L9