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

Handle STOPPING state correctly #109

Closed
wants to merge 1 commit into from
Closed

Conversation

@Manishearth
Copy link
Member

Manishearth commented Dec 16, 2019

Tapping the start menu button works now. It does hide the app, but this is how openxr expects you to handle the stopping state.

One thing that doesn't work right now is that when you hit the home button on the start menu, we do not get any events, nor do we get any events when you tap the greyed-out servo window. This puts the session in a state where it's idle but can't be told that it should wake up again. Not quite sure what to do here.

Also, doing this completely inside handle_openxr_events isn't great; because now we can't receive quit events. It might be worth working this into the larger webxr event loop, though that would involve some tricky control flow.

Fixes #76

Progress towards servo/servo#24722, won't fix that issue until we can exit the session when you tap home.

r? @jdm @paulrouget

@Manishearth Manishearth force-pushed the Manishearth:stopping branch from c9c0344 to dacb9b7 Dec 17, 2019
@Manishearth
Copy link
Member Author

Manishearth commented Dec 17, 2019

One thing that doesn't work right now is that when you hit the home button on the start menu, we do not get any events, nor do we get any events when you tap the greyed-out servo window. This puts the session in a state where it's idle but can't be told that it should wake up again. Not quite sure what to do here.

According to Bryce this is a bug in the runtime, we should be getting READY when you tap the servo window. They hope to have this fixed by the next release (Jan).

@@ -347,6 +367,10 @@ impl OpenXrDevice {
Some(_) => {
// FIXME: Handle other events
}
None if stopped => {
// wait a bit before repolling for events
thread::sleep(Duration::from_millis(200));

This comment has been minimized.

@Manishearth

Manishearth Dec 17, 2019

Author Member

Actually I'm not sure this is a good idea, this means the XR thread is indefinitely blocked while we're in the IDLE state, and since the XR thread is the main thread right now (@asajeffrey, can we change this for Hololens?) this means we're blocking the main thread too

This comment has been minimized.

@asajeffrey

asajeffrey Dec 17, 2019

Member

Yeah, this doesn't look great. I don't know if we can run the HL AR APIs off the main thread, it would be nice if we could.

This comment has been minimized.

@Manishearth

Manishearth Dec 17, 2019

Author Member

Pretty sure we can, there's no documentation about main thread affinity

This comment has been minimized.

@jdm

jdm Dec 17, 2019

Member

I specifically asked about thread affinity back when we were starting openxr and was told that openxr can run on any thread on the hololens.

@Manishearth
Copy link
Member Author

Manishearth commented Dec 17, 2019

Also, doing this completely inside handle_openxr_events isn't great; because now we can't receive quit events. It might be worth working this into the larger webxr event loop, though that would involve some tricky control flow.

So I'm thinking about this and I'm not really enjoying any of the approaches available:


Approach 1: Make the XR event loop handle this

We can split out poll_for_events() out of wait_for_animation_frame(), and have that signal state changes. When any device goes into an idle state, this stops handling frame requests, and instead only handles other messages as well as doing the "occasionally poll for events" that we're doing here.

This involves changing stuff all the way up to run_one_frame, which is ugly.


Approach 2: New thread

Spawn a new thread that loops and polls for events, and put the main thread in "don't listen to raf calls" mode where it just remembers all the raf calls it gets. This is ... okay, but it's ugly since a lot of OpenXR state stuff leaks into webxr-api.


Approach 3: double quit channel

The only thing we care about is the quit message, everything else can wait until the next frame. Perhaps we instead quit() send two messages, one which goes directly to the device which can be checked by the device. We can then use an approach similar to here. We'd still have to figure out how not to block the main thread.

@asajeffrey thoughts?

Copy link
Member

asajeffrey left a comment

If we can run the HL AR APIs off the main thread, would this get round all of this? In the medium term, we should probably revisit the embedding story to move as much of Servo as we can off the main thread.

@Manishearth
Copy link
Member Author

Manishearth commented Dec 17, 2019

@asajeffrey running things off main thread would let us pick Option 3, which is the cleanest.

@bors-servo
Copy link
Contributor

bors-servo commented Mar 5, 2020

The latest upstream changes (presumably #133) made this pull request unmergeable. Please resolve the merge conflicts.

@jdm
Copy link
Member

jdm commented Mar 5, 2020

If we can run the HL AR APIs off the main thread, would this get round all of this? In the medium term, we should probably revisit the embedding story to move as much of Servo as we can off the main thread.

This question is doubly interesting now that we are running openxr in its own thread.

@Manishearth
Copy link
Member Author

Manishearth commented Mar 6, 2020

@jdm now that we've done this, approach 3 can Just Work, though i'm not super happy with it

@Manishearth Manishearth force-pushed the Manishearth:stopping branch from dacb9b7 to 3f3fcb2 Mar 25, 2020
@Manishearth Manishearth force-pushed the Manishearth:stopping branch from 3f3fcb2 to 5606811 Mar 25, 2020
@Manishearth
Copy link
Member Author

Manishearth commented Mar 25, 2020

Oops, reused a branch name

@Manishearth
Copy link
Member Author

Manishearth commented Mar 25, 2020

Superseded by #139

bors-servo added a commit that referenced this pull request Mar 25, 2020
openxr: Handle STOPPING event and visibility state changes

The runtime now correctly triggers the `READY` state, and we're also now at a reasonable FPS that the `visible-blurred` state actually can occur.

Tested on both examples that run at 60FPS and 20FPS (where the `STOPPING` is immediately triggered when you hit the menu button)

Supersedes #109

Fixes #76

r? @jdm
@Manishearth Manishearth deleted the Manishearth:stopping branch Mar 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.