1305889 - [webvr] Enumerate Oculus Touch Controllers through the Gamepad API

Status: RESOLVED FIXED

(Core :: Graphics, defect, P3)

Comment 1

[Daosheng Mu[:daoshengmu]]

VRDisplayOculusand and VRControllerManagerOculus should share the same ovrSession for polling sensor state. Otherwise, it would drop framerate.

Comment 2

[Daosheng Mu[:daoshengmu]]

I purpose to do some refactor on the current architecture. We need to create an OculusDeviceManager / OpenVRDeviceManager to help us handle the creation of ovrSession and IVRSystem. ovrSession and IVRSystem should be shared between VRDisplayManager and VRControllerManager. Therefore, in this design, the DeviceManager is responsible for loading runtime, ovrSession or IVRSystem creation, and destroy VRDisplayManager and VRControllerManager.

Comment 3

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 1305889 - Part 1: Move GamepadHand API to VRControllerHost;

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

Comment 4

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 1305889 - Part 2: Using VRControllerInfo as the parameter for AddGamepad;

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

Comment 5

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 1305889 - Part 3: Enumerate Oculus Touch Controllers;

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

Comment 6

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8833891 [details]
Bug 1305889 - Part 2: Using VRControllerInfo as the parameter for AddGamepad;

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

Comment 7

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8833892 [details]
Bug 1305889 - Part 3: Enumerate Oculus Touch Controllers;

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

Comment 8

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8833892 [details]
Bug 1305889 - Part 3: Enumerate Oculus Touch Controllers;

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

Comment 9

[Daosheng Mu[:daoshengmu]]

(In reply to Daosheng Mu[:daoshengmu] from comment #8)
> Comment on attachment 8833892 [details]
> Bug 1305889 - Part 3: Enumerate Oculus Touch Controllers;
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/110008/diff/2-3/

I am going to change the coding style of global constants to be prefix with k and make them to be static const as Bug 1299937 Comment 52.

Comment 10

[:kip (Kearwood Gilbert)]

Comment on attachment 8833892 [details]
Bug 1305889 - Part 3: Enumerate Oculus Touch Controllers;

https://reviewboard.mozilla.org/r/110008/#review111234

This all looks good.  The only change I would recommend is to use a more specific literal for the Oculus Touch controlers as we can describe them more directly than as "Oculus Gamepad".

r=me with the literal change.

Thanks!

::: gfx/vr/gfxVROculus.cpp:840
(Diff revision 3)
> +
> +VRControllerOculus::VRControllerOculus()
> +  : VRControllerHost(VRDeviceType::Oculus)
> +{
> +  MOZ_COUNT_CTOR_INHERITED(VRControllerOculus, VRControllerHost);
> +  mControllerInfo.mControllerName.AssignLiteral("Oculus Gamepad");

In this case, we know that the controller is an Oculus Touch controller.  Could we assign a more specific literal, such as "Oculus Touch Controller" here?

Comment 11

[:kip (Kearwood Gilbert)]

Comment on attachment 8833891 [details]
Bug 1305889 - Part 2: Using VRControllerInfo as the parameter for AddGamepad;

https://reviewboard.mozilla.org/r/110006/#review111228

This looks good, thanks!

Comment 12

[:kip (Kearwood Gilbert)]

Comment on attachment 8833890 [details]
Bug 1305889 - Part 1: Move GamepadHand API to VRControllerHost;

https://reviewboard.mozilla.org/r/110004/#review111242

This looks good

Comment 13

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8833892 [details]
Bug 1305889 - Part 3: Enumerate Oculus Touch Controllers;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/110008/diff/3-4/

Comment 14

[Daosheng Mu[:daoshengmu]]

(In reply to Daosheng Mu[:daoshengmu] from comment #13)
> Comment on attachment 8833892 [details]
> Bug 1305889 - Part 3: Enumerate Oculus Touch Controllers;
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/110008/diff/3-4/

Follow Comment 10, replace "Oculus Gamepad" with "Oculus Touch Controller". I change "OpenVR Gamepad" to "OpenVR Controller" as well.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/270900fec932
Part 1: Move GamepadHand API to VRControllerHost; r=kip
https://hg.mozilla.org/integration/autoland/rev/595bbe23999f
Part 2: Using VRControllerInfo as the parameter for AddGamepad; r=kip
https://hg.mozilla.org/integration/autoland/rev/7ec2ddd042e6
Part 3: Enumerate Oculus Touch Controllers; r=kip

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/270900fec932
https://hg.mozilla.org/mozilla-central/rev/595bbe23999f
https://hg.mozilla.org/mozilla-central/rev/7ec2ddd042e6