fix #2452 - removing and adding scatter(gl) as not the first module #2455
Conversation
| if(oldModules[i].name === 'scatter') { | ||
| hadScatter = true; | ||
| } | ||
| break; |
There was a problem hiding this comment.
The main problem was this break being outside the if. But then since we had the same loop twice and it's unlikely in general for both scatter and scattergl to be present, I combined them into one and dropped the break altogether.
| @@ -610,7 +610,9 @@ function sceneUpdate(gd, subplot) { | |||
| scene.selectBatch = null; | |||
| scene.unselectBatch = null; | |||
|
|
|||
| delete subplot._scene; | |||
| // we can't just delete _scene, because `destroy` is called in the | |||
| // middle of supplyDefaults, before relinkPrivateKeys which will put it back. | |||
There was a problem hiding this comment.
cleanPlot
If you ever run into a situation like this "but I deleted that, how come it's still here???" try setting it to null instead.
There was a problem hiding this comment.
So after fixing the break issue so traces could get removed, scattergl had a new bug that it wouldn't come back after being deleted - because the scene had been destroyed but we were trying to draw traces into it again. Hence my tests below that remove and then re-add the traces. There were related issues, like if you hid the last scattergl trace and then tried to add a totally new one it wouldn't show up either, but it's all the same issue so I just tested original case.
There was a problem hiding this comment.
Nice. I vaguely remember reading that delete was slow. Perhaps setting subplot._scene = null will be a perf boost too 🐎
|
This also fixes #2309 🎉 |
| } | ||
| moduleName = newModules[i].name; | ||
| if(moduleName === 'scatter') hasScatter = true; | ||
| else if(moduleName === 'scattergl') hasGl = true; |
There was a problem hiding this comment.
Great. We should starting thinking about generalizing this pattern.
In short, this loop and its has?? flags exist because scatter and scattergl do not clear their content on redraws unlike other trace types. A goal of ours should be to update all cartesian trace module plot steps to something more d3-iomatic for (1) work with animations (2) what could be nice performance gain on redraws.
|
Nicely done 💃 🎉 |
We have special logic for when scatter and scattergl modules are removed from the plot, and it was broken.
cc @etpinard