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

Add experimental hand tracking support #26316

Merged
merged 6 commits into from Apr 28, 2020
Merged

Add experimental hand tracking support #26316

merged 6 commits into from Apr 28, 2020

Conversation

@Manishearth
Copy link
Member

Manishearth commented Apr 25, 2020

Depends on servo/webxr#162

Adds support for the experimental hand tracking API (with some tweaks made that I intend to upstream).

This needs servo/webxr#163 to actually run on any backend, however that depends on some openxrs changes.

If folks want to try this out, patch in
servo/webxr#163 and run https://manishearth.net/sand/three.js/examples/webxr_vr_paint.html . You also need to toggle the dom.webxr.hand pref.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #___ (GitHub issue number if applicable)
  • There are tests for these changes OR
  • These changes do not require tests because this is an experimental API
@highfive
Copy link

highfive commented Apr 25, 2020

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/webidls/XRFrame.webidl, components/script/dom/xrjointpose.rs, components/script/dom/webidls/XRHand.webidl, components/script/dom/webidls/XRJointPose.webidl, components/script/dom/xrspace.rs and 9 more
  • @KiChjang: components/script/dom/webidls/XRFrame.webidl, components/script/dom/xrjointpose.rs, components/script/dom/webidls/XRHand.webidl, components/script/dom/webidls/XRJointPose.webidl, components/script/dom/xrspace.rs and 9 more
@highfive
Copy link

highfive commented Apr 25, 2020

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
Copy link
Member Author

Manishearth commented Apr 25, 2020

@bors-servo
Copy link
Contributor

bors-servo commented Apr 25, 2020

The latest upstream changes (presumably #26312) made this pull request unmergeable. Please resolve the merge conflicts.

@Manishearth Manishearth force-pushed the Manishearth:hands branch from 50b311f to a1f941b Apr 25, 2020
@Manishearth Manishearth marked this pull request as ready for review Apr 25, 2020
@Manishearth
Copy link
Member Author

Manishearth commented Apr 25, 2020

This can be landed now. The actual feature is behind a pref, and it doesn't work anyway without the webxr openxr patches (servo/webxr#163), but we can get the impl in-tree at least.

@Manishearth Manishearth force-pushed the Manishearth:hands branch from a1f941b to bba2bfb Apr 25, 2020
@Manishearth Manishearth force-pushed the Manishearth:hands branch from bba2bfb to 2a75d2e Apr 27, 2020
Copy link
Member

asajeffrey left a comment

Just minor nits. Feel free to ignore them and r=me.

@@ -299,6 +299,8 @@ mod gen {
test: bool,
#[serde(default)]
glwindow: bool,
#[serde(default)]
hand: bool,

This comment has been minimized.

@asajeffrey

asajeffrey Apr 27, 2020

Member

should the pref be dom.webxr.hand or dom.webxr.hand.enabled?

This comment has been minimized.

@Manishearth

Manishearth Apr 27, 2020

Author Member

done, dom.webxr.hands.enabled (the API is called webxr-hands-input, so I picked this option instead)

@@ -881,6 +882,58 @@ where
}
}

unsafe impl<J> JSTraceable for Hand<J>

This comment has been minimized.

@asajeffrey

asajeffrey Apr 27, 2020

Member

Hmm, it would be nice if we could derive this, it'll be very easy to mess this up down the road :(

This comment has been minimized.

@Manishearth

Manishearth Apr 27, 2020

Author Member

Can't without introducing a mozjs dep to webxr

@@ -24,4 +24,7 @@ interface XRInputSource {
[SameObject] readonly attribute XRSpace? gripSpace;
// [SameObject] readonly attribute Gamepad? gamepad;
/* [SameObject] */ readonly attribute /* FrozenArray<DOMString> */ any profiles;

[Pref="dom.webxr.hand"]
readonly attribute XRHand? hand;

This comment has been minimized.

@asajeffrey

asajeffrey Apr 27, 2020

Member

I guess this is a spec nit, but hand singular?

This comment has been minimized.

@Manishearth

Manishearth Apr 27, 2020

Author Member

An input source only has one hand. If your input source has multiple hands, it might be time to call your doctor and/or plastic surgeon.

This comment has been minimized.

This comment has been minimized.

@Manishearth

Manishearth Apr 27, 2020

Author Member

(This also matches with gamepad -- a single input source is a single controller in a single hand, and it can have one gamepad and one associated hand)

@Manishearth Manishearth force-pushed the Manishearth:hands branch from 2a75d2e to 51f1c5e Apr 27, 2020
@Manishearth
Copy link
Member Author

Manishearth commented Apr 27, 2020

@bors-servo r=asajeffrey

@bors-servo
Copy link
Contributor

bors-servo commented Apr 27, 2020

📌 Commit 51f1c5e has been approved by asajeffrey

@bors-servo
Copy link
Contributor

bors-servo commented Apr 27, 2020

Testing commit 51f1c5e with merge 595160d...

bors-servo added a commit that referenced this pull request Apr 27, 2020
Add experimental hand tracking support

Depends on servo/webxr#162

Adds support for [the experimental hand tracking API](https://github.com/immersive-web/webxr-hands-input/blob/master/explainer.md) (with some tweaks made that I intend to upstream).

This needs servo/webxr#163 to actually run on any backend, however that depends on some openxrs changes.

If folks want to try this out, patch in
servo/webxr#163 and run https://manishearth.net/sand/three.js/examples/webxr_vr_paint.html . You also need to toggle the `dom.webxr.hand` pref.

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because this is an experimental API

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented Apr 27, 2020

💔 Test failed - status-taskcluster

@Manishearth Manishearth force-pushed the Manishearth:hands branch from 51f1c5e to 96eb24d Apr 27, 2020
@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2020

📌 Commit c30ad6c has been approved by asajeffrey

@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2020

Testing commit c30ad6c with merge b4f382e...

bors-servo added a commit that referenced this pull request Apr 28, 2020
Add experimental hand tracking support

Depends on servo/webxr#162

Adds support for [the experimental hand tracking API](https://github.com/immersive-web/webxr-hands-input/blob/master/explainer.md) (with some tweaks made that I intend to upstream).

This needs servo/webxr#163 to actually run on any backend, however that depends on some openxrs changes.

If folks want to try this out, patch in
servo/webxr#163 and run https://manishearth.net/sand/three.js/examples/webxr_vr_paint.html . You also need to toggle the `dom.webxr.hand` pref.

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because this is an experimental API

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2020

💔 Test failed - status-taskcluster

@Manishearth
Copy link
Member Author

Manishearth commented Apr 28, 2020

@bors-servo retry

  ▶ TIMEOUT [expected PASS] /html/infrastructure/common-microsyntaxes/colours/parsing-legacy-colour-value-ascii-case-insensitive.html
  │ 
  │ _RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL.
  │ Shutting down the Constellation after generating an output file or exit flag specified
  │ 24 threads are still running after shutdown (bad).
  └ _RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL.
  ▶ TIMEOUT [expected OK] /css/css-color/rgb-rounding-001.html
  ▶ TIMEOUT [expected OK] /css/css-text/white-space/seg-break-transformation-001.html
  ▶ TIMEOUT [expected OK] /css/css-text-decor/parsing/text-decoration-color-invalid.html
  ▶ TIMEOUT [expected OK] /css/css-text/inheritance.html

@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2020

Testing commit c30ad6c with merge 970c0ed...

bors-servo added a commit that referenced this pull request Apr 28, 2020
Add experimental hand tracking support

Depends on servo/webxr#162

Adds support for [the experimental hand tracking API](https://github.com/immersive-web/webxr-hands-input/blob/master/explainer.md) (with some tweaks made that I intend to upstream).

This needs servo/webxr#163 to actually run on any backend, however that depends on some openxrs changes.

If folks want to try this out, patch in
servo/webxr#163 and run https://manishearth.net/sand/three.js/examples/webxr_vr_paint.html . You also need to toggle the `dom.webxr.hand` pref.

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because this is an experimental API

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2020

Testing commit c30ad6c with merge 000fd93...

bors-servo added a commit that referenced this pull request Apr 28, 2020
Add experimental hand tracking support

Depends on servo/webxr#162

Adds support for [the experimental hand tracking API](https://github.com/immersive-web/webxr-hands-input/blob/master/explainer.md) (with some tweaks made that I intend to upstream).

This needs servo/webxr#163 to actually run on any backend, however that depends on some openxrs changes.

If folks want to try this out, patch in
servo/webxr#163 and run https://manishearth.net/sand/three.js/examples/webxr_vr_paint.html . You also need to toggle the `dom.webxr.hand` pref.

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because this is an experimental API

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2020

💔 Test failed - status-taskcluster

@Manishearth
Copy link
Member Author

Manishearth commented Apr 28, 2020

@bors-servo retry

7 unexpected results that are NOT known-intermittents:
  ▶ TIMEOUT [expected FAIL] /css/css-text/shaping/shaping-007.html
  ▶ TIMEOUT [expected PASS] /css/css-text/shaping/shaping-003.html
  ▶ TIMEOUT [expected FAIL] /css/CSS2/css21-errata/s-11-1-1b-009.html
  ▶ TIMEOUT [expected FAIL] /css/css-text/shaping/shaping-005.html
  ▶ TIMEOUT [expected PASS] /css/css-text/shaping/shaping-002.html
  │ 
  │ Shutting down the Constellation after generating an output file or exit flag specified
  └ 24 threads are still running after shutdown (bad).
  ▶ TIMEOUT [expected PASS] /css/css-text/shaping/shaping-001.html
  │ 
  │ Shutting down the Constellation after generating an output file or exit flag specified
  └ 45 threads are still running after shutdown (bad).
  ▶ TIMEOUT [expected FAIL] /css/css-text/shaping/shaping-006.html
  │ 
  │ Shutting down the Constellation after generating an output file or exit flag specified
  └ 45 threads are still running after shutdown (bad).
@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2020

Testing commit c30ad6c with merge 9ea4b10...

bors-servo added a commit that referenced this pull request Apr 28, 2020
Add experimental hand tracking support

Depends on servo/webxr#162

Adds support for [the experimental hand tracking API](https://github.com/immersive-web/webxr-hands-input/blob/master/explainer.md) (with some tweaks made that I intend to upstream).

This needs servo/webxr#163 to actually run on any backend, however that depends on some openxrs changes.

If folks want to try this out, patch in
servo/webxr#163 and run https://manishearth.net/sand/three.js/examples/webxr_vr_paint.html . You also need to toggle the `dom.webxr.hand` pref.

<!-- Please describe your changes on the following line: -->

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `___` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [ ] These changes fix #___ (GitHub issue number if applicable)

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because this is an experimental API

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->
@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2020

💥 Test timed out

@jdm
Copy link
Member

jdm commented Apr 28, 2020

@bors-servo retry

@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2020

Testing commit c30ad6c with merge 1f34c55...

@bors-servo
Copy link
Contributor

bors-servo commented Apr 28, 2020

☀️ Test successful - status-taskcluster
Approved by: asajeffrey
Pushing 1f34c55 to master...

@bors-servo bors-servo merged commit 1f34c55 into servo:master Apr 28, 2020
2 checks passed
2 checks passed
Community-TC (pull_request) TaskGroup: success
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.