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 firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/8767eeeef633 Using VRSystemManager to manage vr displays and controllers; r=kip
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.