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

fix for Scattergl animation bug #6452

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 13 additions & 14 deletions src/traces/scattergl/plot.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ var exports = module.exports = function plot(gd, subplot, cdata) {
return;
}

var count = scene.count;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be better to just var count = cdata.length in that case - and remove all the other changes?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! @eiriklv Could you please fetch upstream/master on your fork and test it by opening a PR?

var regl = fullLayout._glcanvas.data()[0].regl;

// that is needed for fills
Expand All @@ -74,28 +73,28 @@ var exports = module.exports = function plot(gd, subplot, cdata) {
scene.fill2d = createLine(regl);
}
if(scene.glText === true) {
scene.glText = new Array(count);
for(i = 0; i < count; i++) {
scene.glText = new Array(cdata.length);
for(i = 0; i < cdata.length; i++) {
scene.glText[i] = new Text(regl);
}
}

// update main marker options
if(scene.glText) {
if(count > scene.glText.length) {
if(cdata.length > scene.glText.length) {
// add gl text marker
var textsToAdd = count - scene.glText.length;
var textsToAdd = cdata.length - scene.glText.length;
for(i = 0; i < textsToAdd; i++) {
scene.glText.push(new Text(regl));
}
} else if(count < scene.glText.length) {
} else if(cdata.length < scene.glText.length) {
// remove gl text marker
var textsToRemove = scene.glText.length - count;
var removedTexts = scene.glText.splice(count, textsToRemove);
var textsToRemove = scene.glText.length - cdata.length;
var removedTexts = scene.glText.splice(cdata.length, textsToRemove);
removedTexts.forEach(function(text) { text.destroy(); });
}

for(i = 0; i < count; i++) {
for(i = 0; i < cdata.length; i++) {
scene.glText[i].update(scene.textOptions[i]);
}
}
Expand Down Expand Up @@ -128,7 +127,7 @@ var exports = module.exports = function plot(gd, subplot, cdata) {
}

// fill requires linked traces, so we generate it's positions here
scene.fillOrder = Lib.repeat(null, count);
scene.fillOrder = Lib.repeat(null, cdata.length);
if(scene.fill2d) {
scene.fillOptions = scene.fillOptions.map(function(fillOptions, i) {
var cdscatter = cdata[i];
Expand Down Expand Up @@ -255,7 +254,7 @@ var exports = module.exports = function plot(gd, subplot, cdata) {
var isSelectMode = selectMode(dragmode);
var clickSelectEnabled = fullLayout.clickmode.indexOf('select') > -1;

for(i = 0; i < count; i++) {
for(i = 0; i < cdata.length; i++) {
var cd0 = cdata[i][0];
var trace = cd0.trace;
var stash = cd0.t;
Expand Down Expand Up @@ -306,8 +305,8 @@ var exports = module.exports = function plot(gd, subplot, cdata) {

// use unselected styles on 'context' canvas
if(scene.scatter2d) {
var unselOpts = new Array(count);
for(i = 0; i < count; i++) {
var unselOpts = new Array(cdata.length);
for(i = 0; i < cdata.length; i++) {
unselOpts[i] = scene.selectBatch[i].length || scene.unselectBatch[i].length ?
scene.markerUnselectedOptions[i] :
{};
Expand Down Expand Up @@ -348,7 +347,7 @@ var exports = module.exports = function plot(gd, subplot, cdata) {
(yaxis._rl || yaxis.range)[1]
]
};
var vpRange = Lib.repeat(vpRange0, scene.count);
var vpRange = Lib.repeat(vpRange0, cdata.length);
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about this one

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this change required?


// upload viewport/range data to GPU
if(scene.fill2d) {
Expand Down