[webvr] Refactor VR display and controller managers

RESOLVED FIXED in Firefox 54

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: daoshengmu, Assigned: daoshengmu)

Tracking

unspecified
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
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)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Assignee: nobody → dmu

Comment 4

2 years ago
mozreview-review
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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 7

2 years ago
(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

2 years ago
Pushed by dmu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8767eeeef633
Using VRSystemManager to manage vr displays and controllers; r=kip

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8767eeeef633
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.