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

Add startSqueezing/endSqueezing/simulateSqueeze #49

Open
Artyom17 opened this issue Feb 12, 2020 · 21 comments
Open

Add startSqueezing/endSqueezing/simulateSqueeze #49

Artyom17 opened this issue Feb 12, 2020 · 21 comments

Comments

@Artyom17
Copy link

To match the 'select' event testing, why don't we add the startSqueezing/endSqueezing/simulateSqueeze methods, as well as squeezingStarted, squeezingClicked states?

@Manishearth
Copy link
Contributor

Yeah, we should add these

@alcooper91
Copy link
Contributor

I'm a little hesitant on this. The select button is required on an XRInputSource, and as such we didn't define the select button in FakeXrButtonType. However, the grip does exist in FakeXrButtonType, so these would really just be a convenience wrapper around updateButtonState

@Manishearth
Copy link
Contributor

It's possible for an XR device to support squeeze but not a gamepad (we're doing this for hands on Hololens)

@alcooper91
Copy link
Contributor

updateButtonState doesn't require gamepad though?

It still feels wrong to expose two ways of doing the same thing

@Manishearth
Copy link
Contributor

Oh, I see. Yeah.

However: I don't think the updateButtonState can simulate cancellation; which is possible for both select and squeeze events (and leads to a selectend event not paired with a select event). Not a big deal, but something to think about.

@alcooper91
Copy link
Contributor

alcooper91 commented Feb 12, 2020

I don't think our mechanism for select can simulate cancellation right now?

we have startSelection/endSelection and then simulateSelect, which from the explainer is a subframe select (so guaranteed to send the select on the next frame, which I think calling startSelection/endSelection may not guarantee)

@alcooper91
Copy link
Contributor

One compromise here could be to allow the webxr_util class to polyfill in such a helper. (at least for the start/stop)

@Manishearth
Copy link
Contributor

Manishearth commented Feb 12, 2020

Hmm, we perhaps should change the APIs then to startSelect/endSelect/cancelSelect?

That maps more cleanly to the "start, end, cancel" framing that's in the spec: https://immersive-web.github.io/webxr/#primary-squeeze-action

IMO this API should hook directly into such spec algorithms (which are triggered by the device) as much as possible. Basically, wherever the core spec ends up in a "obtain this data from the device" or "when the device decides to do X", we should have an API for that.

@Manishearth
Copy link
Contributor

In the current test API this hooks more directly into the user side rather than the device side.

@Manishearth
Copy link
Contributor

Also, if we're doing this, it might be worth having just startSelect/endSelect/cancelSelect with a squeeze optional boolean argument

@alcooper91
Copy link
Contributor

That kind of feels like overriding the existing method for the sake of not adding a new one, in a way that feels neither scalable nor intuitive IMO

@Manishearth
Copy link
Contributor

That's fair. We can also add a new one then.

@alcooper91
Copy link
Contributor

If we're okay with removing one (that we maybe keep polyfilled in) We could also do something like

startPrimaryAction(FakeXrPrimaryAction action)
stopPrimaryAction(FakeXrPrimaryAction action)

where currently

enum FakeXrPrimaryAction {
  "select",
  "squeeze",
}

@alcooper91
Copy link
Contributor

Though "squeeze" would still be possible via updateButtonState, unless we wanted to explicitly disallow that.

@Artyom17
Copy link
Author

Artyom17 commented Feb 12, 2020

So, should the implementation of updateButtonState check, that the button[1] has been changed, therefore fire the squeeze event? Currently, it just updates the gamepad object (in Chromium implementation) and this doesn't and wouldn't cause the squeeze events firing.

Also, should the updating button[0] cause select events to fire somehow?

EDIT: talked to Alex offline, seems like it is just an issue in Google's implementation of updateButtonState and it indeed should fire select/squeeze events....

@alcooper91
Copy link
Contributor

The goal of the Test API has been to mimic a "real" device. updateButtonState is our way of saying "Hey, when you next check for data pretend that the state of this button has changed"
Currently in Chrome, we only update the Gamepad object, because we didn't have any other mechanism of passing that data into blink.

Updating button[0] (since this is the gamepad button that maps to the select event) should fire that event. However, the way that we have currently designed the API (taking a FakeXrButtonType which doesn't include "select"), makes it impossible to update the select state via "updateButtonState"

@Manishearth
Copy link
Contributor

The enum solution sounds good to me!

@alcooper91
Copy link
Contributor

alcooper91 commented Feb 13, 2020

Actually, that still leaves us with two ways to press some buttons.
What if we:

  • remove FakeXRInputSourceInit selectionStarted/selectionClicked
  • move startSelection/endSelection/simulateSelection to be polyfilled in via webxr_util.js
  • Add "select" to FakeXRButtonType
  • Enforce via spec text that when processing sequence<FakeXRButtonStateInit> that if no button of type "select" is provided, that one is default-added.
  • Optionally: to FakeXRInputSourceInit add a "sequence<FakeXRButtonType> clickedButtons" to indicate buttons that should immediately send a click upon connection (for transient, this provides a way for spec text to still support the 'selectionClicked' item, if we can't remove them).

I think all of these except the removal of selectionStarted/selectionClicked can be polyfilled back-in or otherwise backwards compatibly added, unless we're okay with the compat breaking changes.

If we are okay with compat breaking changes, We may want to change FakeXRButtonType (and similar) to something like FakeXRActionType (and similar) to make it more generic for Hands and other forms of input.

@Manishearth
Copy link
Contributor

The thing that gives me pause is that the FakeXRButton stuff contains a lot of gamepad stuff as well, and ideally that would be separate (we don't implement it).

But otherwise this seems fine. I'm also okay with the compat breaking changes, really.

to make it more generic for Hands and other forms of input.

The hands api isn't fixed enough for this to make sense, yet.

@alcooper91
Copy link
Contributor

The thing that gives me pause is that the FakeXRButton stuff contains a lot of gamepad stuff as well, and ideally that would be separate (we don't implement it).

I don't think that would be a major issue. Your implementation can simply ignore buttons it doesn't know/doesn't want to handle. Really the only place that the state of those other buttons should bubble up would be via the Gamepad/gamepad tests. So as long as you just ignore the values, the other tests should be fine?

to make it more generic for Hands and other forms of input.

The hands api isn't fixed enough for this to make sense, yet.

While I understand that, if we were to bubble up any additional events, they would likely be considered an "Input Action" hence my proposal to name them FakeXRActionType rather than FakeXRButtonType

I don't believe we can do partial enums, but I think it would be fine to add spec text to say that certain enum values may not be supported if they are required by a module that the UA doesn't support.
Alternatively, we could do a similar approach to the "feature-descriptors" that the spec uses (though maybe enforce strings), and give "valid" strings on a per-module basis.

@Manishearth
Copy link
Contributor

I don't believe we can do partial enums, but I think it would be fine to add spec text to say that certain enum values may not be supported if they are required by a module that the UA doesn't support.

This is fine.

While I understand that, if we were to bubble up any additional events, they would likely be considered an "Input Action" hence my proposal to name them FakeXRActionType rather than FakeXRButtonType

I believe the current thinking is that hands will also have select and squeeze events, and nothing more, everything else is exposed as state and not events.

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

No branches or pull requests

3 participants