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

Generalize spikelines #2379

Merged
merged 9 commits into from
Feb 19, 2018
Merged

Generalize spikelines #2379

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ed19b7c
generalize spikelines
alexcjohnson Feb 14, 2018
e2c8c04
simplify hover_label_test with more clearThrottle
alexcjohnson Feb 17, 2018
965bb88
lint - remove no longer needed imports
alexcjohnson Feb 17, 2018
d34ba81
remove gl2d_line_select mock that got pushed to master accidentally
alexcjohnson Feb 17, 2018
5164cdf
add test for no spikelines on scatter fills
alexcjohnson Feb 17, 2018
a2fc07a
one more flaky test suite
alexcjohnson Feb 17, 2018
ad38f8d
fix and comment on bar positionFn passVal
alexcjohnson Feb 19, 2018
eb98c91
fix #2370
alexcjohnson Feb 19, 2018
31e4e34
more comments on (hover|spike)distance and point vs area objects
alexcjohnson Feb 19, 2018
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
3 changes: 0 additions & 3 deletions src/components/fx/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,6 @@
'use strict';

module.exports = {
// max pixels away from mouse to allow a point to highlight
MAXDIST: 20,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

not using this constant anymore, we just have the defaults in layout.hoverdistance and layout.spikedistance, with which the users of this constant were in conflict.


// hover labels for multiple horizontal bars get tilted by this angle
YANGLE: 60,

Expand Down
24 changes: 10 additions & 14 deletions src/components/fx/helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
'use strict';

var Lib = require('../../lib');
var constants = require('./constants');

// look for either subplot or xaxis and yaxis attributes
exports.getSubplot = function getSubplot(trace) {
Expand Down Expand Up @@ -62,19 +61,16 @@ exports.getClosest = function getClosest(cd, distfn, pointData) {
return pointData;
};

// for bar charts and others with finite-size objects: you must be inside
// it to see its hover info, so distance is infinite outside.
// But make distance inside be at least 1/4 MAXDIST, and a little bigger
// for bigger bars, to prioritize scatter and smaller bars over big bars
//
// note that for closest mode, two inbox's will get added in quadrature
// args are (signed) difference from the two opposite edges
// count one edge as in, so that over continuous ranges you never get a gap
exports.inbox = function inbox(v0, v1) {
if(v0 * v1 < 0 || v0 === 0) {
return constants.MAXDIST * (0.6 - 0.3 / Math.max(3, Math.abs(v0 - v1)));
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This built-in constant return value for Fx.inbox was limiting our flexibility. inbox is now a much simpler function (so in fact we could just 🔪 but it still makes the code a little more concise I think), just returning Infinity outside the box or the passVal you give it inside the box.

}
return Infinity;
/*
* pseudo-distance function for hover effects on areas: inside the region
* distance is finite (`passVal`), outside it's Infinity.
*
* @param {number} v0: signed difference between the current position and the left edge
* @param {number} v1: signed difference between the current position and the right edge
* @param {number} passVal: the value to return on success
*/
exports.inbox = function inbox(v0, v1, passVal) {
return (v0 * v1 < 0 || v0 === 0) ? passVal : Infinity;
};

exports.quadrature = function quadrature(dx, dy) {
Expand Down
148 changes: 85 additions & 63 deletions src/components/fx/hover.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,8 +144,7 @@ exports.loneHover = function loneHover(hoverItem, opts) {
rotateLabels: false,
bgColor: opts.bgColor || Color.background,
container: container3,
outerContainer: outerContainer3,
hoverdistance: constants.MAXDIST
outerContainer: outerContainer3
};

var hoverLabel = createHoverText([pointData], fullOpts, opts.gd);
Expand All @@ -162,9 +161,10 @@ function _hover(gd, evt, subplot, noHoverEvent) {
// use those instead of finding overlayed plots
var subplots = Array.isArray(subplot) ? subplot : [subplot];

var fullLayout = gd._fullLayout,
plots = fullLayout._plots || [],
plotinfo = plots[subplot];
var fullLayout = gd._fullLayout;
var plots = fullLayout._plots || [];
var plotinfo = plots[subplot];
var hasCartesian = fullLayout._has('cartesian');

// list of all overlaid subplots to look at
if(plotinfo) {
Expand Down Expand Up @@ -351,9 +351,29 @@ function _hover(gd, evt, subplot, noHoverEvent) {
trace: trace,
xa: xaArray[subploti],
ya: yaArray[subploti],

// max distances for hover and spikes - for points that want to show but do not
// want to override other points, set distance/spikeDistance equal to max*Distance
// and it will not get filtered out but it will be guaranteed to have a greater
// distance than any point that calculated a real distance.
maxHoverDistance: hoverdistance,
maxSpikeDistance: spikedistance,

// point properties - override all of these
index: false, // point index in trace - only used by plotly.js hoverdata consumers
distance: Math.min(distance, hoverdistance), // pixel distance or pseudo-distance

// distance/pseudo-distance for spikes. This distance should always be calculated
// as if in "closest" mode, and should only be set if this point should
// generate a spike.
spikeDistance: Infinity,

// in some cases the spikes have different positioning from the hover label
// they don't need x0/x1, just one position
xSpike: undefined,
ySpike: undefined,

// where and how to display the hover label
color: Color.defaultLine, // trace color
name: trace.name,
x0: undefined,
Expand Down Expand Up @@ -418,29 +438,37 @@ function _hover(gd, evt, subplot, noHoverEvent) {
}

// in closest mode, remove any existing (farther) points
// and don't look any farther than this latest point (or points, if boxes)
// and don't look any farther than this latest point (or points, some
// traces like box & violin make multiple hover labels at once)
if(hovermode === 'closest' && hoverData.length > closedataPreviousLength) {
hoverData.splice(0, closedataPreviousLength);
distance = hoverData[0].distance;
}

// Now if there is range to look in, find the points to draw the spikelines
// Do it only if there is no hoverData
if(fullLayout._has('cartesian') && (spikedistance !== 0)) {
if(hasCartesian && (spikedistance !== 0)) {
if(hoverData.length === 0) {
pointData.distance = spikedistance;
pointData.index = false;
var closestPoints = trace._module.hoverPoints(pointData, xval, yval, 'closest', fullLayout._hoverlayer);
if(closestPoints) {
closestPoints = closestPoints.filter(function(point) {
// some hover points, like scatter fills, do not allow spikes,
// so will generate a hover point but without a valid spikeDistance
return point.spikeDistance <= spikedistance;
});
}
if(closestPoints && closestPoints.length) {
var tmpPoint;
var closestVPoints = closestPoints.filter(function(point) {
return point.xa.showspikes;
});
if(closestVPoints.length) {
var closestVPt = closestVPoints[0];
if(isNumeric(closestVPt.x0) && isNumeric(closestVPt.y0)) {
tmpPoint = fillClosestPoint(closestVPt);
if(!spikePoints.vLinePoint || (spikePoints.vLinePoint.distance > tmpPoint.distance)) {
tmpPoint = fillSpikePoint(closestVPt);
if(!spikePoints.vLinePoint || (spikePoints.vLinePoint.spikeDistance > tmpPoint.spikeDistance)) {
spikePoints.vLinePoint = tmpPoint;
}
}
Expand All @@ -452,8 +480,8 @@ function _hover(gd, evt, subplot, noHoverEvent) {
if(closestHPoints.length) {
var closestHPt = closestHPoints[0];
if(isNumeric(closestHPt.x0) && isNumeric(closestHPt.y0)) {
tmpPoint = fillClosestPoint(closestHPt);
if(!spikePoints.hLinePoint || (spikePoints.hLinePoint.distance > tmpPoint.distance)) {
tmpPoint = fillSpikePoint(closestHPt);
if(!spikePoints.hLinePoint || (spikePoints.hLinePoint.spikeDistance > tmpPoint.spikeDistance)) {
spikePoints.hLinePoint = tmpPoint;
}
}
Expand All @@ -464,47 +492,28 @@ function _hover(gd, evt, subplot, noHoverEvent) {
}

function selectClosestPoint(pointsData, spikedistance) {
if(!pointsData.length) return null;
var resultPoint;
var pointsDistances = pointsData.map(function(point, index) {
var xa = point.xa,
ya = point.ya,
xpx = xa.c2p(xval),
ypx = ya.c2p(yval),
dxy = function(point) {
var rad = point.kink,
dx = (point.x1 + point.x0) / 2 - xpx,
dy = (point.y1 + point.y0) / 2 - ypx;
return Math.max(Math.sqrt(dx * dx + dy * dy) - rad, 1 - 3 / rad);
};
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We're not replicating the distance function here, because that only works for scatter anyhow. Now we're letting each hoverPoints module tell us both a distance (based on hovermode) and a spikeDistance (always as if in closest mode, but may be calculated differently or omitted if that hover label should not make a spike line)

var distance = dxy(point);
return {distance: distance, index: index};
});
pointsDistances = pointsDistances
.filter(function(point) {
return point.distance <= spikedistance;
})
.sort(function(a, b) {
return a.distance - b.distance;
});
if(pointsDistances.length) {
resultPoint = pointsData[pointsDistances[0].index];
} else {
resultPoint = null;
var resultPoint = null;
var minDistance = Infinity;
var thisSpikeDistance;
for(var i = 0; i < pointsData.length; i++) {
thisSpikeDistance = pointsData[i].spikeDistance;
if(thisSpikeDistance < minDistance && thisSpikeDistance <= spikedistance) {
resultPoint = pointsData[i];
minDistance = thisSpikeDistance;
}
}
return resultPoint;
}

function fillClosestPoint(point) {
function fillSpikePoint(point) {
if(!point) return null;
return {
xa: point.xa,
ya: point.ya,
x0: point.x0,
x1: point.x1,
y0: point.y0,
y1: point.y1,
x: point.xSpike !== undefined ? point.xSpike : (point.x0 + point.x1) / 2,
y: point.ySpike !== undefined ? point.ySpike : (point.y0 + point.y1) / 2,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If a module wants spikes NOT to just be drawn at the midpoint of the box hover labels are attached to, it just needs to specify xSpike and/or ySpike.

Copy link
Contributor

Choose a reason for hiding this comment

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

Brilliant ✨

distance: point.distance,
spikeDistance: point.spikeDistance,
curveNumber: point.trace.index,
color: point.color,
pointNumber: point.index
Expand All @@ -525,34 +534,34 @@ function _hover(gd, evt, subplot, noHoverEvent) {
gd._spikepoints = newspikepoints;

// Now if it is not restricted by spikedistance option, set the points to draw the spikelines
if(fullLayout._has('cartesian') && (spikedistance !== 0)) {
if(hasCartesian && (spikedistance !== 0)) {
if(hoverData.length !== 0) {
var tmpHPointData = hoverData.filter(function(point) {
return point.ya.showspikes;
});
var tmpHPoint = selectClosestPoint(tmpHPointData, spikedistance);
spikePoints.hLinePoint = fillClosestPoint(tmpHPoint);
spikePoints.hLinePoint = fillSpikePoint(tmpHPoint);

var tmpVPointData = hoverData.filter(function(point) {
return point.xa.showspikes;
});
var tmpVPoint = selectClosestPoint(tmpVPointData, spikedistance);
spikePoints.vLinePoint = fillClosestPoint(tmpVPoint);
spikePoints.vLinePoint = fillSpikePoint(tmpVPoint);
}
}

// if hoverData is empty check for the spikes to draw and quit if there are none
if(hoverData.length === 0) {
var result = dragElement.unhoverRaw(gd, evt);
if(fullLayout._has('cartesian') && ((spikePoints.hLinePoint !== null) || (spikePoints.vLinePoint !== null))) {
if(hasCartesian && ((spikePoints.hLinePoint !== null) || (spikePoints.vLinePoint !== null))) {
if(spikesChanged(oldspikepoints)) {
createSpikelines(spikePoints, spikelineOpts);
}
}
return result;
}

if(fullLayout._has('cartesian')) {
if(hasCartesian) {
if(spikesChanged(oldspikepoints)) {
createSpikelines(spikePoints, spikelineOpts);
}
Expand Down Expand Up @@ -653,20 +662,31 @@ function createHoverText(hoverData, opts, gd) {
// show the common label, if any, on the axis
// never show a common label in array mode,
// even if sometimes there could be one
var showCommonLabel = c0.distance <= opts.hoverdistance &&
(hovermode === 'x' || hovermode === 'y');
var showCommonLabel = (
(t0 !== undefined) &&
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this extra t0 !== undefined check?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Related to #2379 (comment)
Previously c0.distance <= opts.hoverdistance was strangely being used as a proxy for which trace types should show the common label. I suspect that clause could be removed entirely at this point, in fact... But now that we're not using that, it's important to ensure that there is a common label to display, otherwise you can get that little empty arrow in some cases.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good to know. Thanks!

(c0.distance <= opts.hoverdistance) &&
(hovermode === 'x' || hovermode === 'y')
);

// all hover traces hoverinfo must contain the hovermode
// to have common labels
var i, traceHoverinfo;
for(i = 0; i < hoverData.length; i++) {
traceHoverinfo = hoverData[i].hoverinfo || hoverData[i].trace.hoverinfo;
var parts = Array.isArray(traceHoverinfo) ? traceHoverinfo : traceHoverinfo.split('+');
if(parts.indexOf('all') === -1 &&
parts.indexOf(hovermode) === -1) {
showCommonLabel = false;
break;
if(showCommonLabel) {
var i, traceHoverinfo;
var allHaveZ = true;
for(i = 0; i < hoverData.length; i++) {
if(allHaveZ && hoverData[i].zLabel === undefined) allHaveZ = false;

traceHoverinfo = hoverData[i].hoverinfo || hoverData[i].trace.hoverinfo;
var parts = Array.isArray(traceHoverinfo) ? traceHoverinfo : traceHoverinfo.split('+');
if(parts.indexOf('all') === -1 &&
parts.indexOf(hovermode) === -1) {
showCommonLabel = false;
break;
}
}

// xyz labels put all info in their main label, so have no need of a common label
if(allHaveZ) showCommonLabel = false;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This logic about whether to show the common label has gotten a bit more complex... previously it was being implicitly handled by c0.distance <= opts.hoverdistance, since most of the cases that shouldn't show a common label (like heatmaps) were also setting a pseudo-distance greater than the distance limit, which was always too convoluted to really understand, and we can't do anymore anyway due to the way we're calling hoverPoints separately for spikelines if no points were found in the first round for hover labels. Now it's at least more explicit what's going on, but there might be a way to clean it up even more by perhaps having the hoverPoints methods flag points that should or should not get a common label.

Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps having the hoverPoints methods flag points that should or should not get a common label.

This sounds like the most robust way forward down the road 👍

}

var commonLabel = container.selectAll('g.axistext')
Expand Down Expand Up @@ -1170,7 +1190,9 @@ function cleanPoint(d, hovermode) {
fill('fontColor', 'htc', 'hoverlabel.font.color');
fill('nameLength', 'hnl', 'hoverlabel.namelength');

d.posref = hovermode === 'y' ? (d.x0 + d.x1) / 2 : (d.y0 + d.y1) / 2;
d.posref = hovermode === 'y' ?
(d.xa._offset + (d.x0 + d.x1) / 2) :
(d.ya._offset + (d.y0 + d.y1) / 2);

// then constrain all the positions to be on the plot
d.x0 = Lib.constrain(d.x0, 0, d.xa._length);
Expand Down Expand Up @@ -1262,8 +1284,8 @@ function createSpikelines(closestPoints, opts) {
hLinePointX = evt.pointerX;
hLinePointY = evt.pointerY;
} else {
hLinePointX = xa._offset + (hLinePoint.x0 + hLinePoint.x1) / 2;
hLinePointY = ya._offset + (hLinePoint.y0 + hLinePoint.y1) / 2;
hLinePointX = xa._offset + hLinePoint.x;
hLinePointY = ya._offset + hLinePoint.y;
}
var dfltHLineColor = tinycolor.readability(hLinePoint.color, contrastColor) < 1.5 ?
Color.contrast(contrastColor) : hLinePoint.color;
Expand Down Expand Up @@ -1338,8 +1360,8 @@ function createSpikelines(closestPoints, opts) {
vLinePointX = evt.pointerX;
vLinePointY = evt.pointerY;
} else {
vLinePointX = xa._offset + (vLinePoint.x0 + vLinePoint.x1) / 2;
vLinePointY = ya._offset + (vLinePoint.y0 + vLinePoint.y1) / 2;
vLinePointX = xa._offset + vLinePoint.x;
vLinePointY = ya._offset + vLinePoint.y;
}
var dfltVLineColor = tinycolor.readability(vLinePoint.color, contrastColor) < 1.5 ?
Color.contrast(contrastColor) : vLinePoint.color;
Expand Down
11 changes: 9 additions & 2 deletions src/components/fx/layout_attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,11 @@ module.exports = {
editType: 'none',
description: [
'Sets the default distance (in pixels) to look for data',
'to add hover labels (-1 means no cutoff, 0 means no looking for data)'
'to add hover labels (-1 means no cutoff, 0 means no looking for data).',
'This is only a real distance for hovering on point-like objects,',
'like scatter points. For area-like objects (bars, scatter fills, etc)',
'hovering is on inside the area and off outside, but these objects',
'will not supersede hover on point-like objects in case of conflict.'
].join(' ')
},
spikedistance: {
Expand All @@ -57,7 +61,10 @@ module.exports = {
editType: 'none',
description: [
'Sets the default distance (in pixels) to look for data to draw',
'spikelines to (-1 means no cutoff, 0 means no looking for data).'
'spikelines to (-1 means no cutoff, 0 means no looking for data).',
'As with hoverdistance, distance does not apply to area-like objects.',
'In addition, some objects can be hovered on but will not generate',
'spikelines, such as scatter fills.'
].join(' ')
},
hoverlabel: {
Expand Down
Loading