-
-
Notifications
You must be signed in to change notification settings - Fork 576
Implement forwarding XR_EXT_user_presence from client to server to Steam #3052
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
base: master
Are you sure you want to change the base?
Conversation
ff8928e to
837760a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As you may have guessed, we used to support the proximity sensor. This was when we only supported Oculus headsets through the Oculus API.
I believe you should rename "user presence" to "proximity state"; despite being official terminology, it's a bit ambiguous. Before reading through it I thought it was some sort of co-location extension.
Since this PR adds a new packet, it needs bumping the -devXX version
|
Instead of using a proximity state packet, what if we allow connections only when the headset is worn, and disconnect it when it's removed? maybe we could do this ina future PR but it seems that it would not be too much work to do directly instead in this PR |
You mean disconnecting the client from the server? I would not do this. Even if the HMD is not worn it can still provide data. Like controller info or the position of the HMD. There might be use cases for this. Like doing calibration of the HMD positions by putting it at a specific spot and clicking a button in a seperate application when ready to read position data. Also re-establishing the connection might take longer than just sending the user presence packet. I would stay as close to the OpenXR spec there. It is conceptually an event there that can happen independently of the session. |
|
The reason I propose this it's because that's already what happens but in a more unreliable way. Once you remove the headset some seconds later (but it could be a bit longer) the session goes into idle and triggers the disconnection. Also, the user often wants to disconnect the client temporarily to apply certain settings that are not real-time |
|
That is not the case for me. When I remove the Quest 3 from my head it continues streaming. I can move it around and can see the output of the VR preview. The only thing that turns off when the proximity sensor isn't covered is the display of the HMD. |
|
You probably changed some system settings then. Then let's keep the separate events |
|
Yes. I disabled all sleep timers and did set them to max values. If somebody needs an explicit disconnect when taking off the headset it could be added later I think. |
|
Anything else I should tackle? Should I force push a cleaned up history or do you squash the things anyway when merging? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the client connects, the server always sets the proximity sensor to true. Then the client updates it. Do you think this is ok? usually the client connects when the user puts the headset on, but this is not always true when you disable sleeping.
No need, PRs are always squashed |
I was also thinking about this. I tried to implement adding a flag to the initial handshake message but it spiraled into changing so many things to forward the flag along with having to think about thread synchronization that I gave up on it to have something working quickly. I will make another attempt now that things have been settled. The gain might be that the worn state is not true for a couple of moments until the |
|
Ok. If you want try making this in a separate commit in this PR |
I added a commit with the changes and tested it. It does prevent having a wrong worn state for a couple of moments when unplugging and replugging the USB cable for me. I'm not entirely sure if an atomic bool was needed there but since there are multiple threads involved I wanted to do the safe thing. It's also more complexity now. What do you think? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the long wait, I didn't realize I had pending comments, I didn't submit the review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whilst just sending a proximity status update after the connection context finishes would do the trick to fix the potential desync with no other changes required, getting rid of it passing all the api layers related to connection having started would imo be a big improvement on this pr.
| // todo | ||
| } | ||
| xr::Event::UserPresenceChangedEXT(event) => { | ||
| alvr_common::info!("user present: {:?}", event.is_user_present()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this should be an info log, but a debug one (and also preferrably import it directly)
| .send(ServerCoreEvent::ClientConnected { | ||
| headset_is_worn: headset_is_worn_initial_state, | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I can't properly place this due to the way reviews work)
Also no event is sent when the client disconnects, that should automatically set the presence state to not present. (Maybe we should initialize the presence state to present before the initial presence state I proposed arrives, but I don't think that matters.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ope, forgot to click the box
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to submit review again
| ClientControlPacket::ProximityState(headset_is_worn) => { | ||
| ctx.events_sender | ||
| .send(ServerCoreEvent::ProximityState(headset_is_worn)) | ||
| .ok(); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Put this above the Reserved cases
|
@zmerp @The-personified-devil Maybe we can first get the general direction straight. I first had a version that just sends the ProximityState packet after reconnecting to get the initial state right. I did it this because I found the added complexity not worth the gain. Then I made another attempt to see what it would look like asking for zmerp's opinion. When the reiview continued without an mention of it I assumed zmerp would agree to the general appraoch and the review continued. IMHO the added complexity is not worth it but I don't have a strong opinion. So I'd just like to have some confirmation that this is what the two of you would prefer before going for more fixes of smaller details. |
|
Personally, for the sake of pushing this through, I would accept not merging the extra code for initialization correctness. This would be relevant only when launching the stream when the headset is being wore, which is a niche edge case. More than the value being flicked on and off, the thing I'm most concerned about is that there are no race conditions that make the after-initialization message being lost, which doesn't see to be the case anyway with the previous code. |
|
All main methods how SteamVR (and other VR runtime backends) communicate the proximity info to the app is based on events (just like the openxr source) and, from what I've read, it only actually triggers non presence events when there's also no movement. This means it's less like rapidly switching state and more like only delaying the event delivery, which seems like no big problem on it's own. I would however like to keep as close to a default state for the event when establishing a connection, which means setting the state at disconnection to not produce potentially rapid events on connection. If it's actually true by default this calls into question whether setting it as false on disconnection is a good idea, because we're supplying other information about the connection state that SteamVR probably also considers in sending the events. If the default state is false setting it to false on disconnection makes perfect sense. |
|
And I'm not sure what games would really do with that information anyway, except automatically entering the pause menu, so even if we get it wrong, this seems like something with fairly minor impact. Let's do the simple version and if we notice it's inadequate from user reports (which I don't expect, but eh) we can still think about using the more complex version later on (so maybe keep that commit around locally). |
This forwards the OpenXR 1.1 extension XR_EXT_user_presence to an application by:
/proximityof OpenVR to SteamVR/proximityinfo