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

Make XR/VR presenting code async #22649

Merged
merged 3 commits into from Jan 9, 2019

Conversation

Projects
None yet
4 participants
@Manishearth
Copy link
Member

Manishearth commented Jan 8, 2019

Previously we only pretended to be async, we returned promises that were already resolved because we synchronously blocked on the channel.

This:

  • Moves XR presentation to session creation (where it belongs, see immersive-web/webxr#453)
  • Factors out common presentation code in a way that supports asynchronously resolving promises
  • Uses this for VRDisplay::RequestPresent() and XR::RequestSession()

r? @jdm


This change is Reviewable

@highfive

This comment has been minimized.

Copy link

highfive commented Jan 8, 2019

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/vrdisplay.rs, components/script/dom/xr.rs, components/script/dom/xrsession.rs
  • @KiChjang: components/script/dom/vrdisplay.rs, components/script/dom/xr.rs, components/script/dom/xrsession.rs
@highfive

This comment has been minimized.

Copy link

highfive commented Jan 8, 2019

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@Manishearth

This comment has been minimized.

Copy link
Member Author

Manishearth commented Jan 8, 2019

I haven't actually tested this, it may blow up because the vr code doesn't expect an empty context

@Manishearth

This comment has been minimized.

Copy link
Member Author

Manishearth commented Jan 8, 2019

Oh, it definitely will, we have some unwraps there.

@Manishearth

This comment has been minimized.

Copy link
Member Author

Manishearth commented Jan 8, 2019

And the spec hasn't really ... decided what to do with empty contexts.

(now I remember why I didn't do this in the first place)

The RAF thread complicates this, but I might be able to do something relatively obvious until they decide the behavior.

Show resolved Hide resolved components/script/dom/vrdisplay.rs

@Manishearth Manishearth force-pushed the Manishearth:xr-promises branch from 768a53e to e2522d3 Jan 8, 2019

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Jan 8, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 8, 2019

📌 Commit e2522d3 has been approved by jdm

@jdm

jdm approved these changes Jan 8, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 8, 2019

⌛️ Testing commit e2522d3 with merge ce95b96...

bors-servo added a commit that referenced this pull request Jan 8, 2019

Auto merge of #22649 - Manishearth:xr-promises, r=jdm
Make XR/VR presenting code async

Previously we only pretended to be async, we returned promises that were already resolved because we synchronously blocked on the channel.

This:

 - Moves XR presentation to session creation (where it belongs, see immersive-web/webxr#453)
 - Factors out common presentation code in a way that supports asynchronously resolving promises
 - Uses this for `VRDisplay::RequestPresent()` and `XR::RequestSession()`

r? @jdm

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22649)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 9, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 9, 2019

👀 Test was successful, but fast-forwarding failed: 422 Update is not a fast forward

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Jan 9, 2019

@bors-servo retry

@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 9, 2019

⌛️ Testing commit e2522d3 with merge 44cbe96...

bors-servo added a commit that referenced this pull request Jan 9, 2019

Auto merge of #22649 - Manishearth:xr-promises, r=jdm
Make XR/VR presenting code async

Previously we only pretended to be async, we returned promises that were already resolved because we synchronously blocked on the channel.

This:

 - Moves XR presentation to session creation (where it belongs, see immersive-web/webxr#453)
 - Factors out common presentation code in a way that supports asynchronously resolving promises
 - Uses this for `VRDisplay::RequestPresent()` and `XR::RequestSession()`

r? @jdm

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22649)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

bors-servo commented Jan 9, 2019

@bors-servo bors-servo merged commit e2522d3 into servo:master Jan 9, 2019

4 checks passed

Taskcluster (pull_request) TaskGroup: success
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@Manishearth

This comment has been minimized.

Copy link
Member Author

Manishearth commented Jan 9, 2019

oh this wasn't supposed to merge, like I said this will panic unconditionally

(and that's a bit tricky to fix)

I've got a WIP for fixing this.

Since I'm the only one using XR rn it's fine to leave it as is though

@Manishearth Manishearth deleted the Manishearth:xr-promises branch Jan 9, 2019

@jdm

This comment has been minimized.

Copy link
Member

jdm commented Jan 9, 2019

Oops!

bors-servo added a commit that referenced this pull request Jan 24, 2019

Auto merge of #22699 - Manishearth:render_state, r=jdm
Update XR code to support null layers and new spec changes

Last time we landed an incomplete PR (#22649) that corrected the promise model but left XR sessions in panicky states.

This updates the code to not panic all the time, and also includes changes from immersive-web/webxr#458

r? @jdm

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/22699)
<!-- Reviewable:end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment