add support for properly providing the Android Activity to the runtime

[Timbals]

The context provided by ndk-context is not guaranteed to be an Activity context.

The android-activity crate has recently changed to providing an Application context to ndk-context instead (rust-mobile/android-activity#229), but XR_KHR_android_create_instance requires a reference to the Activity.

The new create_instance_android function requires the user to pass in the Activity reference themselves.

Trying to use the normal create_instance function will produce the following error, which points users to the correct function:

error[E0599]: no method named `create_instance` found for reference `&openxr::Entry` in the current scope                                                                                                                                                              
  --> main.rs:73:26
   |
73 |     let instance = entry.create_instance(
   |                    ------^^^^^^^^^^^^^^^
   |
help: there is a method `create_instance_android` with a similar name
   |
73 |     let instance = entry.create_instance_android(
   |                                         ++++++++

For more information about this error, try `rustc --explain E0599`.                                                                                                                                                                                                    
error: could not compile `engine` (lib) due to 1 previous error                                                                                                                                                                                                        
Error: Command `cargo build --target aarch64-linux-android` had a non-zero exit code.

The vulkan example didn't compile for Android anymore, because the ndk_glue::main proc-macro doesn't parse c string literals. ndk-glue was deprecated some time ago (rust-mobile/ndk#372), and should probably be replaced with android-activity.

[Timbals]

clippy failure in CI is due to a new Rust version and not related to the changes of this PR

[Ralith]

If the activity context is still global state, why bother plumbing it through rather than fetching it internally the way we did before? I'm not a huge fan of the API special-case required.

[Timbals]

There is no API to fetch the Activity internally. It's also not strictly global, as multiple activities can exist at the same time. The current activity could also get destroyed on configuration change, which would require re-creating the OpenXR instance with a new activity.
Admittedly an immersive OpenXR app doesn't really interact with the typical Android activity lifecycle in that way.

With the current implementation and android-activity 0.6.1 the session will never leave XR_SESSION_STATE_IDLE on Meta Quest devices.

[Ralith]

You have updated the example to use ndk_glue::native_activity().activity().cast(), which fetches an activity from global state. It's not clear to me why (something equivalent to) this couldn't be substituted in directly for the existing purely internal activity lookup that you've identified as broken.

[Timbals]

That's because the library doesn't depend on ndk-glue, only the examples do.
It also shouldn't depend on it, because that would force users of the library to use it for their glue code, when they should be using android-activity.

[Ralith]

Does android-activity have an equivalent call? Could we use it internally?

[Timbals]

It's AndroidApp::activity_as_ptr, but that requires the AndroidApp object, which is passed to the android_main entry point and not global.

[Ralith]

Thanks for clarifying; that's a good reason to plumb it through (assuming they have a good reason for structuring it that way, anyway).

I'm still not a fan of having different platform-specific entry points that must be cfg-gated by the caller, though it's not immediately obvious to me what we should do instead. Maybe some sort of instance builder? That would be a much more robust pattern for arbitrary extension support in general, at the cost of being a much larger change...

[Timbals]

I looked at how others deal with this:

• hello_xr example uses a IPlatformPlugin interface

• winit uses a builder, with platform gated extension trait and panics when that is not called.

• eframe takes the AndroidApp as part of an options struct and returns an error when that is not set.

I think the reasonable options are:

1. two different functions for Android and non-Android (currently implemented)
2. builder pattern
   • could be used to implement the debug messenger for XR_EXT_debug_utils as well
   • should the current arguments be split into smaller methods on the builder? (Builder::new(application_info) or Builder::new().application_info(application_info) or Builder::new().application_name(application_name).application_version(application_version))
3. introduce a PlatformInfo trait and always take a impl PlatformInfo
   • could be implemented for () on non-Android
   • struct AndroidPlatformInfo on Android, which is constructed with the activity JNI reference
4. always take a Option<*mut c_void>, and only require it to be Some on Android
   • this would require marking create_instance as unsafe for the Android case

I prefer option 3.

[Timbals]

@Ralith I moved the passing of the Activity reference to the already existing Entry::initialize_android_loader function. That keeps create_instance the same for all platforms.

That means multiple/restarting activities are not supported, but I think that's fine for an XR application.

[Ralith]

introduce a PlatformInfo trait and always take a impl PlatformInfo

Good idea; that matches what we're doing for graphics APIs. For clarity and forwards-compat, we could use empty structs for platforms that don't (yet) need anything. I guess this could basically be seen as builders with platform-specific types.

I moved the passing of the Activity reference to the already existing Entry::initialize_android_loader function

Hmm, I strongly prefer not to have hidden global state. Could this be a mutex-guarded field in Entry, at least? I think your other idea is ultimately the most attractive though.

In retrospect, I wonder why we have initialize_android_loader at all. From the doc it seems like we might as well call it on the user's behalf. Not an issue for this PR though.

[Timbals]

I think your other idea is ultimately the most attractive though.

Ok, i've implemented that now.
Notably, this could allow users to do arbitrary modifications to the xrCreateInstance call by implementing PlatformInfo themselves. Not sure if that's good, but I could seal the trait if we want to prevent that.

In retrospect, I wonder why we have initialize_android_loader at all. From the doc it seems like we might as well call it on the user's behalf.

The original PR mentions this was to not change the error type for Entry::load and Entry::load_from.

[Ralith]

Notably, this could allow users to do arbitrary modifications to the xrCreateInstance call by implementing PlatformInfo themselves. Not sure if that's good, but I could seal the trait if we want to prevent that.

I don't think there's anything wrong with that, so long as the safety invariants are clear. Preventing a sufficiently determined user from blowing their foot off is not a priority.

[Ralith]

The original PR mentions this was to not change the error type for Entry::load and Entry::load_from.

Perhaps we could do it in create_instance, though?

[Timbals]

The original PR mentions this was to not change the error type for Entry::load and Entry::load_from.

Perhaps we could do it in create_instance, though?

No, it has to precede all other calls to OpenXR, including Entry::enumerate_extensions and Entry::enumerate_layers.
I think this should be called implicitly when creating the Entry. Changing the error types should be fine, it's just a breaking change.

[Ralith]

I think this should be called implicitly when creating the Entry. Changing the error types should be fine, it's just a breaking change.

Yeah, reviewing the spec I think you're right. It was nice having Entry::linked be infallible; oh well.