Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Sep 13, 2024

Conversation

elima
Copy link
Contributor

@elima elima commented Apr 12, 2024

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 elima self-assigned this Apr 12, 2024
Copy link
Member

@svillar svillar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great! I haven't had the chance to test it yet but I've reviewed the code. Everything seems pretty clean and easy to follow. I don't have an informed opinion on the shaders part but trust you on that regard.

Added some comments inline

app/src/main/cpp/TrackedKeyboardRenderer.h Outdated Show resolved Hide resolved
app/src/openxr/cpp/OpenXRInput.cpp Show resolved Hide resolved
app/src/main/cpp/DeviceDelegate.h Outdated Show resolved Hide resolved
app/src/main/cpp/TrackedKeyboardRenderer.cpp Outdated Show resolved Hide resolved
app/src/main/cpp/TrackedKeyboardRenderer.cpp Outdated Show resolved Hide resolved
app/src/main/cpp/TrackedKeyboardRenderer.cpp Outdated Show resolved Hide resolved
app/src/main/cpp/TrackedKeyboardRenderer.cpp Outdated Show resolved Hide resolved
app/src/main/cpp/TrackedKeyboardRenderer.cpp Outdated Show resolved Hide resolved
app/src/main/cpp/TrackedKeyboardRenderer.cpp Outdated Show resolved Hide resolved
app/src/main/cpp/TrackedKeyboardRenderer.cpp Outdated Show resolved Hide resolved
@elima
Copy link
Contributor Author

elima commented Apr 15, 2024

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

Copy link
Member

@svillar svillar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple of comments more inline.

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

app/src/main/cpp/TrackedKeyboardRenderer.h Outdated Show resolved Hide resolved
app/src/main/cpp/TrackedKeyboardRenderer.cpp Show resolved Hide resolved
@elima elima force-pushed the elima/XR_FB_keyboard_tracking-rendering branch from 30fe338 to c3d2d99 Compare April 16, 2024 07:08
@elima
Copy link
Contributor Author

elima commented Apr 16, 2024

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

Done, thanks!

Copy link
Member

@svillar svillar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I've now got the chance to test it.

I have to admit that the tracking is pretty accurate and that it's really simple to use the external keyboard. However after testing it I think we should address two very important issues before merging it in case you have time.

  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 ?
  2. The keyboard is always shown if supported. That's bad because it's distracting. It should be enabled/disabled as the soft keyboard.

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.

@elima
Copy link
Contributor Author

elima commented Apr 16, 2024

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
Copy link
Member

svillar commented Apr 16, 2024

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
Copy link
Contributor Author

elima commented Apr 17, 2024

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
Copy link
Member

svillar commented Apr 17, 2024

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 svillar marked this pull request as draft June 7, 2024 10:41
@svillar svillar force-pushed the elima/XR_FB_keyboard_tracking-rendering branch from c3d2d99 to 3d2556b Compare August 12, 2024 08:08
@svillar svillar marked this pull request as ready for review August 12, 2024 11:37
@svillar svillar force-pushed the elima/XR_FB_keyboard_tracking-rendering branch from 539b5a1 to 92da296 Compare August 12, 2024 11:43
@svillar
Copy link
Member

svillar commented Aug 12, 2024

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

  • Move the submodule to where the other submodules are
  • Allow Wolvic to handle keyboard configuration changes. This way the activity is not destroyed and recreated by the system

@svillar svillar dismissed their stale review August 12, 2024 16:06

Feedback was already addressed with the latest commits

elima and others added 4 commits September 4, 2024 10:48
We need 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).
This patch integrates Wolvic's BrowserWorld with the keyboard tracking
functionality already present in OpenXR backend.

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.
This way the activity is not destroyed and recreated every time the
connection of the hardware keyboard changes.
@svillar svillar force-pushed the elima/XR_FB_keyboard_tracking-rendering branch from 92da296 to c43428b Compare September 4, 2024 08:49
@svillar
Copy link
Member

svillar commented Sep 4, 2024

Rebased & fixed conflicts

Copy link
Contributor

@felipeerias felipeerias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. It works great.

There are small issues here and there, for example the beams are still visible while typing and sometimes you can have accidental clicks, but I think it is better to merge this initial PR and continue working.

@svillar svillar merged commit 3066aa0 into main Sep 13, 2024
22 checks passed
@svillar svillar deleted the elima/XR_FB_keyboard_tracking-rendering branch September 13, 2024 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants