Normalize zoom speed and wheel behavior across trace types by rreusser · Pull Request #2041 · plotly/plotly.js

rreusser · 2017-09-28T03:07:30Z

This PR attempts to normalize mouse wheel behavior across cartesian plots as well as gl2d and gl3d trace types. In particular, it:

multiplies the default zoom speed of gl plots by ten in order to get them more inline with non-gl plots
~~makes zoom speed configurable through layout.zoomspeed (should this be a config parameter?)~~ Edit: no longer configurable; just faster
removes logic that prevents mousewheel zooming while gl plots are in pan mode (corresponding to analogous svg behavior, which does not block mousewheel zoom in pan mode)

rreusser · 2017-09-28T03:08:34Z

src/plots/plots.js

    delete gd._transitionData;
    delete gd._transitioning;
    delete gd._initialAutoSize;
+    delete gd._transitioningWithDuration;


This was just one minor cleanup tweak I noticed along the way.

Good catch 👌

etpinard

Looking good. Thanks for doing this @rreusser 🎉

etpinard · 2017-09-28T13:30:27Z

src/plots/gl2d/camera.js

        var lastX = result.lastPos[0],
            lastY = result.lastPos[1];

-        switch(scene.fullLayout.dragmode) {


Why is this gone? fullLayout.dragmode: false should still be a thing.

From what I can tell, dragmode: false isn't an option, is it? If I set that, it defaults to zoom. This is removed for a slightly different reason though: because the dragmode should not be affecting mousewheel behavior at all.

Ahhh possible bug…… 🕷

When setting dragmode: false with gl3d, dragging is disabled but gd._fullLayout.dragmode = 'zoom'. That suggests it's sanitizing it but using gd.layout.dragmode instead of gd._fullLayout.dragmode. Actually setting dragmode: 'zoom' behaves correctly.

Looks like we only support layout.scene.dragmode: false (ie 3D), not layout.dragmode: false
@rreusser good catch that this should not be disabled in zoom mode anyway.

Oh, I see. It inherits false from layout.dragmode despite layout.dragmode: false not actually being a valid default. I'm okay with that.

Oopppppppps, that line is in gl2d/camera.js (i.e. NOT gl3d). Funny that thing has been there since v1.0.0.

🔪 🔪 🔪 🔪 🔪 🔪

Yeah. Definitely undesirable. 😄 🔪

(next most glaring thing, as briefly mentioned to @alexcjohnson is double-click to revert zoom to autoscale, which is the top top top thing that always gets me about scattergl plots.)

Of course not sure the meaning/relevance once regl comes through, but still seems right given the relatively low effort involved for these tweaks.

which is the top top top thing that always gets me about scattergl plots.)

No need to waste time on this. Double-click will just work after @dfcreative 's #1869

Works for me. Should have bothered to do it long ago, but I'm content to wait for #1869. 🎉

etpinard · 2017-09-28T13:30:59Z

src/plots/gl2d/camera.js

-                scene.relayoutCallback();
-                break;
-        }
+        var scale = Math.exp(scene.fullLayout.zoomspeed * 10.0 * dy / (viewBox[3] - viewBox[1]));


Nice. Can we put that 10.0 in https://github.com/plotly/plotly.js/blob/master/src/constants/interactions.js ?

etpinard · 2017-09-28T13:31:20Z

src/plots/plots.js

    delete gd._transitionData;
    delete gd._transitioning;
    delete gd._initialAutoSize;
+    delete gd._transitioningWithDuration;


Good catch 👌

alexcjohnson · 2017-09-28T13:39:43Z

Nice, definitely better parity this way. Playing with it now though, I wonder if something a little slower (maybe half this speed?) might be best for both versions. I still think you're right not to have it configurable though.

Also, we decided long ago that 3D should have scroll zoom on always, but for SVG you need to have config.scrollZoom: true to enable it. Seems like gl2D should respect this as well, right?

rreusser · 2017-09-28T13:44:06Z

Mousewheel speed is a contentious issue. I'd support half speed for all. They at least felt same-ish
to me with these numbers. 🐭

Can you clarify the last point? For me when scrollZoom: false,

scatter: no mousewheel zoom
scattergl: mousewheel zoom
scatter3d: mousewheel zoom

alexcjohnson · 2017-09-28T13:46:03Z

Can you clarify the last point? For me when scrollZoom: false,

scatter: no mousewheel zoom

scattergl: mousewheel zoom

scatter3d: mousewheel zoom

I think scatter and scattergl should match, but scatter3d should have wheel zoom enabled always.

rreusser · 2017-09-28T14:03:11Z

Okay, I've 1/2'd all zoom speeds (including svg, which previously wasn't affected by this PR) and scattergl now obeys config.scrollZoom.

etpinard · 2017-09-28T14:06:34Z

I think scatter and scattergl should match, but scatter3d should have wheel zoom enabled always.

👍 for me here.

But, I think in v2 all subplot types should obey scrollZoom. Moreover, I think should add a corresponding layout attribute as people might want to turn on/off scroll interactions via a relayout call. Thoughts?

rreusser · 2017-09-28T14:07:49Z

Agreed. Including gl3d. I actually rather dislike scrollZoom being a config parameter. I find embedplot, for one, suddenly has dramatic scroll interactions I never saw when developing the plot.

rreusser · 2017-09-28T21:06:36Z

Okay, I'm now content with this. Had to fix test values. The big change is cartesian zoom speed *= 1/2. Which doesn't seem all that noticeable because of inertia. A much bigger change is gl zoom speed *= 5.

alexcjohnson · 2017-09-28T22:11:06Z

💃 from my side. I have more thoughts about scrollZoom but we can discuss that more in the v2 issue #420

etpinard · 2017-09-28T22:20:28Z

@rreusser what about those scroll factors. Looks like cartesian, gl2d and gl3d all use different one. Is that why you didn't bother putting them in a constants.js file?

rreusser · 2017-09-28T22:38:06Z

@etpinard Yes. I debated whether to copy the zoom factor logic. gl3d, for one, takes for granted that the window height is actually relevant:

-camera.zoomSpeed * flipY * dy / window.innerHeight * (t - view.lastT()) / 100.0;

Perhaps that should be modified to use the plot height and conform to the same math as SVG.

etpinard · 2017-09-29T13:37:59Z

Perhaps that should be modified to use the plot height and conform to the same math as SVG.

No need. I was just curious why my comment didn't get addressed.

I'm happy with the current state of this PR 💃

Normalize zoom speed and wheel behavior across trace types

394014e

rreusser commented Sep 28, 2017

View reviewed changes

Remove zoomspeed parameter

e489a72

etpinard suggested changes Sep 28, 2017

View reviewed changes

rreusser added 2 commits September 28, 2017 07:01

1/2 zoom speed for all and disableable zoom in scattergl

99101fe

Lint it

e231233

etpinard added status: reviewable bug something broken labels Sep 28, 2017

etpinard added this to the v1.31.0 milestone Sep 28, 2017

rreusser added 2 commits September 28, 2017 13:17

Modify zoom test values to match new wheel rate

8ddb965

Fix zoom values in click test

4265a38

etpinard approved these changes Sep 29, 2017

View reviewed changes

alexcjohnson mentioned this pull request Sep 29, 2017

wishlist for potential breaking changes since v1 #420

Closed

15 tasks

rreusser merged commit 2f2b300 into master Sep 29, 2017

rreusser deleted the normalize-zoom-speed branch September 29, 2017 15:37

etpinard mentioned this pull request Sep 29, 2017

Make scrollZoom speed configurable #1085

Open

Uh oh!

Conversation

rreusser commented Sep 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etpinard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rreusser Sep 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rreusser Sep 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etpinard Sep 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rreusser Sep 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexcjohnson commented Sep 28, 2017

Uh oh!

rreusser commented Sep 28, 2017

Uh oh!

alexcjohnson commented Sep 28, 2017

Uh oh!

rreusser commented Sep 28, 2017

Uh oh!

etpinard commented Sep 28, 2017

Uh oh!

rreusser commented Sep 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rreusser commented Sep 28, 2017

Uh oh!

alexcjohnson commented Sep 28, 2017

Uh oh!

etpinard commented Sep 28, 2017

Uh oh!

rreusser commented Sep 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

etpinard commented Sep 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rreusser commented Sep 28, 2017 •

edited

Loading

rreusser Sep 28, 2017 •

edited

Loading

rreusser Sep 28, 2017 •

edited

Loading

etpinard Sep 28, 2017 •

edited

Loading

rreusser Sep 28, 2017 •

edited

Loading

rreusser commented Sep 28, 2017 •

edited

Loading

rreusser commented Sep 28, 2017 •

edited

Loading