WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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+
Details
Formatted Diff
Diff
View All
Add attachment
proposed patch, testcase, etc.
Sergio Villar Senin
Comment 1
2018-02-20 09:57:53 PST
Created
attachment 334276
[details]
Patch
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
Committed
r228867
: <
https://trac.webkit.org/changeset/228867
>
Radar WebKit Bug Importer
Comment 5
2018-02-21 03:01:02 PST
<
rdar://problem/37739975
>
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.
Top of Page
Format For Printing
XML
Clone This Bug