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

[bugzilla-daemon at webkit.org]

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

--- Comment #6 from Imanol Fernandez <ifernandez at igalia.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
>> +Ref<WebXRBoundedReferenceSpace> WebXRBoundedReferenceSpace::create(Document& document, WeakPtr<WebXRSession>&& session, XRReferenceSpaceType type)
> 
> I would pass a WebXRSession& if possible.

I don't think it's safe because JS can hold XRSpace instances after a XRSession is destroyed.

>> Source/WebCore/Modules/webxr/WebXRBoundedReferenceSpace.cpp:82
>> +    Ref<WebXRRigidTransform> offset = WebXRRigidTransform::create(m_originOffset->rawTransform() * offsetTransform.rawTransform());
> 
> auto

done

>> 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/

done

>> Source/WebCore/Modules/webxr/WebXRFrame.cpp:58
>> +    auto isOutsideNativeBoundsOfBoundedReferenceSpace = [] (const WebXRSpace& space, const WebXRSpace&) {
> 
> Might be better as a free function.

done

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

Only if the distance is greater than 15m. I want to implement the limits logic there and in other paths in a separate follow-up patch to not make this one more complex.

>> Source/WebCore/Modules/webxr/WebXRFrame.cpp:97
>> +    bool emulatedPosition;
> 
> Might be good to initialise the value if possible

done

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

done

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

done

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

done

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

done, moved to the constructor

>> Source/WebCore/Modules/webxr/WebXRFrame.cpp:213
>> +        ++index;
> 
> We could replace index by checking xrViews size

done

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

done

>> 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.

done

>> 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?

I moved it to the constructor

>> 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?

done

>> 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.

Done. I'm used to default immutability in Rust so I'll tend to add const by default :)

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

done

>> LayoutTests/platform/wpe/TestExpectations:641
>> +imported/w3c/web-platform-tests/webxr/xrFrame_getViewerPose_getPose.https.html [ Pass ]
> 
> Could they be enabled cross-platform?

I'll address that as a follow-up

-- 
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/20210215/e237c2fd/attachment-0001.htm>