Closed Bug 1299929 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: kip, Assigned: daoshengmu)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(4 files, 6 obsolete files)

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
No description provided.
Blocks: 1299928
No longer blocks: 1299926
Blocks: 1299926
No longer blocks: 1299928
Whiteboard: [gfx-noted]
Assignee: nobody → dmu
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.
This bug depends on Bug 1299928's patches, we need to wait those patches to be landed.
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 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?
(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.
(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.
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)
Attachment #8800586 - Attachment is obsolete: true
Attachment #8800586 - Flags: review?(kgilbert)
Attachment #8801056 - Flags: review?(kgilbert)
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)
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+
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+
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 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+
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+
Comment on attachment 8802384 [details]
Bug 1299929 - Part 3: Handle VRController button inputs;

Carry r+ from Comment 15
Attachment #8802384 - Flags: review?(kgilbert) → review+
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.
Keywords: checkin-needed
Attachment #8801054 - Attachment is obsolete: true
Attachment #8801056 - Attachment is obsolete: true
Attachment #8801057 - Attachment is obsolete: true
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)
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 on attachment 8802384 [details]
Bug 1299929 - Part 3: Handle VRController button inputs;

https://reviewboard.mozilla.org/r/85856/#review86170
Attachment #8802384 - Flags: 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
(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)
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+
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+
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)
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)
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+
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+
Comment on attachment 8802384 [details]
Bug 1299929 - Part 3: Handle VRController button inputs;

https://reviewboard.mozilla.org/r/85856/#review86254
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+
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 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+
You need to log in before you can comment on or make changes to this bug.