-
Notifications
You must be signed in to change notification settings - Fork 401
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
Conversation
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=]. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this 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:
- 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...
- Is the expectation that left/right motion controllers will each fire their own event even if they're detected on the same frame?
- 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?
Pushed some changes to this PR, thanks for your patience!
Huh. Turns out that the spec was missing the XRInputSourceChangeEvent entirely! Fixed that now, as well as making sure the algorithms populate the 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?
Nope, that was an oversight on my part. Fixed in the latest commit.
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. |
There was a problem hiding this 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?)
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. |
Also fixes #632 |
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.
Update done with a sick, sleeping 2yo on my lap. Apologies from any oversights/typos as a result.
Done!
I've updated the algorithms to use some pre-defined list operations, which should remove some of the ambiguity around how it should behave. |
There was a problem hiding this 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>. |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
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.