[OpenVR][WebVR] Retrieve FrameData in WebVR's rAF
Created attachment 337078 [details] Patch
Created attachment 337261 [details] Patch Mac/iOS build fix
Created attachment 337349 [details] Patch Build fix for WPE
Created attachment 337350 [details] Patch Rebased patch
Created attachment 337351 [details] Patch Another build fix. Seems that the way unified sources are built affect a lot the compilation results
Created attachment 337352 [details] Patch And another build fix :(
Created attachment 337353 [details] Patch Mac build fix
Created attachment 337355 [details] Patch Hopefully the last Mac build fix
Comment on attachment 337355 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337355&action=review > Source/WebCore/Modules/webvr/VRDisplay.cpp:113 > + auto* document = downcast<Document>(scriptExecutionContext()); This can be moved into the (!m_scriptedAnimationController) block. > Source/WebCore/Modules/webvr/VRDisplay.cpp:118 > + // FIXME: Get the display id of the HMD as it should use the HMD native refresh rate. > + PlatformDisplayID displayID = document->page() ? document->page()->chrome().displayID() : 0; > + m_scriptedAnimationController = ScriptedAnimationController::create(*document, displayID); Is this necessary at this point, for animation frame functionality? I would just default to the 0 ID until it's possible to acquire the display ID from the HMD device. > Source/WebCore/Modules/webvr/VRFrameData.cpp:91 > + return projectionMatrix; You could use the 16-argument TransformationMatrix constructor here, and pass all the values in the return statement. > Source/WebCore/Modules/webvr/VRFrameData.cpp:124 > + return matrix; Same here. You could also alias normalizedQuaternion variable to something shorter, e.g. `nq`, and then compute all the values on the fly. > Source/WebCore/Modules/webvr/VRFrameData.cpp:148 > + FloatPoint3D leftEyeOffset = leftEye.rawOffset(); > + m_leftViewMatrix = rotationMatrix; > + applyHeadToEyeTransform(m_leftViewMatrix, leftEyeOffset); Inline leftEye.rawOffset() into the applyHeadToEyeTransform() call. > Source/WebCore/Modules/webvr/VRFrameData.cpp:152 > + FloatPoint3D rightEyeOffset = rightEye.rawOffset(); > + m_rightViewMatrix = rotationMatrix; > + applyHeadToEyeTransform(m_rightViewMatrix, rightEyeOffset); Ditto. > Source/WebCore/Modules/webvr/VRFrameData.h:28 > + Unnecessary whitespace. > Source/WebCore/Modules/webvr/VRPose.cpp:48 > + auto position = Float32Array::create(3); > + float* positionData = position->data(); > + positionData[0] = m_trackingInfo.position->x(); > + positionData[1] = m_trackingInfo.position->y(); > + positionData[2] = m_trackingInfo.position->z(); > + return position.releaseNonNull(); To follow other Float32Array::create() calls, first construct positionData[3] and then call `Float32Array::create(positionData, 3).releaseNonNull();`. > Source/WebCore/Modules/webvr/VRPose.cpp:72 > + auto orientation = Float32Array::create(4); > + float* orientationData = orientation->data(); > + orientationData[0] = m_trackingInfo.orientation->x; > + orientationData[1] = m_trackingInfo.orientation->y; > + orientationData[2] = m_trackingInfo.orientation->z; > + orientationData[3] = m_trackingInfo.orientation->w; > + return orientation.releaseNonNull(); Ditto. > Source/WebCore/Modules/webvr/VRPose.h:28 > + Unnecessary whitespace. > Source/WebCore/platform/vr/VRPlatformDisplay.h:157 > + double timestamp; Default-initialize this to 0.
(In reply to Zan Dobersek from comment #9) > Comment on attachment 337355 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=337355&action=review > > > Source/WebCore/Modules/webvr/VRDisplay.cpp:113 > > + auto* document = downcast<Document>(scriptExecutionContext()); > > This can be moved into the (!m_scriptedAnimationController) block. > > > Source/WebCore/Modules/webvr/VRDisplay.cpp:118 > > + // FIXME: Get the display id of the HMD as it should use the HMD native refresh rate. > > + PlatformDisplayID displayID = document->page() ? document->page()->chrome().displayID() : 0; > > + m_scriptedAnimationController = ScriptedAnimationController::create(*document, displayID); > > Is this necessary at this point, for animation frame functionality? I would > just default to the 0 ID until it's possible to acquire the display ID from > the HMD device. I think it does not hurt and we're going to need it really soon. > > Source/WebCore/Modules/webvr/VRFrameData.cpp:91 > > + return projectionMatrix; > > You could use the 16-argument TransformationMatrix constructor here, and > pass all the values in the return statement. Yeah, but the end result is really confusing IMHO. So unless you feel strongly about it I prefer to leave it as is. > > Source/WebCore/Modules/webvr/VRFrameData.cpp:124 > > + return matrix; > > Same here. You could also alias normalizedQuaternion variable to something > shorter, e.g. `nq`, and then compute all the values on the fly. I don't think it's a good idea to compute all the values on the fly because we precisely store some of them in variables in order to save some multiplications. That said, in this case I think we could use the 16-argument constructor here as variable names are short in this case. > > Source/WebCore/Modules/webvr/VRFrameData.cpp:148 > > + FloatPoint3D leftEyeOffset = leftEye.rawOffset(); > > + m_leftViewMatrix = rotationMatrix; > > + applyHeadToEyeTransform(m_leftViewMatrix, leftEyeOffset); > > Inline leftEye.rawOffset() into the applyHeadToEyeTransform() call. ACK > > Source/WebCore/Modules/webvr/VRFrameData.cpp:152 > > + FloatPoint3D rightEyeOffset = rightEye.rawOffset(); > > + m_rightViewMatrix = rotationMatrix; > > + applyHeadToEyeTransform(m_rightViewMatrix, rightEyeOffset); > > Ditto. > > > Source/WebCore/Modules/webvr/VRFrameData.h:28 > > + > > Unnecessary whitespace. > > > Source/WebCore/Modules/webvr/VRPose.cpp:48 > > + auto position = Float32Array::create(3); > > + float* positionData = position->data(); > > + positionData[0] = m_trackingInfo.position->x(); > > + positionData[1] = m_trackingInfo.position->y(); > > + positionData[2] = m_trackingInfo.position->z(); > > + return position.releaseNonNull(); > > To follow other Float32Array::create() calls, first construct > positionData[3] and then call `Float32Array::create(positionData, > 3).releaseNonNull();`. OK, will do > > Source/WebCore/Modules/webvr/VRPose.cpp:72 > > + auto orientation = Float32Array::create(4); > > + float* orientationData = orientation->data(); > > + orientationData[0] = m_trackingInfo.orientation->x; > > + orientationData[1] = m_trackingInfo.orientation->y; > > + orientationData[2] = m_trackingInfo.orientation->z; > > + orientationData[3] = m_trackingInfo.orientation->w; > > + return orientation.releaseNonNull(); > > Ditto. > > > Source/WebCore/Modules/webvr/VRPose.h:28 > > + > > Unnecessary whitespace. > > > Source/WebCore/platform/vr/VRPlatformDisplay.h:157 > > + double timestamp; > > Default-initialize this to 0.
Comment on attachment 337355 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337355&action=review >>> Source/WebCore/Modules/webvr/VRFrameData.cpp:91 >>> + return projectionMatrix; >> >> You could use the 16-argument TransformationMatrix constructor here, and pass all the values in the return statement. > > Yeah, but the end result is really confusing IMHO. So unless you feel strongly about it I prefer to leave it as is. I don't feel strongly about it. >>> Source/WebCore/Modules/webvr/VRFrameData.cpp:124 >>> + return matrix; >> >> Same here. You could also alias normalizedQuaternion variable to something shorter, e.g. `nq`, and then compute all the values on the fly. > > I don't think it's a good idea to compute all the values on the fly because we precisely store some of them in variables in order to save some multiplications. That said, in this case I think we could use the 16-argument constructor here as variable names are short in this case. OK, makes sense.
Created attachment 337490 [details] Patch
Comment on attachment 337490 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=337490&action=review > Source/WebCore/Modules/webvr/VRFrameData.h:59 > + double m_timestamp; This too should be default-initialized to 0. > Source/WebCore/Modules/webvr/VRPose.cpp:43 > + float positionData[3] = { m_trackingInfo.position->x(), m_trackingInfo.position->y(), m_trackingInfo.position->z() }; Nit: IMO it's cleaner to retrieve the reference of the m_trackingInfo.position object and then access x/y/z values through that: ``` auto& position = *m_trackingInfo.position; float positionData[3] = { position.x(), position.y(), position.z() }; ``` > Source/WebCore/Modules/webvr/VRPose.cpp:62 > + float orientationData[4] = { m_trackingInfo.orientation->x, m_trackingInfo.orientation->y, m_trackingInfo.orientation->z, m_trackingInfo.orientation->w }; Nit: similarly: ``` auto& orientation = *m_trackingInfo.orientation; float orientationData[4] = { orientation.x, orientation.y, orientation.z, orientation.w }; ```
Committed r230428: <https://trac.webkit.org/changeset/230428>
<rdar://problem/39279853>