Validate position DOMPointInit #568

Manishearth · 2019-03-21T21:36:24Z

Given that it's a float, comparison with 1 may not be the best idea, but I feel like erroring here is marginally better than ignoring invalid w values.

klausw · 2019-03-22T00:05:47Z

I think people have a somewhat irrational (no pun intended) distrust of floating point arithmetic. If you start out with w=1, and apply matrix transforms involving add/multiply of values that are also exactly 0 or 1 (as is the case for the bottom row of a rigid transform matrix), the result will still exactly equal 1.0. Floating point arithmetic is exact for integers as long as you don't exceed the mantissa range (up to 2^52 or so for doubles), and the typical things you'd be doing with w values wouldn't lead to an approximately-but-not-exactly-1.0 value.

Manishearth · 2019-03-22T05:18:34Z

Yeah, this was the assumption I was making too, floating point should be fine for ints here. And the fix for this error is straightforward, you can always set the last value to 1 if you need.

toji · 2019-03-25T18:21:28Z

Hm... so this would explicitly prevent me from doing something along the lines of the following, right?

let transform = new XRRigidTransform({x: 1, y: 2, z: 3}, someOrientation);

Because that actually seems pretty convenient.

Manishearth · 2019-03-25T18:24:30Z

It would allow it as long as someOrientation has a unit value 1 -- if it doesn't you have a bug anyway since it's going to get overwritten

Manishearth · 2019-03-25T18:25:11Z

Oh, sorry, position. That is a problem, hmm. Maybe provide our own init dicts?

Manishearth · 2019-03-25T18:27:01Z

It seems like w=1 by default, so that looks like it would still work?

toji · 2019-03-25T18:28:02Z

Oh really? Could you link to the relevant spec text? (I'm gonna try and hunt it down too.)

toji · 2019-03-25T18:29:13Z

I guess this is the right link to cite: https://www.w3.org/TR/geometry-1/#DOMPoint

And in light of that, I'm good with this being a strict validation. Thanks!

Added in immersive-web/webxr#568

@jdm

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  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23159)

Added in immersive-web/webxr#568

@jdm

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  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23159)

@jdm

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  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23159)

@jdm

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  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23159)

Added in immersive-web/webxr#568

Validate position DOMPointInit

65b1b4d

NellWaliczek approved these changes Mar 25, 2019

View reviewed changes

toji merged commit ef4f46c into immersive-web:master Mar 25, 2019

AdaRoseCannon added the ed:spec Include in newsletter, spec change label Mar 29, 2019

Manishearth added a commit to Manishearth/servo that referenced this pull request Apr 3, 2019

Validate the position passed to XRRigidTransform

24c6147

Added in immersive-web/webxr#568

Manishearth added a commit to Manishearth/servo that referenced this pull request Apr 3, 2019

Validate the position passed to XRRigidTransform

6f0a44a

Added in immersive-web/webxr#568

Manishearth mentioned this pull request Apr 3, 2019

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

Merged

Manishearth added a commit to Manishearth/servo that referenced this pull request Apr 4, 2019

Validate the position passed to XRRigidTransform

1f3f372

Added in immersive-web/webxr#568

lucasfantacuci pushed a commit to lucasfantacuci/servo that referenced this pull request Apr 8, 2019

Validate the position passed to XRRigidTransform

e73efa1

Added in immersive-web/webxr#568

Manishearth deleted the position-w branch August 13, 2019 05:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Validate position DOMPointInit #568

Validate position DOMPointInit #568

Uh oh!

Manishearth commented Mar 21, 2019

Uh oh!

klausw commented Mar 22, 2019

Uh oh!

Manishearth commented Mar 22, 2019

Uh oh!

toji commented Mar 25, 2019

Uh oh!

Manishearth commented Mar 25, 2019

Uh oh!

Manishearth commented Mar 25, 2019

Uh oh!

Manishearth commented Mar 25, 2019

Uh oh!

toji commented Mar 25, 2019

Uh oh!

toji commented Mar 25, 2019

Uh oh!

Uh oh!

Validate position DOMPointInit #568

Validate position DOMPointInit #568

Uh oh!

Conversation

Manishearth commented Mar 21, 2019

Uh oh!

klausw commented Mar 22, 2019

Uh oh!

Manishearth commented Mar 22, 2019

Uh oh!

toji commented Mar 25, 2019

Uh oh!

Manishearth commented Mar 25, 2019

Uh oh!

Manishearth commented Mar 25, 2019

Uh oh!

Manishearth commented Mar 25, 2019

Uh oh!

toji commented Mar 25, 2019

Uh oh!

toji commented Mar 25, 2019

Uh oh!

Uh oh!