[WebVR][OpenVR] Retrieve stage parameters
Created attachment 334276 [details] Patch
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);`.
(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.
Committed r228867: <https://trac.webkit.org/changeset/228867>
<rdar://problem/37739975>
Broke the build
(In reply to Chris Dumez from comment #6) > Broke the build Tried to fix it in <https://trac.webkit.org/changeset/228874>.
BTW, why isn't this behind a build time flag? I find it surprising we are building this code on Mac / iOS at all.
Still trying to fix build: <https://trac.webkit.org/changeset/228876>
(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.