-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Plotly.react and scattergl #3405
Comments
Could be related to #2334 also. Thanks very much for the report! |
Is there any additional changes I need to make to change Plotly.react to Plotly.newPlot in https://github.com/plotly/react-plotly.js/blob/master/src/factory.js as a workaround for this bug? Thanks. |
I didn't want to take the performance hit of newPlot vs react, so I wrote a test a choose which one to call based on the data sizes. if (this.plotlyBug(data)) {
Plotly.newPlot(this.node, data, layout, options)
} else {
Plotly.react(this.node, data, layout, options)
} With this test function to determine which to use: /*
* Addresses Plotly Issue:
* - https://github.com/plotly/plotly.js/issues/3405
*/
ColorScatter.prototype.plotlyBug = function(newData) {
var oldData = this.node.data
var oldSizes = _.map(oldData, trace => trace.x.length)
var newSizes = _.map(newData, trace => trace.x.length)
var plotBug = false
for(var i = 0; i < oldSizes.length; i++) {
if ((oldSizes[i] > 10000) && (newSizes[i] <= 10000)) {
plotBug = true
}
}
return plotBug
} Seems to have worked so far - haven't noticed the behavior since adding this. And most of the time its not triggered (and so Plotly.react) is used. |
Due to PR #3578 this issue seems to now revolve around 100k points instead of 10k points. For example, use my codepen above. Run it once with N=100001, M=100001. The points will saturate the area. Then run it again with N=100001, M=90001 and there will be many many less points (much more than just a 10% reduction). |
…tory.js, except checks for plotly bug plotly/plotly.js#3405
Seems better? Though it's hard to tell! My original code-pen seems to not have the issue now either which makes me wonder if something changed in the browser too? |
@deto I noticed missing points on your example at https://codepen.io/anon/pen/JwvEgE. |
Actually, yeah, it's just hard to tell with the 100k-point threshold. If you zoom into a smaller region (e.g., a 0.2 x 0.2 square) then the difference is apparent in the older code-pen example. However, in the new example posted by @archmoj it appears to be fixed! Probably a better demo to compare would be something that puts points on a regular grid so that it would be easy to see if points are missing. |
Great! Yes, with this it's very apparent. If you run it with: N=100,010 For example, it looks good in the updated code. In the older version (with plotly 1.43.1) it looks fine initially, but if you zoom in you can see whole segments of the spiral that get dropped out. Nice work! Hope your changes get merged in soon. |
Closed by #4323 . |
I've been trying to update a plot containing scattergl traces dynamically with Plotly.react.
However, I've found that in some cases, some of the points are missing and I get a "glDrawElements: attempt to access out of range vertices in attribute 0" error in the console.
I've isolated a minimal example of this at the following codepen:
-- https://codepen.io/anon/pen/JwvEgE
What this codepen is doing:
a) Create a plot with a single scattergl trace with N points (with Plotly.newPlot)
b) Update this plot to instead contain a scattergl trace with M points (with Plotly.react)
In the final plot, the trace has many points not rendering for me (on up-to-date Chrome 71.0.3578.98).
This issue is unusual in that it seems to depend on the precise number of points in each trace. I've done some experiments, and it seems like it only occurs when (N > 10,000) and (M <= 10,000), which makes me think that perhaps arrays are being handled differently based on their size and something incompatible is resulting.
I know a different Plotly.react + scattergl issue was reported before, but that one seemed to be a bit different.
The text was updated successfully, but these errors were encountered: