native-activity: Propagate `NativeWindow` redraw/resize and `ContentRectChanged` callbacks to main loop #70

MarijnS95 · 2023-03-23T03:09:01Z

rib · 2023-03-29T15:07:16Z

Ah, yep, good catch.

MarijnS95 · 2023-03-29T15:34:14Z

@rib I don't recall whether game-activity suffers the same problem or if there are other callbacks that are also accidentally ignored... 😅

MarijnS95 · 2023-03-29T15:36:37Z

android-activity/android-activity/game-activity-csrc/game-activity/native_app_glue/android_native_app_glue.c

Lines 438 to 443 in f88f914

    
           static void onNativeWindowResized(GameActivity* activity, ANativeWindow* window, 
        
                                             int32_t width, int32_t height) { 
        
               LOGV("NativeWindowResized: %p -- %p ( %d x %d )", activity, window, width, 
        
                    height); 
        
               android_app_write_cmd(ToApp(activity), APP_CMD_WINDOW_RESIZED); 
        
           }

GameActivity forwards it in C.

rib · 2023-04-01T11:52:49Z

@rib I don't recall whether game-activity suffers the same problem or if there are other callbacks that are also accidentally ignored... 😅

Hopefully not.

I was all ready to write something like "told you so ;-p" (with less snark) here because I was going to guess that this was an unintentional regression from when I ported the android_native_app.c C code to Rust but it looks like actually the previous C code also didn't forward the resize event either.

It looks like my first incarnation of android-activity never used the very latest version of android_native_app.c.

We might want to review this patch to check if there are a few other ANativeActivity callbacks we should handle: https://android.googlesource.com/platform/ndk/+/15f35ce79c2455e015a91db2802651f67a1762e8%5E%21/

also: https://developer.android.com/ndk/reference/struct/a-native-activity-callbacks

rib · 2023-04-01T11:57:00Z

It looks like we also need to handle onNativeWindowRedrawNeeded and onContentRectChanged

MarijnS95 · 2023-04-01T12:27:46Z

@rib I don't recall whether game-activity suffers the same problem or if there are other callbacks that are also accidentally ignored...

Hopefully not.

See above, at least the resize is already handled.

I was all ready to write something like "told you so ;-p" (with less snark) here because I was going to guess that this was an unintentional regression from when I ported the android_native_app.c C code to Rust but it looks like actually the previous C code also didn't forward the resize event either.

And I would have said "just port ndk-glue" which already has all callbacks (or at least I hope so) and would be very similar in terms of forwarding events to the Looper.

It looks like my first incarnation of android-activity never used the very latest version of android_native_app.c.

We might want to review this patch to check if there are a few other ANativeActivity callbacks we should handle: https://android.googlesource.com/platform/ndk/+/15f35ce79c2455e015a91db2802651f67a1762e8%5E%21/

I was going to validate (probably by copy-pasting and sorting) whether we assign to all members of ANativeActivityCallbacks 😉

rib · 2023-04-01T14:02:34Z

android-activity/src/native_activity/glue.rs

@@ -396,6 +397,18 @@ impl WaitableNativeActivityState {
        });
    }

+    pub fn notify_window_resized(&self, native_window: *mut ndk_sys::ANativeWindow) {
+        let mut guard = self.mutex.lock().unwrap();
+        debug_assert_eq!(guard.window.as_ref().unwrap().ptr().as_ptr(), native_window);


I feel a little uneasy about asserting the value of the .window state here, which is conceptually owned by the android_main() thread, not the Java main thread - but I thiink this might be OK, due to the unique way that window state change are explicitly synchronized between the Java and android_main() thread.

It could be good to have a comment above this explaining why we think it's safe to assert this.

The assertion relies on the fact that set_window() (called by on_native_window_created and on_native_window_destroyed) will always synchronize .pending_window -> .window changes between the Java and android_main thread, and assumes the android_main() thread will never spontaneously / pre-emptively clear the .window to None.

It also assumes that the ANativeActivity doesn't do anything surprising and ever send a spurious resized event before onNativeWindowCreated or after onNativeWindowDestroyed. That seems like it should be a fair/logical assumption but also not 100% sure that's guaranteed.

Yeah exactly, this assert is here may we eventually queue up a pending_window which hasn't been adopted by the main loop yet, but a resize already arrives.

That should be fine though as the main loop will have to process TermWindow+InitWindow first, before picking up the window. If Android calls all these callbacks from a single Java thread (it does, right) we can't ever have set_window and notify_window_resized run concurrently anyway, and as you say set_window will block until the main thread adopted the window (i.e. running in lockstep at that point).

In other words pending_window should never be Some outside of set_window(), so we can't even check that hence the window must always be adopted by the main loop.

Then this assert is only validating that Android isn't sending us garbage.

Fwiw I think this is also going to be interesting with multiple activities, where we could solely use the NativeWindow pointer to distinguish windows. And could a single activity somehow have multiple windows (i.e. they also give us the pointer in onNativeWindowDestroyed which could be used for validation).

rib · 2023-04-01T14:03:47Z

android-activity/src/native_activity/glue.rs

+
+    pub fn notify_window_redraw_needed(&self, native_window: *mut ndk_sys::ANativeWindow) {
+        let mut guard = self.mutex.lock().unwrap();
+        debug_assert_eq!(guard.window.as_ref().unwrap().ptr().as_ptr(), native_window);


same initial thought as above - it could be good to at least add a comment explaining why we think it's safe to assert this.

rib · 2023-04-01T14:19:49Z

android-activity/src/native_activity/glue.rs

@@ -436,6 +449,12 @@ impl WaitableNativeActivityState {
        guard.pending_window = None;
    }

+    unsafe fn set_content_rect(&self, rect: *const ndk_sys::ARect) {
+        let mut guard = self.mutex.lock().unwrap();
+        guard.content_rect = *rect;


This is the first event where we have extra state that is passed side-band without synchronization and we maybe need to consider if this is OK like this.

Hypothetically I suppose you could end up with the Java thread buffering multiple commands (including multiple calls to set_content_rect) before they are picked up by the android_main() thread and the android_main() thread will always just see the most recent content_rect from the Activity.

That sounds like the ideal behaviour here though.

Yes they'd get buffered up, and we might want to "deduplicate" this event when it's in the pipe?

OTOH I figured this might be why you left these MainEvents as non_exhaustive:

android-activity/android-activity/src/lib.rs

Lines 203 to 204 in 36ddfaa

#[non_exhaustive]

ContentRectChanged {},

So that we could perhaps remove the getter altogether and only pass the value to the user once, letting them deal with that?

Also don't see if/how we could use pending_content_rect in this case unless also implementing special synchronization.

mockersf · 2023-04-15T14:38:33Z

I would love to see this one in a released version 🙂

I tested it on a few devices, it works for me and solves a few issues I have. Anything I can do to help it moving along?

rib · 2023-04-16T06:58:00Z

I can aim to have a look at landing this soon, yep. I might just add a few extra comments to the code as noted above, and/or remove the window assertion. The change itself looks good to me.

…ain loop

Some items were imported in scope while most of the code uses fully qualified names.

rib · 2023-04-19T12:44:37Z

thanks for the added comments @MarijnS95, will merge now

we can look at bumping the MSRV separately to address the current CI issue

MarijnS95 · 2023-04-19T12:45:49Z

Yeah cargo-ndk is rather aggressive wrt MSRV bumps and removing compatibility with older Rust/NDK for such a widely-relied-on tool.

mockersf · 2023-04-19T13:27:51Z

Could a version not bumping the MSRV with this PR be published? From a semver point of view, this can be published in a patch, the MSRV bump would need to be a minor version and require updating all the ecosystem to propagate...

MarijnS95 · 2023-04-19T13:39:26Z

@mockersf the solution is to stop using cargo-ndk.. Or install its v2 which is also ""maintained"" for everyone suffering these eager MSRV bumps.

Regardless, the issue is that the CI cannot build+install cargo-ndk itself, that MSRV has nothing to do with android-activity. However, if I remember correctly that the newer cargo-ndk 3.x also won't be able to compile projects with pre-Rust-1.68 (and maybe you can compile cargo-ndk with older Rust, it just uses the MSRV field to communicate toolchain/std compatibility when using it to compile for Android).

rib · 2023-04-19T16:35:50Z

Since Rust 1.68 bumped the version of the ndk that the standard library links with to version 25 there's some special significance for why the latest cargo ndk has made the choice to move to 1.68.

It probably is a good general baseline for any Android projects based on Rust at this point - and I would generally recommend that Rust Android projects all aim to move to at least NDK v25 and Rust 1.68+ too ideally.

The solution isn't to "stop using cargo-ndk" :) cargo ndk is a perfectly good tool for what it does.

In this case cargo ndk isn't even a dev-dependency for android-activity it's just used as part of the CI so we don't strictly need to bump the MSRV for android-activity if we don't want to - it was just my initial thought.

Yeah, it could be good to be able to do a patch release for this without bumping the msrv too aggressively - though at the same time 1.68 is particularly significant for Android.

In terms of semver I don't think msrv changes require an semver bump. Obviously it could be seen as bad etiquate to be inconsiderate of downstream msrv policies though.

Looking at the Bevy project it looks like the policy there is "the latest stable release":

MSRV: Bevy relies heavily on improvements in the Rust language and compiler.
As a result, the Minimum Supported Rust Version (MSRV) is "the latest stable release" of Rust.

and so since the latest stable release is 1.68, maybe you'd actually be ok with a 1.68 bump @mockersf ?

@MarijnS95: Besides taking a jibe at cargo ndk would you see a real problem with raising the msrv to 1.68, with the specific context that we're talking about Rust + Android development where it's not that likely to have entrenched projects at this stage that aren't able to follow stable Rust releases.

It's not that I want to follow an aggressive msrv policy here but it certainly is tempting to at least set the bar at 1.68, due to the android ndk update that comes along with that.

If we do have big concerns with bumping to 1.68 then we should be able to tweak the CI to just use an older version of cargo ndk I guess.

mockersf · 2023-04-19T21:52:19Z

I don't have any problem with bumping the MSRV as high as you want, 1.68 is good for me. As you noticed, Bevy is "latest stable".

I'm mostly hoping for a patch release because in a strict SemVer interpretation, any bump of the MSRV is a breaking change, so at least a minor release (since this crate is still in the 0.X range). And that would mean I'll have to wait for a winit update before it's usable in Bevy.

As cargo-ndk isn't a dependency of this crate, its update (and so the MSRV bump) is not strictly needed for a release. Having a MSRV of 1.68 is still better when using it.

It depends on how strict you want to be with SemVer.

MarijnS95 · 2023-04-20T12:15:00Z

Just for good measure, we're all saying the exact same thing: cargo-ndk is not a dependency of android-activity so their MSRVs are strictly unrelated.

In terms of semver I don't think msrv changes require an semver bump. Obviously it could be seen as bad etiquate to be inconsiderate of downstream msrv policies though.

I've seen various interpretations of this. Personally I mostly align with the view that an MSRV bump should be breaking, unless the new MSRV is over 6 months old. It is always super annoying if external crates/tools bump their MSRV too eagerly, breaking downstream consumer projects and/or their CI (as is happening here... though because IIRC we install the latest cargo-ndk rather than requiring v2, which is totally fine if such breaks are incidental).

In our case we have to watch what winit is going to set and expect. For the time being I'd be totally happy not forcing users to compile the latest android-activity against 1.68+, simply to not force an arbitrary virtual barrier. You say using Rust with NDK r25 is better, but besides build-system annoyance to get it to build and link against r25 (the workarounds cargo-apk, xbuild and later also cargo-ndk carried) I don't see any major issue in allowing some backwards compatibility.

TL;DR:

Make a patch release with this PR to serve winit customers now;
Leave MSRV where its at unless android-activity stops compiling (i.e. needs newer compiler features);
OR: if we come up with a tangible reason to force a higher 1.78 MSRV now, do a breaking release with it.

NiklasEi · 2023-06-22T13:58:20Z

Make a patch release with this PR to serve winit customers now;

As far as I can see there has not been a release with this bug fix. Are you still planning a patch release?

MarijnS95 · 2023-06-22T19:40:04Z

@NiklasEi FWIW that was a suggestion, not an action as I am not the release manager for android-activity 😉

rib · 2023-06-22T20:54:42Z

Yeah, sorry, I'd like to make a release of android-activity too, though I also had a few things I was hoping to look at addressing for a next release so held off a little bit.

One thing I had wanted to do was update all the rust-android-examples and smoke test the Activity.finish() change which I've basically done - I just need to disable to the openxr example in CI for now.

I'd also like to have an api that exposes the equivalent of getUnicodeChar which we really need in Winit but that's going to be a bit more fiddly than I was initially expecting so can probably let that slide for now.

MarijnS95 · 2023-06-25T21:03:51Z

@rib fwiw we could have a simple patch release soon-ish and plan the next breaking release (will we need/have any?) right before the next winit drop which is supposedly scheduled for August.

rib · 2023-06-27T16:56:21Z

Okey, just published a 0.4.2 release - sorry for the delay with releasing here: https://github.com/rust-mobile/android-activity/releases/tag/v0.4.2

MarijnS95 changed the title ~~native-activity: Propagate NativeWindow resize event to main loop~~ native-activity: Propagate NativeWindow redraw/resize and ContentRectChanged callbacks to main loop Apr 1, 2023

MarijnS95 requested a review from rib April 1, 2023 13:16

rib reviewed Apr 1, 2023

View reviewed changes

MarijnS95 mentioned this pull request Apr 7, 2023

Wrong size is being reported by ScaleFactorChanged on Android some times rust-windowing/winit#2760

Closed

mockersf mentioned this pull request Apr 7, 2023

Sizing problem between desktop and mobile (UI/2D) bevyengine/bevy#8322

Closed

mockersf approved these changes Apr 15, 2023

View reviewed changes

MarijnS95 and others added 4 commits April 19, 2023 14:09

native-activity: Propagate onNativeWindowResized callback to main loop

a04c483

native-activity: Propagate onNativeWindowRedrawNeeded callback to m…

87fe6a8

…ain loop

native-activity: Propagate onContentRectChanged callback to main loop

0c3f16c

Clean up partial module imports

fe9d68c

Some items were imported in scope while most of the code uses fully qualified names.

MarijnS95 force-pushed the na-resize branch from 20212a6 to fe9d68c Compare April 19, 2023 12:09

rib merged commit 03b06b8 into rust-mobile:main Apr 19, 2023

MarijnS95 deleted the na-resize branch April 19, 2023 12:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

native-activity: Propagate `NativeWindow` redraw/resize and `ContentRectChanged` callbacks to main loop #70

native-activity: Propagate `NativeWindow` redraw/resize and `ContentRectChanged` callbacks to main loop #70

MarijnS95 commented Mar 23, 2023 •

edited

Loading

rib commented Mar 29, 2023

MarijnS95 commented Mar 29, 2023

MarijnS95 commented Mar 29, 2023

rib commented Apr 1, 2023

rib commented Apr 1, 2023

MarijnS95 commented Apr 1, 2023 •

edited

Loading

rib Apr 1, 2023

MarijnS95 Apr 19, 2023

MarijnS95 Apr 19, 2023

rib Apr 1, 2023

rib Apr 1, 2023

MarijnS95 Apr 19, 2023

MarijnS95 Apr 19, 2023

mockersf commented Apr 15, 2023

rib commented Apr 16, 2023

rib commented Apr 19, 2023

MarijnS95 commented Apr 19, 2023

mockersf commented Apr 19, 2023

MarijnS95 commented Apr 19, 2023 •

edited

Loading

rib commented Apr 19, 2023

mockersf commented Apr 19, 2023 •

edited

Loading

MarijnS95 commented Apr 20, 2023 •

edited

Loading

NiklasEi commented Jun 22, 2023

MarijnS95 commented Jun 22, 2023

rib commented Jun 22, 2023

MarijnS95 commented Jun 25, 2023

rib commented Jun 27, 2023

native-activity: Propagate NativeWindow redraw/resize and ContentRectChanged callbacks to main loop #70

native-activity: Propagate NativeWindow redraw/resize and ContentRectChanged callbacks to main loop #70

Conversation

MarijnS95 commented Mar 23, 2023 • edited Loading

rib commented Mar 29, 2023

MarijnS95 commented Mar 29, 2023

MarijnS95 commented Mar 29, 2023

rib commented Apr 1, 2023

rib commented Apr 1, 2023

MarijnS95 commented Apr 1, 2023 • edited Loading

rib Apr 1, 2023

Choose a reason for hiding this comment

MarijnS95 Apr 19, 2023

Choose a reason for hiding this comment

MarijnS95 Apr 19, 2023

Choose a reason for hiding this comment

rib Apr 1, 2023

Choose a reason for hiding this comment

rib Apr 1, 2023

Choose a reason for hiding this comment

MarijnS95 Apr 19, 2023

Choose a reason for hiding this comment

MarijnS95 Apr 19, 2023

Choose a reason for hiding this comment

mockersf commented Apr 15, 2023

rib commented Apr 16, 2023

rib commented Apr 19, 2023

MarijnS95 commented Apr 19, 2023

mockersf commented Apr 19, 2023

MarijnS95 commented Apr 19, 2023 • edited Loading

rib commented Apr 19, 2023

mockersf commented Apr 19, 2023 • edited Loading

MarijnS95 commented Apr 20, 2023 • edited Loading

NiklasEi commented Jun 22, 2023

MarijnS95 commented Jun 22, 2023

rib commented Jun 22, 2023

MarijnS95 commented Jun 25, 2023

rib commented Jun 27, 2023

native-activity: Propagate `NativeWindow` redraw/resize and `ContentRectChanged` callbacks to main loop #70

native-activity: Propagate `NativeWindow` redraw/resize and `ContentRectChanged` callbacks to main loop #70

MarijnS95 commented Mar 23, 2023 •

edited

Loading

MarijnS95 commented Apr 1, 2023 •

edited

Loading

MarijnS95 commented Apr 19, 2023 •

edited

Loading

mockersf commented Apr 19, 2023 •

edited

Loading

MarijnS95 commented Apr 20, 2023 •

edited

Loading