Skip to content

/ servo

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

Some XRSpace improvements #23055

Merged
merged 8 commits into from Mar 21, 2019
Merged

Some XRSpace improvements #23055

merged 8 commits into from Mar 21, 2019

Conversation

@Manishearth
Copy link
Member

Manishearth commented Mar 18, 2019 •
edited by SimonSapin

Proper XRSpace support is blocked on immersive-web/webxr#565 , but in the meantime this improves XRSpace support a little bit, preparing both for support in getViewerPose and getPose as well as handling input spaces eventually.

r? @jdm

This change is Reviewable
Manishearth added 7 commits Mar 14, 2019
@Manishearth
Add XRRigidTransform constructor
c775820
@Manishearth
Add XRRigidTransform attributes
503ea3d
@Manishearth
Add originOffset to XRReferenceSpace
ffacb11
@Manishearth
Add session parameter to XRSpace
8f14007
@Manishearth
Add XRReferenceSpace::identity
f2a6164
@Manishearth
Add requestReferenceSpace
191fcf6
@Manishearth
Cleanup XRFrame
a2c4270
@highfive
Copy link

highfive commented Mar 18, 2019

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/webidls/XRFrame.webidl, components/script/dom/webidls/XRSession.webidl, components/script/dom/xrrigidtransform.rs, components/script/dom/webidls/XRReferenceSpace.webidl, components/script/dom/webidls/XRStationaryReferenceSpace.webidl and 7 more
  • @KiChjang: components/script/dom/webidls/XRFrame.webidl, components/script/dom/webidls/XRSession.webidl, components/script/dom/xrrigidtransform.rs, components/script/dom/webidls/XRReferenceSpace.webidl, components/script/dom/webidls/XRStationaryReferenceSpace.webidl and 7 more
@highfive
Copy link

highfive commented Mar 18, 2019

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@Manishearth Manishearth force-pushed the Manishearth:xrspace branch 3 times, most recently from 8282fd5 to d192be5 Mar 18, 2019
@asajeffrey
asajeffrey approved these changes Mar 19, 2019
View changes
Copy link
Member

asajeffrey left a comment

LGTM, with some commenting nits. What's our testing story? Should we add some WPT tests?
components/script/dom/xrframe.rs Outdated
fn GetViewerPose(&self, reference: &XRReferenceSpace) -> Option<DomRoot<XRViewerPose>> {
if let Some(_) = reference.downcast::<XRStationaryReferenceSpace>() {
// For 3DOF devices all three kinds of reference spaces are identical
// XXXManishearth support originOffset

This comment has been minimized.

Sign in to view
@asajeffrey

asajeffrey Mar 19, 2019

Member

If we're checking this in, it should be a TODO preferably with a link to an issue.
components/script/dom/xrframe.rs Outdated
let right = XRView::new(&self.global(), &self.session, XREye::Right, &self.data);
Some(XRViewerPose::new(&self.global(), &left, &right))
} else {
// XXXManishearth support identity reference spaces

This comment has been minimized.

Sign in to view
@asajeffrey

asajeffrey Mar 19, 2019

Member

Ditto.
@Manishearth
Copy link
Member Author

Manishearth commented Mar 19, 2019

The spec is in a state of flux so there aren't any tests IIRC :( Mostly trying to keep up with it and build scaffolding for features which aren't yet finalized (but I know the general shape of)
@Manishearth Manishearth mentioned this pull request Mar 21, 2019
Open
@Manishearth
Improve getViewerPose()
Loading status checks…
22e5ce5
@Manishearth Manishearth force-pushed the Manishearth:xrspace branch from d192be5 to 22e5ce5 Mar 21, 2019
@Manishearth
Copy link
Member Author

Manishearth commented Mar 21, 2019

@bors-servo r=asajeffrey

Filed #23070

I actually figured out the spec issue behind the fixme (see immersive-web/webxr#565 (comment)), but I'll let this land first.

Filed testing issue #23071
@bors-servo
Copy link
Contributor

bors-servo commented Mar 21, 2019

📌 Commit 22e5ce5 has been approved by asajeffrey
@highfive highfive assigned asajeffrey and unassigned jdm Mar 21, 2019
@bors-servo
Copy link
Contributor

bors-servo commented Mar 21, 2019

⌛ Testing commit 22e5ce5 with merge e582268...
bors-servo added a commit that referenced this pull request Mar 21, 2019
@bors-servo
Auto merge of #23055 - Manishearth:xrspace, r=asajeffrey
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
e582268 
Some XRSpace improvements

Proper XRSpace support is blocked on immersive-web/webxr#565 , but in the meantime this improves XRSpace support a little bit, preparing both for support in getViewerPose and getPose as well as handling input spaces eventually.

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/23055)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Mar 21, 2019

☀️ Test successful - android-mac, arm32, arm64, linux-rel-css, linux-rel-wpt, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, magicleap, status-taskcluster
Approved by: asajeffrey
Pushing e582268 to master...
@bors-servo bors-servo merged commit 22e5ce5 into servo:master Mar 21, 2019
3 of 4 checks passed
3 of 4 checks passed
continuous-integration/appveyor/pr AppVeyor build failed
Details
Taskcluster (pull_request) TaskGroup: success
Details
Travis CI - Pull Request Build Passed
Details
homu Test successful
Details
@Manishearth Manishearth mentioned this pull request Apr 3, 2019
Merged
@Manishearth Manishearth deleted the Manishearth:xrspace branch May 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@asajeffrey asajeffrey

Assignees

@asajeffrey asajeffrey

Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
@Manishearth @highfive @bors-servo @asajeffrey @jdm
You can’t perform that action at this time.