If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 52

Status

()

Core
Graphics
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: kip, Assigned: daoshengmu)

Tracking

unspecified
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

(Whiteboard: [gfx-noted])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments, 6 obsolete attachments)

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

Updated

a year ago
Blocks: 1299928
No longer blocks: 1299926
(Reporter)

Updated

a year ago
Blocks: 1299926
No longer blocks: 1299928

Updated

a year ago
Whiteboard: [gfx-noted]
(Assignee)

Updated

a year ago
Assignee: nobody → dmu
(Assignee)

Comment 1

a year 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 2

11 months ago
Created attachment 8800585 [details] [diff] [review]
Part 1: Send GamepadChannelType in the the button and axis events; r?kip, lenzak800
Attachment #8800585 - Flags: review?(kgilbert)
Attachment #8800585 - Flags: review?(cleu)
(Assignee)

Comment 3

11 months ago
Created attachment 8800586 [details] [diff] [review]
Bug 1299929 - Part 2: Add extra info for VRController; r?kip
Attachment #8800586 - Flags: review?(kgilbert)
(Assignee)

Comment 4

11 months ago
Created attachment 8800588 [details] [diff] [review]
Bug 1299929 - Part 3: Handle VRController button inputs; r?kip
Attachment #8800588 - Flags: review?(kgilbert)
(Assignee)

Comment 5

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

Comment 6

11 months ago
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.
(Assignee)

Comment 7

11 months 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

11 months 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

11 months 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

11 months ago
Created attachment 8801054 [details] [diff] [review]
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.
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

11 months ago
Created attachment 8801056 [details] [diff] [review]
Bug 1299929 - Part 2: Add extra info for VRController;  r?kip
Attachment #8800586 - Attachment is obsolete: true
Attachment #8800586 - Flags: review?(kgilbert)
Attachment #8801056 - Flags: review?(kgilbert)
(Assignee)

Comment 12

11 months ago
Created attachment 8801057 [details] [diff] [review]
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)
Attachment #8800588 - Attachment is obsolete: true
Attachment #8800588 - Flags: review?(kgilbert)
Attachment #8801057 - Flags: review?(kgilbert)
(Reporter)

Comment 13

11 months 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

11 months 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

11 months 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

11 months 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

11 months 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

11 months 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

11 months 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

11 months ago
Keywords: checkin-needed
(Assignee)

Updated

11 months ago
Attachment #8801054 - Attachment is obsolete: true
(Assignee)

Updated

11 months ago
Attachment #8801056 - Attachment is obsolete: true
(Assignee)

Updated

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

Comment 24

11 months 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+

Comment 25

11 months ago
could you update the comments/review flag in mozreview so we are able to use autoland ?
Flags: needinfo?(dmu)
(Assignee)

Comment 26

11 months 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

11 months 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

11 months 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

11 months 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

11 months 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

11 months 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

11 months 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

11 months 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

11 months 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

11 months 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

11 months 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

11 months 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

11 months 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

11 months 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

11 months 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: 11 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.