Bug 182962 – [WebVR][OpenVR] Retrieve eye parameters and field of view

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sergio Villar Senin]

[WebVR][OpenVR] Retrieve eye parameters and field of view

[Sergio Villar Senin]

Created attachment 334258 [details]
Patch

[Zan Dobersek]

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.

[Sergio Villar Senin]

Created attachment 334269 [details]
Patch

Applied the suggested changes

[Zan Dobersek]

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`.

[Sergio Villar Senin]

Committed r228819: <https://trac.webkit.org/changeset/228819>

[Radar WebKit Bug Importer]

<rdar://problem/37707716>