1332989 - [webvr] Refactor VR display and controller managers

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Daosheng Mu[:daoshengmu]]

Although We did our OpenVR controller support already, I meet some architecture design problem for the current Oculus controller work. Because ovrSession and IVRSystem should be shared between VRDisplayHost and VRControllerHost, especially for Oculus, otherwise, it would affect our VR system thread.

I purpose that we should rename VRDisplayManager to be VRSystemManager, and it is responsible for managing VRDisplayHost and VRControllerHost. VRControllerManager is no longer needed.

Comment 1

[Daosheng Mu[:daoshengmu]]

How do you think about this? Kip.

Comment 2

[:kip (Kearwood Gilbert)]

This seems reasonable, given what VRDisplayManager actually "manages" is VR API sessions, which enumerate both displays and controllers.

I'd continue with your suggestion for now.  If VRSystemManager grows very large, perhaps we would later split some display and controller functionality into separate classes that are composed with VRSystemManager.

For now, I think there is no need to have separate classes.

Thanks, Daosheng!

Comment 3

[Daosheng Mu[:daoshengmu]]

Attachment: Bug 1332989 - Using VRSystemManager to manage vr displays and controllers;

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

Comment 4

[:kip (Kearwood Gilbert)]

Comment on attachment 8829796 [details]
Bug 1332989 - Using VRSystemManager to manage vr displays and controllers;

https://reviewboard.mozilla.org/r/106786/#review108070

Just one small whitespace nit, r+ from me with that.

Looks great, thanks!

::: gfx/vr/gfxVR.h:212
(Diff revision 1)
> -  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(VRDisplayManager)
> +  NS_INLINE_DECL_THREADSAFE_REFCOUNTING(VRSystemManager)
>  
>    virtual bool Init() = 0;
>    virtual void Destroy() = 0;
>    virtual void GetHMDs(nsTArray<RefPtr<VRDisplayHost>>& aHMDResult) = 0;
> +  

Small nit: Spaces on empty line

Comment 5

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8829796 [details]
Bug 1332989 - Using VRSystemManager to manage vr displays and controllers;

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

Comment 6

[Daosheng Mu[:daoshengmu]]

Comment on attachment 8829796 [details]
Bug 1332989 - Using VRSystemManager to manage vr displays and controllers;

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

Comment 7

[Daosheng Mu[:daoshengmu]]

(In reply to Daosheng Mu[:daoshengmu] from comment #6)
> Comment on attachment 8829796 [details]
> Bug 1332989 - Using VRSystemManager to manage vr displays and controllers;
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/106786/diff/2-3/

Replace ScanForDevices / RemoveDevices with ScanForControllers / RemoveControllers.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8767eeeef633
Using VRSystemManager to manage vr displays and controllers; r=kip

Comment 9

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8767eeeef633