Bug 182976 – [WebVR][OpenVR] Retrieve stage parameters

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

[Sergio Villar Senin]

[WebVR][OpenVR] Retrieve stage parameters

[Sergio Villar Senin]

Created attachment 334276 [details]
Patch

[Zan Dobersek]

Comment on attachment 334276 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=334276&action=review

> Source/WebCore/Modules/webvr/VRDisplay.cpp:78
> +    auto displayInfo = m_display->getDisplayInfo();

Not an issue for this patch, but VRPlatformDisplay::getDisplayInfo() should return a reference (const, if possible). As it stands, the returned VRPlatformDisplayInfo object is copied on every call.

> Source/WebCore/Modules/webvr/VRStageParameters.cpp:42
> +    RELEASE_ASSERT(transform);
> +    float* transformData = transform->data();
> +    transformData[0] = m_sittingToStandingTransform.m11();
> +    transformData[1] = m_sittingToStandingTransform.m12();

This can be shortened. Put a TransformationMatrix::FloatMatrix4 object on the stack, pass it to TransformationMatrix::toColumnMajorFloatArray(), then create the Float32Array object via Float32Array::create(FloatMatrix4&, 16).

The Float32Array::create(float*, N) constructor should be used in other places as well, where possible.

> Source/WebCore/Modules/webvr/VRStageParameters.h:39
> +        if (!sittingToStandingTransform || !playAreaBounds)
> +            return nullptr;

Is it expected for any of these two to be nullopt? I don't see it possible with the OpenVR implementation.

> Source/WebCore/platform/vr/openvr/VRPlatformDisplayOpenVR.cpp:89
> +        m_displayInfo.sittingToStandingTransform = TransformationMatrix(1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1, 0, 0, 0.75, 0, 1);

IMO it would be easier to read to just call `m_displayInfo.sittingToStandingTransform.makeIdentity();` and then separately call `setM42(0.75);`.

[Sergio Villar Senin]

(In reply to Zan Dobersek from comment #2)
> Comment on attachment 334276 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=334276&action=review
> 
> > Source/WebCore/Modules/webvr/VRDisplay.cpp:78
> > +    auto displayInfo = m_display->getDisplayInfo();
> 
> Not an issue for this patch, but VRPlatformDisplay::getDisplayInfo() should
> return a reference (const, if possible). As it stands, the returned
> VRPlatformDisplayInfo object is copied on every call.

Sure, that's already under my radar. The thing is that we should also consider enabling some caching at some point because some stuff is not going to change while some other is. So as we add more data to that display info we'd have to cook a plan to deal with caching and with data updating.

> > Source/WebCore/Modules/webvr/VRStageParameters.cpp:42
> > +    RELEASE_ASSERT(transform);
> > +    float* transformData = transform->data();
> > +    transformData[0] = m_sittingToStandingTransform.m11();
> > +    transformData[1] = m_sittingToStandingTransform.m12();
> 
> This can be shortened. Put a TransformationMatrix::FloatMatrix4 object on
> the stack, pass it to TransformationMatrix::toColumnMajorFloatArray(), then
> create the Float32Array object via Float32Array::create(FloatMatrix4&, 16).

Sounds good, will try it.

> The Float32Array::create(float*, N) constructor should be used in other
> places as well, where possible.
> 
> > Source/WebCore/Modules/webvr/VRStageParameters.h:39
> > +        if (!sittingToStandingTransform || !playAreaBounds)
> > +            return nullptr;
> 
> Is it expected for any of these two to be nullopt? I don't see it possible
> with the OpenVR implementation.

Yes, on my initial testing I was getting null values for them sometimes. I then checked Firefox and they include the same fallback I'm adding.

> > Source/WebCore/platform/vr/openvr/VRPlatformDisplayOpenVR.cpp:89
> > +        m_displayInfo.sittingToStandingTransform = TransformationMatrix(1, 0, 0, 0, 0, 1, 0, 0, 0, 0, 1, 0, 0, 0.75, 0, 1);
> 
> IMO it would be easier to read to just call
> `m_displayInfo.sittingToStandingTransform.makeIdentity();` and then
> separately call `setM42(0.75);`.

Ah clever, will change it.

[Sergio Villar Senin]

Committed r228867: <https://trac.webkit.org/changeset/228867>

[Radar WebKit Bug Importer]

<rdar://problem/37739975>

[Chris Dumez]

Broke the build

[Chris Dumez]

(In reply to Chris Dumez from comment #6)
> Broke the build

Tried to fix it in <https://trac.webkit.org/changeset/228874>.

[Chris Dumez]

BTW, why isn't this behind a build time flag? I find it surprising we are building this code on Mac / iOS at all.

[Chris Dumez]

Still trying to fix build:
 <https://trac.webkit.org/changeset/228876>

[Sergio Villar Senin]

(In reply to Chris Dumez from comment #9)
> Still trying to fix build:
>  <https://trac.webkit.org/changeset/228876>

(In reply to Chris Dumez from comment #8)
> BTW, why isn't this behind a build time flag? I find it surprising we are
> building this code on Mac / iOS at all.

Well, the idea from the very beginning was to develop it for all the platforms as OpenVR is supported also on Mac. However I do not have enough experience on Mac building system to get it building OpenVR stuff just for MacOS and not for iOS. See https://bugs.webkit.org/show_bug.cgi?id=182047 if you are interested.