Properly support transforms in WebXR #23097

Manishearth · 2019-03-25T22:41:16Z

Still need to test this

There are also a bunch of fixmes that I should get to in this PR.

This change is


        Add XRSpace::get_viewer_pose()

highfive · 2019-03-25T22:41:21Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/xrrenderstate.rs, components/script/dom/webidls/XRSession.webidl, components/script/dom/vreyeparameters.rs, components/script/dom/xrrigidtransform.rs, components/script/dom/vrdisplay.rs and 8 more
@KiChjang: components/script/dom/xrrenderstate.rs, components/script/dom/webidls/XRSession.webidl, components/script/dom/vreyeparameters.rs, components/script/dom/xrrigidtransform.rs, components/script/dom/vrdisplay.rs and 8 more

highfive · 2019-03-25T22:41:22Z

Warning

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

avadacatavra · 2019-03-26T02:17:44Z

@Manishearth is this ready for a first pass review?

Manishearth · 2019-03-26T02:27:44Z

Sure!

Manishearth · 2019-03-26T05:27:52Z

This is tested. It can be merged as-is if someone reviews it first (@asajeffrey or @jdm ?) but I also have a bunch of improvements I wish to land.


        Compute view matrix from viewer pose


        Fix xrviewport.height


        Precompute XRRigidTransform's matrices


        Add XRRigidTransform::Inverse

asajeffrey

LGTM, just some minor nits and comments.

asajeffrey · 2019-03-26T19:19:44Z

components/script/dom/bindings/trace.rs

+        // Do nothing
+    }
+}
+


Nit: unsafe_no_jsmanaged_fields!?

IIRC it doesn't work with generics that well? I'll try.

asajeffrey · 2019-03-26T19:19:44Z

components/script/dom/xrframe.rs

-        }
+        let pose = reference.get_viewer_pose(&self.data);
+        let left = XRView::new(
+            &self.global(),


This is building new XRView objects every time it is called? Is that required by the spec, or are we allowed to update in place?

Chrome updates in-place. This change can be made.

Actually, I'd rather not fix this just yet, I'll bring it up on the spec. This is observable.

immersive-web/webxr#572

asajeffrey · 2019-03-26T19:19:44Z

components/script/dom/xrrigidtransform.rs

+    pub fn matrix(&self) -> Transform3D<f64> {
+        // Spec says the orientation applies first,
+        // so post-multiply (?)
+        self.translate.post_mul(&self.rotate.to_transform())


Using Euclid typed units to keep track of coordinate spaces would make this (?) less speculative!

It doesn't help since there's no term for the intermediate space, and these transforms can be applied in multiple different ways.

asajeffrey · 2019-03-26T19:19:44Z

components/script/dom/xrviewport.rs

@@ -57,7 +57,7 @@ impl XRViewportMethods for XRViewport {

    /// https://immersive-web.github.io/webxr/#dom-xrviewport-width
    fn Width(&self) -> i32 {
-        self.height as i32
+        self.width as i32
    }



        Clean up trace impls

Manishearth · 2019-03-26T21:32:47Z

@bors-servo r=asajeffrey

Most of the TODOs are better done after servo/euclid#328 lands, since that lets us simplify a bunch of the code here.

bors-servo · 2019-03-26T21:32:48Z

📌 Commit 240b5e7 has been approved by asajeffrey


        Allow unused

Manishearth · 2019-03-26T21:34:17Z

@bors-servo r=asajeffrey

bors-servo · 2019-03-26T21:34:18Z

📌 Commit 2e62805 has been approved by asajeffrey

Manishearth · 2019-03-27T01:04:00Z

@bors-servo p=1

bors seems to be sleepy?

bors-servo · 2019-03-27T01:04:40Z

⌛ Testing commit 2e62805 with merge 364d0e0...


        Auto merge of #23097 - Manishearth:transforms, r=asajeffrey

Properly support transforms in WebXR Still need to test this There are also a bunch of fixmes that I should get to in this PR.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23097)

bors-servo · 2019-03-27T02:34:36Z

💔 Test failed - status-taskcluster

Manishearth · 2019-03-27T02:38:06Z

@bors-servo retry

BHM failure? (Will look closer when I'm back home)

jdm · 2019-03-27T03:00:33Z

Filed #23109 for that failure.

bors-servo · 2019-03-27T04:05:05Z

⌛ Testing commit 2e62805 with merge 3c27dc9...


        Auto merge of #23097 - Manishearth:transforms, r=asajeffrey

Properly support transforms in WebXR Still need to test this There are also a bunch of fixmes that I should get to in this PR.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23097)

bors-servo · 2019-03-27T05:35:38Z

☀️ 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 3c27dc9 to master...

Add XRSpace::get_viewer_pose()

d1d8e97

highfive assigned avadacatavra Mar 25, 2019

highfive added the S-awaiting-review label Mar 25, 2019

Manishearth force-pushed the Manishearth:transforms branch from 5ecd4ee to a42692d Mar 26, 2019

Manishearth force-pushed the Manishearth:transforms branch 2 times, most recently from 964c7d7 to 84dc98d Mar 26, 2019

Manishearth added 4 commits Mar 21, 2019

Compute view matrix from viewer pose

2e48606

Fix xrviewport.height

77e8578

Precompute XRRigidTransform's matrices

6fda2f2

Add XRRigidTransform::Inverse

Loading status checks…

4f128e4

Manishearth force-pushed the Manishearth:transforms branch from d9956fc to 4f128e4 Mar 26, 2019

asajeffrey approved these changes Mar 26, 2019

View changes

jdm assigned asajeffrey and unassigned avadacatavra Mar 26, 2019

Clean up trace impls

Loading status checks…

240b5e7

Manishearth changed the title ~~[WIP] Properly support transforms in WebXR~~ Properly support transforms in WebXR Mar 26, 2019

highfive added S-awaiting-merge and removed S-awaiting-review labels Mar 26, 2019

Allow unused

Loading status checks…

2e62805

highfive added S-awaiting-review and removed S-awaiting-merge labels Mar 26, 2019

highfive added S-awaiting-merge and removed S-awaiting-review labels Mar 26, 2019

highfive added S-tests-failed and removed S-awaiting-merge labels Mar 27, 2019

highfive added S-awaiting-merge and removed S-tests-failed labels Mar 27, 2019

highfive removed the S-awaiting-merge label Mar 27, 2019

Manishearth deleted the Manishearth:transforms branch Mar 27, 2019

servo / servo

Properly support transforms in WebXR #23097

Properly support transforms in WebXR #23097

Manishearth commented Mar 25, 2019 •

edited

highfive commented Mar 25, 2019

highfive commented Mar 25, 2019

avadacatavra commented Mar 26, 2019

Manishearth commented Mar 26, 2019

Manishearth commented Mar 26, 2019

asajeffrey left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Manishearth commented Mar 26, 2019

bors-servo commented Mar 26, 2019

Manishearth commented Mar 26, 2019

bors-servo commented Mar 26, 2019

Manishearth commented Mar 27, 2019

bors-servo commented Mar 27, 2019

bors-servo commented Mar 27, 2019

Manishearth commented Mar 27, 2019

jdm commented Mar 27, 2019

bors-servo commented Mar 27, 2019

bors-servo commented Mar 27, 2019

servo / servo

Join GitHub today

Properly support transforms in WebXR #23097

Properly support transforms in WebXR #23097

Conversation

Manishearth commented Mar 25, 2019 • edited

highfive commented Mar 25, 2019

highfive commented Mar 25, 2019

avadacatavra commented Mar 26, 2019

Manishearth commented Mar 26, 2019

Manishearth commented Mar 26, 2019

asajeffrey left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Manishearth commented Mar 26, 2019

bors-servo commented Mar 26, 2019

Manishearth commented Mar 26, 2019

bors-servo commented Mar 26, 2019

Manishearth commented Mar 27, 2019

bors-servo commented Mar 27, 2019

bors-servo commented Mar 27, 2019

Manishearth commented Mar 27, 2019

jdm commented Mar 27, 2019

bors-servo commented Mar 27, 2019

bors-servo commented Mar 27, 2019

Manishearth commented Mar 25, 2019 •

edited