Bug 238968 – [WebXR] Implement the WebXRFrame methods for getting joints' poses and radii

• Component: New Bugs
• Status: RESOLVED FIXED

[Ada Chan]

[WebXR] Implement the WebXR Hand Input module

[Ada Chan]

Created attachment 456998 [details]
Patch

[Ada Chan]

Created attachment 457119 [details]
Patch for landing

[Ada Chan]

Created attachment 457134 [details]
Patch for landing 2

[Chris Dumez]

Comment on attachment 457134 [details]
Patch for landing 2

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

Nothing critical that would prevent committing as is but here is some feedback.

> Source/WebCore/Modules/webxr/WebXRFrame.cpp:282
> +ExceptionOr<bool> WebXRFrame::fillJointRadii(const Vector<RefPtr<WebXRJointSpace>> jointSpaces, Float32Array& radii)

Why is the second array passed as non-const reference?

> Source/WebCore/Modules/webxr/WebXRHand.cpp:52
> +        m_joints.append(WebXRJointSpace::create(*document, *this, static_cast<XRHandJoint>(i)));

It seems the size of m_joint is static. Ideally, it would be a FixedVector instead of a Vector.

> Source/WebCore/Modules/webxr/WebXRHand.cpp:95
> +    const PlatformXR::Device::FrameData::HandJointsVector& handJoints = *(inputSource.handJoints);

that's a long type, I would use auto&

> Source/WebCore/Modules/webxr/WebXRHand.h:50
> +    unsigned size() { return m_joints.size(); }

should be const.

> Source/WebCore/Modules/webxr/WebXRJointSpace.cpp:45
> +WebXRJointSpace::WebXRJointSpace(Document& document, WebXRHand& hand, XRHandJoint jointName, const std::optional<PlatformXR::Device::FrameData::InputSourceHandJoint>& joint)

std::optional<PlatformXR::Device::FrameData::InputSourceHandJoint>&&

> Source/WebCore/Modules/webxr/WebXRJointSpace.cpp:50
> +    updateFromJoint(joint);

m_joint(WTFMove(joint)) in the initializer list?

> Source/WebCore/Modules/webxr/WebXRJointSpace.h:45
>  class WebXRJointSpace : public RefCounted<WebXRJointSpace>, public WebXRSpace {

This class can probably be marked as final.

> Source/WebCore/platform/xr/PlatformXR.h:326
> +        typedef Vector<std::optional<InputSourceHandJoint>> HandJointsVector;

We prefer the modern:
using HandJointsVector = Vector<std::optional<InputSourceHandJoint>>;

> Source/WebCore/platform/xr/PlatformXR.h:651
> +    if (!decoder.decode(handJoint.pose))

This is the old style of decoding, modern code should use:
std::optional<InputSourcePose> pose;
decoder >> pose;
if (!pose)
    return std::nullopt;
std::optional<float> radius;
decoder >> radius;
if (!radius)
    return std::nullopt;
return { { WTFMove(*pose), *radius } };

> Source/WebCore/testing/WebFakeXRInputController.h:91
> +    std::optional<PlatformXR::Device::FrameData::HandJointsVector> m_handJoints;

So we will need to distinguish the vector not being initialized vs the vector being empty and that's why it is optional?
I might have missed it but the patch doesn't seem to rely on that.

[Ada Chan]

(In reply to Chris Dumez from comment #4)

Thanks Chris! I really appreciate your detailed review and feedback.

> Comment on attachment 457134 [details]
> Patch for landing 2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457134&action=review
> 
> Nothing critical that would prevent committing as is but here is some
> feedback.
> 
> > Source/WebCore/Modules/webxr/WebXRFrame.cpp:282
> > +ExceptionOr<bool> WebXRFrame::fillJointRadii(const Vector<RefPtr<WebXRJointSpace>> jointSpaces, Float32Array& radii)
> 
> Why is the second array passed as non-const reference?

In this method, we'll be filling in that radii array so it can't be a const reference.

> 
> > Source/WebCore/Modules/webxr/WebXRHand.cpp:52
> > +        m_joints.append(WebXRJointSpace::create(*document, *this, static_cast<XRHandJoint>(i)));
> 
> It seems the size of m_joint is static. Ideally, it would be a FixedVector
> instead of a Vector.

Fixed!

> 
> > Source/WebCore/Modules/webxr/WebXRHand.cpp:95
> > +    const PlatformXR::Device::FrameData::HandJointsVector& handJoints = *(inputSource.handJoints);
> 
> that's a long type, I would use auto&

Fixed!

> 
> > Source/WebCore/Modules/webxr/WebXRHand.h:50
> > +    unsigned size() { return m_joints.size(); }
> 
> should be const.

Fixed!

> 
> > Source/WebCore/Modules/webxr/WebXRJointSpace.cpp:45
> > +WebXRJointSpace::WebXRJointSpace(Document& document, WebXRHand& hand, XRHandJoint jointName, const std::optional<PlatformXR::Device::FrameData::InputSourceHandJoint>& joint)
> 
> std::optional<PlatformXR::Device::FrameData::InputSourceHandJoint>&&
> 
> > Source/WebCore/Modules/webxr/WebXRJointSpace.cpp:50
> > +    updateFromJoint(joint);
> 
> m_joint(WTFMove(joint)) in the initializer list?

Fixed!

> 
> > Source/WebCore/Modules/webxr/WebXRJointSpace.h:45
> >  class WebXRJointSpace : public RefCounted<WebXRJointSpace>, public WebXRSpace {
> 
> This class can probably be marked as final.

Fixed!

> 
> > Source/WebCore/platform/xr/PlatformXR.h:326
> > +        typedef Vector<std::optional<InputSourceHandJoint>> HandJointsVector;
> 
> We prefer the modern:
> using HandJointsVector = Vector<std::optional<InputSourceHandJoint>>;

Fixed!

> 
> > Source/WebCore/platform/xr/PlatformXR.h:651
> > +    if (!decoder.decode(handJoint.pose))
> 
> This is the old style of decoding, modern code should use:
> std::optional<InputSourcePose> pose;
> decoder >> pose;
> if (!pose)
>     return std::nullopt;
> std::optional<float> radius;
> decoder >> radius;
> if (!radius)
>     return std::nullopt;
> return { { WTFMove(*pose), *radius } };

Fixed!

> 
> > Source/WebCore/testing/WebFakeXRInputController.h:91
> > +    std::optional<PlatformXR::Device::FrameData::HandJointsVector> m_handJoints;
> 
> So we will need to distinguish the vector not being initialized vs the
> vector being empty and that's why it is optional?
> I might have missed it but the patch doesn't seem to rely on that.

I made it optional to be consistent with how PlatformXR::Device::InputSource's handJoints is also optional. By making it optional, I feel it matches better the semantics that specifying joints is optional. If there's a compelling reason that omitting the optional is better, please let me know!

[Ada Chan]

Created attachment 457192 [details]
Patch for landing 3

[EWS]

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

[Ada Chan]

Created attachment 457301 [details]
Patch for landing 4

[Chris Dumez]

Comment on attachment 457301 [details]
Patch for landing 4

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

> Source/WebCore/Modules/webxr/WebXRHand.cpp:55
> +        joints.append(WebXRJointSpace::create(*document, *this, static_cast<XRHandJoint>(i)));

FYI, you can use uncheckedAppend() here since you called reserveInitialCapacity earlier.

[Ada Chan]

(In reply to Chris Dumez from comment #9)
> Comment on attachment 457301 [details]
> Patch for landing 4
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=457301&action=review
> 
> > Source/WebCore/Modules/webxr/WebXRHand.cpp:55
> > +        joints.append(WebXRJointSpace::create(*document, *this, static_cast<XRHandJoint>(i)));
> 
> FYI, you can use uncheckedAppend() here since you called
> reserveInitialCapacity earlier.

OK, will fix!

[Ada Chan]

Created attachment 457345 [details]
Patch for landing 5

[EWS]

Committed r292780 (249564@main): <https://commits.webkit.org/249564@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 457345 [details].

[Radar WebKit Bug Importer]

<rdar://problem/91640876>