RESOLVED FIXED 182976
[WebVR][OpenVR] Retrieve stage parameters
https://bugs.webkit.org/show_bug.cgi?id=182976
Summary [WebVR][OpenVR] Retrieve stage parameters
Sergio Villar Senin
Reported 2018-02-20 09:54:05 PST
[WebVR][OpenVR] Retrieve stage parameters
Attachments
Patch (10.27 KB, patch)
2018-02-20 09:57 PST, Sergio Villar Senin
zan: review+
Sergio Villar Senin
Comment 1 2018-02-20 09:57:53 PST
Zan Dobersek
Comment 2 2018-02-21 01:17:33 PST
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
Comment 3 2018-02-21 01:53:14 PST
(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
Comment 4 2018-02-21 02:47:21 PST
Radar WebKit Bug Importer
Comment 5 2018-02-21 03:01:02 PST
Chris Dumez
Comment 6 2018-02-21 08:56:24 PST
Broke the build
Chris Dumez
Comment 7 2018-02-21 09:00:39 PST
(In reply to Chris Dumez from comment #6) > Broke the build Tried to fix it in <https://trac.webkit.org/changeset/228874>.
Chris Dumez
Comment 8 2018-02-21 09:03:21 PST
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
Comment 9 2018-02-21 09:09:08 PST
Still trying to fix build: <https://trac.webkit.org/changeset/228876>
Sergio Villar Senin
Comment 10 2018-02-22 01:00:15 PST
(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.
Note You need to log in before you can comment on or make changes to this bug.