Generalize spikelines #2379

alexcjohnson · 2018-02-17T05:15:43Z

Fixes #2336 and cleans up our spikelines framework so that each module's hoverPoints method can define its own spikelines behavior. Ensures that spikelines work correctly for all cartesian trace types, including scattergl, see #2247 (comment)

alexcjohnson · 2018-02-17T05:28:44Z

test/image/mocks/gl2d_line_select.json

-        "width": 400,
-        "height": 400
-    }
-}


cc @dfcreative - needed so tests will pass, if this PR merges before #2377 you can merge master and put this back in there.

alexcjohnson · 2018-02-17T05:31:02Z

src/components/fx/constants.js

@@ -9,9 +9,6 @@
 'use strict';

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


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.

alexcjohnson · 2018-02-17T05:34:59Z

src/components/fx/helpers.js

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


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.

alexcjohnson · 2018-02-17T05:37:48Z

src/components/fx/hover.js

-            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,


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.

Brilliant ✨

alexcjohnson · 2018-02-17T05:44:06Z

src/components/fx/hover.js

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


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.

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 👍

alexcjohnson · 2018-02-17T05:47:12Z

src/components/fx/hover.js

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


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)

alexcjohnson · 2018-02-17T05:51:31Z

test/jasmine/tests/box_test.js

@@ -266,6 +266,7 @@ describe('Test box hover:', function() {
            fig.layout.hovermode = 'x';
            return fig;
        },
+        pos: [215, 200],


I'm not exactly sure how this happened, but it seems like box points are a little bit greedier now about taking hover events. The behavior feels right to me when playing with it, so I just moved this point over a little so that it's again grabbed by the box as intended.

alexcjohnson · 2018-02-17T05:54:21Z

test/jasmine/tests/scatterpolargl_test.js

-        nums: 'r: 2.920135\nθ: 138.2489°',
-        name: 'Trial 4'
+        nums: 'r: 3.886013\nθ: 125.2822°',
+        name: 'Trial 3'


This (and below) is a weird one - as far as I can tell the new point is in fact the closest to the cursor, and the old one is a couple of pixels farther away, but I don't know what was making us pick the other one previously. I will note however that scattergl (which this inherits from) and scatter use different distance functions (mostly having to do with special handling when you're inside two markers, to prioritize the smaller marker), I didn't try to harmonize that here but we probably should at some point.

alexcjohnson · 2018-02-17T17:30:21Z

@etpinard I had a spurious test failure in a gl test - do we want to allow tests to be marked BOTH @gl and @flaky?
(edit: failure here - https://circleci.com/gh/plotly/plotly.js/6963)

etpinard · 2018-02-19T16:34:51Z

do we want to allow tests to be marked BOTH @gl and @flaky?

I don't see a problem here.

etpinard

Nice job 🔒ing down spike behavior for all cartesian trace types 🥇

Along with adding a comment in bar/hover.js, I'd like to see a few words in the spikedistance and hoverdistance attribute descriptions about how they only apply to point-like objects.

Making the hoverPoints methods have more responsibility is definitely the right call. There's a lot ways we could clean them up, but at least we're getting a better handle of all the things they should do 👌

Now, this PR almost fixes #2370, off this branch I get:

where only the labels off the middle point are wrong.

etpinard · 2018-02-19T17:16:52Z

src/components/fx/hover.js

-            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,


Brilliant ✨

etpinard · 2018-02-19T17:17:34Z

src/components/fx/hover.js

-    var showCommonLabel = c0.distance <= opts.hoverdistance &&
-                          (hovermode === 'x' || hovermode === 'y');
+    var showCommonLabel = (
+        (t0 !== undefined) &&


Why this extra t0 !== undefined check?

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.

Good to know. Thanks!

etpinard · 2018-02-19T17:25:56Z

src/traces/violin/hover.js

@@ -63,6 +63,14 @@ module.exports = function hoverPoints(pointData, xval, yval, hovermode, hoverLay
                kdePointData[pLetter + '1'] = pOnPath[1];
                kdePointData[vLetter + '0'] = kdePointData[vLetter + '1'] = vValPx;
                kdePointData[vLetter + 'Label'] = vLetter + ': ' + Axes.hoverLabelText(vAxis, vVal) + ', ' + cd[0].t.labels.kde + ' ' + kdeVal.toFixed(3);
+
+                // move the spike to the KDE point


Good call

😍

etpinard · 2018-02-19T17:27:36Z

test/jasmine/tests/hover_label_test.js

@@ -1309,21 +1309,20 @@ describe('hover on fill', function() {

    function assertLabelsCorrect(mousePos, labelPos, labelText) {
        return new Promise(function(resolve) {
+            Lib.clearThrottle();


Nice touch!

etpinard · 2018-02-19T17:30:10Z

test/jasmine/tests/hover_spikeline_test.js

+
+    it('positions spikes at the data value on grouped bars', function(done) {
+        function _assertBarSpikes() {
+            // regardless of hovermode, you must be actually over the bar to see its spikes


thanks for comment here 📚

etpinard · 2018-02-19T17:53:59Z

src/components/fx/hover.js

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


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 👍

etpinard · 2018-02-19T17:55:45Z

src/traces/bar/hover.js

@@ -49,15 +51,24 @@ module.exports = function hoverPoints(pointData, xval, yval, hovermode) {
            return Math.max(thisBarMaxPos(di), di.p + t.bardelta / 2);
        };

+    function _positionFn(_minPos, _maxPos) {
+        return Fx.inbox(_minPos - posVal, _maxPos - posVal,
+            maxHoverDistance - Math.min(1, Math.abs(_maxPos - _minPos) / pRangeCalc));


Would you mind adding a one-line comment description what this passVal corresponds to?

Ah, good call - it was actually wrong -> ad38f8d

alexcjohnson · 2018-02-19T21:36:31Z

Now, this PR almost fixes #2370

And now it really does -> eb98c91

alexcjohnson · 2018-02-19T21:37:39Z

I'd like to see a few words in the spikedistance and hoverdistance attribute descriptions about how they only apply to point-like objects.

Good point. How's 31e4e34?

etpinard · 2018-02-19T21:47:11Z

Nicely done. Thanks very much 💃

alexcjohnson added 4 commits February 16, 2018 23:45

generalize spikelines

ed19b7c

simplify hover_label_test with more clearThrottle

e2c8c04

lint - remove no longer needed imports

965bb88

remove gl2d_line_select mock that got pushed to master accidentally

d34ba81

alexcjohnson added type: bug status: has TODOs labels Feb 17, 2018

add test for no spikelines on scatter fills

5164cdf

alexcjohnson commented Feb 17, 2018

View reviewed changes

one more flaky test suite

a2fc07a

alexcjohnson added status: reviewable and removed status: has TODOs labels Feb 17, 2018

alexcjohnson mentioned this pull request Feb 17, 2018

Fix color clustering #2377

Merged

1 task

jackparmer requested review from mdtusz and removed request for mdtusz February 19, 2018 04:04

etpinard reviewed Feb 19, 2018

View reviewed changes

alexcjohnson added 3 commits February 19, 2018 14:53

fix and comment on bar positionFn passVal

ad38f8d

fix #2370

eb98c91

more comments on (hover|spike)distance and point vs area objects

31e4e34

alexcjohnson merged commit ce5bc97 into master Feb 19, 2018

alexcjohnson deleted the generalize-spikelines branch February 19, 2018 22:24

This was referenced Feb 23, 2018

Add spikelines #2247

Merged

scattergl doesn't work with 'showspikes'; whereas scatter works as expected! #2409

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generalize spikelines #2379

Generalize spikelines #2379

alexcjohnson commented Feb 17, 2018

alexcjohnson Feb 17, 2018

alexcjohnson Feb 17, 2018

alexcjohnson Feb 17, 2018

alexcjohnson Feb 17, 2018

etpinard Feb 19, 2018

alexcjohnson Feb 17, 2018

etpinard Feb 19, 2018

alexcjohnson Feb 17, 2018

alexcjohnson Feb 17, 2018

alexcjohnson Feb 17, 2018

alexcjohnson commented Feb 17, 2018 •

edited

Loading

etpinard commented Feb 19, 2018

etpinard left a comment

etpinard Feb 19, 2018

etpinard Feb 19, 2018

alexcjohnson Feb 19, 2018

etpinard Feb 19, 2018

etpinard Feb 19, 2018

etpinard Feb 19, 2018

etpinard Feb 19, 2018

etpinard Feb 19, 2018

etpinard Feb 19, 2018

alexcjohnson Feb 19, 2018

alexcjohnson commented Feb 19, 2018

alexcjohnson commented Feb 19, 2018

etpinard commented Feb 19, 2018

Generalize spikelines #2379

Generalize spikelines #2379

Conversation

alexcjohnson commented Feb 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcjohnson commented Feb 17, 2018 • edited Loading

etpinard commented Feb 19, 2018

etpinard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcjohnson commented Feb 19, 2018

alexcjohnson commented Feb 19, 2018

etpinard commented Feb 19, 2018

alexcjohnson commented Feb 17, 2018 •

edited

Loading