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.

Sign up for GitHub

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

Jump to bottom

scattergl visible restyle fix #2442

Merged
merged 6 commits into from
Mar 6, 2018
Merged

scattergl visible restyle fix #2442

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5c6576a
fix #2441 - don't attempt to draw empty option items
etpinard Mar 5, 2018
187ac23
fixup - no need to override opacity editType twice
etpinard Mar 5, 2018
91cac2e
DRY up scattergl
etpinard Mar 5, 2018
687a1f5
Merge branch 'master' into scattergl-visible-fix
etpinard Mar 6, 2018
a98d1d5
bring back scene.update + :lock: down its used during drag
etpinard Mar 6, 2018
4776832
no need to extendFlat attrs that 'only' get overrode by overrideAll
etpinard Mar 6, 2018
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
4 changes: 1 addition & 3 deletions src/traces/scattergl/attributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,9 +83,7 @@ var attrs = module.exports = overrideAll({
marker: scatterAttrs.unselected.marker
},

opacity: extendFlat({}, plotAttrs.opacity, {
editType: 'calc'
}),
opacity: extendFlat({}, plotAttrs.opacity)
Copy link
Collaborator

Choose a reason for hiding this comment

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

you don't even need the extendFlat, right? overrideAll will take care of that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in 4776832


}, 'calc', 'nested');

Expand Down
183 changes: 76 additions & 107 deletions src/traces/scattergl/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,13 +141,9 @@ function sceneOptions(gd, subplot, trace, positions) {
var fullLayout = gd._fullLayout;
var count = positions.length / 2;
var markerOpts = trace.marker;
var xaxis = AxisIDs.getFromId(gd, trace.xaxis);
var yaxis = AxisIDs.getFromId(gd, trace.yaxis);
var ptrX = 0;
var ptrY = 0;
var i;

var hasLines, hasErrorX, hasErrorY, hasError, hasMarkers, hasFill;
var hasLines, hasErrorX, hasErrorY, hasMarkers, hasFill;

if(trace.visible !== true) {
hasLines = false;
Expand All @@ -159,7 +155,6 @@ function sceneOptions(gd, subplot, trace, positions) {
hasLines = subTypes.hasLines(trace) && positions.length > 1;
hasErrorX = trace.error_x && trace.error_x.visible === true;
hasErrorY = trace.error_y && trace.error_y.visible === true;
hasError = hasErrorX || hasErrorY;
hasMarkers = subTypes.hasMarkers(trace);
hasFill = !!trace.fill && trace.fill !== 'none';
}
Expand All @@ -169,67 +164,16 @@ function sceneOptions(gd, subplot, trace, positions) {
var selectedOptions, unselectedOptions;
var linePositions;

// get error values
var errorVals = hasError ?
Registry.getComponentMethod('errorbars', 'calcFromTrace')(trace, fullLayout) :
null;

if(hasErrorX) {
errorXOptions = {};
errorXOptions.positions = positions;
var errorsX = new Float64Array(4 * count);

if(xaxis.type === 'log') {
for(i = 0; i < count; ++i) {
errorsX[ptrX++] = positions[i * 2] - xaxis.d2l(errorVals[i].xs) || 0;
errorsX[ptrX++] = xaxis.d2l(errorVals[i].xh) - positions[i * 2] || 0;
errorsX[ptrX++] = 0;
errorsX[ptrX++] = 0;
}
} else {
for(i = 0; i < count; ++i) {
errorsX[ptrX++] = positions[i * 2] - errorVals[i].xs || 0;
errorsX[ptrX++] = errorVals[i].xh - positions[i * 2] || 0;
errorsX[ptrX++] = 0;
errorsX[ptrX++] = 0;
}
}
if(hasErrorX || hasErrorY) {
var calcFromTrace = Registry.getComponentMethod('errorbars', 'calcFromTrace');
var errorVals = calcFromTrace(trace, fullLayout);

if(trace.error_x.copy_ystyle) {
trace.error_x = trace.error_y;
if(hasErrorX) {
errorXOptions = makeErrorOptions('x', trace.error_x, errorVals);
}

errorXOptions.errors = errorsX;
errorXOptions.capSize = trace.error_x.width * 2;
errorXOptions.lineWidth = trace.error_x.thickness;
errorXOptions.color = trace.error_x.color;
}

if(hasErrorY) {
errorYOptions = {};
errorYOptions.positions = positions;
var errorsY = new Float64Array(4 * count);

if(yaxis.type === 'log') {
for(i = 0; i < count; ++i) {
errorsY[ptrY++] = 0;
errorsY[ptrY++] = 0;
errorsY[ptrY++] = positions[i * 2 + 1] - yaxis.d2l(errorVals[i].ys) || 0;
errorsY[ptrY++] = yaxis.d2l(errorVals[i].yh) - positions[i * 2 + 1] || 0;
}
} else {
for(i = 0; i < count; ++i) {
errorsY[ptrY++] = 0;
errorsY[ptrY++] = 0;
errorsY[ptrY++] = positions[i * 2 + 1] - errorVals[i].ys || 0;
errorsY[ptrY++] = errorVals[i].yh - positions[i * 2 + 1] || 0;
}
if(hasErrorY) {
errorYOptions = makeErrorOptions('y', trace.error_y, errorVals);
}

errorYOptions.errors = errorsY;
errorYOptions.capSize = trace.error_y.width * 2;
errorYOptions.lineWidth = trace.error_y.thickness;
errorYOptions.color = trace.error_y.color;
}

if(hasLines) {
Expand Down Expand Up @@ -330,6 +274,33 @@ function sceneOptions(gd, subplot, trace, positions) {
markerOptions.positions = positions;
}

function makeErrorOptions(axLetter, errorOpts, vals) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

var options = {};
options.positions = positions;

var ax = AxisIDs.getFromId(gd, trace[axLetter + 'axis']);
var errors = options.errors = new Float64Array(4 * count);
var pOffset = {x: 0, y: 1}[axLetter];
var eOffset = {x: [0, 1, 2, 3], y: [2, 3, 0, 1]}[axLetter];

for(var i = 0, p = 0; i < count; i++, p += 4) {
errors[p + eOffset[0]] = positions[i * 2 + pOffset] - ax.d2l(vals[i][axLetter + 's']) || 0;
errors[p + eOffset[1]] = ax.d2l(vals[i][axLetter + 'h']) - positions[i * 2 + pOffset] || 0;
errors[p + eOffset[2]] = 0;
errors[p + eOffset[3]] = 0;
}

if(errorOpts.copy_ystyle) {
errorOpts = trace.error_y;
}

options.capSize = errorOpts.width * 2;
options.lineWidth = errorOpts.thickness;
options.color = errorOpts.color;

return options;
}

function makeSelectedOptions(selected, markerOpts) {
var options = {};

Expand Down Expand Up @@ -486,39 +457,42 @@ function sceneUpdate(gd, subplot) {
var scene = subplot._scene;
var fullLayout = gd._fullLayout;

var reset = {
// number of traces in subplot, since scene:subplot → 1:1
count: 0,
// whether scene requires init hook in plot call (dirty plot call)
dirty: true,
// last used options
lineOptions: [],
fillOptions: [],
markerOptions: [],
selectedOptions: [],
unselectedOptions: [],
errorXOptions: [],
errorYOptions: []
};

var first = {
selectBatch: null,
unselectBatch: null,
// regl- component stubs, initialized in dirty plot call
fill2d: false,
scatter2d: false,
error2d: false,
line2d: false,
select2d: null
};

if(!subplot._scene) {
scene = subplot._scene = {
// number of traces in subplot, since scene:subplot → 1:1
count: 0,

// whether scene requires init hook in plot call (dirty plot call)
dirty: true,

// last used options
lineOptions: [],
fillOptions: [],
markerOptions: [],
selectedOptions: [],
unselectedOptions: [],
errorXOptions: [],
errorYOptions: [],
selectBatch: null,
unselectBatch: null,

// regl- component stubs, initialized in dirty plot call
fill2d: false,
scatter2d: false,
error2d: false,
line2d: false,
select2d: null
};
scene = subplot._scene = Lib.extendFlat({}, reset, first);

// apply new option to all regl components
// apply new option to all regl components (used on drag)
scene.update = function update(opt) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @dfcreative you were right! scene.update is still used on drag. Added spy+assertion below to 🔏 this thing down.

var opts = Array(scene.count);
var opts = new Array(scene.count);
for(var i = 0; i < scene.count; i++) {
opts[i] = opt;
}

if(scene.fill2d) scene.fill2d.update(opts);
if(scene.scatter2d) scene.scatter2d.update(opts);
if(scene.line2d) scene.line2d.update(opts);
Expand All @@ -532,21 +506,24 @@ function sceneUpdate(gd, subplot) {
scene.draw = function draw() {
var i;
for(i = 0; i < scene.count; i++) {
if(scene.fill2d) scene.fill2d.draw(i);
if(scene.fill2d && scene.fillOptions[i]) {
// must do all fills first
scene.fill2d.draw(i);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Clear improvement over master - but this does raise the question of whether this (not your change, but the structure surrounding it) is really the right behavior, particularly with fill: 'tonext' but also with multiple fills to zero. I can try this out tomorrow and see if my interpretation of what this is doing is accurate, but In svg scatter, each fill gets drawn below any trace that it's attached to (even the trace before it if it's a fill: 'tonext') but above any other trace that comes before it. If all the lines and points are above all the fills, you can get points and lines that look disconnected from their own fills.

}
}
for(i = 0; i < scene.count; i++) {
if(scene.line2d) {
if(scene.line2d && scene.lineOptions[i]) {
scene.line2d.draw(i);
}
if(scene.error2d) {
if(scene.error2d && scene.errorXOptions[i]) {
scene.error2d.draw(i);
}
if(scene.error2d && scene.errorYOptions[i]) {
scene.error2d.draw(i + scene.count);
}
if(scene.scatter2d) {
if(scene.scatter2d && scene.markerOptions[i] && (!scene.selectBatch || !scene.selectBatch[i])) {
// traces in no-selection mode
if(!scene.selectBatch || !scene.selectBatch[i]) {
scene.scatter2d.draw(i);
}
scene.scatter2d.draw(i);
}
}

Expand Down Expand Up @@ -639,15 +616,7 @@ function sceneUpdate(gd, subplot) {

// In case if we have scene from the last calc - reset data
if(!scene.dirty) {
scene.dirty = true;
scene.count = 0;
scene.lineOptions = [];
scene.fillOptions = [];
scene.markerOptions = [];
scene.selectedOptions = [];
scene.unselectedOptions = [];
scene.errorXOptions = [];
scene.errorYOptions = [];
Lib.extendFlat(scene, reset);
}

return scene;
Expand Down
48 changes: 48 additions & 0 deletions test/jasmine/tests/gl2d_plot_interact_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,10 +282,16 @@ describe('@gl Test gl2d plots', function() {
gd.on('plotly_relayout', relayoutCallback);
})
.then(function() {
var scene = gd._fullLayout._plots.xy._scene;
spyOn(scene, 'draw');

// Drag scene along the X axis
return mouseTo([200, 200], [220, 200]);
})
.then(function() {
var scene = gd._fullLayout._plots.xy._scene;
expect(scene.draw).toHaveBeenCalledTimes(3);

expect(gd.layout.xaxis.autorange).toBe(false);
expect(gd.layout.yaxis.autorange).toBe(false);
expect(gd.layout.xaxis.range).toBeCloseToArray(newX, precision);
Expand Down Expand Up @@ -379,6 +385,48 @@ describe('@gl Test gl2d plots', function() {
.then(done);
});

it('should be able to toggle trace with different modes', function(done) {
Plotly.newPlot(gd, [{
// a trace with all regl2d objects
type: 'scattergl',
y: [1, 2, 1],
error_x: {value: 10},
error_y: {value: 10},
fill: 'tozeroy'
}, {
type: 'scattergl',
mode: 'markers',
y: [0, 1, -1]
}])
.then(function() {
var scene = gd._fullLayout._plots.xy._scene;
spyOn(scene.fill2d, 'draw');
spyOn(scene.line2d, 'draw');
spyOn(scene.error2d, 'draw');
spyOn(scene.scatter2d, 'draw');

return Plotly.restyle(gd, 'visible', 'legendonly', [0]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

On master, this gives:

peek 2018-03-05 17-29

})
.then(function() {
var scene = gd._fullLayout._plots.xy._scene;
expect(scene.fill2d.draw).toHaveBeenCalledTimes(0);
expect(scene.line2d.draw).toHaveBeenCalledTimes(0);
expect(scene.error2d.draw).toHaveBeenCalledTimes(0);
expect(scene.scatter2d.draw).toHaveBeenCalledTimes(1);

return Plotly.restyle(gd, 'visible', true, [0]);
})
.then(function() {
var scene = gd._fullLayout._plots.xy._scene;
expect(scene.fill2d.draw).toHaveBeenCalledTimes(1);
expect(scene.line2d.draw).toHaveBeenCalledTimes(1);
expect(scene.error2d.draw).toHaveBeenCalledTimes(2, 'twice for x AND y');
expect(scene.scatter2d.draw).toHaveBeenCalledTimes(3, 'both traces have markers');
})
.catch(fail)
.then(done);
});

it('@noCI should display selection of big number of regular points', function(done) {
// generate large number of points
var x = [], y = [], n = 2e2, N = n * n;
Expand Down