Closed Bug 1332989 Opened 3 years ago Closed 3 years ago

[webvr] Refactor VR display and controller managers

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: daoshengmu, Assigned: daoshengmu)

Details

Attachments

(1 file)

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.
How do you think about this? Kip.
Flags: needinfo?(kgilbert)
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!
Flags: needinfo?(kgilbert)
Assignee: nobody → dmu
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
Attachment #8829796 - Flags: review?(kgilbert) → review+
(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.
Pushed by dmu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8767eeeef633
Using VRSystemManager to manage vr displays and controllers; r=kip
https://hg.mozilla.org/mozilla-central/rev/8767eeeef633
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.