Change XRRigidTransform inverse from a method to an attribute #560
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
NellWaliczek
approved these changes
Apr 1, 2019
|
|
||
| An {{XRRigidTransform}} with a {{XRRigidTransform/position}} of <code>{ x: 0, y: 0, z: 0 w: 1 }</code> and an {{XRRigidTransform/orientation}} of <code>{ x: 0, y: 0, z: 0, w: 1 }</code> is known as an <dfn>identity transform</dfn>. | ||
| The <dfn attribute for="XRRigidTransform">inverse</dfn> attribute returns a {{XRRigidTransform}} which, if applied to an object that had previously been transformed by the original {{XRRigidTransform}}, would undo the transform and return the object to it's initial pose. This attribute SHOULD be lazily evaluated. |
There was a problem hiding this comment.
Should we say that the inverse.inverse is the original object?
There was a problem hiding this comment.
How about saying that T.inverse.inverse is equivalent to the original transform, but it may not be exactly equal due to rounding errors? I assume that's what you meant, but "is the original object" could be read as requiring that T.inverse.inverse === T in the sense of a true object identity.
I think the implementation should be allowed to handle each inverse as an individual calculation that returns a new object, and that wouldn't generally result in exact equality unless this variant is special-cased.
There was a problem hiding this comment.
I could see it go either way. It wouldn't be a huge burden to retain the "parent" transform. There's possibly some minor memory concerns that would come along with that, but it's probably not a big deal. It may improve performance in some rather weird edge cases, but I don't think we should be optimizing for that.
I would be tempted to leave it up to the UA, but there IS a minor compat issue that would come out of it. The biggest functionality fallout of a same-object policy here would be that if someone patches an attribute onto the transform, and then calls T.inverse.inverse.inverse.inverse.inverse.inverse.inverse.inverse that patched value would still be expected to be there. I don't know if that's behavior we necessarily want people to be relying on?
There was a problem hiding this comment.
After some further thought, I view this question as worthy of it's own discussion but not something that needs to block this particular change. Filed #576 to track the question.
bors-servo
pushed a commit
to servo/servo
that referenced
this pull request
Apr 4, 2019
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
pushed a commit
to servo/servo
that referenced
this pull request
Apr 4, 2019
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
pushed a commit
to servo/servo
that referenced
this pull request
Apr 4, 2019
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
pushed a commit
to servo/servo
that referenced
this pull request
Apr 4, 2019
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 -->
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Suggesting this change based on a conversation with @bfgeek about API ergonomics. His assertion was that if the XRRigidTransform object was (and always would be) immutable (which it is) then it would make sense to have the inverse be exposed as an attribute (lazily evaluated, of course) rather than a method. Using a method for a case like this is appropriate in scenarios where the desired behavior is to return something based on the current snapshot of a mutable object's state. But since the XRRigidTransform is immutable (and therefore will only ever have one inverse) we could opt for the arguably simpler syntax.
Thoughts?