Gamepad API implementation #16260

MortimerGoro · 2017-04-04T18:19:32Z

Gamepad API implementation. Tested with HTC Vive and Daydream controllers ;)

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #10977

There are tests for these changes OR
These changes do not require tests because : current gamepad tests are manual (https://github.com/w3c/web-platform-tests/tree/master/gamepad). There is a open issue about the best way to test WebVR/Gamepad without real devices immersive-web/webxr#187. We'll work on the testing suite & mock devices/data on a separate issue.

This change is

highfive · 2017-04-04T18:19:36Z

Heads up! This PR modifies the following files:

@asajeffrey: components/constellation/constellation.rs
@wafflespeanut: python/tidy/servo_tidy/tidy.py
@fitzgen: components/script/dom/gamepadbutton.rs, components/script/dom/navigator.rs, components/script/dom/gamepadbuttonlist.rs, components/script/dom/mod.rs, components/script/dom/vr.rs, components/script/dom/gamepad.rs, components/script_traits/lib.rs, components/script_traits/lib.rs, components/script/dom/webidls/GamepadEvent.webidl, components/script/dom/webidls/GamepadList.webidl, components/script/dom/gamepadlist.rs, components/script/dom/vrpose.rs, components/script/dom/webidls/Navigator.webidl, components/script/dom/webidls/GamepadButton.webidl, components/script/dom/webidls/GamepadButtonList.webidl, components/script/dom/gamepadevent.rs, components/script/dom/webidls/Gamepad.webidl, components/script/script_thread.rs
@KiChjang: components/script/dom/gamepadbutton.rs, components/script/dom/navigator.rs, components/script/dom/gamepadbuttonlist.rs, components/script/dom/mod.rs, components/script/dom/vr.rs, components/script/dom/gamepad.rs, components/script_traits/lib.rs, components/script_traits/lib.rs, components/script/dom/webidls/GamepadEvent.webidl, components/script/dom/webidls/GamepadList.webidl, components/script/dom/gamepadlist.rs, components/script/dom/vrpose.rs, components/script/dom/webidls/Navigator.webidl, components/script/dom/webidls/GamepadButton.webidl, components/script/dom/webidls/GamepadButtonList.webidl, components/script/dom/gamepadevent.rs, components/script/dom/webidls/Gamepad.webidl, components/script/script_thread.rs

highfive · 2017-04-04T18:19:37Z

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!

nox

First review pass.

nox · 2017-04-05T08:35:19Z

components/script/dom/gamepad.rs

+#[dom_struct]
+pub struct Gamepad {
+    reflector_: Reflector,
+    gamepad_id: Cell<u64>,


This field does not appear to need to be mutable, use a bare u64 instead.

nox · 2017-04-05T08:35:19Z

components/script/dom/gamepad.rs

+pub struct Gamepad {
+    reflector_: Reflector,
+    gamepad_id: Cell<u64>,
+    id: DOMRefCell<String>,


This field does not appear to need to be mutable, use a bare String instead.

nox · 2017-04-05T08:35:19Z

components/script/dom/gamepad.rs

+    reflector_: Reflector,
+    gamepad_id: Cell<u64>,
+    id: DOMRefCell<String>,
+    index: Cell<u32>,


This field does not appear to need to be mutable, use a bare u32 instead.

Now it's mutable to ensure that the index is correct if more Gamepad providers are added in the future.

If more gamepad providers are added, why would the index of this particular gamepad change?

I'm talking about not WebVR related gamepad providers. The index is unkown until it is added to the global gamepad array here: 42f8a81#diff-4ca88ffda625c3517cee6640aac60b99R140 The index must match the final position in the array.

I guess I don't understand why this field is needed then. Could you document it?

Oh I understand now. Could you find a different design for this? I'm pretty sure we don't even need the index at all for now, it would matter only if you had hundreds of gamepads. No need to keep the index in the list in mind, just linearly search for it in the GamepadList object.

In order to do the dynamic search I would need to add a reference to the Gamepadlist in a Gamepad object (Gamepads are unaware of the container) which seems worse than a plain i32 automatically updated by the gamepadlist. Another way would be to get the gamepadlist from window->navigator->gamepadlist. Not too fond on both ideas, but I prefer the second solution. What do you think?

nox · 2017-04-05T08:35:19Z

components/script/dom/gamepad.rs

+    id: DOMRefCell<String>,
+    index: Cell<u32>,
+    connected: Cell<bool>,
+    timestamp: Cell<f64>,


This field does not appear to need to be mutable, use a bare f64 instead.

nox · 2017-04-05T08:35:19Z

components/script/dom/gamepad.rs

+    index: Cell<u32>,
+    connected: Cell<bool>,
+    timestamp: Cell<f64>,
+    mapping_type: DOMRefCell<String>,


This field does not appear to need to be mutable, use a bare String instead.

nox · 2017-04-05T08:35:19Z

components/script/dom/vr.rs

+                     .map(|g| Root::from_ref(&**g))
+    }
+
+    fn sync_gamepad(&self, data: Option<WebVRGamepadData>, state: &WebVRGamepadState) {


Take a Option<&WebVRGamepadData> here, this avoids a cloning operation when handling WebVRGamepadEvent::Connect above.

Changed to use move semantics, so the clone above is not required either

nox · 2017-04-05T08:35:19Z

components/script_traits/lib.rs

-    /// Notifies the script thread of a WebVR device event
-    WebVREvent(PipelineId, WebVREventMsg)
+    /// Notifies the script thread of WebVR events.
+    WebVREvent(PipelineId, Vec<WebVREventMsg>)


Rename this WebVREvents.

nox · 2017-04-05T08:35:19Z

components/script_traits/lib.rs

-    /// Dispatch a WebVR event to the subscribed script threads.
-    WebVREvent(Vec<PipelineId>, WebVREventMsg),
+    /// Dispatch WebVR events to the subscribed script threads.
+    WebVREvent(Vec<PipelineId>, Vec<WebVREventMsg>),


Rename this WebVREvents.

nox · 2017-04-05T08:35:19Z

components/webvr/webvr_thread.rs

+            match ev {
+                VREvent::Display(ev) => WebVREventMsg::DisplayEvent(ev),
+                VREvent::Gamepad(ev) => WebVREventMsg::GamepadEvent(ev),
+            }


Why do we even have a WebVREventMsg type if VREvent is exactly the same?

I did that to have the possibility to add some internal events in the future. But it's not needed now, so I unified the event and will only change that if required in the future.

nox · 2017-04-05T08:35:19Z

python/tidy/servo_tidy/tidy.py

@@ -74,6 +74,7 @@
    "//url.spec.whatwg.org",
    "//xhr.spec.whatwg.org",
    "//w3c.github.io",
+    "//www.w3.org",


No, change the spec links.

nox · 2017-04-06T14:16:27Z

What is the point of the GamepadList object if it doesn't hold onto the gamepads? What hold onto them currently?

MortimerGoro · 2017-04-06T14:46:54Z

The GamepadList object has 2 main purposes:

It's used as a fast & GC friendly container to share the gamepad list between rust and javascript, without creating/updating JS arrays every frame.
Allows to implement the IndexGetter which will be used to hide gamepads according to privacy rules. The gamepad draft allows things like [null, [object Gamepad]]

In the prev comment I meant that Gamepad objects don't have a "parent" equivalent reference now, So I need to add the reference or go throught window.navigator to perform the linear search for the index property.

nox · 2017-04-06T14:50:28Z

Sorry, I still don't understand why you need this index. I feel like this lacks a lot of documentation.

nox · 2017-04-06T14:51:59Z

OOOH, disregard everything I just said, only now I realise the index is actually exposed to the Web and it is a concept defined in the spec. I guess storing the index isn't a bad idea then. Will review the rest of the changes you made.

MortimerGoro · 2017-04-06T15:02:55Z

Ok, I'll store it as i32 field then. I was confused when reading the spec too, it feels weird to expose the index of an external array inside the gamepad object, but it's defined that way: https://w3c.github.io/gamepad/#dom-gamepad-index

nox · 2017-04-07T11:56:42Z

components/script/dom/gamepad.rs

+unsafe_no_jsmanaged_fields!(WebVRGamepadHand);
+
+impl Gamepad {
+    pub fn new_inherited(gamepad_id: u32,


Doesn't need to be public.

nox · 2017-04-07T11:56:42Z

components/script/dom/gamepad.rs

+    display_id: u32
+}
+
+unsafe_no_jsmanaged_fields!(WebVRGamepadHand);


Put that in the trace module, not here in the middle of nowhere.

nox · 2017-04-07T11:56:42Z

components/script/dom/gamepadbuttonlist.rs

+use dom_struct::dom_struct;
+use webvr_traits::WebVRGamepadButton;
+
+// https://www.w3.org/TR/GamepadButton/


This spec link is wrong.

nox · 2017-04-07T11:56:42Z

components/script/dom/gamepadbuttonlist.rs

+
+impl GamepadButtonList {
+    #[allow(unrooted_must_root)]
+    fn new_inherited(list: Vec<JS<GamepadButton>>) -> GamepadButtonList {


Pass &[&GamepadButton] here (see the r method on [JS<T>]).

Thanks, the r() method is very useful for this ;)

nox · 2017-04-07T11:56:42Z

components/script/dom/gamepadlist.rs

+    }
+
+    fn add(&self, gamepad: &Gamepad) {
+        self.list.borrow_mut().push(JS::from_ref(&*gamepad));


What happens if the gamepad was already in the list?

This method was only called internally by add_if_not_exists. I have removed it to avoid confusions with the add_if_not_exists method.

nox · 2017-04-07T11:56:42Z

components/script/dom/gamepadlist.rs

+
+    pub fn add_if_not_exists(&self, gamepads: &[Root<Gamepad>]) {
+        for gamepad in gamepads {
+            if self.list.borrow().iter().any(|g| g.gamepad_id() == gamepad.gamepad_id()) {


This will add the gamepad only if it is already in the list.

nox · 2017-04-07T11:56:42Z

components/script/dom/gamepadlist.rs

+    // https://w3c.github.io/gamepad/#dom-navigator-getgamepads
+    fn Item(&self, index: u32) -> Option<Root<Gamepad>> {
+        if (index as usize) < self.list.borrow().len() {
+            Some(Root::from_ref(&*(self.list.borrow()[index as usize])))


Do the same thing as in GamepadButtonList::Item here.

nox · 2017-04-07T11:57:04Z

Cc @larsbergstrom or anyone else with a VR kit that want to manually test this branch.

nox · 2017-04-11T08:33:36Z

This needs a squash, and I would like @larsbergstrom to take a quick look. LGTM otherwise.

nox · 2017-04-12T13:11:03Z

.gitignore

@@ -27,3 +27,5 @@ Servo.app

 # VSCode
 .vscode
+
+*.iml


What is this file extension btw?

It's a file automatically created by Android Studio (I use it to test on Android).

I'll remove it from here because that change belongs more to #15773, which is about to be merged


        Implement Gamepad API

MortimerGoro · 2017-04-12T16:38:31Z

Squash ready ;)

larsbergstrom · 2017-04-13T21:10:39Z

@bors-servo r=nox,larsbergstrom

I've tested the gamekit stuff and can confirm controllers work :-)

bors-servo · 2017-04-13T21:10:40Z

📌 Commit 0158b5b has been approved by nox,larsbergstrom

bors-servo · 2017-04-13T21:10:44Z

⌛ Testing commit 0158b5b with merge 26c4527...


        Auto merge of #16260 - MortimerGoro:gamepad, r=nox,larsbergstrom

Gamepad API implementation  Gamepad API implementation. Tested with HTC Vive and Daydream controllers ;) ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #10977  - [ ] There are tests for these changes OR - [X] These changes do not require tests because : current gamepad tests are manual (https://github.com/w3c/web-platform-tests/tree/master/gamepad). There is a open issue about the best way to test WebVR/Gamepad without real devices immersive-web/webxr#187. We'll work on the testing suite & mock devices/data on a separate issue.    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16260)

bors-servo · 2017-04-13T21:47:07Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-msvc-dev
Approved by: nox,larsbergstrom
Pushing 26c4527 to master...

danShumway · 2019-01-10T04:20:18Z

Did this ever make its way into master?

I'm still getting navigator.getGamepads is not a function. Looking around, I don't see a build flag mentioned anywhere. Any chance there's something obvious I'm missing?

jdm · 2019-01-10T04:21:56Z

It's prefed off via the dom.gamepad.enabled preference. That can be controlled from prefs.json or running with --pref dom.gamepad.enabled.

danShumway · 2019-01-10T13:07:47Z

Thanks! Just for future reference so I don't need to bother people as much, I assume this is the list of existing preferences?

jdm · 2019-01-10T15:26:29Z

Generally yes.

highfive assigned asajeffrey Apr 4, 2017

highfive added the S-awaiting-review label Apr 4, 2017

nox assigned nox and unassigned asajeffrey Apr 5, 2017

nox requested changes Apr 5, 2017

View changes

nox added S-needs-code-changes and removed S-awaiting-review labels Apr 5, 2017

highfive added S-awaiting-review and removed S-needs-code-changes labels Apr 6, 2017

MortimerGoro force-pushed the MortimerGoro:gamepad branch 2 times, most recently from dd6acf8 to 42f8a81 Apr 6, 2017

nox requested changes Apr 7, 2017

View changes

MortimerGoro force-pushed the MortimerGoro:gamepad branch from 56976e3 to 50a5c7c Apr 7, 2017

nox assigned larsbergstrom Apr 11, 2017

nox added the S-needs-squash label Apr 11, 2017

nox approved these changes Apr 11, 2017

View changes

MortimerGoro force-pushed the MortimerGoro:gamepad branch from 50a5c7c to fc92e9d Apr 11, 2017

nox reviewed Apr 12, 2017

View changes

Implement Gamepad API

Loading status checks…

0158b5b

MortimerGoro force-pushed the MortimerGoro:gamepad branch from fc92e9d to 0158b5b Apr 12, 2017

highfive unassigned larsbergstrom Apr 13, 2017

highfive added S-awaiting-merge and removed S-awaiting-review S-needs-squash labels Apr 13, 2017

bors-servo merged commit 0158b5b into servo:master Apr 13, 2017
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Apr 13, 2017

bors-servo mentioned this pull request Apr 13, 2017

[WIP] PointerEvent #16273

Closed

8 of 11 tasks complete

servo / servo

Join GitHub today

Gamepad API implementation #16260

Gamepad API implementation #16260

Conversation

MortimerGoro commented Apr 4, 2017 • edited

highfive commented Apr 4, 2017

highfive commented Apr 4, 2017

nox 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.

MortimerGoro Apr 6, 2017 • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

MortimerGoro Apr 6, 2017 • edited

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.

MortimerGoro Apr 6, 2017 • edited

This comment has been minimized.

nox commented Apr 6, 2017

MortimerGoro commented Apr 6, 2017

nox commented Apr 6, 2017

nox commented Apr 6, 2017

MortimerGoro commented Apr 6, 2017 • edited

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.

nox commented Apr 7, 2017

nox commented Apr 11, 2017

This comment has been minimized.

This comment has been minimized.

MortimerGoro commented Apr 12, 2017

larsbergstrom commented Apr 13, 2017

bors-servo commented Apr 13, 2017

bors-servo commented Apr 13, 2017

bors-servo commented Apr 13, 2017

danShumway commented Jan 10, 2019

jdm commented Jan 10, 2019

danShumway commented Jan 10, 2019

jdm commented Jan 10, 2019

MortimerGoro commented Apr 4, 2017 •

edited

MortimerGoro Apr 6, 2017 •

edited

MortimerGoro Apr 6, 2017 •

edited

MortimerGoro Apr 6, 2017 •

edited

MortimerGoro commented Apr 6, 2017 •

edited