Update XRRay vector constructor to use correct webidl semantics #85

Manishearth · 2020-04-09T19:45:27Z

Fixes #84

This adds a new initializer type so that the defaults work correctly.

This also throws an error for non-1 w coordinates for the origin, similar to XRRigidTransform.

r? @bialpio

Preview | Diff

Fixes immersive-web#84 This adds a new initializer type so that the defaults work correctly.

index.bs

Manishearth · 2020-04-09T19:47:02Z

index.bs

  1. Initialize |ray|'s {{XRRay/direction}} to <code>{ x: 0.0, y: 0.0, z: -1.0, w: 0.0 }</code>.
-  1. If |origin| was set, initialize |ray|'s {{XRRay/origin}}’s {{DOMPointReadOnly/x}} value to |origin|'s x dictionary member, {{DOMPointReadOnly/y}} value to |origin|'s y dictionary member, and {{DOMPointReadOnly/z}} value to |origin|'s z dictionary member.
-  1. If |direction| was set, initialize |ray|'s {{XRRay/direction}}’s {{DOMPointReadOnly/x}} value to |direction|'s x dictionary member, {{DOMPointReadOnly/y}} value to |direction|'s y dictionary member, and {{DOMPointReadOnly/z}} value to |direction|'s z dictionary member.
+  1. If all of |direction|'s {{XRRayDirectionInit/x}}, {{XRRayDirectionInit/y}}, and {{XRRayDirectionInit/z}} are zero, throw a {{TypeError}} and abort these steps.


Chrome throws an InvalidStateError, but this should be a TypeError (XRRigidTransform also uses TypeErrors for this)

index.bs

bialpio · 2020-04-10T19:57:13Z

Looks good, merging now.

Manishearth · 2020-04-10T20:27:18Z

On the Chrome side, y'all ideally should update webidl parsing to the newer thing, however if not you should then make the default behavior work as much as possible and change the error type to TypeError

Manishearth · 2020-04-10T20:27:59Z

Oh, and the tests need fixing, but I might end up doing that myself if my Servo changes land first.

Implements changes from immersive-web/hit-test#85

bialpio · 2020-04-10T22:02:58Z

Chrome's behavior was incorrect because the implementation relied on multiple overloads for the constructor, I've reached out internally and was told that the change to require = {} in Web IDL was a syntactic change, w/o changing the semantics.

For reference, I've sent out initial CL with the fix (including WPTs): https://crrev.com/c/2145937.

Manishearth · 2020-04-10T22:05:19Z

@bialpio it changes the semantics in the sense that you can no longer receive "nullable" dictionaries in webidl

Also, you should use assert_throws in the test, see https://github.com/Manishearth/servo/blob/2bb06b338c8dc606078736c43c5fda9c13e6357c/tests/wpt/web-platform-tests/webxr/hit-test/xrRay_constructor.https.html#L91 . I can just directly upstream those test changes if you'd like.

bialpio · 2020-04-10T23:03:13Z

you can no longer receive "nullable" dictionaries in webidl

IIUC, that's the case even prior to this change, but the reason why we were getting them is because Chrome's XRRay Web IDL was not matching the one in the spec: https://chromium-review.googlesource.com/c/chromium/src/+/2145937/1/third_party/blink/renderer/modules/xr/xr_ray.idl#b11 - note the 0, 1 and 2 parameter overloads. I think I got it working in Chrome in a way that made sense in WPTs but forgot to adjust the specification. :(

If you're ready to upstream your changes to the tests then go for it, I think it'll take me a while to go through CL with my change.

Implements changes from immersive-web/hit-test#85

Manishearth · 2020-04-11T07:56:32Z

Ah, I see! I'd just assumed the spec had the webidl chrome was using :)

My tests can be found in web-platform-tests/wpt#22868 , if you review that PR we can just go ahead and land it!

Implements changes from immersive-web/hit-test#85

Automatic update from web-platform-tests Update xrRay_constructor test to not throw See immersive-web/hit-test#83, this test is currently impossible to pass for a browser implementing the current WebIDL specification. -- Update XRRay semantics Implements changes from immersive-web/hit-test#85 -- Fix world generation to have vertices member -- wpt-commits: 7513c92e130e043def925bda97e9fb9d8fd0b48f, 0fce2e94f0feb04966868a0b5795d76ea1db3906, 08c95d8ca3466c37df307cb775502a916e22b814 wpt-pr: 22868

Implements changes from immersive-web/hit-test#85

Automatic update from web-platform-tests Update xrRay_constructor test to not throw See immersive-web/hit-test#83, this test is currently impossible to pass for a browser implementing the current WebIDL specification. -- Update XRRay semantics Implements changes from immersive-web/hit-test#85 -- Fix world generation to have vertices member -- wpt-commits: 7513c92e130e043def925bda97e9fb9d8fd0b48f, 0fce2e94f0feb04966868a0b5795d76ea1db3906, 08c95d8ca3466c37df307cb775502a916e22b814 wpt-pr: 22868 UltraBlame original commit: 309322031e57b9f606c20274ad42711703753086

Bring XRRay up to spec, adjusting the behavior with changes introduced by PR: immersive-web/hit-test#85 Additionally, fix Chrome's Web IDL to not rely on overloads. Change-Id: I98af1a4ed90dbeb8e311795e818efb64bb15034d

Bring XRRay up to spec, adjusting the behavior with changes introduced by PR: immersive-web/hit-test#85 Additionally, fix Chrome's Web IDL to not rely on overloads. Change-Id: I98af1a4ed90dbeb8e311795e818efb64bb15034d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2145937 Reviewed-by: Klaus Weidner <klausw@chromium.org> Reviewed-by: Jeremy Roman <jbroman@chromium.org> Commit-Queue: Piotr Bialecki <bialpio@chromium.org> Commit-Queue: Jeremy Roman <jbroman@chromium.org> Auto-Submit: Piotr Bialecki <bialpio@chromium.org> Cr-Commit-Position: refs/heads/master@{#761766}

Bring XRRay up to spec, adjusting the behavior with changes introduced by PR: immersive-web/hit-test#85 Additionally, fix Chrome's Web IDL to not rely on overloads. Change-Id: I98af1a4ed90dbeb8e311795e818efb64bb15034d

Bring XRRay up to spec, adjusting the behavior with changes introduced by PR: immersive-web/hit-test#85 Additionally, fix Chrome's Web IDL to not rely on overloads. Change-Id: I98af1a4ed90dbeb8e311795e818efb64bb15034d Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2145937 Reviewed-by: Klaus Weidner <klausw@chromium.org> Reviewed-by: Jeremy Roman <jbroman@chromium.org> Commit-Queue: Piotr Bialecki <bialpio@chromium.org> Commit-Queue: Jeremy Roman <jbroman@chromium.org> Auto-Submit: Piotr Bialecki <bialpio@chromium.org> Cr-Commit-Position: refs/heads/master@{#761766}

…pec, a=testonly Automatic update from web-platform-tests WebXR - AR - adjust XRRay to match the spec Bring XRRay up to spec, adjusting the behavior with changes introduced by PR: immersive-web/hit-test#85 Additionally, fix Chrome's Web IDL to not rely on overloads. Change-Id: I98af1a4ed90dbeb8e311795e818efb64bb15034d -- wpt-commits: fd045759df43fa155b34a5e4fe4187610ad1bb24 wpt-pr: 23126

…pec, a=testonly Automatic update from web-platform-tests WebXR - AR - adjust XRRay to match the spec Bring XRRay up to spec, adjusting the behavior with changes introduced by PR: immersive-web/hit-test#85 Additionally, fix Chrome's Web IDL to not rely on overloads. Change-Id: I98af1a4ed90dbeb8e311795e818efb64bb15034d -- wpt-commits: fd045759df43fa155b34a5e4fe4187610ad1bb24 wpt-pr: 23126 UltraBlame original commit: b4c21d6c137e0e39e8b8a9364434136980cf957b

…pec, a=testonly Automatic update from web-platform-tests WebXR - AR - adjust XRRay to match the spec Bring XRRay up to spec, adjusting the behavior with changes introduced by PR: immersive-web/hit-test#85 Additionally, fix Chrome's Web IDL to not rely on overloads. Change-Id: I98af1a4ed90dbeb8e311795e818efb64bb15034d -- wpt-commits: fd045759df43fa155b34a5e4fe4187610ad1bb24 wpt-pr: 23126 UltraBlame original commit: 32e3888c79b097fdd320539a1b9b1895f9cd03ff

…pec, a=testonly Automatic update from web-platform-tests WebXR - AR - adjust XRRay to match the spec Bring XRRay up to spec, adjusting the behavior with changes introduced by PR: immersive-web/hit-test#85 Additionally, fix Chrome's Web IDL to not rely on overloads. Change-Id: I98af1a4ed90dbeb8e311795e818efb64bb15034d -- wpt-commits: fd045759df43fa155b34a5e4fe4187610ad1bb24 wpt-pr: 23126 UltraBlame original commit: b4c21d6c137e0e39e8b8a9364434136980cf957b

…pec, a=testonly Automatic update from web-platform-tests WebXR - AR - adjust XRRay to match the spec Bring XRRay up to spec, adjusting the behavior with changes introduced by PR: immersive-web/hit-test#85 Additionally, fix Chrome's Web IDL to not rely on overloads. Change-Id: I98af1a4ed90dbeb8e311795e818efb64bb15034d -- wpt-commits: fd045759df43fa155b34a5e4fe4187610ad1bb24 wpt-pr: 23126 UltraBlame original commit: 32e3888c79b097fdd320539a1b9b1895f9cd03ff

…pec, a=testonly Automatic update from web-platform-tests WebXR - AR - adjust XRRay to match the spec Bring XRRay up to spec, adjusting the behavior with changes introduced by PR: immersive-web/hit-test#85 Additionally, fix Chrome's Web IDL to not rely on overloads. Change-Id: I98af1a4ed90dbeb8e311795e818efb64bb15034d -- wpt-commits: fd045759df43fa155b34a5e4fe4187610ad1bb24 wpt-pr: 23126 UltraBlame original commit: b4c21d6c137e0e39e8b8a9364434136980cf957b

…pec, a=testonly Automatic update from web-platform-tests WebXR - AR - adjust XRRay to match the spec Bring XRRay up to spec, adjusting the behavior with changes introduced by PR: immersive-web/hit-test#85 Additionally, fix Chrome's Web IDL to not rely on overloads. Change-Id: I98af1a4ed90dbeb8e311795e818efb64bb15034d -- wpt-commits: fd045759df43fa155b34a5e4fe4187610ad1bb24 wpt-pr: 23126 UltraBlame original commit: 32e3888c79b097fdd320539a1b9b1895f9cd03ff

Implements changes from immersive-web/hit-test#85

Automatic update from web-platform-tests Update xrRay_constructor test to not throw See immersive-web/hit-test#83, this test is currently impossible to pass for a browser implementing the current WebIDL specification. -- Update XRRay semantics Implements changes from immersive-web/hit-test#85 -- Fix world generation to have vertices member -- wpt-commits: 7513c92e130e043def925bda97e9fb9d8fd0b48f, 0fce2e94f0feb04966868a0b5795d76ea1db3906, 08c95d8ca3466c37df307cb775502a916e22b814 wpt-pr: 22868

…pec, a=testonly Automatic update from web-platform-tests WebXR - AR - adjust XRRay to match the spec Bring XRRay up to spec, adjusting the behavior with changes introduced by PR: immersive-web/hit-test#85 Additionally, fix Chrome's Web IDL to not rely on overloads. Change-Id: I98af1a4ed90dbeb8e311795e818efb64bb15034d -- wpt-commits: fd045759df43fa155b34a5e4fe4187610ad1bb24 wpt-pr: 23126

Manishearth added 3 commits April 8, 2020 17:18

Fix webidl for XRRay

0efd1b5

Update XRRay vector constructor to use correct webidl semantics

dd9d471

Fixes immersive-web#84 This adds a new initializer type so that the defaults work correctly.

Throw error for non-1 w coordinate for XRRay constructor's position

15a1697

Manishearth requested a review from bialpio April 9, 2020 19:45

Manishearth mentioned this pull request Apr 9, 2020

Current XRRay construction semantics incorrect #84

Closed

Manishearth commented Apr 9, 2020

View reviewed changes

index.bs Show resolved Hide resolved

Manishearth commented Apr 9, 2020

View reviewed changes

index.bs Show resolved Hide resolved

bialpio reviewed Apr 9, 2020

View reviewed changes

index.bs Show resolved Hide resolved

Throw on nonzero direction.w

9dba19d

bialpio merged commit 09bf18a into immersive-web:master Apr 10, 2020

Manishearth deleted the xrray-ctor branch April 10, 2020 20:26

Manishearth added a commit to Manishearth/servo that referenced this pull request Apr 10, 2020

Update XRRay semantics

a826463

Implements changes from immersive-web/hit-test#85

Manishearth added a commit to Manishearth/servo that referenced this pull request Apr 11, 2020

Update XRRay semantics

8d22310

Implements changes from immersive-web/hit-test#85

Manishearth added a commit to Manishearth/servo that referenced this pull request Apr 11, 2020

Update XRRay semantics

039a01d

Implements changes from immersive-web/hit-test#85

servo-wpt-sync pushed a commit to servo-wpt-sync/web-platform-tests that referenced this pull request Apr 11, 2020

Update XRRay semantics

58352d1

Implements changes from immersive-web/hit-test#85

servo-wpt-sync pushed a commit to servo-wpt-sync/web-platform-tests that referenced this pull request Apr 13, 2020

Update XRRay semantics

4b4c8fd

Implements changes from immersive-web/hit-test#85

Manishearth added a commit to web-platform-tests/wpt that referenced this pull request Apr 14, 2020

Update XRRay semantics

0fce2e9

Implements changes from immersive-web/hit-test#85

Manishearth added a commit to Manishearth/servo that referenced this pull request Apr 14, 2020

Update XRRay semantics

b294788

Implements changes from immersive-web/hit-test#85

Manishearth added a commit to Manishearth/servo that referenced this pull request Apr 14, 2020

Update XRRay semantics

9d9ee81

Implements changes from immersive-web/hit-test#85

Manishearth added a commit to Manishearth/servo that referenced this pull request Apr 16, 2020

Update XRRay semantics

1d38eb3

Implements changes from immersive-web/hit-test#85

Manishearth added a commit to Manishearth/servo that referenced this pull request Apr 19, 2020

Update XRRay semantics

378f321

Implements changes from immersive-web/hit-test#85

Manishearth added a commit to Manishearth/servo that referenced this pull request Apr 20, 2020

Update XRRay semantics

83f92a3

Implements changes from immersive-web/hit-test#85

chromium-wpt-export-bot mentioned this pull request Apr 21, 2020

WebXR - AR - adjust XRRay to match the spec web-platform-tests/wpt#23126

Merged

nosark pushed a commit to nosark/servo that referenced this pull request Jul 19, 2020

Update XRRay semantics

bd16741

Implements changes from immersive-web/hit-test#85

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Update XRRay vector constructor to use correct webidl semantics #85

Update XRRay vector constructor to use correct webidl semantics #85

Uh oh!

Manishearth commented Apr 9, 2020 •

edited by pr-preview bot

Loading

Uh oh!

Uh oh!

Manishearth Apr 9, 2020

Uh oh!

Uh oh!

Uh oh!

bialpio commented Apr 10, 2020

Uh oh!

Manishearth commented Apr 10, 2020

Uh oh!

Manishearth commented Apr 10, 2020

Uh oh!

bialpio commented Apr 10, 2020

Uh oh!

Manishearth commented Apr 10, 2020

Uh oh!

bialpio commented Apr 10, 2020

Uh oh!

Manishearth commented Apr 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Update XRRay vector constructor to use correct webidl semantics #85

Update XRRay vector constructor to use correct webidl semantics #85

Uh oh!

Conversation

Manishearth commented Apr 9, 2020 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Manishearth Apr 9, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

bialpio commented Apr 10, 2020

Uh oh!

Manishearth commented Apr 10, 2020

Uh oh!

Manishearth commented Apr 10, 2020

Uh oh!

bialpio commented Apr 10, 2020

Uh oh!

Manishearth commented Apr 10, 2020

Uh oh!

bialpio commented Apr 10, 2020

Uh oh!

Manishearth commented Apr 11, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Manishearth commented Apr 9, 2020 •

edited by pr-preview bot

Loading