throttle updateControllerList while keeping getGamepads call #3112

ngokevin · 2017-10-03T14:50:12Z

Description:

Changes proposed:

dmarcos

I believe we had this in the past and we removed it because we needed to request a new gamepad object in chrome in order to get an updated pose

machenmusik · 2017-10-03T15:23:31Z

Make sure you test chrome rift/vive/daydream here, as @dmarcos notes we have encountered quirks or issues. (Actually I am not sure whether we characterized Edge once we realized that we broke Chrome)

ngokevin · 2017-10-03T15:56:01Z

If you check the code, see that we still call getGamepads. The only thing I'm throttling is rebuilding the controller list. It's different from what we did before.

machenmusik · 2017-10-03T17:17:20Z

Yeah, we started down this avenue of approach but backed off at the time with #2984. One suggestion is to make the interval a system configurable parameter, as for some apps 500ms may be slower than desired (as this impacts all tracked controls, and we no longer have the event listeners which would fire immediately when available)

machenmusik · 2017-10-03T17:20:31Z

I suspect that if we changed the throttle once we have seen 1 (for 3DOF) or 2 (for 6DOF) tracked controls, we could reduce the interval to almost never.

ngokevin · 2017-10-03T22:26:01Z

#2984 was to fix the Chrome issue with navigator.getGamepads not getting called, this avenue was not approached. I can't imagine a couple hundred milliseconds affecting applications in controller detection, it will be hardly noticeable.

cvan · 2017-10-05T00:27:03Z

src/systems/tracked-controls.js

-    this.updateControllerList();
+    var gamepads;
+    // Call getGamepads for Chrome.
+    gamepads = navigator.getGamepads && navigator.getGamepads();


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

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

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.

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.

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.

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.

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

@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?

cvan · 2017-10-05T04:02:36Z

src/systems/tracked-controls.js

@@ -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);


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

ngokevin · 2017-10-05T15:32:43Z

Works on FF. Need to test on Chrome/Daydream/GearVR.

machenmusik · 2017-10-05T16:49:26Z

(We should test recent support for Edge as well)

vincentfretin · 2017-10-06T10:08:32Z

It seems to be working fine on GearVR with Oculus browser. I don't see any difference.

dmarcos · 2017-10-06T11:28:08Z

@vincentfretin thanks for looking into it! Do you see any performance difference in the controller motion with and without the patch?

vincentfretin · 2017-10-06T12:14:01Z

With or without the patch, the controller motion is smooth. I don't see any difference.

machenmusik · 2017-10-06T13:32:16Z

Nice - for completeness I think we need to confirm
chromium/Rift+Touch and chrome/daydream - the former being what stopped us before IIRC.

ngokevin · 2017-10-25T20:07:58Z

Worked fine on Chromium/Rift. Is that enough? Anyone want to try to Daydream?

cvan · 2017-10-26T22:48:02Z

I tried this on Gear VR: Oculus Browser and Samsung Internet on a Samsung Galaxy S8. compared to master, the controller doesn't seem worse. I can't perceive perf improvements, if there are any.

@ngokevin: which examples are you testing with? Laser Controls, Tracked Controls?

ngokevin · 2017-10-26T22:57:54Z

Thanks. Either, anything with hands. Perf improvements are memory-leak/garbage related:

cvan

looks good

ngokevin · 2017-11-03T19:16:47Z

@dmarcos r?

machenmusik · 2017-11-07T14:41:13Z

I thought we merged this already, apparently I was wrong. Is anything blocking, or just haven't gotten to it yet?

dmarcos · 2017-11-07T16:13:58Z

@machemusik Have you had the chance to test it a bit? Do you remember the platforms where we had problems with low framerate controllers updates?

ngokevin · 2017-11-07T17:23:04Z

@dmarcos This was tested on Chromium/Rift which was one of the platforms. The Gamepad call is called every tick, it's just A-Frame's controller list rebuild is throttled.

ngokevin added the needs review label Oct 3, 2017

dmarcos reviewed Oct 3, 2017

View reviewed changes

ngokevin force-pushed the throttlecontrollerlist branch 3 times, most recently from 11567b3 to e86bca3 Compare October 3, 2017 16:08

cvan reviewed Oct 5, 2017

View reviewed changes

ngokevin added needs answers / feedback and removed needs review labels Oct 5, 2017

ngokevin added needs review and removed needs answers / feedback labels Oct 25, 2017

throttle updateControllerList while keeping getGamepads call

80e5401

ngokevin force-pushed the throttlecontrollerlist branch from e86bca3 to 80e5401 Compare October 26, 2017 23:01

cvan approved these changes Oct 27, 2017

View reviewed changes

dmarcos merged commit 7cc2cef into aframevr:master Nov 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

throttle updateControllerList while keeping getGamepads call #3112

throttle updateControllerList while keeping getGamepads call #3112

ngokevin commented Oct 3, 2017

dmarcos left a comment

machenmusik commented Oct 3, 2017

ngokevin commented Oct 3, 2017

machenmusik commented Oct 3, 2017

machenmusik commented Oct 3, 2017

ngokevin commented Oct 3, 2017

cvan Oct 5, 2017 •

edited

Loading

machenmusik Oct 5, 2017

cvan Oct 5, 2017 •

edited

Loading

machenmusik Oct 5, 2017

cvan Oct 5, 2017

dmarcos Oct 5, 2017

cvan Oct 5, 2017

cvan Oct 5, 2017

ngokevin commented Oct 5, 2017

machenmusik commented Oct 5, 2017

vincentfretin commented Oct 6, 2017

dmarcos commented Oct 6, 2017

vincentfretin commented Oct 6, 2017

machenmusik commented Oct 6, 2017

ngokevin commented Oct 25, 2017

cvan commented Oct 26, 2017

ngokevin commented Oct 26, 2017

cvan left a comment

ngokevin commented Nov 3, 2017

machenmusik commented Nov 7, 2017

dmarcos commented Nov 7, 2017

ngokevin commented Nov 7, 2017

throttle updateControllerList while keeping getGamepads call #3112

throttle updateControllerList while keeping getGamepads call #3112

Conversation

ngokevin commented Oct 3, 2017

Changes proposed:

dmarcos left a comment

Choose a reason for hiding this comment

machenmusik commented Oct 3, 2017

ngokevin commented Oct 3, 2017

machenmusik commented Oct 3, 2017

machenmusik commented Oct 3, 2017

ngokevin commented Oct 3, 2017

cvan Oct 5, 2017 • edited Loading

Choose a reason for hiding this comment

machenmusik Oct 5, 2017

Choose a reason for hiding this comment

cvan Oct 5, 2017 • edited Loading

Choose a reason for hiding this comment

machenmusik Oct 5, 2017

Choose a reason for hiding this comment

cvan Oct 5, 2017

Choose a reason for hiding this comment

dmarcos Oct 5, 2017

Choose a reason for hiding this comment

cvan Oct 5, 2017

Choose a reason for hiding this comment

cvan Oct 5, 2017

Choose a reason for hiding this comment

ngokevin commented Oct 5, 2017

machenmusik commented Oct 5, 2017

vincentfretin commented Oct 6, 2017

dmarcos commented Oct 6, 2017

vincentfretin commented Oct 6, 2017

machenmusik commented Oct 6, 2017

ngokevin commented Oct 25, 2017

cvan commented Oct 26, 2017

ngokevin commented Oct 26, 2017

cvan left a comment

Choose a reason for hiding this comment

ngokevin commented Nov 3, 2017

machenmusik commented Nov 7, 2017

dmarcos commented Nov 7, 2017

ngokevin commented Nov 7, 2017

cvan Oct 5, 2017 •

edited

Loading

cvan Oct 5, 2017 •

edited

Loading