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.

Sign up for GitHub

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

Jump to bottom

throttle updateControllerList while keeping getGamepads call #3112

Merged
merged 1 commit into from
Nov 7, 2017
Merged

throttle updateControllerList while keeping getGamepads call #3112

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions src/systems/tracked-controls.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
var registerSystem = require('../core/system').registerSystem;
var utils = require('../utils');

/**
* Tracked controls system.
Expand All @@ -10,7 +11,8 @@ module.exports.System = registerSystem('tracked-controls', {

this.controllers = [];

this.updateControllerList();
this.updateControllerList(navigator.getGamepads && navigator.getGamepads());
this.throttledUpdateControllerList = utils.throttle(this.updateControllerList, 500, this);
Copy link
Contributor

Choose a reason for hiding this comment

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

it might be nice to allow this throttle duration to be configured by a component value (with a default value of 500 milliseconds)


if (!navigator.getVRDisplays) { return; }

Expand All @@ -22,20 +24,21 @@ module.exports.System = registerSystem('tracked-controls', {
},

tick: function () {
this.updateControllerList();
var gamepads;
// Call getGamepads for Chrome.
gamepads = navigator.getGamepads && navigator.getGamepads();
Copy link
Contributor

@cvan cvan Oct 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

how does this help if navigator.getGamepads() is still getting called on every tick?

why not keep this in updateControllerList (which is now throttled)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Some backstory, that @ngokevin @dmarcos and I ran into last time around - if you don't call navigator.getGamepads(), then if you are using Chromium/Chrome (which includes derivatives like Oculus Browser) you won't get gamepad updates until the next one. Firefox doesn't care, but on several prominent platforms (GearVR, Daydream) the only native WebVR browser options are Chromium/Chrome-based. As I mentioned, I don't recall if we characterized Edge in this regard.

Copy link
Contributor

@cvan cvan Oct 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

thanks - yep, I know that about Chrome. that inconsistency in browser implementations is what this spec issue is for: w3c/gamepad#8

what I'm suggesting to do is move the navigator.getGamepads() call to be inside the throttled method, throttledUpdateControllerList. unless I'm missing something, that would still handle the Chrome case, since it's called on every tick.

Copy link
Contributor

Choose a reason for hiding this comment

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

I thought the point of the throttle was to NOT call the throttled method every tick, but rather to enforce a minimum interval by ignoring calls that happen too soon since the last one.

Copy link
Contributor

Choose a reason for hiding this comment

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

that’s what I’m suggesting. the code as is here checks for gamepads on every tick. can’t tell if I’m misunderstanding or not heh.

Copy link
Member

Choose a reason for hiding this comment

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

This has to be tested across all browsers before being considered for merge. The reason navigator.getGamepads is called on every tick is because is needed in order to get updated poses for the controllers in some browser. The gamepad handle pose won’t update automatically and you see judder in the controller motion. Calling navigator.getGamepads might not be enough and retriving a new gamepad object might be necessary. That’s has to be determined after testing

Copy link
Contributor

Choose a reason for hiding this comment

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

@dmarcos: awesome, thanks for the explanation. the pose information in each Gamepad object is the reason.

in Chrome, navigator.getGamepads() is always an array of length 4. and just get an index from that array, and the Gamepad object is live. can’t use reference variables, obviously, as you know.

I think a possible improvement here would be to check for conencted/disconnected gamepads every 500 milliseconds, and have tick have separate checks for each individual Gamepad object.

that’s admittedly a lot of work for perhaps little benefit. any flare charts available to look at for some examples?

this.throttledUpdateControllerList(gamepads);
},

/**
* Update controller list.
*/
updateControllerList: function () {
updateControllerList: function (gamepads) {
var controllers = this.controllers;
var gamepad;
var gamepads;
var i;
var prevCount;

gamepads = navigator.getGamepads && navigator.getGamepads();
if (!gamepads) { return; }

prevCount = controllers.length;
Expand Down