[OpenXR] Remove simple khronos interaction profile #1431
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
svillar
force-pushed
the
remove_simple_khronos_profile
branch
from
39a9df7
to
944c530
Compare
|
Moved to draft as there might be a simpler solution that does not require so many changes. |
|
After reevaluating the alternatives I've decided to push for this solution again. We can make Meta work by simply not adding the simple profile by default, but this PR is a much more generic fix that allows us to, among other things, get rid of the nasty code of emulated interaction profiles. That code was added to circumvent the issue of hand tracking not working just after launching Wolvic because there was no active input mapping. Note that the runtime does only emit the update interaction profile event for physical controllers. The main issue with that code is that hand tracking works not because we set something up in the runtime, but just because we can trick the Wolvic code to believe that there is an active interaction profile (input mapping). |
felipeerias
approved these changes
Jun 10, 2024
There was a problem hiding this comment.
LGTM. This fixes #1430 without introducing any regressions as far as I could test. The code looks good as well.
Setting the correct device type is important among other things to load the proper 3D models for controllers, set the right input mappings or select the correct WebXR input profile. We used to pick the device type from the mapping, but that is not a good idea because in the future we could have input mappings shared by different devices. Also there might be the case that a device has no input mapping (like devices without controllers). For that reason it's better to store the device type that we retrieve from the system and use it.
Currently the beam for the controllers is created only after the controllers are in a "ready" state. That can vary among backends but for example in the case of OpenXR that happens only after having a valid interaction profile for the controllers. That makes sense for devices with physical controllers with interaction profiles but does not apply to hand tracking only devices that might not have interaction profiles. In reality, to create the beam, we do only need the ControllerDelegate objects to be created which normally happens very early (at the DeviceDelegate creation/initialization). That's why we split the beam initialization and controllers' 3D model loading in two different callbacks. The 3D model will still happen in the ready callback but the beam initialization is moved to an early stage, to the create callback. By proceeding this way we will always create the beam even without interaction profiles, and for devices with both physical controllers and hand tracking, the 3D model loading will still work even if Wolvic is started in hand tracking mode.
Interaction profiles define a collection of buttons and other input sources (like poses) in a physical arrangement which allow apps to set mappings between those input sources and actions. For the case of hand tracking, we don't really need any interaction profile yet as we're using extensions to gather hand data and emulate a controller. The current code assumed that we must always had an active input mapping. We were workarounding it by using the default Khronos interaction profile for devices without controllers. But that is not really needed. The right fix is to allow the code to run without any active interaction profile.
As we had to always have an active interaction profile, for devices using hand tracking we had to provide an emulated profile to be used. As we no longer require an active profile, we can get rid of that nasty code.
We do have interaction profiles for all the supported devices with physical controllers. We don't really need it for the hand tracking only devices which should work fine after the previous work that allows Wolvic to run without any active mapping. Allowing multiple interaction profiles has some drawbacks and thus it's better to remove it. For example in the case of Meta devices, if the Khronos simple interaction profile is enabled, then we start to receive not valid hand tracking data (like all the flags being zero). Fixes #1430
svillar
force-pushed
the
remove_simple_khronos_profile
branch
from
944c530
to
40e957e
Compare
javifernandez
approved these changes
Jun 10, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR fixes #1430 by removing the simple khronos interaction profile which is causing unexpected behaviours and interactions with other interaction profiles and hand tracking. In particular when enabled in Meta devices this profile
causes all the hand tracking data to become zero, effectively disabling it.
Removing it is not as simple as it might look like. The current code heavily depend on having an active OpenXR interaction profile (also called input mapping in the code) which does make sense for devices with physical controllers. However devices with no controllers should not need any. That's why in order to remove that profile we had first to remove that constraint which required several previous steps, namely:
The nice thing about this sequence of patches is that the fix comes in the last patch but none of the previous ones break hand-tracking only devices as the Lynx R1 or the Lenovo A3