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

RESOLVED FIXED in Firefox 52

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: kip, Assigned: daoshengmu)

Tracking

unspecified
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [gfx-noted])

Attachments

(4 attachments, 6 obsolete attachments)

2.70 KB, patch
Details | Diff | Splinter Review
58 bytes, text/x-review-board-request
kip
: review+
cleu
: review+
Details
58 bytes, text/x-review-board-request
kip
: review+
Details
58 bytes, text/x-review-board-request
kip
: review+
Details
Comment hidden (empty)
(Reporter)

Updated

3 years ago
Blocks: 1299928
No longer blocks: 1299926
(Reporter)

Updated

3 years ago
Blocks: 1299926
No longer blocks: 1299928

Updated

3 years ago
Whiteboard: [gfx-noted]
(Assignee)

Updated

3 years ago
Assignee: nobody → dmu
(Assignee)

Comment 1

3 years ago
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.
(Assignee)

Comment 5

3 years ago
This bug depends on Bug 1299928's patches, we need to wait those patches to be landed.
(Assignee)

Comment 6

3 years ago
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.
(Assignee)

Comment 7

3 years ago
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?
(Reporter)

Comment 8

3 years ago
(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.
(Reporter)

Comment 9

3 years ago
(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.
(Assignee)

Comment 10

3 years ago
Update the patch based on Bug 1299928 Comment 75.
Attachment #8800585 - Attachment is obsolete: true
Attachment #8800585 - Flags: review?(kgilbert)
Attachment #8800585 - Flags: review?(cleu)
Attachment #8801054 - Flags: review?(kgilbert)
Attachment #8801054 - Flags: review?(cleu)
(Assignee)

Comment 11

3 years ago
Attachment #8800586 - Attachment is obsolete: true
Attachment #8800586 - Flags: review?(kgilbert)
Attachment #8801056 - Flags: review?(kgilbert)
(Assignee)

Comment 12

3 years ago
Mark undefined button inputs for the current OpenVR devices. (Steam Vive in OpenVR SDK 1.0.3)
Attachment #8800588 - Attachment is obsolete: true
Attachment #8800588 - Flags: review?(kgilbert)
Attachment #8801057 - Flags: review?(kgilbert)
(Reporter)

Comment 13

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

LGTM
Attachment #8801054 - Flags: review?(kgilbert) → review+
(Reporter)

Comment 14

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

Looks good
Attachment #8801056 - Flags: review?(kgilbert) → review+
(Reporter)

Comment 15

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

Looks great, thanks!
Attachment #8801057 - Flags: review?(kgilbert) → review+
Attachment #8801054 - Flags: review?(cleu) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 19

3 years ago
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
Attachment #8802382 - Flags: review?(kgilbert)
Attachment #8802382 - Flags: review?(cleu)
Attachment #8802382 - Flags: review+
(Assignee)

Comment 20

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

Carry r+ from Comment 14
Attachment #8802383 - Flags: review?(kgilbert) → review+
(Assignee)

Comment 21

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

Carry r+ from Comment 15
Attachment #8802384 - Flags: review?(kgilbert) → review+
(Assignee)

Comment 22

3 years ago
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.
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Attachment #8801054 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8801056 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8801057 - Attachment is obsolete: true
Comment hidden (mozreview-request)
(Assignee)

Comment 24

3 years ago
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().
Attachment #8802384 - Flags: review?(kgilbert) → review+
could you update the comments/review flag in mozreview so we are able to use autoland ?
Flags: needinfo?(dmu)
(Assignee)

Comment 26

3 years ago
mozreview-review
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.
(Assignee)

Comment 27

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

https://reviewboard.mozilla.org/r/85856/#review86170
Attachment #8802384 - Flags: review+
(Assignee)

Comment 28

3 years ago
mozreview-review
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
(Assignee)

Comment 29

3 years ago
(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...
Flags: needinfo?(dmu)
(Assignee)

Comment 30

3 years ago
mozreview-review
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
Attachment #8802384 - Flags: review+
(Assignee)

Comment 31

3 years ago
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.
Attachment #8802382 - Flags: review?(kgilbert)
Attachment #8802382 - Flags: review?(cleu)
Attachment #8802382 - Flags: review+
(Assignee)

Comment 32

3 years ago
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.
Attachment #8802383 - Flags: review+ → review?(kgilbert)
(Assignee)

Comment 33

3 years ago
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.
Attachment #8802384 - Flags: review+ → review?(kgilbert)
(Reporter)

Comment 34

3 years ago
mozreview-review
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
Attachment #8802382 - Flags: review?(kgilbert) → review+
(Reporter)

Comment 35

3 years ago
mozreview-review
Comment on attachment 8802383 [details]
Bug 1299929 - Part 2: Add extra info for VRController;

https://reviewboard.mozilla.org/r/85854/#review86252
Attachment #8802383 - Flags: review?(kgilbert) → review+
(Reporter)

Comment 36

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

https://reviewboard.mozilla.org/r/85856/#review86254
(Reporter)

Comment 37

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

https://reviewboard.mozilla.org/r/85856/#review86256
Attachment #8802384 - Flags: review?(kgilbert) → review+

Comment 38

3 years ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed

Comment 39

3 years ago
mozreview-review
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
Attachment #8802382 - Flags: review?(cleu) → review+

Comment 40

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/90dd6a4de109
https://hg.mozilla.org/mozilla-central/rev/031f5a617e7a
https://hg.mozilla.org/mozilla-central/rev/924d0cb42117
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.