Closed Bug 1332989 Opened 3 years ago Closed 3 years ago
[webvr] Refactor VR display and controller managers
59 bytes, text/x-review-board-request
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.
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 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 email@example.com: https://hg.mozilla.org/integration/autoland/rev/8767eeeef633 Using VRSystemManager to manage vr displays and controllers; r=kip
You need to log in before you can comment on or make changes to this bug.