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

Update XR code to use rigid transforms and new pose/transform stuff from the spec #23159

Merged
merged 13 commits into from Apr 4, 2019

Conversation

Projects
None yet
5 participants
@Manishearth
Copy link
Member

commented Apr 3, 2019

This updates our XR code to use euclid's new RigidTransform3D type, which is more efficent and convenient to work with.

It additionally brings us up to speed with the spec:

Furthermore, it adds support for XRRigidTransform.matrix.

While fixing this I also noticed that our view matrix code was incorrect, we calculated view matrices as pose.to_column_major_array(), whereas it should be pose.inverse().to_row_major_array() (since Euclid uses row vectors, whenever the spec says it wants a column major array we should use .to_row_major_array() since all web specs implicitly use column vectors). For 3DOF devices poses are mostly rotations anyway, so the effective transpose behaved like an inversion, but was incorrect.

This PR gets rid of view.viewMatrix anyway, however I felt like I should mention this discrepancy, since otherwise the replacement of view.viewMatrix with view.transform.inverse.matrix doesn't make sense

r? @jdm


This change is Reviewable

@highfive

This comment has been minimized.

Copy link

commented Apr 3, 2019

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/mod.rs, components/script/dom/webidls/XRView.webidl, components/script/Cargo.toml, components/script/dom/xrpose.rs, components/script/dom/xrreferencespace.rs and 12 more
  • @KiChjang: components/script/dom/mod.rs, components/script/dom/webidls/XRView.webidl, components/script/Cargo.toml, components/script/dom/xrpose.rs, components/script/dom/xrreferencespace.rs and 12 more
@highfive

This comment has been minimized.

Copy link

commented Apr 3, 2019

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
  • These commits modify script code, but no tests are modified. Please consider adding a test!

@Manishearth Manishearth requested review from asajeffrey and jdm Apr 3, 2019

@Manishearth

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

Actually, probably should be r? @asajeffrey since he reviewed #23055

@highfive highfive assigned asajeffrey and unassigned jdm Apr 3, 2019

@Manishearth

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

There are still some changes left out of this PR which I may add later: namely, XRStationaryReferenceSpace::get_pose is a lie, it should be ::get_viewer_pose(). I was not interpreting the nature of reference spaces correctly, however my misinterpretation only affects the observed behavior of getPose(), which isn't implemented yet.

Nell mentioned that she had some feedback to give on immersive-web/webxr#565, so I'm going to hold off on making those changes till I know the real mental model involved.

@asajeffrey

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

LGTM. You might want to squash before merging.

@Manishearth

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

The commits are all self-contained, incremental, and building, and I prefer to have more commits whenever possible.

@bors-servo r=asajeffrey

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

📌 Commit e8d172a has been approved by asajeffrey

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

⌛️ Testing commit e8d172a with merge 6e797f6...

bors-servo added a commit that referenced this pull request Apr 4, 2019

Auto merge of #23159 - Manishearth:rigid-transforms, r=asajeffrey
Update XR code to use rigid transforms and new pose/transform stuff from the spec

This updates our XR code to use euclid's new [RigidTransform3D type](servo/euclid#328), which is more efficent and convenient to work with.

It additionally brings us up to speed with the spec:

 - `XRViewerPose` was made a subclass of `XRPose` (immersive-web/webxr#496)
 - `XRView.viewMatrix` was removed in favor of `XRRigidTransform.inverse.matrix` (immersive-web/webxr#531)
 - `XRRigidTransform.inverse` is an attribute (immersive-web/webxr#560)
 - `XRRigidTransform` now validates positions in its constructor (immersive-web/webxr#568)

Furthermore, it adds support for `XRRigidTransform.matrix`.

While fixing this I also noticed that our view matrix code was incorrect, we calculated view matrices as `pose.to_column_major_array()`, whereas it *should* be `pose.inverse().to_row_major_array()` (since Euclid uses row vectors, whenever the spec says it wants a column major array we should use `.to_row_major_array()` since all web specs implicitly use column vectors). For 3DOF devices poses are mostly rotations anyway, so the effective transpose behaved _like_ an inversion, but was incorrect.

This PR gets rid of `view.viewMatrix` anyway, however I felt like I should mention this discrepancy, since otherwise the replacement of `view.viewMatrix` with `view.transform.inverse.matrix` doesn't make sense

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/23159)
<!-- Reviewable:end -->
@Manishearth

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

(This PR is also like ... five changes at once)

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

💔 Test failed - mac-rel-css1

@Manishearth

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

@bors-servo retry

weird timeout in urlsearchparams-getall

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

💔 Test failed - mac-rel-css1

@Manishearth

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

Huh, it's a different URLSearchParams test.

@Manishearth

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

@bors-servo try retry r-

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

⌛️ Trying commit e055884 with merge 21defb9...

bors-servo added a commit that referenced this pull request Apr 4, 2019

Auto merge of #23159 - Manishearth:rigid-transforms, r=<try>
Update XR code to use rigid transforms and new pose/transform stuff from the spec

This updates our XR code to use euclid's new [RigidTransform3D type](servo/euclid#328), which is more efficent and convenient to work with.

It additionally brings us up to speed with the spec:

 - `XRViewerPose` was made a subclass of `XRPose` (immersive-web/webxr#496)
 - `XRView.viewMatrix` was removed in favor of `XRRigidTransform.inverse.matrix` (immersive-web/webxr#531)
 - `XRRigidTransform.inverse` is an attribute (immersive-web/webxr#560)
 - `XRRigidTransform` now validates positions in its constructor (immersive-web/webxr#568)

Furthermore, it adds support for `XRRigidTransform.matrix`.

While fixing this I also noticed that our view matrix code was incorrect, we calculated view matrices as `pose.to_column_major_array()`, whereas it *should* be `pose.inverse().to_row_major_array()` (since Euclid uses row vectors, whenever the spec says it wants a column major array we should use `.to_row_major_array()` since all web specs implicitly use column vectors). For 3DOF devices poses are mostly rotations anyway, so the effective transpose behaved _like_ an inversion, but was incorrect.

This PR gets rid of `view.viewMatrix` anyway, however I felt like I should mention this discrepancy, since otherwise the replacement of `view.viewMatrix` with `view.transform.inverse.matrix` doesn't make sense

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/23159)
<!-- Reviewable:end -->
@Manishearth

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

I suspect that intermittent comes from one of the WPT updates but it's not clear why cc @jdm

It could also be due to the euclid bump but the relevant test isn't using euclid. A possibility is that a test running in parallel is using up all the cpu and causing timeouts elsewhere.

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

@jdm

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

@bors-servo r=asajeffrey

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

📌 Commit e055884 has been approved by asajeffrey

bors-servo added a commit that referenced this pull request Apr 4, 2019

Auto merge of #23159 - Manishearth:rigid-transforms, r=asajeffrey
Update XR code to use rigid transforms and new pose/transform stuff from the spec

This updates our XR code to use euclid's new [RigidTransform3D type](servo/euclid#328), which is more efficent and convenient to work with.

It additionally brings us up to speed with the spec:

 - `XRViewerPose` was made a subclass of `XRPose` (immersive-web/webxr#496)
 - `XRView.viewMatrix` was removed in favor of `XRRigidTransform.inverse.matrix` (immersive-web/webxr#531)
 - `XRRigidTransform.inverse` is an attribute (immersive-web/webxr#560)
 - `XRRigidTransform` now validates positions in its constructor (immersive-web/webxr#568)

Furthermore, it adds support for `XRRigidTransform.matrix`.

While fixing this I also noticed that our view matrix code was incorrect, we calculated view matrices as `pose.to_column_major_array()`, whereas it *should* be `pose.inverse().to_row_major_array()` (since Euclid uses row vectors, whenever the spec says it wants a column major array we should use `.to_row_major_array()` since all web specs implicitly use column vectors). For 3DOF devices poses are mostly rotations anyway, so the effective transpose behaved _like_ an inversion, but was incorrect.

This PR gets rid of `view.viewMatrix` anyway, however I felt like I should mention this discrepancy, since otherwise the replacement of `view.viewMatrix` with `view.transform.inverse.matrix` doesn't make sense

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/23159)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

⌛️ Testing commit e055884 with merge 709cfeb...

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

💔 Test failed - linux-rel-css

@jdm

This comment has been minimized.

Copy link
Member

commented Apr 4, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

💣 Failed to start rebuilding: Unknown error

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

⌛️ Testing commit e055884 with merge f1b82f8...

bors-servo added a commit that referenced this pull request Apr 4, 2019

Auto merge of #23159 - Manishearth:rigid-transforms, r=asajeffrey
Update XR code to use rigid transforms and new pose/transform stuff from the spec

This updates our XR code to use euclid's new [RigidTransform3D type](servo/euclid#328), which is more efficent and convenient to work with.

It additionally brings us up to speed with the spec:

 - `XRViewerPose` was made a subclass of `XRPose` (immersive-web/webxr#496)
 - `XRView.viewMatrix` was removed in favor of `XRRigidTransform.inverse.matrix` (immersive-web/webxr#531)
 - `XRRigidTransform.inverse` is an attribute (immersive-web/webxr#560)
 - `XRRigidTransform` now validates positions in its constructor (immersive-web/webxr#568)

Furthermore, it adds support for `XRRigidTransform.matrix`.

While fixing this I also noticed that our view matrix code was incorrect, we calculated view matrices as `pose.to_column_major_array()`, whereas it *should* be `pose.inverse().to_row_major_array()` (since Euclid uses row vectors, whenever the spec says it wants a column major array we should use `.to_row_major_array()` since all web specs implicitly use column vectors). For 3DOF devices poses are mostly rotations anyway, so the effective transpose behaved _like_ an inversion, but was incorrect.

This PR gets rid of `view.viewMatrix` anyway, however I felt like I should mention this discrepancy, since otherwise the replacement of `view.viewMatrix` with `view.transform.inverse.matrix` doesn't make sense

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/23159)
<!-- Reviewable:end -->
@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

💔 Test failed - linux-rel-wpt

@Manishearth

This comment has been minimized.

Copy link
Member Author

commented Apr 4, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2019

@bors-servo bors-servo merged commit e055884 into servo:master Apr 4, 2019

3 of 4 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
Taskcluster (pull_request) TaskGroup: success
Details
Travis CI - Pull Request Build Passed
Details
homu Test successful
Details

@Manishearth Manishearth deleted the Manishearth:rigid-transforms branch May 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.