Skip to content

Normalize zoom speed and wheel behavior across trace types #2041

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

Merged
merged 6 commits into from
Sep 29, 2017
Merged

Normalize zoom speed and wheel behavior across trace types #2041

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
394014e
Normalize zoom speed and wheel behavior across trace types
rreusser Sep 28, 2017
e489a72
Remove zoomspeed parameter
rreusser Sep 28, 2017
99101fe
1/2 zoom speed for all and disableable zoom in scattergl
rreusser Sep 28, 2017
e231233
Lint it
rreusser Sep 28, 2017
8ddb965
Modify zoom test values to match new wheel rate
rreusser Sep 28, 2017
4265a38
Fix zoom values in click test
rreusser Sep 28, 2017
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
11 changes: 11 additions & 0 deletions src/components/fx/layout_attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,17 @@ module.exports = {
'3D scenes.'
].join(' ')
},
zoomspeed: {
valType: 'number',
role: 'info',
dflt: 1,
min: 0,
editType: 'none',
description: [
'Sets the speed at which the mouse wheel zooms the plot. Larger',
'values are faster.'
].join(' ')
},
hovermode: {
valType: 'enumerated',
role: 'info',
Expand Down
1 change: 1 addition & 0 deletions src/components/fx/layout_defaults.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ module.exports = function supplyLayoutDefaults(layoutIn, layoutOut, fullData) {
}

coerce('dragmode');
coerce('zoomspeed');

var hovermodeDflt;
if(layoutOut._has('cartesian')) {
Expand Down
2 changes: 1 addition & 1 deletion src/plots/cartesian/dragbox.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,7 +380,7 @@ module.exports = function dragBox(gd, plotinfo, x, y, w, h, ns, ew) {
return;
}

var zoom = Math.exp(-Math.min(Math.max(wheelDelta, -20), 20) / 100),
var zoom = Math.exp(-Math.min(Math.max(wheelDelta, -20), 20) / 100 * fullLayout.zoomspeed),
gbb = mainplot.draglayer.select('.nsewdrag')
.node().getBoundingClientRect(),
xfrac = (e.clientX - gbb.left) / gbb.width,
Expand Down
49 changes: 21 additions & 28 deletions src/plots/gl2d/camera.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,34 +262,27 @@ function createCamera(scene) {
var lastX = result.lastPos[0],
lastY = result.lastPos[1];

switch(scene.fullLayout.dragmode) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@rreusser rreusser Sep 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@rreusser rreusser Sep 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@etpinard etpinard Sep 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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

🔪 🔪 🔪 🔪 🔪 🔪

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. Definitely undesirable. 😄 🔪

Copy link
Contributor Author

@rreusser rreusser Sep 28, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

(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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

case 'zoom':
break;

case 'pan':
var scale = Math.exp(0.1 * dy / (viewBox[3] - viewBox[1]));

var cx = lastX /
(viewBox[2] - viewBox[0]) * (dataBox[2] - dataBox[0]) +
dataBox[0];
var cy = lastY /
(viewBox[3] - viewBox[1]) * (dataBox[3] - dataBox[1]) +
dataBox[1];

dataBox[0] = (dataBox[0] - cx) * scale + cx;
dataBox[2] = (dataBox[2] - cx) * scale + cx;
dataBox[1] = (dataBox[1] - cy) * scale + cy;
dataBox[3] = (dataBox[3] - cy) * scale + cy;

scene.setRanges(dataBox);

result.lastInputTime = Date.now();
unSetAutoRange();
scene.cameraChanged();
scene.handleAnnotations();
scene.relayoutCallback();
break;
}
var scale = Math.exp(scene.fullLayout.zoomspeed * 10.0 * dy / (viewBox[3] - viewBox[1]));
Copy link
Contributor

Choose a reason for hiding this comment

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

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


var cx = lastX /
(viewBox[2] - viewBox[0]) * (dataBox[2] - dataBox[0]) +
dataBox[0];
var cy = lastY /
(viewBox[3] - viewBox[1]) * (dataBox[3] - dataBox[1]) +
dataBox[1];

dataBox[0] = (dataBox[0] - cx) * scale + cx;
dataBox[2] = (dataBox[2] - cx) * scale + cx;
dataBox[1] = (dataBox[1] - cy) * scale + cy;
dataBox[3] = (dataBox[3] - cy) * scale + cy;

scene.setRanges(dataBox);

result.lastInputTime = Date.now();
unSetAutoRange();
scene.cameraChanged();
scene.handleAnnotations();
scene.relayoutCallback();

return true;
});
Expand Down
2 changes: 1 addition & 1 deletion src/plots/gl3d/camera.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ function createCamera(element, options) {
if(Math.abs(dx) > Math.abs(dy)) {
view.rotate(t, 0, 0, -dx * flipX * Math.PI * camera.rotateSpeed / window.innerWidth);
} else {
var kzoom = -camera.zoomSpeed * flipY * dy / window.innerHeight * (t - view.lastT()) / 100.0;
var kzoom = -camera.zoomSpeed * flipY * dy / window.innerHeight * (t - view.lastT()) / 10.0;
view.pan(t, 0, 0, distance * (Math.exp(kzoom) - 1));
}
}, true);
Expand Down
1 change: 1 addition & 0 deletions src/plots/gl3d/scene.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,7 @@ function initializeGLPlot(scene, fullLayout, canvas, gl) {
center: [cameraData.center.x, cameraData.center.y, cameraData.center.z],
eye: [cameraData.eye.x, cameraData.eye.y, cameraData.eye.z],
up: [cameraData.up.x, cameraData.up.y, cameraData.up.z],
zoomSpeed: fullLayout.zoomspeed,
zoomMin: 0.1,
zoomMax: 100,
mode: 'orbit'
Expand Down
1 change: 1 addition & 0 deletions src/plots/plots.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1332,6 +1332,7 @@ plots.purge = function(gd) {
delete gd._transitionData;
delete gd._transitioning;
delete gd._initialAutoSize;
delete gd._transitioningWithDuration;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch 👌


// remove all event listeners
if(gd.removeAllListeners) gd.removeAllListeners();
Expand Down