[WebVR][OpenVR] Retrieve eye parameters and field of view
Created attachment 334258 [details] Patch
Comment on attachment 334258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334258&action=review > Source/WebCore/Modules/webvr/VREyeParameters.cpp:32 > +VREyeParameters::VREyeParameters(FloatPoint3D offset, VRPlatformDisplayInfo::FieldOfView& fieldOfView, IntSize renderSize) These parameters should be const references, assuming we shouldn't have to be changing them. > Source/WebCore/Modules/webvr/VRFieldOfView.h:47 > + VRFieldOfView(VRPlatformDisplayInfo::FieldOfView fieldOfView) This, again, should be a const reference. > Source/WebCore/Modules/webvr/VRFieldOfView.h:58 > + double m_upDegrees; > + double m_rightDegrees; > + double m_downDegrees; > + double m_leftDegrees; Given the close relation between all these member variables, I got used to just grouping them inside one struct: struct { double up; double right; double down; double left; } m_degrees; If you find this sensible, feel free to adopt it. > Source/WebCore/platform/vr/VRPlatformDisplay.h:52 > + enum Eye { EyeLeft = 0, EyeRight, NumEyes }; > + FloatPoint3D eyeTranslation[Eye::NumEyes]; You could maybe group the eye stuff together. > Source/WebCore/platform/vr/VRPlatformDisplay.h:53 > + IntSize renderSize; I don't think this should be an IntSize, since you're retrieving unsigned values from OpenVR and then again returning unsigned values in VREyeParameters. Try using: struct RenderSize { unsigned width; unsigned height; } renderSize; and then use VRPlatformDisplayInfo::RenderSize as the type to store this information in VREyeParameters. > Source/WebCore/platform/vr/VRPlatformDisplay.h:59 > + float upDegrees; > + float downDegrees; > + float leftDegrees; > + float rightDegrees; These are converted to doubles in VRFieldOfView. IMO this struct should follow the Web-facing API, and we should be performing the float-to-double conversion in the OpenVR-specific code. > Source/WebCore/platform/vr/VRPlatformDisplay.h:61 > + }; > + FieldOfView eyeFieldOfView[Eye::NumEyes]; This can be rolled into one statement, like with the RenderSize proposal. > Source/WebCore/platform/vr/openvr/VRPlatformDisplayOpenVR.cpp:67 > + m_system->GetProjectionRaw(static_cast<vr::Hmd_Eye>(eye), &left, &right, &top, &bottom); > + fieldOfView.upDegrees = -rad2deg(atanf(top)); > + fieldOfView.downDegrees = rad2deg(atanf(bottom)); > + fieldOfView.leftDegrees = -rad2deg(atanf(left)); > + fieldOfView.rightDegrees = rad2deg(atanf(right)); As said above, we are retrieving floats here, since that's what OpenVR is providing, but the fieldOfView attributes should be of the double type. > Source/WebCore/platform/vr/openvr/VRPlatformDisplayOpenVR.cpp:68 > + return fieldOfView; This can be return { -rad2deg(), rad2deg(), -rad2deg(), rad2deg() }; > Source/WebCore/platform/vr/openvr/VRPlatformDisplayOpenVR.cpp:71 > +void VRPlatformDisplayOpenVR::updateEyeParameters() I guess this should go before computeFieldOfView(), following the declaration order in the header, but it's not super-important. > Source/WebCore/platform/vr/openvr/VRPlatformDisplayOpenVR.cpp:87 > +VRPlatformDisplayOpenVR::~VRPlatformDisplayOpenVR() > +{ This should follow the constructor, and should be defaulted. > Source/WebCore/platform/vr/openvr/VRPlatformDisplayOpenVR.h:35 > explicit VRPlatformDisplayOpenVR(vr::IVRSystem*, vr::IVRChaperone*, vr::IVRCompositor*); > > + ~VRPlatformDisplayOpenVR(); No space required.
Created attachment 334269 [details] Patch Applied the suggested changes
Comment on attachment 334269 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=334269&action=review > Source/WebCore/Modules/webvr/VREyeParameters.h:36 > +class IntSize; This can be dropped. > Source/WebCore/platform/vr/VRPlatformDisplay.h:24 > +#include "IntSize.h" This can be dropped. > Source/WebCore/platform/vr/openvr/VRPlatformDisplayOpenVR.cpp:61 > + /* OpenVR returns the tangents of the half-angles from the center view axis. */ // is a more common single-line comment pattern. > Source/WebCore/platform/vr/openvr/VRPlatformDisplayOpenVR.cpp:68 > + for (unsigned eye = 0; eye < VRPlatformDisplayInfo::NumEyes; eye++) { Usually better to increment via prefix: `++eye`.
Committed r228819: <https://trac.webkit.org/changeset/228819>
<rdar://problem/37707716>