Bug 224931 – Implement OpenXR input sources

• Status: RESOLVED
• Resolution: FIXED
• Component: WebXR

[Imanol Fernandez]

Now that bug 223257 has landed we can add the OpenXR platform implementation for input sources

[Imanol Fernandez]

Created attachment 426814 [details]
Patch

[Imanol Fernandez]

Created attachment 426904 [details]
Patch

Fix compilation error in debug builds

[Radar WebKit Bug Importer]

<rdar://problem/77321283>

[Sergio Villar Senin]

Comment on attachment 426904 [details]
Patch

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

Looking great.

> Source/WebCore/ChangeLog:42
> +        (PlatformXR::XrPoseIdentity): Creates a identify pose.

identify->identity

> Source/WebCore/Sources.txt:2200
> +platform/xr/openxr/OpenXRInputSource.cpp @no-unify

Not for this patch, but as the feature matures we should think about removing the @no-unify at some point and get the benefit of unified builds.

> Source/WebCore/platform/xr/openxr/OpenXRInput.cpp:52
> +        m_handleIndex++;

Does 0 have any special meaning or can it be freely used as handle index.

> Source/WebCore/platform/xr/openxr/OpenXRInput.cpp:54
> +        if (inputSource)

You can instead do

if (auto inputSource = OpenXRInputSource::create(m_instance, m_session, handeness, m_handleIndex))

> Source/WebCore/platform/xr/openxr/OpenXRInput.h:39
> +    OpenXRInput(XrInstance, XrSession, XrSpace);

I guess the constructor should be private.

> Source/WebCore/platform/xr/openxr/OpenXRInputMappings.h:98
> +    const OpenXRProfileId* profileIds { nullptr };

Why not using a Vector?. Same for the other fields

> Source/WebCore/platform/xr/openxr/OpenXRInputMappings.h:110
> +

It's unfortunate to have all this specific stuff for one device, i.e., it isn't scalable to include here every single device profile. Isn't there any library providing those (or any other generic mechanism)?

> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:34
> +    return input;

We can even use a ternary operator here

> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:39
> +    if (m_actionSet != XR_NULL_HANDLE)

We can do it in a followup patch but we should definitely add a isXrNullHandle() (or similar) method to the OpenXRUtils header and replace all those comparisons.

> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:52
> +    m_handle = handle;

I prefer to have these initializations in the constructor.

> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:59
> +    String prefix = "input_" + handenessName;

Doesn't those special prefixes/suffixes have a name defined in OpenXR? If not, maybe it does make sense to define some constexpr for them.

> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:96
> +        RETURN_RESULT_IF_FAILED(createBinding(profile.path, m_pointerAction, m_subactionPathName + "/input/aim/pose", bindings), m_instance);

Ditto, using constexpr for the strings

> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:158
> +        return WTF::nullopt; // Trigger is mandatory in xr-standard mapping

Move the comment above the if.

BTW comments must have a full stop at the end.

> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:211
> +            for (size_t i = 0; i < profile.profileIdsSize; ++i)

for (auto profileId : profile.profileIds)

> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:223
> +    ASSERT(space);

Knowing that it cannot be null, we better pass a reference here.

> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:235
> +    ASSERT(action);

Ditto reference.

> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:249
> +    ASSERT(actions);

Ditto.

> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:280
> +    ASSERT(pose);

Ditto

> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:337
> +    ASSERT(value);

Ditto

> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:352
> +    ASSERT(value);

Ditto

> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:367
> +    ASSERT(value);

Ditto.

> Source/WebCore/platform/xr/openxr/OpenXRUtils.h:130
> +    case XRHandedness::Right: return "right";

Nit: I think the returns should be in a different line

> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:586
> +    if (!didNotifyInputInitialization) {

Better invert the condition and early return. That way we could save an indentation level

> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:590
> +            fprintf(stderr, "OpenXRDevice::updateInteractionProfile6 \n");

Remove debug code or use LOG() instead

[Imanol Fernandez]

Created attachment 429155 [details]
Patch

Address review feedback

[Imanol Fernandez]

Comment on attachment 426904 [details]
Patch

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

>> Source/WebCore/ChangeLog:42
>> +        (PlatformXR::XrPoseIdentity): Creates a identify pose.
> 
> identify->identity

done

>> Source/WebCore/Sources.txt:2200
>> +platform/xr/openxr/OpenXRInputSource.cpp @no-unify
> 
> Not for this patch, but as the feature matures we should think about removing the @no-unify at some point and get the benefit of unified builds.

makes sense, I'll address it as a follow-up

>> Source/WebCore/platform/xr/openxr/OpenXRInput.cpp:54
>> +        if (inputSource)
> 
> You can instead do
> 
> if (auto inputSource = OpenXRInputSource::create(m_instance, m_session, handeness, m_handleIndex))

done

>> Source/WebCore/platform/xr/openxr/OpenXRInput.h:39
>> +    OpenXRInput(XrInstance, XrSession, XrSpace);
> 
> I guess the constructor should be private.

done

>> Source/WebCore/platform/xr/openxr/OpenXRInputMappings.h:98
>> +    const OpenXRProfileId* profileIds { nullptr };
> 
> Why not using a Vector?. Same for the other fields

Vector doesn't support constexpr. I'm trying to define a constexpr with all the available bindings in a declarative way.

>> Source/WebCore/platform/xr/openxr/OpenXRInputMappings.h:110
>> +
> 
> It's unfortunate to have all this specific stuff for one device, i.e., it isn't scalable to include here every single device profile. Isn't there any library providing those (or any other generic mechanism)?

AFAIK there isn't any library or generic way, just the khr/simple_controller fallback, but only supports a limited subset of controller buttons. Chromium is also definining bindings for each controller they want to have full support of.

>> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:34
>> +    return input;
> 
> We can even use a ternary operator here

unique_ptr complains when using the ternary operator because of the deleted copy constructor will nullptr. I also tried with {} but didn't work either.

>> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:39
>> +    if (m_actionSet != XR_NULL_HANDLE)
> 
> We can do it in a followup patch but we should definitely add a isXrNullHandle() (or similar) method to the OpenXRUtils header and replace all those comparisons.

Ok, I'll address that as a follow-up

>> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:52
>> +    m_handle = handle;
> 
> I prefer to have these initializations in the constructor.

Done

>> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:59
>> +    String prefix = "input_" + handenessName;
> 
> Doesn't those special prefixes/suffixes have a name defined in OpenXR? If not, maybe it does make sense to define some constexpr for them.

I have created a constexpr as they are not defined in OpenXR constant.

>> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:96
>> +        RETURN_RESULT_IF_FAILED(createBinding(profile.path, m_pointerAction, m_subactionPathName + "/input/aim/pose", bindings), m_instance);
> 
> Ditto, using constexpr for the strings

done

>> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:158
>> +        return WTF::nullopt; // Trigger is mandatory in xr-standard mapping
> 
> Move the comment above the if.
> 
> BTW comments must have a full stop at the end.

done

>> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:223
>> +    ASSERT(space);
> 
> Knowing that it cannot be null, we better pass a reference here.

Done

>> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:235
>> +    ASSERT(action);
> 
> Ditto reference.

Done

>> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:280
>> +    ASSERT(pose);
> 
> Ditto

Done

>> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:337
>> +    ASSERT(value);
> 
> Ditto

Done.

>> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:352
>> +    ASSERT(value);
> 
> Ditto

Done.

>> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:367
>> +    ASSERT(value);
> 
> Ditto.

Done.

>> Source/WebCore/platform/xr/openxr/OpenXRUtils.h:130
>> +    case XRHandedness::Right: return "right";
> 
> Nit: I think the returns should be in a different line

Done

>> Source/WebCore/platform/xr/openxr/PlatformXROpenXR.cpp:590
>> +            fprintf(stderr, "OpenXRDevice::updateInteractionProfile6 \n");
> 
> Remove debug code or use LOG() instead

Done

[Sergio Villar Senin]

Comment on attachment 429155 [details]
Patch

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

Pretty happy with the patch. I have just a few comments on some stuff I didn't spot on the first review.

> Source/WebCore/platform/xr/openxr/OpenXRInput.cpp:82
> +    Vector<Device::FrameData::InputSource> result;

Supernit for landing: move the declaration down to the place we first use it.

> Source/WebCore/platform/xr/openxr/OpenXRInput.h:40
> +    Vector<Device::FrameData::InputSource> getInputSources(const XrFrameState&) const;

Didn't spot this in this first pass but we don't usually use the "get" prefix for methods. This is not exactly a getter anyway as we're constructing it, so I'd use another prefix like collect/construct/...

> Source/WebCore/platform/xr/openxr/OpenXRInputMappings.h:42
> +constexpr std::array<OpenXRButtonType, 7> OpenXRButtonTypes {

This should be openXRButtonTypes. Alternatively if you want to preserve the capitalized "O" then prepend something like availableOpenXR..., validOpenXR..., supportedOpenXR... what

> Source/WebCore/platform/xr/openxr/OpenXRInputMappings.h:76
> +constexpr std::array<OpenXRAxisType, 2> OpenXRAxisTypes {

Ditto openXRAxisTypes

> Source/WebCore/platform/xr/openxr/OpenXRInputMappings.h:133
> +// Default fallback when thre isn't a specific controller binding.

Nit: thre -> there

> Source/WebCore/platform/xr/openxr/OpenXRInputMappings.h:152
> +constexpr std::array<OpenXRInputProfile, 2> OpenXRInputProfiles { HTCViveInputProfile, KHRSimpleInputProfile };

Ditto openXRInputProfiles

> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:65
> +    // Initialize Action Set

Nit: missing dot at the end. Same for the other comments.

> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:218
> +        if (!strcmp(profile.path, buffer)) {

Can we use strncmp with writtenCount ?

> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:244
> +    createInfo.subactionPaths = &m_subactionPath;

Why are you getting a reference here? Shouldn't be just m_subactionPath ?

> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:273
> +    if (it != bindings.end())

I guess we can do
if (auto it = bindings.find(profilePath); it != bindings.end())

since "it" is only used in this branch

> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:303
> +    if (actions.press != XR_NULL_HANDLE && XR_SUCCEEDED(getActionState(actions.press, &result.pressed)))

Looks like we can convert these tree if conditions into a lambda as they're basically doing the same with different arguments. Also the hasValue=true assignment could be moved into the lambda as well. We could call it getActionStateIfNotNull() for example.

> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:321
> +    return result;

return hasValue ? makeOptional(result) : WTF::nullopt;

> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:334
> +    return WTF::nullopt;

I know it's a matter of taste but I think I prefer to do the failure return in the if block. Otherwise it looks like the "normal" flow is to return a null axis.

[Imanol Fernandez]

Created attachment 429521 [details]
Patch

Address review feedback

[Imanol Fernandez]

Created attachment 429522 [details]
Patch

Fix nits in OpenXRInput.cpp

[Imanol Fernandez]

Comment on attachment 429155 [details]
Patch

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

>> Source/WebCore/platform/xr/openxr/OpenXRInput.cpp:82
>> +    Vector<Device::FrameData::InputSource> result;
> 
> Supernit for landing: move the declaration down to the place we first use it.

done

>> Source/WebCore/platform/xr/openxr/OpenXRInput.h:40
>> +    Vector<Device::FrameData::InputSource> getInputSources(const XrFrameState&) const;
> 
> Didn't spot this in this first pass but we don't usually use the "get" prefix for methods. This is not exactly a getter anyway as we're constructing it, so I'd use another prefix like collect/construct/...

done

>> Source/WebCore/platform/xr/openxr/OpenXRInputMappings.h:42
>> +constexpr std::array<OpenXRButtonType, 7> OpenXRButtonTypes {
> 
> This should be openXRButtonTypes. Alternatively if you want to preserve the capitalized "O" then prepend something like availableOpenXR..., validOpenXR..., supportedOpenXR... what

done

>> Source/WebCore/platform/xr/openxr/OpenXRInputMappings.h:76
>> +constexpr std::array<OpenXRAxisType, 2> OpenXRAxisTypes {
> 
> Ditto openXRAxisTypes

done

>> Source/WebCore/platform/xr/openxr/OpenXRInputMappings.h:133
>> +// Default fallback when thre isn't a specific controller binding.
> 
> Nit: thre -> there

done

>> Source/WebCore/platform/xr/openxr/OpenXRInputMappings.h:152
>> +constexpr std::array<OpenXRInputProfile, 2> OpenXRInputProfiles { HTCViveInputProfile, KHRSimpleInputProfile };
> 
> Ditto openXRInputProfiles

done

>> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:65
>> +    // Initialize Action Set
> 
> Nit: missing dot at the end. Same for the other comments.

done

>> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:218
>> +        if (!strcmp(profile.path, buffer)) {
> 
> Can we use strncmp with writtenCount ?

done

>> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:244
>> +    createInfo.subactionPaths = &m_subactionPath;
> 
> Why are you getting a reference here? Shouldn't be just m_subactionPath ?

It's because createInfo.subactionPaths expects a pointer to an array, and we are treating a single variable as a one sized array,

>> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:273
>> +    if (it != bindings.end())
> 
> I guess we can do
> if (auto it = bindings.find(profilePath); it != bindings.end())
> 
> since "it" is only used in this branch

done

>> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:303
>> +    if (actions.press != XR_NULL_HANDLE && XR_SUCCEEDED(getActionState(actions.press, &result.pressed)))
> 
> Looks like we can convert these tree if conditions into a lambda as they're basically doing the same with different arguments. Also the hasValue=true assignment could be moved into the lambda as well. We could call it getActionStateIfNotNull() for example.

good idea, done.

>> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:321
>> +    return result;
> 
> return hasValue ? makeOptional(result) : WTF::nullopt;

done

>> Source/WebCore/platform/xr/openxr/OpenXRInputSource.cpp:334
>> +    return WTF::nullopt;
> 
> I know it's a matter of taste but I think I prefer to do the failure return in the if block. Otherwise it looks like the "normal" flow is to return a null axis.

Makes sense, done.

[Sergio Villar Senin]

Comment on attachment 429522 [details]
Patch

Nice! Looking forward to trying it locally

[EWS]

Committed r277954 (238081@main): <https://commits.webkit.org/238081@main>

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