gblazex / smoothscroll

An extension for Google Chrome (150,000+ users)
https://chrome.google.com/webstore/detail/smoothscroll/nbokbjkabcmbfdlbddjidfmibcpneigj
Other
474 stars 98 forks source link

Bad new Date() implementation results in non-smooth scroll #81

Closed unphased closed 10 years ago

unphased commented 10 years ago

Here is some related code with my own debug modification (the console.log line)

    var step = function (time) {

        var now = +new Date;
        console.log(now);
        var scrollX = 0;
        var scrollY = 0; 

        for (var i = 0; i < que.length; i++) {

            var item = que[i];
            var elapsed  = now - item.start;
            var finished = (elapsed >= options.animationTime);

            // scroll position: [0, 1]
            var position = (finished) ? 1 : elapsed / options.animationTime;

When running this on a 120Hz display on the latest Chrome, the now variable only takes on times on 1/60 second steps even though the value provided is 1/1000 second precision (an integer millisecond). This means while step runs 120 times a sec, it's in a step-function in 60-times-a-second steps.

1389496052888 sscr.js:243
1389496052904 sscr.js:243
1389496052904 sscr.js:243
1389496052919 sscr.js:243
1389496052919 sscr.js:243
1389496052935 sscr.js:243
1389496052935 sscr.js:243
1389496052951 sscr.js:243
1389496052951 sscr.js:243
1389496052966 sscr.js:243

This makes my pages scroll only as smooth as it would on a regular screen, not nearly as butter-smooth as it used to when this new Date() wasn't freaking broken. I'm the only coder on earth with overclocked eyeballs (who has rare 120Hz monitors) it seems. I wasn't born with them. Just trained through years of counterstrike.

Proposed fix possibilities:

A. some sort of estimator or accumulator/estimator contraption that converges upon the velocity, rather than computing instantaneous time difference (elapsed is 0.016, then 0, then 0.016, then 0, etc.... it needs to be 0.008, 0.008, 0.009, 0.008... etc.)

B. do not depend on now to compute offsets. Just assume that the step is always some X amount of time after the last execution of step. It's okay if there is error here and the scroll takes a fraction longer time to fully complete (neither will this property hold with solution A). With this, e.g. a 120Hz monitor would perceive the speed of the animation complete twice as fast! This will be much appreciated by the folks who ordinarily use 120Hz monitors. Would not be a bug by my standard.

Edit: It's annoying me sufficiently that I'm probably gonna get you a pull request shortly

unphased commented 10 years ago

I attempted some sort of hybrid solution between A and B and it did not work well, it was difficult to get it not to negatively impact the smoothness when run on my monitor set at 60Hz.

What's making it difficult is the scrolling offset computation is completely dependent on the time sample. This is good design but this damned chrome bug is making it really hard to hack a fix for it quickly.

unphased commented 10 years ago

I am finding that the code from my Chrome is more recent than the code on this repo. There is a change in here for the wheel event. That makes my planned pull request a little weird.

diff --git a/src/sscr.js b/src/sscr.js
old mode 100755
new mode 100644
index dfb101d..92c9ce7
--- a/src/sscr.js
+++ b/src/sscr.js
@@ -225,9 +225,9 @@ function scrollArray(elem, left, top, delay) {
     que.push({
         x: left,
         y: top,
+        now: 0,
         lastX: (left < 0) ? 0.99 : -0.99,
         lastY: (top  < 0) ? 0.99 : -0.99,
-        start: +new Date
     });

     // don't act if there's a pending queue
@@ -246,7 +246,9 @@ function scrollArray(elem, left, top, delay) {
         for (var i = 0; i < que.length; i++) {

             var item = que[i];
-            var elapsed  = now - item.start;
+
+            elapsed = item.now;
+            item.now += 16;
             var finished = (elapsed >= options.animationTime);

             // scroll position: [0, 1]
@@ -580,6 +582,9 @@ function pulse(x) {
     return pulse_(x);
 }

+// new standard wheel event from Chrome 31+
+var wheelEvent = "onwheel" in document.createElement("div") ? "wheel" : "mousewheel";
+
 addEvent("mousedown", mousedown);
-addEvent("mousewheel", wheel);
-addEvent("load", init);
\ No newline at end of file
+addEvent(wheelEvent, wheel);
+addEvent("load", init);
(END)
gblazex commented 10 years ago

Alright so it's a timer resolution issue. Can you try this new version of the test page to see if performance.now works better:

http://jsbin.com/fps-test/3

gblazex commented 10 years ago

Btw if you feel the behavior is unacceptable I think you should file a Chrome bug report here: https://code.google.com/p/chromium/issues/list

unphased commented 10 years ago

Yep sorry it took a while to get around to this. Here's how the numbers show in the jsbin test page (at 115 Hz):

8.000000000038199
8.999999999957708
9.000000000014552
9.000000000014552
7.999999999981355
9.000000000014552
7.999999999981355
9.000000000014552
9.000000000014552
8.999999999957708
8.000000000038199
8.999999999957708
9.000000000014552
8.000000000038199
8.999999999957708
9.000000000014552
7.999999999981355
9.000000000014552
9.000000000014552
7.999999999981355
9.000000000014552

Checks out (the precision is 1ms, and the average should be 8.7ms).

gblazex commented 10 years ago

1. Here is a test version with performance.now(), let me know how it works:

2. Can you also run this test page? http://jsbin.com/fps-test/5 I'm curious how Date.now() behaves.

unphased commented 10 years ago

For 2:

Date.now deltas
0
802
26
6
15
9
9
8
9
9
8
9
9
8
9
9
9
8
9
9
8
9
9
8
9
9
8
9
9
9
8
9
9
8
9
9
8
9
9
9
8
9
9
8
9
9
8
9
9
10
7
18
8
9
9
8
9
9
8
9
9
11
8
10
7
10
17
7
11
16
8
10
7
8
9
9
8
9
9
8
9
9
8
9
9
9
8
9
9
8
11
7
9
8
9
9
8
9
9

FWIW, I did reinstall the plugin just now (blowing away my custom changes noted in the OP of this issue) and it seems like I am getting proper full refresh rate behavior. However, when the Chrome window is Maximized, it produces this:

Date.now deltas
0
47
9
27
30
17
25
27
26
26
26
26
26
26
26
26
26
26
26
27
26
26
26
27
28
23
26
26
26
26
27
26
26
26
26
26
20
18
18
17
17
18
17
17
18
17
18
17
17
14
21
18
18
16
18
18
17
17
18
17
17
15
22
12
20
19
18
17
17
17
18
19
16
18
17
17
18
17
18
17
17
18
17
18
17
17
18
17
18
17
17
18
17
17
18
17
18
17
17

It's pretty aggravating (also, what the hell's with the 26's?)

gblazex commented 10 years ago

It could be that Chrome cannot render that large paint area so it caps the FPS at 60 for full screen. And even then some frames run over budget (>16ms).

But point (1) and (2) use different timestamps.

So the test version in (1) performs as expected you would say?

gblazex commented 10 years ago

Also can you test performance.now() with a full screen window as well? http://jsbin.com/fps-test/3

unphased commented 10 years ago
performance.now deltas
0
14.000000001033186
54.00000000008731
21.99999999902502
4.000000000814907
10.000000000218279
13.999999999214197
4.000000000814907
6.999999999607098
9.000000000014552
18.000000000029104
16.999999999825377
16.999999999825377
16.999999999825377
18.000000000029104
16.999999999825377
16.999999999825377
13.00000000082946
19.00000000023283
15.99999999962165
18.000000000029104
15.99999999962165
16.999999999825377
16.999999999825377
16.999999999825377
17.000000001644366
16.999999999825377
15.99999999962165
16.999999999825377
18.000000000029104
16.999999999825377
18.000000000029104
16.999999999825377
16.999999999825377
18.000000000029104
16.999999999825377
20.000000000436557
19.00000000023283
16.999999999825377
16.999999999825377
16.999999999825377
16.999999999825377
18.000000000029104
16.999999999825377
18.000000000029104
16.999999999825377
17.000000001644366
23.999999999432475
25.99999999983993
25.99999999983993
27.000000000043656
24.999999999636202
25.99999999983993
25.99999999983993
25.99999999983993
26.00000000165892
25.99999999983993
25.99999999983993
25.99999999983993
27.000000000043656
25.99999999983993
25.99999999983993
25.99999999983993
25.99999999983993
25.99999999983993
25.99999999983993
25.99999999983993
26.00000000165892
25.99999999983993
25.99999999983993
27.000000000043656
25.99999999983993
25.99999999983993
25.99999999983993
25.99999999983993
25.99999999983993
25.99999999983993
25.99999999983993
25.99999999983993
27.000000000043656
25.00000000145519
25.99999999983993
27.000000000043656
25.99999999983993
25.99999999983993
25.99999999983993
25.99999999983993
25.99999999983993
25.99999999983993
25.99999999983993
27.000000000043656
24.999999999636202
25.99999999983993
26.00000000165892
27.000000000043656
25.99999999983993
25.99999999983993
25.99999999983993
25.99999999983993
25.99999999983993

Well, this is pretty crappy, It's definitely not a performance thing. I can manually resize a normal window to larger than maximized size and it works fine. (and it's mostly not even firing at 60Hz, it's just barely over 30)

unphased commented 10 years ago

Could you show me how to install an extension from zip?

gblazex commented 10 years ago

Ah sorry. Forgot to include that.

1) Extract to any directory. 2) Go to this URL in Chrome: chrome://extensions/ 3) Check 'Developer Mode' 4) Click the 'Load Unpacked Extension...' button

gblazex commented 10 years ago

I think Chrome will not be able to render consistently at 120 FPS on full screen. I'll change the timers to use Date.now() as it seems to give better results.

unphased commented 10 years ago

They have been changing things, it seems. I have been seeing smooth performance under all conditions recently, not sure if I am still running code that I modified, though.

gblazex commented 10 years ago

great :)