[Webkit-unassigned] [Bug 221225] Implement WebXR getViewerPose and getPose

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=221225

--- Comment #5 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 420125
  --> https://bugs.webkit.org/attachment.cgi?id=420125
Patch

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

> Source/WebCore/Modules/webxr/WebXRBoundedReferenceSpace.cpp:41


I would pass a WebXRSession& if possible.

> Source/WebCore/Modules/webxr/WebXRBoundedReferenceSpace.cpp:82


auto

> Source/WebCore/Modules/webxr/WebXRFrame.cpp:49
> +    , m_session(WTFMove(session))

Would be slightly more consistent to initialise m_session before m_IsAnimationFrame.
Also s/m_IsAnimationFrame/m_isAnimationFrame/

> Source/WebCore/Modules/webxr/WebXRFrame.cpp:58
> +    auto isOutsideNativeBoundsOfBoundedReferenceSpace = [] (const WebXRSpace& space, const WebXRSpace&) {

Might be better as a free function.

> Source/WebCore/Modules/webxr/WebXRFrame.cpp:74
> +    auto isLocalReferenceSpace = [] (const WebXRSpace& space) {

Ditto.

> Source/WebCore/Modules/webxr/WebXRFrame.cpp:87
> +        // than 15m (suggested by specs) return true.

Should we return true here?

> Source/WebCore/Modules/webxr/WebXRFrame.cpp:97
> +    bool emulatedPosition;

Might be good to initialise the value if possible

> Source/WebCore/Modules/webxr/WebXRFrame.cpp:192
> +            const double near = m_session->renderState().depthNear();

s/const //

> Source/WebCore/Modules/webxr/WebXRFrame.cpp:203
> +                aspect = (double) layer->framebufferWidth() / (double) layer->framebufferHeight();

static_cast

> Source/WebCore/Modules/webxr/WebXRFrame.cpp:205
> +            const double far = m_session->renderState().depthFar();

s/const //

> Source/WebCore/Modules/webxr/WebXRFrame.cpp:209
> +        xrView->setProjectionMatrix(projection);

WTFMove(projection)?

> Source/WebCore/Modules/webxr/WebXRFrame.cpp:213
> +        ++index;

We could replace index by checking xrViews size

> Source/WebCore/Modules/webxr/WebXRFrame.cpp:237
> +    RefPtr<WebXRPose> pose = WebXRPose::create(WebXRRigidTransform::create(populateValue->transform), populateValue->emulatedPosition);

directly return here?

> Source/WebCore/Modules/webxr/WebXRSession.cpp:493
> +        auto frame = WebXRFrame::create(makeRef(*this), true /*isAnimationFrame*/);

Would read better by using an enum instead of a bool.

> Source/WebCore/Modules/webxr/WebXRSession.cpp:550
> +    // 2. If the request does not originate from document, return false.

It does not seem like you implement the check here.

> Source/WebCore/Modules/webxr/WebXRSpace.cpp:41
> +WebXRSpace::WebXRSpace(Document& document, WeakPtr<WebXRSession>&& session, Ref<WebXRRigidTransform>&& offset)

I would pass a WebXRSession&

> Source/WebCore/Modules/webxr/WebXRView.cpp:50
> +void WebXRView::setProjectionMatrix(const std::array<float, 16>& projection)

Might be left to a follow-up but why not passing directly a Ref<Float32Array>&& here?

> Source/WebCore/Modules/webxr/WebXRView.h:50
> +    const Float32Array& projectionMatrix() const { return *m_projection; }

How can we guarantee that m_projection is not null?
Is there a way we could pass a Ref<Float32Array> in the constructor?

> Source/WebCore/platform/graphics/transforms/TransformationMatrix.cpp:606
> +    const double upTan = tan(fovUp);

why const? upTan is only used once so I am not sure it adds much.

> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:308
> +    XrPosef identityPose = {

s / = //

> LayoutTests/platform/wpe/TestExpectations:641
> +imported/w3c/web-platform-tests/webxr/xrFrame_getViewerPose_getPose.https.html [ Pass ]

Could they be enabled cross-platform?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20210212/fc132429/attachment-0001.htm>