Igalia / wolvic

A fast and secure browser for standalone virtual-reality and augmented-reality headsets.
https://wolvic.org
Mozilla Public License 2.0
796 stars 100 forks source link

Add support for tracked keyboards via OpenXR's XR_FB_keyboard_tracking #1355

Closed elima closed 2 weeks ago

elima commented 5 months ago

This patch integrates Wolvic's BrowserWorld with the keyboard tracking functionality already present in OpenXR backend.

A build-time dependency on KTX-Software suite is added, to use libktx library which allows us to decode KTX2 textures (Oculus's OpenXR runtime is currently giving textures in KTX2 format via the render model extension).

A new class TrackedKeyboardRenderer is added to load the keyboard model using tinygltf and libktx; and draw the keyboard in the scene using a OpenGL(ES) directly. I tried to encapsulate the glTF model loading and rendering as much as possible, thinking in possibly making it a stand-alone, generic model class in the future; that can be added to vrb and used to replace all our other models. But that would require a significantly larger effort.

There is one issue which I spent quite some time debugging but didn't manage to solve: While in Wolvic, if the keyboard is turned on or off using its physical button, Wolvic closes. At first I thought it was a crash, but there is no sign of a crash in the traces (logcat), nor I found any error in the lifetime cycle of the objects that would point to a bug. So my conclusion is that the Oculus system is shutting down Wolvic whenever the keyboard connects/disconnects from the system.

elima commented 5 months ago

Just pushed a couple of commits addressing all comments. Thanks a lot for the review!

elima commented 5 months ago

BTW as the rest looks fine do please squash the review comments in the comments directly

Done, thanks!

elima commented 5 months ago
1. The crash when connecting the keyboard. We're indeed receiving APP_CDM_DESTROY from the OS not sure why. Perhaps because we don't handle the APP_CMD_INPUT_CHANGED ?

Good tip, I will investigate that path.

2. The keyboard is always shown if supported. That's bad because it's distracting. It should be enabled/disabled as the soft keyboard.

Right, this should be easy. Also, I realized that we should probably disable it also when in passthrough mode, wdyt?

As a bonus point, although not a blocker as the other two we should not show the soft keyboard if the hardware keyboard is available.

I will look into it too, after the other two issues.

Thanks!

svillar commented 5 months ago
1. The crash when connecting the keyboard. We're indeed receiving APP_CDM_DESTROY from the OS not sure why. Perhaps because we don't handle the APP_CMD_INPUT_CHANGED ?

Good tip, I will investigate that path.

2. The keyboard is always shown if supported. That's bad because it's distracting. It should be enabled/disabled as the soft keyboard.

Right, this should be easy. Also, I realized that we should probably disable it also when in passthrough mode, wdyt?

Good point! Let's do that too

elima commented 5 months ago
1. The crash when connecting the keyboard. We're indeed receiving APP_CDM_DESTROY from the OS not sure why. Perhaps because we don't handle the APP_CMD_INPUT_CHANGED ?

Good tip, I will investigate that path.

This is the exact sequence of events we are getting:

APP_CMD_PAUSE
APP_CMD_TERM_WINDOW
APP_CMD_TERM_WINDOW
APP_CMD_SAVE_STATE
APP_CMD_INPUT_CHANGED
APP_CMD_DESTROY

It is somewhat consistent with what's expected on Android when a system config changes, per this doc: https://developer.android.com/guide/components/activities/state-changes#cco

When a configuration change occurs, the activity is destroyed and recreated. This triggers the following callbacks in the original activity instance:

    [onPause()](https://developer.android.com/reference/android/app/Activity#onPause())
    [onStop()](https://developer.android.com/reference/android/app/Activity#onStop())
    [onDestroy()](https://developer.android.com/reference/android/app/Activity#onDestroy())

A new instance of the activity is created, and the following callbacks are triggered:

    [onCreate()](https://developer.android.com/reference/android/app/Activity#onCreate(android.os.Bundle))
    [onStart()](https://developer.android.com/reference/android/app/Activity#onStart())
    [onResume()](https://developer.android.com/reference/android/app/Activity#onResume())

Now, I think we don't support closing and re-launching a new activity, do we? I see we don't implement the SAVE_STATE command, which a-priori seems daunting considering all state the can be changed by the user.

I'm not sure how to proceed :/. On one hand, it would be great to support this life-cycle correctly, specially being able to save/restore the running state of the browser. On the other hand, it's hard to estimate this effort, but seems like it would be a large change.

WDYT?

I will be working on task 2. in the meantime.

svillar commented 5 months ago
1. The crash when connecting the keyboard. We're indeed receiving APP_CDM_DESTROY from the OS not sure why. Perhaps because we don't handle the APP_CMD_INPUT_CHANGED ?

Good tip, I will investigate that path.

This is the exact sequence of events we are getting:

APP_CMD_PAUSE
APP_CMD_TERM_WINDOW
APP_CMD_TERM_WINDOW
APP_CMD_SAVE_STATE
APP_CMD_INPUT_CHANGED
APP_CMD_DESTROY

It is somewhat consistent with what's expected on Android when a system config changes, per this doc: https://developer.android.com/guide/components/activities/state-changes#cco

When a configuration change occurs, the activity is destroyed and recreated. This triggers the following callbacks in the original activity instance:

    [onPause()](https://developer.android.com/reference/android/app/Activity#onPause())
    [onStop()](https://developer.android.com/reference/android/app/Activity#onStop())
    [onDestroy()](https://developer.android.com/reference/android/app/Activity#onDestroy())

A new instance of the activity is created, and the following callbacks are triggered:

    [onCreate()](https://developer.android.com/reference/android/app/Activity#onCreate(android.os.Bundle))
    [onStart()](https://developer.android.com/reference/android/app/Activity#onStart())
    [onResume()](https://developer.android.com/reference/android/app/Activity#onResume())

Now, I think we don't support closing and re-launching a new activity, do we? I see we don't implement the SAVE_STATE command, which a-priori seems daunting considering all state the can be changed by the user.

I'm not sure how to proceed :/. On one hand, it would be great to support this life-cycle correctly, specially being able to save/restore the running state of the browser. On the other hand, it's hard to estimate this effort, but seems like it would be a large change.

WDYT?

I will be working on task 2. in the meantime.

Nice findings.

We do support saving/restoring the state of the browser. It's "automatically" done when closing/starting Wolvic. We store the status of the sessions on an XML file and then reload it when starting if found.

That said I don't think we do manage pretty well the sequence of steps mentioned in the docs mainly because I am not sure we end up in a consistent status that allows us to handle an onCreate again. At least we could try to handle the SAVE_STATE and see what happens.

What I don't understand is why the activity is not recreated, because in my testing the app was closed but not restarted although I didn't check the logs, maybe it tries to restart it but it's unable to

svillar commented 1 month ago

I've just committed two changes. With those I think we're ready to merge:

svillar commented 3 weeks ago

Rebased & fixed conflicts