Closed Bug 1305889 Opened 3 years ago Closed 3 years ago

[webvr] Enumerate Oculus Touch Controllers through the Gamepad API

Categories

(Core :: Graphics, defect, P3)

x86_64
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: kip, Assigned: daoshengmu)

References

Details

(Keywords: feature, Whiteboard: [gfx-noted][webvr])

Attachments

(3 files)

No description provided.
Keywords: feature
OS: Unspecified → Windows
Priority: -- → P3
Hardware: Unspecified → x86_64
Whiteboard: [gfx-noted]
Whiteboard: [gfx-noted] → [gfx-noted][webvr]
Assignee: nobody → dmu
VRDisplayOculusand and VRControllerManagerOculus should share the same ovrSession for polling sensor state. Otherwise, it would drop framerate.
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.
(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 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?
Attachment #8833892 - Flags: review?(kgilbert) → review+
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!
Attachment #8833891 - Flags: review?(kgilbert) → review+
Comment on attachment 8833890 [details]
Bug 1305889 - Part 1: Move GamepadHand API to VRControllerHost;

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

This looks good
Attachment #8833890 - Flags: review?(kgilbert) → review+
(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.
Pushed by dmu@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.