Skip to content

Describe how the input source list is maintained #628

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

Merged
merged 3 commits into from
May 14, 2019
Merged

Describe how the input source list is maintained #628

merged 3 commits into from
May 14, 2019

Conversation

toji
Copy link
Member

@toji toji commented May 6, 2019

Fixes #465.

Defines what should happen as input sources for a session are added,
removed, and changed. Also clarifies the definitions for some of the
related concepts.

@toji toji added this to the May 2019 milestone May 6, 2019
@toji toji requested a review from NellWaliczek May 6, 2019 19:30
index.bs Outdated

1. Let |oldInputSource| be the {{XRInputSource}} in |session|'s [=list of active XR input sources=] previously associated with the [=XR input source=] that changed.
1. Let |newInputSource| be a new {{XRInputSource}}.
1. Remove |oldInputSource| from |session|'s [=list of active XR input sources=].
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also unset an active flag on the source? Make getPose() on inactive input source spaces returns null (or throw an error for better diagnostics?).

In other words, force people to reload input source lists whenever they change. Otherwise they'll continue using a nonexistant input space.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good suggestion! I'll incorporate that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Following up: This is ever-so-slightly more complex than I had originally anticipated, primarily because I think the "active" flag should actually reside on the space and not the input source. That implies that we probably want to make more consistent use of it elsewhere in the spec, so I wanted to give it some more thought before jumping into a change like that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Totally okay with it being a followup, just something to keep in mind.

Copy link
Member

@NellWaliczek NellWaliczek 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 general questions before doing a more detailed review:

  1. Is there a reason this PR isn't reflecting the behavior of the inputsourceschange event as currently described in the explainer? Specifically, I was a bit surprised the "added" and "removed" event properties aren't included in this PR...
  2. Is the expectation that left/right motion controllers will each fire their own event even if they're detected on the same frame?
  3. Are there any developer expectations we should be setting about when input sources may change? For example, I'm assuming because the task is being queued that the event won't fire "mid frame" even if control is returned to the UA?

@toji
Copy link
Member Author

toji commented May 10, 2019

Pushed some changes to this PR, thanks for your patience!

Is there a reason this PR isn't reflecting the behavior of the inputsourceschange event...

Huh. Turns out that the spec was missing the XRInputSourceChangeEvent entirely! Fixed that now, as well as making sure the algorithms populate the added/removed arrays as needed.

One minor concern: The event name is "inputsourceschange" while the interface, as defined in the explainer, is "XRInputSourceChangeEvent" (note the singular "Source"). We should probably normalize that, and it could easily be done in this PR. I suggest making the interface use the plural form?

Is the expectation that left/right motion controllers will each fire their own event even if they're detected on the same frame?

Nope, that was an oversight on my part. Fixed in the latest commit.

Are there any developer expectations we should be setting about when input sources may change?

We certainly can, though what exactly those expectations should be is not readily apparent to me at the moment and is probably best discussed in a separate issue.

Copy link
Member

@NellWaliczek NellWaliczek left a comment

Choose a reason for hiding this comment

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

Sweet. Thanks for the updates!

I support the plural form of the event :)

I'm also ok with filing a separate issue to discussing the overall timing guarantee (though I'd like to resolve the specific comments I've left on this iteration of the PR before we merge).

(Also, GitHub isn't letting me add a comment on Manish's "active" issue. If we're going to defer that one, can we get the issue filed before merging and refer to it here?)

@NellWaliczek
Copy link
Member

Looking at the other open PRs made me realize that one really important thing is missing. We haven't defined the expected behavior of the array when an item is added/removed. Do gaps fill in? What about for transient input sources?

@Manishearth
Copy link
Contributor

Looking at the other open PRs made me realize that one really important thing is missing. We haven't defined the expected behavior of the array when an item is added/removed. Do gaps fill in? What about for transient input sources?

@NellWaliczek There's some discussion on that in #624. It's not touching array ordering concerns, though.

@cwilso
Copy link
Member

cwilso commented May 13, 2019

Also fixes #632

toji added 2 commits May 13, 2019 16:44
Fixes #465.

Defines what should happen as input sources for a session are added,
removed, and changed. Also clarifies the definitions for some of the
related concepts.
@toji
Copy link
Member Author

toji commented May 14, 2019

Update done with a sick, sleeping 2yo on my lap. Apologies from any oversights/typos as a result.

I support the plural form of the event :)

Done!

We haven't defined the expected behavior of the array when an item is added/removed.

I've updated the algorithms to use some pre-defined list operations, which should remove some of the ambiguity around how it should behave.

Copy link
Member

@NellWaliczek NellWaliczek left a comment

Choose a reason for hiding this comment

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

Poor kiddo.... and poor Brandon!

But the good news is... LGTM!!

@@ -1229,7 +1232,7 @@ Gamepad API Integration {#gamepad-api-integration}

- {{XRInputSource/gamepad}} MUST NOT be included in the array returned by {{navigator.getGamepads()}}.
- {{XRInputSource/gamepad}}'s {{Gamepad/index}} attribute MUST be <code>0</code>.
Copy link
Member

Choose a reason for hiding this comment

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

{{XRInputSource/gamepad}}'s {{Gamepad/index}} attribute MUST be 0.

Gah this makes me so nervous.. people are going to use this and index into inputSources.... Can we file a separate issue (and label it to be an external dependency) to figure out how we want to handle this with the Gamepad spec folks? (yes, I'm looking at you @toji)

Copy link
Member Author

Choose a reason for hiding this comment

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

Re-cast #583 as a broader issue to address this topic.

@toji toji merged commit 8050a01 into master May 14, 2019
@toji toji deleted the input-list branch May 14, 2019 05:14
@AdaRoseCannon AdaRoseCannon added the ed:spec Include in newsletter, spec change label Jun 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ed:spec Include in newsletter, spec change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document how the list of active input sources is maintained
5 participants