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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

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

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.

None yet

2 participants