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

Clarify usage of ButtonIndex #692

Closed
alcooper91 opened this issue Jun 12, 2019 · 17 comments
Closed

Clarify usage of ButtonIndex #692

alcooper91 opened this issue Jun 12, 2019 · 17 comments
Assignees
Labels
help wanted This is a good issue for anyone to pick up and work on filing a PR for.
Milestone

Comments

@alcooper91
Copy link
Contributor

Currently the XRInputSourceEvent is only fired for select events, which should be the primary button, and per both the xr-standard mapping described in the WebXR spec and the general Gamepad spec, seems like it should always be 0 (or null if there is no Gamepad).

The buttonIndex feels like a bit of unnecessary extra data, especially if we aren't going to use XRInputSourceEvents for anything other than select (arguably what's implied by Issue #551 ).

@alcooper91
Copy link
Contributor Author

If the intention is that the UA should fire selectstart/select/selectend for all buttons then their usage for primary actions should be clarified (i.e. buttonIndex=0).

@cwilso cwilso added this to the July 2019 milestone Jun 17, 2019
@cwilso cwilso added the help wanted This is a good issue for anyone to pick up and work on filing a PR for. label Jun 17, 2019
@cwilso
Copy link
Member

cwilso commented Jun 17, 2019

The intent is for all button presses to fire the XRInputSourceEvent. Manish, could you pick up clarifying that?

@ddorwin
Copy link
Contributor

ddorwin commented Jun 17, 2019

It appears that new algorithm(s) are needed for generic button clicks, similar to those for selectstart, etc.

@thetuvix
Copy link
Contributor

@cwilso:

The intent is for all button presses to fire the XRInputSourceEvent. Manish, could you pick up clarifying that?

@toji: Is @cwilso's assertion correct here? My longstanding understanding of selectstart/select/selectend is that it is specifically about the input source's primary action, which may or may not involve a gamepad button. If this was intended for all gamepad buttons, it should probably be named pressdown/press/pressup or such - the term select was used to indicate that it represents the source's primary select action for proceeding in an experience.

@Manishearth
Copy link
Contributor

Yeah, I came across that issue when trying to resolve this issue.

My current plan was to create a concept of an action, and the UA can support as many as it wants, and one of them may be the primary action (buttonindex = 0).

I don't think it's super useful to have two kinds of events. Potentially have an .isPrimary() on XRInputSourceEvent though.

@alcooper91
Copy link
Contributor Author

I think it’s still useful to have both kinds of events, as (IIUC) the primary action is the one that actually counts as a user action for activation and I think many simple buttons would want to just blindly hook up a click handler for select and ignore other actions. Requiring a heavier weight check here feels wrong, especially since the gamepad is an extension that not all controllers have. On top of this, if a controller doesn’t have an xr-standard mapping, is it conceivable that the “primary action” would not be index 0?
We will definitely need another event name to fire the other button events though.

@Manishearth
Copy link
Contributor

We could not rely on the button index and straight up expose .isPrimary()

@alcooper91
Copy link
Contributor Author

I think there's still the disconnect with it being a little heavyweight for things that only care about the primary and a bit of a mismatch in expectations given that the primary is also supposed to count as a user activation.
I could see an argument for having both events and the .isPrimary(), that way a developer could choose if they only care about primary or care about all.

@Manishearth
Copy link
Contributor

@NellWaliczek any thoughts on what this should be here? I can make the simple changes to allow all button presses to generate select events (and enforce buttonIndex = 0 for the primary action, or add isPrimary()), but we might want something different here.

@NellWaliczek
Copy link
Member

sigh @toji and I were both reading the comments in this issue and thinking "yeah... this is a bit confusing as is.. but...hmmmmm" Give us a day or so to think over what would be the right way to handle it. I'm leaning slightly towards two separate events because that makes it more sense in terms of separating developer intent, my main hesitation is that the Gamepad folks are also considering adding an event system.

@Manishearth
Copy link
Contributor

Ah, I see. Thanks!

@toji
Copy link
Member

toji commented Jun 27, 2019

I'm leaning slightly towards two separate events because that makes it more sense in terms of separating developer intent, my main hesitation is that the Gamepad folks are also considering adding an event system.

I'm leaning towards a separation of concerns on the events here as well, but the fact that there is (in theory) some parallel work happening on the gamepad side maybe suggests that we should defer the question of general button press events for a bit. If we removed buttonIndex for the current milestone we preserve our options going forward. Either supporting a second event or re-instating buttonIndex would both be additive to the API, and I would appreciate the opportunity to evaluate this more carefully prior to CR.

@Manishearth
Copy link
Contributor

Cool, I'll go ahead and do that.

@Manishearth
Copy link
Contributor

#741

@NellWaliczek
Copy link
Member

I'm inclined to either close this issue and open a new one tracking the desire to have gamepad button eventing OR rename this issue to reflect that. Any preference?

@Manishearth
Copy link
Contributor

New issue, preferably, just for cleanliness

@NellWaliczek
Copy link
Member

New issue filed. Closing this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted This is a good issue for anyone to pick up and work on filing a PR for.
Projects
None yet
Development

No branches or pull requests

7 participants