1299929 - [webvr] Support HTC Vive button inputs and analogue triggers in the Gamepad API

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Comment 1

[Daosheng Mu[:daoshengmu]]

https://github.com/ValveSoftware/openvr/blob/b20b25705d8dd82be221fe68a61db36ae7e2608e/headers/openvr.h#L497

k_EButton_System, k_EButton_DPad_xx, and k_EButton_A can not receive the pressed events in OpenVR SDK 1.0.3. I have checked their sample, it has the same result.

Comment 2

[Daosheng Mu[:daoshengmu]]

Attachment: Part 1: Send GamepadChannelType in the the button and axis events; r?kip, lenzak800

Comment 3

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 1299929 - Part 2: Add extra info for VRController; r?kip

Comment 4

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 1299929 - Part 3: Handle VRController button inputs; r?kip

Comment 5

[Daosheng Mu[:daoshengmu]]

This bug depends on Bug 1299928's patches, we need to wait those patches to be landed.

Comment 6

[Daosheng Mu[:daoshengmu]]

Attachment: Using runnable thread to monitor input events.

This is an alternative way to monitor input events. However, our compositor message queue is usually busy, so it will be more latency than watching it at the VSync time. In the future, if we could move our VR module away the compositor thread, we can choose this approach.

Comment 7

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8800588 [details] [diff] [review]
Bug 1299929 - Part 3: Handle VRController button inputs; r?kip

Review of attachment 8800588 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/vr/VRManager.cpp
@@ +177,5 @@
>  
> +  for (uint32_t i = 0; i < mControllerManagers.Length(); ++i) {
> +    mControllerManagers[i]->HandleInput();
> +  }
> +

I move HandleInput() from RefreshVRControllers() to here. Because in RefreshVRControllers(), it would only be 90HZ. But, I think button inputs are more sensitive for users. We should not limit its frequency.

::: gfx/vr/gfxVROpenVR.cpp
@@ +67,5 @@
> +  vr::ButtonMaskFromId(vr::EVRButtonId::k_EButton_A),
> +  vr::ButtonMaskFromId(vr::EVRButtonId::k_EButton_SteamVR_Touchpad),
> +  vr::ButtonMaskFromId(vr::EVRButtonId::k_EButton_SteamVR_Trigger)
> +};
> +

In the current OpenVR SDK 1.0.3 of HTC Vive, it only sends k_EButton_ApplicationMenu, k_EButton_Grip, k_EButton_SteamVR_Touchpad, and k_EButton_SteamVR_Trigger events. Kip, do you think we should only keep these button events or list all of them?

Comment 8

[:kip (Kearwood Gilbert)]

(In reply to Daosheng Mu[:daoshengmu] from comment #6)
> Created attachment 8800589 [details] [diff] [review]
> Using runnable thread to monitor input events.
> 
> This is an alternative way to monitor input events. However, our compositor
> message queue is usually busy, so it will be more latency than watching it
> at the VSync time. In the future, if we could move our VR module away the
> compositor thread, we can choose this approach.

We will be moving the VR module away from the compositor thread, so this should help.  While I'm in Toronto next week, I'll discuss the plan with the GFX team and file some bugs for the work needed.

Comment 9

[:kip (Kearwood Gilbert)]

(In reply to Daosheng Mu[:daoshengmu] from comment #7)
> Comment on attachment 8800588 [details] [diff] [review]
> Bug 1299929 - Part 3: Handle VRController button inputs; r?kip
> 
> Review of attachment 8800588 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/vr/VRManager.cpp
> @@ +177,5 @@
> >  
> > +  for (uint32_t i = 0; i < mControllerManagers.Length(); ++i) {
> > +    mControllerManagers[i]->HandleInput();
> > +  }
> > +
> 
> I move HandleInput() from RefreshVRControllers() to here. Because in
> RefreshVRControllers(), it would only be 90HZ. But, I think button inputs
> are more sensitive for users. We should not limit its frequency.
> 
> ::: gfx/vr/gfxVROpenVR.cpp
> @@ +67,5 @@
> > +  vr::ButtonMaskFromId(vr::EVRButtonId::k_EButton_A),
> > +  vr::ButtonMaskFromId(vr::EVRButtonId::k_EButton_SteamVR_Touchpad),
> > +  vr::ButtonMaskFromId(vr::EVRButtonId::k_EButton_SteamVR_Trigger)
> > +};
> > +
> 
> In the current OpenVR SDK 1.0.3 of HTC Vive, it only sends
> k_EButton_ApplicationMenu, k_EButton_Grip, k_EButton_SteamVR_Touchpad, and
> k_EButton_SteamVR_Trigger events. Kip, do you think we should only keep
> these button events or list all of them?

Until we have either physical hardware with the extra buttons or a software test harness to test the other events, I think we can keep just these events for now.

If someone has implemented another OpenVR driver that uses the other events, we could easily add them later.

Comment 10

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 1299929 - Part 1: Send GamepadChannelType in the the button and axis events; r?kip, lenzak800

Update the patch based on Bug 1299928 Comment 75.

Comment 11

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 1299929 - Part 2: Add extra info for VRController; r?kip

Comment 12

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 1299929 - Part 3: Handle VRController button inputs; r?kip

Mark undefined button inputs for the current OpenVR devices. (Steam Vive in OpenVR SDK 1.0.3)

Comment 13

[:kip (Kearwood Gilbert)]

Comment on attachment 8801054 [details] [diff] [review]
Bug 1299929 - Part 1: Send GamepadChannelType in the the  button and axis events; r?kip, lenzak800

LGTM

Comment 14

[:kip (Kearwood Gilbert)]

Comment on attachment 8801056 [details] [diff] [review]
Bug 1299929 - Part 2: Add extra info for VRController;  r?kip

Looks good

Comment 15

[:kip (Kearwood Gilbert)]

Comment on attachment 8801057 [details] [diff] [review]
Bug 1299929 - Part 3: Handle VRController button inputs;  r?kip

Looks great, thanks!

Comment 16

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 1299929 - Part 1: Send GamepadServiceType in the the button and axis events;

Review commit: https://reviewboard.mozilla.org/r/85852/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85852/

Comment 17

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 1299929 - Part 2: Add extra info for VRController;

Review commit: https://reviewboard.mozilla.org/r/85854/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85854/

Comment 18

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 1299929 - Part 3: Handle VRController button inputs;

Review commit: https://reviewboard.mozilla.org/r/85856/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/85856/

Comment 19

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8802382 [details]
Bug 1299929 - Part 1: Send GamepadServiceType in the the button and axis events;

Carry r+ from Comment 13 and Comment 16

Comment 20

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8802383 [details]
Bug 1299929 - Part 2: Add extra info for VRController;

Carry r+ from Comment 14

Comment 21

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8802384 [details]
Bug 1299929 - Part 3: Handle VRController button inputs;

Carry r+ from Comment 15

Comment 22

[Daosheng Mu[:daoshengmu]]

Please just land the patches from MozReview, they are the same.

In this update, I replace nsTArray<uint64> gOpenVRButtonMask and gOpenVRAxisMask with uint64_t[]. Originally, I declare them in the global scope, it would have a memory leak report by BloatView. This is because BloatView throws the report before the program shutdown that the timing of gOpenVRButtonMask's memory to be deallocated. I decide change them to uint64[] that would be more simple and safe.

Try looks good. https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc91f7054e0a.

Comment 23

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8802384 [details]
Bug 1299929 - Part 3: Handle VRController button inputs;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/85856/diff/1-2/

Comment 24

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8802384 [details]
Bug 1299929 - Part 3: Handle VRController button inputs;

Carry r+ from Comment 15

In this update, I move mControllerManagers[i]->HandleInput() to if (bHaveEventListener) is true. Because we need VRControllerManager is inited before call HandleInput().

Comment 25

[Carsten Book [:Tomcat]]

could you update the comments/review flag in mozreview so we are able to use autoland ?

Comment 26

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8802382 [details]
Bug 1299929 - Part 1: Send GamepadServiceType in the the button and axis events;

https://reviewboard.mozilla.org/r/85852/#review86168

Carry r+ from Comment 13, 14, 15, 16.

Comment 27

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8802384 [details]
Bug 1299929 - Part 3: Handle VRController button inputs;

https://reviewboard.mozilla.org/r/85856/#review86170

Comment 28

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8802383 [details]
Bug 1299929 - Part 2: Add extra info for VRController;

https://reviewboard.mozilla.org/r/85854/#review86172

Carry r+ from Comment 14

Comment 29

[Daosheng Mu[:daoshengmu]]

(In reply to Carsten Book [:Tomcat] from comment #25)
> could you update the comments/review flag in mozreview so we are able to use
> autoland ?

I am not sure if I have done. If it was not, I probably need to ask my reviewers to give me a r+ again...

Comment 30

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8802384 [details]
Bug 1299929 - Part 3: Handle VRController button inputs;

https://reviewboard.mozilla.org/r/85856/#review86174

Carry r+ from comment 15

Comment 31

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8802382 [details]
Bug 1299929 - Part 1: Send GamepadServiceType in the the button and axis events;

The same patches but MozReview needs get the r+ for auto-land. I apologize for this.

Comment 32

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8802383 [details]
Bug 1299929 - Part 2: Add extra info for VRController;

The same patches but MozReview needs get the r+ for auto-land. I apologize for this.

Comment 33

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8802384 [details]
Bug 1299929 - Part 3: Handle VRController button inputs;

The same patches but MozReview needs get the r+ for auto-land. I apologize for this.

Comment 34

[:kip (Kearwood Gilbert)]

Comment on attachment 8802382 [details]
Bug 1299929 - Part 1: Send GamepadServiceType in the the button and axis events;

https://reviewboard.mozilla.org/r/85852/#review86250

Comment 35

[:kip (Kearwood Gilbert)]

Comment on attachment 8802383 [details]
Bug 1299929 - Part 2: Add extra info for VRController;

https://reviewboard.mozilla.org/r/85854/#review86252

Comment 36

[:kip (Kearwood Gilbert)]

Comment on attachment 8802384 [details]
Bug 1299929 - Part 3: Handle VRController button inputs;

https://reviewboard.mozilla.org/r/85856/#review86254

Comment 37

[:kip (Kearwood Gilbert)]

Comment on attachment 8802384 [details]
Bug 1299929 - Part 3: Handle VRController button inputs;

https://reviewboard.mozilla.org/r/85856/#review86256

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/90dd6a4de109
Part 1: Send GamepadServiceType in the the button and axis events; r=kip
https://hg.mozilla.org/integration/autoland/rev/031f5a617e7a
Part 2: Add extra info for VRController; r=kip
https://hg.mozilla.org/integration/autoland/rev/924d0cb42117
Part 3: Handle VRController button inputs; r=kip

Comment 39

[Michael Leu[:Lenzak](UTC+8)]

Comment on attachment 8802382 [details]
Bug 1299929 - Part 1: Send GamepadServiceType in the the button and axis events;

https://reviewboard.mozilla.org/r/85852/#review86514

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/90dd6a4de109
https://hg.mozilla.org/mozilla-central/rev/031f5a617e7a
https://hg.mozilla.org/mozilla-central/rev/924d0cb42117