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.

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

Stop performing a full redraw on every update #6

Open
otisg opened this issue Jun 21, 2016 · 15 comments
Open

Stop performing a full redraw on every update #6

otisg opened this issue Jun 21, 2016 · 15 comments

Comments

@otisg
Copy link

otisg commented Jun 21, 2016

I saw "Performs a full redraw on every update, I intend to make this more performant soon." listed as a problem in the README, so I thought I'd add it here to make it easier to track.

Not sure if you're aware of plotly/plotly.js#204, but it looks like you project is the primary hope React+Plotly :)

@benjeffery
Copy link
Owner

Thanks. I'll get round to this when other priorities are out of the way, but currently that is looking like months. Pull requests welcome!

@rreusser
Copy link

rreusser commented Sep 13, 2016

FWIW, I've been working for quite a while now on plotly animations (just released!), and avoiding a full redraw was one of the major obstacles since it's obviously impractical to fully redraw the plot every time a point moves a tiny bit.

Long story short, Plotly is designed from the ground up to redraw so although it's not hopeless, there's only so much that can be done without handling the details manually. My solution for animations was to redraw at the tail end of any tweening. This ensures anything that couldn't be nicely tweened is at least just redrawn (so that if everything can be tweened, the redraw is invisible).

The full redraw approach is good for consistency (and not that slow in most cases), except when you're trying to do something like a simulation where the data changes quickly and a full redraw slows things dramatically and unnecessarily. I'm working on the documentation for animations. Here is an example that explicitly passes {redraw: false} to the animation command so that the data is simply tweened (a.k.a. eased; it's d3) without a redraw.

At this point, I'm making a short story long. So basically, here's a thought on how this might be accomplished: Perhaps it could make optional use of the Plotly.animate feature (which would be nice anyway! Smooth updates when changes occur (currently just for scatter traces)!) and allow passing redraw: true/false in order to optimize the case where a full redraw is unnecessary. I'm not 100% sure whether you'd want to perform a diff and pass the result to Plotly.update or whether it would perform just a well to pass the whole state and let Plotly.update sort it out.

All of this requires more manual massaging than I'd like, but to my knowledge, it's maybe the best that can be done.

Sorry that stops short of a PR, but it seems relevant and it's the best I have for now!

@PaulMest
Copy link

If anybody is looking to work on this issue, here are some high-level guidelines from Plotly on how to work with React.
http://academy.plot.ly/react/3-with-plotly/

@benjeffery
Copy link
Owner

@PaulMest Unless I'm missing something in that tutorial it advises calling newPlot on every state change, which is the current behaviour of this library. Ideally we would do something like @rreusser suggests, where we detect if the state change can be easily tweened, and only resort to newPlot when it is not.

@PaulMest
Copy link

Oh, I see.

When I looked in the repo, I saw shouldComponentUpdate show a TODO to look for changes to the props... but those are actually implemented in componentDidUpdate so I had assumed even some basic logic to prevent redraws wasn't implemented... but it just appears to be in a different place.

Is there any reason why
if (prevProps.data !== this.props.data || prevProps.layout !== this.props.layout) { is in componentDidUpdate and not in shouldComponentUpdate?

@benjeffery
Copy link
Owner

I put the check in componentDidUpdate as you still need a render without a newPlot if ...other props (for style etc.) have changed. A check could be added to shouldComponentUpdate but it will have very little performance benefit as the component only renders a single div.

@benjeffery
Copy link
Owner

@rreusser Sadly I'm getting a 403 for your URL http://plotly.rickyreusser.com/animation.html But I've found https://github.com/rreusser/animation-experiments so I'll check those out.

@rreusser
Copy link

rreusser commented Jun 2, 2017

@benjeffery Is there anything I can help with? To be honest, I've tried to be a bit more careful about what I put out into the open web just a bit since it's better if there's not a lot of unofficial documentation and misleading development examples floating around. I'm glad to help solve issues or bette understand the pain points to help improve the documentation though!

@jackparmer
Copy link

@benjeffery there's also this documentation:
https://plot.ly/javascript/animations/

@TinyTheBrontosaurus
Copy link

Question: is this as simple as having componentDidUpdate call relayout or update instead of newPlot? Maybe i'm oversimplifying this issue. The specific problem i'm having is that if i'm zoomed in, then add a trace, the trace is added but the zoom is lost. If i'm not missing anything, then I'm probably going to make this change.

@TinyTheBrontosaurus
Copy link

Bah. Nevermind that comment was based upon a faulty test. My update() call was doing nothing. Once I started implementing it got more complicated (as you're aware) :-/

@sharq1
Copy link

sharq1 commented Jan 26, 2018

I couldn't get update to work as expected so I went with animate - which unfortunately works only for scatter type charts (see plotly/plotly.js#1019 ). But it's better than nothing :) Here's my implementation:

  componentDidUpdate(prevProps) {
    const { data, layout, config } = this.props;

    if (!_.isEqual(prevProps.layout, layout) || !_.isEqual(prevProps.config, config) || data.some((d) => d.type !== 'scatter')) {
      plotlyInstance.newPlot(this.container, _.cloneDeep(data), _.cloneDeep(layout), config);
      this.attachListeners();
    } else if (!_.isEqual(prevProps.data, data)) {
      plotlyInstance.animate(this.container, { data: _.cloneDeep(data) }, { transition: { duration: 500, easing: 'cubic-in-out' } });
    }
  }

By the way notice that I'm performing cloneDeep on data array - not sure why it wasn't implemented this way, as plotly mutates data as well (it adds uid to each array element) which was messing up with my Redux store.

@jackparmer
Copy link

jackparmer commented Mar 2, 2018

This issue has been solved by using the new Plotly.react method in the official plotly.js react component. Please see plotly/react-plotly.js#2 (comment)

@AyoCodess
Copy link

is there a solution for this?. As the plot automatically re-draws when any the data object changes. i.e an x-axis data point.

@benjeffery
Copy link
Owner

is there a solution for this?. As the plot automatically re-draws when any the data object changes. i.e an x-axis data point.

Not without significant work. I'm no longer working in webdev, so won't be getting to this, happy to check and merge any contributions though.

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

No branches or pull requests

8 participants