refactor(client_openxr): ♻️ Do not recreate OpenXR session #2519
Conversation
zmerp
force-pushed
the
no-session-recreation
branch
from
c73ecf7
to
4986cb3
Compare
| let resume_info = SimultaneousHandsAndControllersTrackingResumeInfoMETA { | ||
| ty: *TYPE_SIMULTANEOUS_HANDS_AND_CONTROLLERS_TRACKING_RESUME_INFO_META, | ||
| next: ptr::null(), | ||
| }; | ||
| unsafe { | ||
| super::xr_res(resume_simultaneous_hands_and_controllers_tracking_meta( | ||
| session.as_raw(), | ||
| &resume_info, |
There was a problem hiding this comment.
I think it would probably be better to only make this create a struct that can both resume and pause and not automatically activate it, would probably make the logic cleaner by actually shifting handling responsibility where it belongs
There was a problem hiding this comment.
If possible i'd avoid any global state. But i agree that since the api is made of global calls, once creating two instances everything falls apart. I'm a little afraid that using a global state would still cause concurrency issues
There was a problem hiding this comment.
is it really global tho? it's specific to the session because it acts upon a specific session
if you make it so you have to pass the session into the pause/start function then that's even more emphasized
or do you have to guarantee that the function is only used with the same session the pointer was grabbed from?
There was a problem hiding this comment.
The action_set becomes bound to the session, so best not to use a different session with the same InteractionContext.
Action sets are application-defined collections of actions. They are attached to a given XrSession with a xrAttachSessionActionSets call. They are enabled or disabled by the application via xrSyncActions depending on the current application context.
Once a action set is bound to a session, if the session gets destroyed, the action set needs to be recreated. But what we can do is creating multiple action sets, local to Lobby and StreamContext. This is a possible direction. One disappointing limitation is that you can call xrAttachSessionActionSets only once, so we can't defer creating an action set when the client connects to the server:
The runtime must return XR_ERROR_ACTIONSETS_ALREADY_ATTACHED if xrAttachSessionActionSets is called more than once for a given session.
So in the end having multiple action sets seems to not improve things. We could instead initialize separately the other inputs not connected to action sets (eye/face/body tracking). But given that there are unintuitive limitations about (for example) concurrent use of multimodal and body tracking, having one centralized system of managing inputs seems the best approach.
There was a problem hiding this comment.
We don't even have to modify any action sets tho? Face + body + eye tracking don't use that iirc and we always register the multimodal action set even if we don't actually unpause it, so that shouldn't be a problem
There was a problem hiding this comment.
Yeah, so if we want to implement "handles" that hold this kind of functionality we have to handle the edge cases like multimodal. So the idea would be to keep InteractionContext, but add a function to create a InteractionSourcesHandle that when dropped removes the requested input functionality. But with this there are still issues. IF we request multimodal for lobby, then when streaming without body tracking, multimodal will still be enabled for stream. This is because multimodal is a feature affecting the behavior of other inputs (of input/grip/pose and hand tracking active states). moreover, multimodal also halves hand and controller refreshrates (that's how it's implemented internally) which is definitely not good to enable always. For this reason, in the end I believe having plain stateful select_sources() call is better and the correct usage responsibility gets elevated to lobby.rs, stream.rs and lib.rs
There was a problem hiding this comment.
ok so TLDR, input functionality is not always "additive", so we cannot implement independent handles to hold the extra functionality.
| fn drop(&mut self) { | ||
| let pause_info = SimultaneousHandsAndControllersTrackingPauseInfoMETA { | ||
| ty: *TYPE_SIMULTANEOUS_HANDS_AND_CONTROLLERS_TRACKING_PAUSE_INFO_META, | ||
| next: ptr::null(), | ||
| }; | ||
| unsafe { | ||
| super::xr_res((self | ||
| .pause_simultaneous_hands_and_controllers_tracking_meta)( | ||
| self.session.as_raw(), | ||
| &pause_info, | ||
| )) | ||
| .ok(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Also don't think implementing drop is worth it, it seems more advantageous to just remain in the last state.
There was a problem hiding this comment.
Yeah so this means going back to global state...
alvr/client_openxr/src/lib.rs
Outdated
| .select_sources(InteractionSourcesConfig { | ||
| face_tracking: None, | ||
| body_tracking: None, | ||
| prefers_multimodal_input: true, |
There was a problem hiding this comment.
don't really see the reason for this? multimodal in lobby is useless and if we can toggle at any time like I suggest then we can only enable it when the server communicated that it wants to and save some perf
There was a problem hiding this comment.
Actually this is used in another branch, i haven't made a PR yet. Basically we poll the detached controllers and show them in the lobby, with a gray color
| pub action_set: xr::ActionSet, | ||
| pub button_actions: HashMap<u64, ButtonAction>, | ||
| pub hands_interaction: [HandInteraction; 2], | ||
| pub uses_multimodal_hands: bool, | ||
| pub multimodal_hands_handle: Option<MultimodalHandsHandle>, |
There was a problem hiding this comment.
This would need to be changed when doing my suggestion to an extra function that actually checks whether it's active
|
Also do all the additional sources get unregistered when the client goes back from using them to lobby to another streaming session not using them? |
They don't. Maybe we should do that. technically it's not that hard since we can call |
Avoid the need to restart the OpenXR session. This reduces the chances of connection issues. This PR is needed to move towards Vive eye/face tracking support that was previously disabled.
Combined eye gaze support is removed for Quest and Vive. For Pico, we force enable combined eye gaze.
Now the
InteractionContextis always initialized with multimodal tracking enabled, and disabled after connection if it's disabled for server.