Skip to content

Adding traces on hover can cause infinite loops #1467

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.

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

Closed
cpsievert opened this issue Mar 11, 2017 · 12 comments
Closed

Adding traces on hover can cause infinite loops #1467

cpsievert opened this issue Mar 11, 2017 · 12 comments
Labels
bug something broken

Comments

@cpsievert
Copy link

MRE of the problem (hover on any of the markers) -- http://codepen.io/cpsievert/pen/dvWydK

FYI this was working pre-1.24.0 -- http://codepen.io/cpsievert/pen/qrmBVx

@rreusser
Copy link
Contributor

rreusser commented Mar 11, 2017

I think either 1.24.1 or 1.24.2 might already fix this. See: #1446

@etpinard etpinard added this to the v1.25.0 milestone Mar 13, 2017
@etpinard
Copy link
Contributor

etpinard commented Mar 13, 2017

Thanks for posting @cpsievert !

#1446 fixed a plotly_unhover -> Plotly.Fx.hover -> plotly_unhover infinite loop. But looks like addTraces suffers from another #1304 side-effect.

We should fix this before releasing 1.25.0.

@etpinard etpinard self-assigned this Mar 13, 2017
@etpinard etpinard added the bug something broken label Mar 13, 2017
@etpinard
Copy link
Contributor

Ok. This problem is pretty tricky and is quite frankly (unless @cpsievert can prove otherwise) an edge case.

The infinite loop is triggered only when addTraces adds a trace containing similar enough coordinates to the one given in the plotly_hover event data. For example take http://codepen.io/etpinard/pen/xqrjza which adds traces outside the hover distance and does not trigger an infinite loop.

In @cpsievert 's http://codepen.io/cpsievert/pen/dvWydK, an infinite loop is triggered since #1304 because:

  • addTraces now leads to a Plots.rehover call (via Plotly.redraw),
  • Plots.rehover leads to a Fx.hover cal which then leads to
  • Fx.hover emiiting plotly_hover even though the mouse position hasn't changed (here hoverChanged(gd, evt, oldhoverdaa) is true as the curveNumber changed after the addTraces call)
  • and then the plotly_hover handler triggers addTraces

Now, how to fix this problem?

In my opinion, I think the most robust solution would be to make rehover not trigger gd.emit('plotly_hover') while still updating gd._hoverdata and the visible hover labels.

@rreusser @alexcjohnson event data updates wasn't 🔒 ed down in the #1304 tests, so I guess this would be ok?

@alexcjohnson
Copy link
Collaborator

Honestly I'm not sure if I would consider this a bug - yes it's an infinite loop, but you'd get an explosion of traces in a case like this anyway, at least for real data sets where you cause hundreds of hover events every time the mouse crosses the plot. And I do think we want rehover-hovers to emit an event so the more typical case of updating something outside the plot (or on a different subplot at least) will still happen.

Is there a more realistic use case for this? There are always going to be ways to trigger infinite loops when you modify a plot from inside an event handler, and I think at some level it's up to the user to avoid these, otherwise we'll be restricting what they can do.

@cpsievert
Copy link
Author

cpsievert commented Mar 13, 2017

Is there a more realistic use case for this?

I'm using it to implement all of this -- https://cpsievert.github.io/plotly_book/linking-views-without-shiny.html

you'd get an explosion of traces in a case like this anyway

I do a check to make sure I don't add the same trace twice. The example I linked to was a simplified version to help isolate the issue.

@rreusser
Copy link
Contributor

rreusser commented Mar 13, 2017

@cpsievert just to clarify: the objective of #1304 is to update the hover state when the plot changes, e.g. streaming or animation and maybe a few other cases @alexcjohnson mentioned that were considered long-running issues where the hover state reflected outdated data until new mouse input triggered a new hover.

I think plotly_hoverupdate on rehover wouldn't be bad if it's not too bad to implement and if it's enough to disambiguate hover state changing as the result of input vs. reflecting new data.

@alexcjohnson
Copy link
Collaborator

I do a check to make sure I don't add the same trace twice. The example I linked to was a simplified version to help isolate the issue.

After that check do you still have a problem (and if so can you post it?), or were you just hoping not to need the check?

I wouldn't want to add a new event type for rehover, because then people would have to know that in the normal case they need to listen to both types. But we could certainly add something to the event to indicate whether it was triggered by a mouse event or something else like a rehover. Like... the mouse event itself? That could come in handy for other purposes too.

@cpsievert
Copy link
Author

After that check do you still have a problem (and if so can you post it?), or were you just hoping not to need the check?

Ooops -- I thought I had implemented that check, but it appears I haven't yet. Will work on it and report back if I still have issues, thanks!

@etpinard
Copy link
Contributor

Looks like this issue is up in the air.

Dropping from 1.25.0 for now. Adding discussion needed.

@etpinard etpinard removed this from the v1.25.0 milestone Mar 13, 2017
@etpinard etpinard removed their assignment Mar 13, 2017
@etpinard etpinard changed the title Adding traces on hover causes infinite loop Adding traces on hover can cause infinite loops Mar 13, 2017
@alexcjohnson
Copy link
Collaborator

This seems to have broken the last codepen on the hover event docs page https://plot.ly/javascript/hover-events/

https://codepen.io/plotly/pen/eJOyej

FYI @cldougl

@jackparmer
Copy link
Contributor

This issue has been tagged with NEEDS SPON$OR

A community PR for this feature would certainly be welcome, but our experience is deeper features like this are difficult to complete without the Plotly maintainers leading the effort.

Sponsorship range: $5k-$10k

What Sponsorship includes:

  • Completion of this feature to the Sponsor's satisfaction, in a manner coherent with the rest of the Plotly.js library and API
  • Tests for this feature
  • Long-term support (continued support of this feature in the latest version of Plotly.js)
  • Documentation at plotly.com/javascript
  • Possibility of integrating this feature with Plotly Graphing Libraries (Python, R, F#, Julia, MATLAB, etc)
  • Possibility of integrating this feature with Dash
  • Feature announcement on community.plotly.com with shout out to Sponsor (or can remain anonymous)
  • Gratification of advancing the world's most downloaded, interactive scientific graphing libraries (>50M downloads across supported languages)

Please include the link to this issue when contacting us to discuss.

@gvwilson
Copy link
Contributor

Hi - this issue has been sitting for a while, so as part of our effort to tidy up our public repositories I'm going to close it. If it's still a concern, we'd be grateful if you could open a new issue (with a short reproducible example if appropriate) so that we can add it to our stack. Cheers - @gvwilson

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug something broken
Projects
None yet
Development

No branches or pull requests

6 participants