fix for spikedistance set to infinity

[antoinerg]

Fixes#4626 and #3805

Commit 9d1325c adds a test which fail.

For #4626:
Before: https://codepen.io/antoinerg/pen/BaNYJve
After: https://codepen.io/antoinerg/pen/oNXEENy

For #3805:
Before: https://codepen.io/anon/pen/eoPXJE
After: https://codepen.io/antoinerg/pen/ExjQQYw

cc @archmoj@alexcjohnson@etpinard

[antoinerg]

The problem stems from thisSpikeDistance being Infinity and minDistance also being Infinity. Without an equal sign, this will never be true and no spikelines will appear hence the bug.

[alexcjohnson]

Are there any cases this changes which point we get spikes to? It might still be OK in that case, I just want to know what those cases are. The loop indicates this way if we have multiple items in pointData all with the same spikeDistance, previously we'd get the first one, but with the new logic we'll get the last one.

[antoinerg]

Are there any cases this changes which point we get spikes to?

@alexcjohnson I'm not sure how to proceed to find out.

[antoinerg]

Should we check whether minDistance is Infinity and handle that case separately?

[archmoj]

That's a much better solution, IMO.

[archmoj]

if((minDistance===Infinity||thisSpikeDistance<minDistance)&&thisSpikeDistance<=spikedistance){

looks like the right logic to apply.

[antoinerg]

Ok so I went with this safer approach. If spikedistance is Infinity we visit that if block. It should preserve any existing behavior but fix the edge case for Infinity.

See 583ad64

[alexcjohnson]

The key is to first identify the relevant test cases. I suspect the relevant ones are (with hovermode: 'x'):

• Two scatter traces with exactly identical points, so there aren't closer and farther ones. Maybe we don't care which one gets the spike line... though offhand I'd suggest the last one, since it'll be drawn on top so you'll see a spike line color matching the visible point.
• Stacked bar traces with the cursor at a larger (or smaller) y value than any of the bars. Ideally I'd think it would be nice to draw the spike line to the one that's closest; but that would require getting away from using Infinity for all points outside the bar, at least for sizeFn. Perhaps we give Fx.inbox another (optional) argument failVal that, in this case, will be set very large, and increasing the farther away you get from the bar either above or below, but not infinite. Or, at that point it seems silly to use Fx.inbox at all, just inline this logic.

[antoinerg]

Thanks for the clarification @alexcjohnson . I now better understand the purpose of this code. I will take another approach.

[archmoj]

@antoinerg Thanks very much for the PR.
Looks good to my eyes.
I'll let a second reviewer to give you dancers.

[archmoj]

Nicely done.
💃

[archmoj]

Please hold on. There is a new comment: #4627 (comment).

[antoinerg]

I made a mess earlier and cleaned up the git history with a git rebase. I left commit ae7db42 with the failing test to fix and made a new solution in ebf75e6 .

P.S. I updated the Codepen above to run the latest build.

[antoinerg]

I think my first solution has to go back in otherwise we have too many non-scatter traces for which no spike lines appear for spikedistance: -1.

@alexcjohnson can you have a look at branch fix-spikedistance. It contains new tests which make sure that:

• all cartesian traces produce spike lines when spikedistance: -1 (first commit)
• test that spikeline is drawn up to the last point as you described
• test that spikeline is drawn up to the closest bar

If it's satisfactory, I can open a new PR or simply force push here. Let me know what you prefer.

[antoinerg]

@archmoj you might also want to review my new branch described in #4627 (comment)

[alexcjohnson]

@alexcjohnson can you have a look at branch fix-spikedistance. It contains new tests...

Looks great, my only question about it is the test "correctly select the closest bar even when setting spikedistance to -1" - you do _hover({xpx: 600, ypx: 400}); and _hover({xpx: 580, ypx: 200});, which look like they're within the bounds of the blue and green bars at E? That's important (though it would be clearer with exactly the same xpx), but I would have expected to also see ypx corresponding to positions not on any bars, but above and below the entire stack - which would probably require increasing the range to create space above and below. I'm not sure if that works right now but I think the goal of #3805 would be that if you get a hover point from a bar, and you set spikedistance=-1, you should get a spike line to the closest one.

[archmoj]

If it's satisfactory, I can open a new PR or simply force push here. Let me know what you prefer.

Force-pushing on PRs is discouraged on the plotly.js repo.
Please simply add the commit to this PR or open a new PR.
Thank you!

[antoinerg]

my only question about it is the test "correctly select the closest bar even when setting spikedistance to -1" - you do _hover({xpx: 600, ypx: 400}); and _hover({xpx: 580, ypx: 200});, which look like they're within the bounds of the blue and green bars at E?

I'm not completely sure: maybe it's above or below. I tested it manually on mock bar_stack-with-gaps to make sure we get the closest one.

That's important (though it would be clearer with exactly the same xpx)

Yes they should have the same xpx, sorry for making things confusing. I will fix that.

but I would have expected to also see ypx corresponding to positions not on any bars, but above and below the entire stack - which would probably require increasing the range to create space above and below. I'm not sure if that works right now

Right now, for bar traces, spike lines will appear only if you hover inside the bar itself for any value spikedistance > 0 (this is also the case on master). My PR fix-spikedistance only fixes the case spikedistance = -1 which is key for unified hover label. I'm not sure if I have time to tackle the other case (ie spikedistance > 0 ) at the moment.

if you get a hover point from a bar, and you set spikedistance=-1, you should get a spike line to the closest one.

I think this case is covered: https://codepen.io/antoinerg/pen/vYORyLV

cc @alexcjohnson

[alexcjohnson]

You're so close! Right now you get a spike to the last bar when you're off the stack, not the closest one (in the gif below see behavior when the mouse is below, spike goes to the green bar). fixing that would require tweaking sizefn to include some large-but-not-infinite value when off the bar, that grows larger as you get farther away.

This is not a super important case, so if you want to move on that's fine, but if you do please make a new issue for this.

[antoinerg]

You're so close! Right now you get a spike to the last bar when you're off the stack, not the closest one (in the gif below see behavior when the mouse is below, spike goes to the green bar). fixing that would require tweaking sizefn to include some large-but-not-infinite value when off the bar, that grows larger as you get farther away.

This is not a super important case, so if you want to move on that's fine, but if you do please make a new issue for this.

I missed that behavior! Let me see if I can get it right :) Thanks for the review @alexcjohnson 🕵️‍♂️

EDIT: I am not sure I can at the moment. I don't understand the current calculation of spikedistance and what it is supposed to represent. The equations make no sense to me. I'll open an issue and revisit later if time permits.

[antoinerg]

Please simply add the commit to this PR or open a new PR.

I'll open a new PR. Closing in favor of #4637