Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

First pass at defining tracked sources #1361

Merged
merged 5 commits into from
Oct 21, 2024

Conversation

cabanier
Copy link
Member

@cabanier cabanier commented Mar 2, 2024

@Maksims
Copy link

Maksims commented Mar 2, 2024

A few quick questions based on spec:

  1. Input source can "move" from trackedSources to inputSources lists and back? Is it the same XRInputSource instance, or a new object?
  2. If it is a new object, but they represent the same real thing, let's say the user takes a controller in hand, and now the input source moves from tracked to primary list, is there information for the developer to associate a previous instance with a new one?

@cabanier
Copy link
Member Author

cabanier commented Mar 2, 2024

A few quick questions based on spec:

  1. Input source can "move" from trackedSources to inputSources lists and back? Is it the same XRInputSource instance, or a new object

It is the same object.

@cabanier cabanier marked this pull request as ready for review March 5, 2024 18:15
@AdaRoseCannon
Copy link
Member

/facetoface

Copy link
Member

@toji toji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few initial comments, but overall I think this is good. Looking forward to discussing in depth at the F2F!

index.bs Outdated
1. [=list/Remove=] each {{XRInputSource}} in |removed| from |session|'s [=list of active XR input sources=].
1. Fire an {{XRInputSourcesChangeEvent}} named {{inputsourceschange!!event}} on |session| with {{XRInputSourcesChangeEvent/removed}} set to |removed|.
1. [=list/Remove=] each {{XRInputSource}} in |removed primary sources| from |session|'s [=list of active XR input sources=].
1. ire an {{XRInputSourcesChangeEvent}} named {{inputsourceschange!!event}} on |session| with {{XRInputSourcesChangeEvent/removed}} set to |removed primary sources|.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
1. ire an {{XRInputSourcesChangeEvent}} named {{inputsourceschange!!event}} on |session| with {{XRInputSourcesChangeEvent/removed}} set to |removed primary sources|.
1. If |removed primary sources| is not empty, fire an {{XRInputSourcesChangeEvent}} named {{inputsourceschange!!event}} on |session| with {{XRInputSourcesChangeEvent/removed}} set to |removed primary sources|.

index.bs Outdated

Note: The user agent MAY fire this event when an input source temporarily loses both position and orientation tracking. It is recommended that this only be done for physical handheld controller input sources. It is not recommended that this event be fired when this happens for tracked hand input sources, because this will happen often, nor is it recommended when this happens for tracker object input sources, since this makes it harder for the application to maintain a notion of identity.

</div>

<div class="algorithm" data-algorithm="on-input-source-change">

When the <dfn for="XRSession" export lt="change input source">{{XRInputSource/handedness}}, {{XRInputSource/targetRayMode}}, {{XRInputSource/profiles}}, or presence of a {{XRInputSource/gripSpace}} for any [=XR input source=]s change</dfn> for {{XRSession}} |session|, the user agent MUST run the following steps:
When the <dfn for="XRSession" export lt="change input source">{{XRInputSource/handedness}}, {{XRInputSource/targetRayMode}}, {{XRInputSource/profiles}}, presence of a {{XRInputSource/gripSpace}} or the state of the [=primary input source=] or [=tracked input source=] for any [=XR input source=]s change</dfn> for {{XRSession}} |session|, the user agent MUST run the following steps:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reads somewhat confusingly to me. I believe you are trying to say that the algorithm should fire if an input source switched from primary to tracked or visa-versa, but "the state of the primary input source or tracked input source" makes it sound like any state change (like a button press) could trigger this.

Not 100% confident in what a better phrasing would be, but perhaps something like this?

Suggested change
When the <dfn for="XRSession" export lt="change input source">{{XRInputSource/handedness}}, {{XRInputSource/targetRayMode}}, {{XRInputSource/profiles}}, presence of a {{XRInputSource/gripSpace}} or the state of the [=primary input source=] or [=tracked input source=] for any [=XR input source=]s change</dfn> for {{XRSession}} |session|, the user agent MUST run the following steps:
When the <dfn for="XRSession" export lt="change input source">{{XRInputSource/handedness}}, {{XRInputSource/targetRayMode}}, {{XRInputSource/profiles}}, presence of a {{XRInputSource/gripSpace}} or the status as a [=primary input source=] or [=tracked input source=] for any [=XR input source=]s change</dfn> for {{XRSession}} |session|, the user agent MUST run the following steps:

index.bs Outdated
@@ -2482,6 +2526,34 @@ The <dfn attribute for="XRInputSourcesChangeEvent">added</dfn> attribute is a [=

The <dfn attribute for="XRInputSourcesChangeEvent">removed</dfn> attribute is a [=/list=] of {{XRInputSource}}s that were removed from the {{XRSession}} at the time of the event.

XRTrackedSourcesChangeEvent {#xrtrackedsourceschangeevent-interface}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't need to be a different interface from XRInputSourcesChangeEvent. They're both identical in structure, and if devs want to introspect them to figure out it it's a primary or tracked event they can look at the event name.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't remember if this was discussed at the face to face?
I think we need a different event otherwise existing experiences will start getting additional events that they won't know how to handle.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To clarify: We can how a different event but still have it use the same interface. The same way that we have a single MouseEvent interface that is provided for mousedown, mouseup, and click events. The proposal here is not to push everything into a single event, it's to re-use the data structure returned with the separate events for input vs. tracked sources.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This issue is still outstanding. If you removed the XRTrackedSourcesChangeEvent and XRTrackedSourcesChangeEventInit and changed the trackedsourceschange event to use XRInputSourcesChangeEvent then I think this is mergable.

index.bs Outdated
1. If |added primary sources| or |removed primary sources| are not empty, fire an {{XRInputSourcesChangeEvent}} named {{inputsourceschange!!event}} on |session| with{{XRInputSourcesChangeEvent/added}} set to |added primary sources| and {{XRInputSourcesChangeEvent/removed}} set to |removed primary sources|.
1. [=list/Remove=] each {{XRInputSource}} in |removed tracked sources| from |session|'s [=list of active XR tracked sources=].
1. [=list/Extend=] |session|'s [=list of active XR input sources=] with |added tracked sources|.
1. If |added tracked sources| or |removed tracked sources| are not empty, fire an {{XRTrackedSourcesChangeEvent}} named {{trackedsourceschange!!event}} on |session| with {{XRTrackedSourcesChangeEvent/added}} set to |added tracked sources| and {{XRTrackedSourcesChangeEvent/removed}} set to |removed tracked sources|.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See note below about the event type.

index.bs Outdated
@@ -1890,7 +1934,7 @@ Note: {{XRInputSource}}s in an {{XRSession}}'s {{XRSession/inputSources}} array

An [=XR input source=] is a <dfn>primary input source</dfn> if it supports a <dfn>primary action</dfn>. The [=primary action=] is a platform-specific action that, when engaged, produces {{XRSession/selectstart}}, {{XRSession/selectend}}, and {{XRSession/select}} events. Examples of possible [=primary action=]s are pressing a trigger, touchpad, or button, speaking a command, or making a hand gesture. If the platform guidelines define a recommended primary input then it should be used as the [=primary action=], otherwise the user agent is free to select one. The device MUST support at least one [=primary input source=].

An [=XR input source=] is an <dfn>auxiliary input source</dfn> if it does not support a [=primary action=], for example [=transient input sources=] associated with secondary screen touches on a multitouch device.
An [=XR input source=] is an <dfn>tracked input source</dfn> if it does not support a [=primary action=], for example [=transient input sources=] associated with secondary screen touches on a multitouch device.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like the example used here is probably worth updating? The current one is a bit difficult to reason about. Also, the example should probably be non-normative. Maybe something like:

Suggested change
An [=XR input source=] is an <dfn>tracked input source</dfn> if it does not support a [=primary action=], for example [=transient input sources=] associated with secondary screen touches on a multitouch device.
An [=XR input source=] is an <dfn>tracked input source</dfn> if it does not support a [=primary action=]. These inputs are primarily intended to provide pose data.
Note: An example of a [=tracked input source=] would be tracking attachments for a users legs or a prop. Tracked hands may also be considered a [=tracked input source=] if there is no gesture recognition being performed to detect [=primary action=]s.

This probably also warrants a clarification in the WebXR Gamepads module that tracked input source MAY still expose gamepad data.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should also have a discussion about what the various spaces on the tracked sources mean? For example, targetRaySpace is somewhat nonsensical for a tracking puck, but still makes some sense for tracked hands or controllers that aren't held. But for a device that isn't firing any select events a "targetRay" is kind of a funny minsomer.

I'm comfortable with some cognitive dissonance in this area, since it's a relatively late addition, but I wouldn't mind it if we could provide some guidance for how future implementers should think about these spaces if we can.

@cabanier
Copy link
Member Author

cabanier commented Jul 8, 2024

@toji I thought we resolved all the issues with tracked sources. Can we merge this?

@cabanier cabanier requested a review from toji October 21, 2024 03:20
@toji
Copy link
Member

toji commented Oct 21, 2024

Apologies on the fact that this hasn't been merged yet, and thank you for the ping. There is one outstanding issue (using XRInputSourcesChangeEvent instead of XRTrackedSourcesChangeEvent) and then I'm comfortable merging it.

Copy link
Member

@toji toji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@toji toji merged commit 27cd3d1 into immersive-web:main Oct 21, 2024
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants