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

Emulated oculus and vive controls #2423

Closed

Conversation

machenmusik
Copy link
Contributor

@machenmusik machenmusik commented Feb 20, 2017

(Per request, this PR no longer extends #2413 / #2415 to oculus-touch-controls as thumbstickmoved)

Should fix #2379.

@machenmusik
Copy link
Contributor Author

Verified on both Vive (via SteamVR/OpenVR support) and Touch (direct Oculus support) with latest experimental Chromium.

@cmvanb
Copy link
Contributor

cmvanb commented Feb 21, 2017

Looks like a nice upgrade. After #2415 was merged, I noticed that the trigger on the HTC Vive also fires onAxisMove (z axis) events. Does this PR handle the trigger axis as 'triggermoved' or as 'trackpadmoved'?

@machenmusik
Copy link
Contributor Author

trigger is not an axis, it is a button with a value associated -- or at least that is the case with Chromium. per discussion with @dmarcos in Slack, we should definitely avoid browser-specific ids and button/axis mappings if possible, bleah.

@machenmusik
Copy link
Contributor Author

machenmusik commented Feb 22, 2017

axismove events should have been fired before #2415 as well, meaning that you could easily handle axismove yourself if need be. but for buttons with values, as trigger should be (and grip for Touch as well), it looks like no specific event was fired when the value changed, only the more generic buttonchanged that is also fired on up/down/touchstart/touchend.

@cmvanb
Copy link
Contributor

cmvanb commented Feb 22, 2017

Ok, so 'buttonchanged' still needs to be fired correctly for this specific case (htc vive trigger), with the correct value? Does the oculus touch grip work like this already?

@machenmusik
Copy link
Contributor Author

buttonchanged should be firing now, see https://github.com/aframevr/aframe/blob/master/src/components/tracked-controls.js#L151

oculus-touch-controls and vive-controls provide mappings to friendly names for up/down/touchstart/touchend, but looks like they don't handle changed. that can be added, but would like to settle the button/axis disparity between Chromium and Nightly first to avoid redoing work. @dmarcos any more word from Gecko folks?

@machenmusik
Copy link
Contributor Author

@cmvanb if you have time in next few days, can you confirm which environments (browser / HMD+controllers) receive events such as triggerchanged with the latest PR?

@cmvanb
Copy link
Contributor

cmvanb commented Feb 22, 2017

I can test FF nightly + Vive for you. I will also try Chromium + Vive, although Chromium didn't work last time I tried. Will do that tonight or tomorrow. I don't have an Oculus to test with.

@machenmusik
Copy link
Contributor Author

(FYI, there is a brand new experimental chromium that resolves previous issues with latest steamvr)

@cmvanb
Copy link
Contributor

cmvanb commented Feb 23, 2017

I tested the branch with vive + FF nightly 54.0a1 (2017-02-23) (64-bit).

When clicking the trackpad, 'trackpadchanged' event is fired (multiple times/frames) as expected.
When sliding a finger over the trackpad, 'trackpadmoved' event (multiple times/frames) is fired as expected. The x/y values are correct.
When pulling the trigger down (fast), several events are fired in this order:
'trackpadmoved'
'trackpadmoved'
'triggerdown'
'triggerchanged'
'trackpadmoved'
'triggerchanged'

On releasing the trigger:
'trackpadmoved'
'triggerup'
'triggerchanged'
'trackpadmoved'
'triggerchanged'

It seems like the 'trackpadmoved' event is still being fired for the trigger. Otherwise, the values seem correct to me.

PS: Chromium still isn't working for me. I'm using the version made available here: https://webvr.info/get-chrome/

@machenmusik
Copy link
Contributor Author

thanks @cmvanb !

I believe there is currently a fundamental difference in how the latest experimental Chromium and Nightly model Vive and Touch expose controllers with respect to buttons and axes.

Chromium represents trigger (Vive and Touch) as a button that has an analog value, and not as an axis... same thing for grip on Touch. The only axes are trackpad (Vive) and thumbstick (touch).

Nightly appears to be representing buttons that have analog values (like trigger) as axes, for example the new Oculus Touch support was exposing controllers with 3 buttons and 4 axes in Nightly, rather than the 6 buttons and 2 axes in Chromium, This causes problems since axismove events are currently fired by tracked-controls when trigger changes, with no indication of which axes changed.

@cmvanb can you confirm whether the triggerchanged button state provides an analog value using Nightly? If not, then even the button events are inconsistent, so suppressing spurious trackpadmoved events would not suffice.

IMO it is highly undesirable to have browser-specific gamepad API mappings for the same controllers on the same machine. @cvan @kearwood @toji - would appreciate your thoughts on how best to resolve discrepancies.


update: function (oldData) {
if ((oldData.emulated !== undefined) && (this.data.emulated !== oldData.emulated)) {
this.checkIfControllersPresent();
Copy link
Member

Choose a reason for hiding this comment

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

This LOC isn't getting covered by the tests. Maybe something wrong with the condition?

I think just if (this.data.emulated !== oldData.emulated) alone should work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the original reason for that was to avoid flagging the first update call (right after init) as needing the controllers check, since play() will do it.

Copy link
Member

Choose a reason for hiding this comment

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

I see. Maybe emulated in oldData + a comment is a minor improvement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in order for that line to be covered, the entity would have to have emulated be true right at initial definition, and the tests use a setup function where no attributes are provided initially.

@ngokevin
Copy link
Member

ngokevin commented Mar 1, 2017

How difficult to split the emulated changes from the axismove additions?

@machenmusik
Copy link
Contributor Author

Easiest way to "split" the axismove handling is to simply comment out the onAxisMoved handler, in which case the code won't fire. but remember there is already a different version of axismove committed into master for vive only, not for oculus, so we'd be disabling that vive-only functionality by doing so

@machenmusik
Copy link
Contributor Author

so rather than split out the axismove handling, I added axis change propagation, which allows culling of spurious moves.

@machenmusik
Copy link
Contributor Author

machenmusik commented Mar 1, 2017

@cmvanb if you get the chance, can you check to see whether the latest version of this PR prevents the spurious trackpadmoved events you were seeing before?

@@ -34,34 +35,43 @@ mappings, events, and a Touch controller model.
| triggerup | Trigger released. |
| triggertouchstart | Trigger touched. |
| triggertouchend | Trigger no longer touched. |
| triggerchanged | Trigger changed. |
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI: Firefox ships with an implementation of experimental Gamepad events: gamepadbuttondown, gamepadbuttonup, and gamepadaxismove (fired on window).

If you're curious, might want to check these links out:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good to know, thanks. this code needs to support other browsers that currently lack those, let's revisit when more widely available?


// since each hand is named differently, avoid enumeration
// Inject with specific gamepad id, if provided. This works around a temporary issue
// where Chromium uses `Oculus Touch (Right)` but Firefox uses `Oculus Touch (right)`.
el.setAttribute('tracked-controls', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I have a similar not-just-tracked-controls set for aframe-arcade, but that's beyond this PR's scope, and last we discussed, core team preference was to keep non-tracked controllers (gearvr, daydream, oculus remote, xbox controller, etc) out of core. maybe file new issue to support controllers included with VR equipment including 0DOF (emulated pose and orientation) and 3DOF (emulated pose, true orientation)?

@@ -202,8 +202,8 @@ module.exports.Component = registerComponent('vive-controls', {
Object.keys(axesMapping).map(function (key) {
var value = axesMapping[key];
var detail = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

where is detail used? perhaps do var detail = evt.detail || {};?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, detail is used to build the translated/remapped event detail.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah, I didn't see it in the diff - my bad

var changed = false;
value.map(function (axisNumber) { changed |= evt.detail.changed[axisNumber]; });
var changed = !evt.detail.changed;
if (!changed) { value.map(function (axisNumber) { changed |= evt.detail.changed[axisNumber]; }); }
Copy link
Contributor

Choose a reason for hiding this comment

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

you can use forEach instead of map

@machenmusik
Copy link
Contributor Author

machenmusik commented Mar 2, 2017

@dmarcos relayed discussion with Mozillans where different Oculus Touch button/axis mapping is not likely to change soon (unlike id string). Can I urge Mozillans to reconsider? At least make analog buttons like trigger and grip generate actual button events, rather than treating them like sliders... otherwise even simple trigger button down/up will become unreliable

@machenmusik machenmusik mentioned this pull request Mar 2, 2017
@dmarcos
Copy link
Member

dmarcos commented Mar 2, 2017

I don't understand at first sight what the emulated property does. If I don't get it quickly that I wrote the original controls many other people will get confused too.

@machenmusik
Copy link
Contributor Author

See #2446 (comment)

@ngokevin
Copy link
Member

ngokevin commented Mar 2, 2017

emulated won't be used often, almost purely for motion capture replaying. Where you want to force controller event listeners to be added despite no controllers being present. Currently, we add controller event listeners regardless which causes events to be double-emitted.

@machenmusik
Copy link
Contributor Author

@ngokevin you might actually be better off also capturing emitted events from oculus-touch-controls and vive-controls (perhaps daydream and gearvr, not sure about leap hands) as well as those from tracked-controls, at which point you don't even need emulated true, you just need the app to be paying attention.

@ngokevin
Copy link
Member

ngokevin commented Mar 9, 2017

@dmarcos Any thoughts? Issue is when we made controls always add event listeners, controller events would double emit for hand-controls since both Oculus + Vive event handlers are attached. The proposal is an emulated property which controls when we force event listeners to be added. Let us know if you have other proposals (e.g., naming forceEventListeners, forcePresent), but the use case is for emulating + motion capture.

@machenmusik
Copy link
Contributor Author

should we deprecate this in favor of #2446?

@ngokevin
Copy link
Member

It's more easy to review if things are in isolation, so we'll try not to clump non-cohesive changes together in one PR. @dmarcos still needs to take a look and understand, but the tl;dr is that we forced event listeners to be added regardless of Gamepad present in order for motion capture, but that causes doubly-emitted events (vive + oculus controls). The proposed emulated property will only add all event listeners if specifically enabled.

@machenmusik
Copy link
Contributor Author

Right. if you merge this PR, #2446 will simplify itself :-)

@ngokevin
Copy link
Member

Would you mind breaking out the axismove changes to another PR?

@machenmusik
Copy link
Contributor Author

machenmusik commented Mar 21, 2017

so which things between this and #2446 want to be separate PRs? there is some overlap so may be nontrivial (e.g. #2423 (comment)), and i am tied up for a little while

i think we definitely want the fix for #2448 that @caseyyee confirmed via cherry-pick, that one is easy (now #2510), but maybe not entirely done with Nightly Rift+Touch without #2446

i think we need to fix the double-event issue introduced unless we move back to auto-detect-controllers paradigm, and IIRC the axismove changes here are needed now to prevent spurious events from prior vive-specific PR / Nightly having control elements be both buttons and axes, so a bit hesitant to separate temporarily only to have them merged back together shortly

@machenmusik
Copy link
Contributor Author

w.r.t. separating PRs: I think these delineations would work

may get a chance to look at that late this evening.

@machenmusik machenmusik force-pushed the emulated-oculus-and-vive-controls branch from 3b8199b to 1751b6c Compare March 21, 2017 23:23
@machenmusik machenmusik changed the title Emulated oculus and vive controls with axismove handler Emulated oculus and vive controls Mar 21, 2017
@ngokevin
Copy link
Member

After discussing in Slack, it seems we'll revert #2314 for another solution, so we don't need emulated. Thanks!

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.

Controller events being emitted double (e.g., vive-controls gripup)
5 participants