From 5c6576afce252cfe6377b96617abd5d8dc7f57bd Mon Sep 17 00:00:00 2001 From: etienne Date: Mon, 5 Mar 2018 17:25:54 -0500 Subject: [PATCH 1/5] fix #2441 - don't attempt to draw empty option items - refs to regl2d objects are kept even when options objects are emptied e.g. in the case of legend visible toggles, so don't try to redraw regl2d object with old options. --- src/traces/scattergl/index.js | 17 ++++---- test/jasmine/tests/gl2d_plot_interact_test.js | 42 +++++++++++++++++++ 2 files changed, 52 insertions(+), 7 deletions(-) diff --git a/src/traces/scattergl/index.js b/src/traces/scattergl/index.js index 208495ba8f6..55546cadb51 100644 --- a/src/traces/scattergl/index.js +++ b/src/traces/scattergl/index.js @@ -556,21 +556,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); + } } 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); } } diff --git a/test/jasmine/tests/gl2d_plot_interact_test.js b/test/jasmine/tests/gl2d_plot_interact_test.js index ffcda67b908..ac277001222 100644 --- a/test/jasmine/tests/gl2d_plot_interact_test.js +++ b/test/jasmine/tests/gl2d_plot_interact_test.js @@ -379,6 +379,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]); + }) + .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; From 187ac230d94f4b1376be91667ea49beaa2cb8527 Mon Sep 17 00:00:00 2001 From: etienne Date: Mon, 5 Mar 2018 17:26:25 -0500 Subject: [PATCH 2/5] fixup - no need to override opacity editType twice --- src/traces/scattergl/attributes.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/traces/scattergl/attributes.js b/src/traces/scattergl/attributes.js index e0d469b6ddf..cffa267c25a 100644 --- a/src/traces/scattergl/attributes.js +++ b/src/traces/scattergl/attributes.js @@ -83,9 +83,7 @@ var attrs = module.exports = overrideAll({ marker: scatterAttrs.unselected.marker }, - opacity: extendFlat({}, plotAttrs.opacity, { - editType: 'calc' - }), + opacity: extendFlat({}, plotAttrs.opacity) }, 'calc', 'nested'); From 91cac2e6b4d27e9d47a87d51aac04b311607febf Mon Sep 17 00:00:00 2001 From: etienne Date: Mon, 5 Mar 2018 17:27:38 -0500 Subject: [PATCH 3/5] DRY up scattergl - use same convert routine for x and y errors, - use same reset options object when initializing and resetting scene options. --- src/traces/scattergl/index.js | 174 ++++++++++++---------------------- 1 file changed, 62 insertions(+), 112 deletions(-) diff --git a/src/traces/scattergl/index.js b/src/traces/scattergl/index.js index 55546cadb51..23032517c15 100644 --- a/src/traces/scattergl/index.js +++ b/src/traces/scattergl/index.js @@ -165,13 +165,9 @@ function sceneOptions(gd, subplot, trace, positions) { var fullLayout = gd._fullLayout; var count = positions.length / 2; var markerOpts = trace.marker; - var xaxis = Axes.getFromId(gd, trace.xaxis); - var yaxis = Axes.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; @@ -183,7 +179,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'; } @@ -193,67 +188,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) { @@ -354,6 +298,33 @@ function sceneOptions(gd, subplot, trace, positions) { markerOptions.positions = positions; } + function makeErrorOptions(axLetter, errorOpts, vals) { + var options = {}; + options.positions = positions; + + var ax = Axes.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 = {}; @@ -510,47 +481,34 @@ function sceneUpdate(gd, subplot) { var scene = subplot._scene; var fullLayout = gd._fullLayout; - 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 - }; + 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: [] + }; - // apply new option to all regl components - scene.update = function update(opt) { - var opts = 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); - if(scene.error2d) scene.error2d.update(opts.concat(opts)); - if(scene.select2d) scene.select2d.update(opts); + var first = { + selectBatch: null, + unselectBatch: null, + // regl- component stubs, initialized in dirty plot call + fill2d: false, + scatter2d: false, + error2d: false, + line2d: false, + select2d: null + }; - scene.draw(); - }; + if(!subplot._scene) { + scene = subplot._scene = Lib.extendFlat({}, reset, first); // draw traces in proper order scene.draw = function draw() { @@ -666,15 +624,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; From a98d1d5cbba0c9a0f5fca4333dfb363556431774 Mon Sep 17 00:00:00 2001 From: etienne Date: Tue, 6 Mar 2018 16:43:53 -0500 Subject: [PATCH 4/5] bring back scene.update + :lock: down its used during drag --- src/traces/scattergl/index.js | 16 ++++++++++++++++ test/jasmine/tests/gl2d_plot_interact_test.js | 6 ++++++ 2 files changed, 22 insertions(+) diff --git a/src/traces/scattergl/index.js b/src/traces/scattergl/index.js index 9c0bcff5a57..abe85db5f1e 100644 --- a/src/traces/scattergl/index.js +++ b/src/traces/scattergl/index.js @@ -486,6 +486,22 @@ function sceneUpdate(gd, subplot) { if(!subplot._scene) { scene = subplot._scene = Lib.extendFlat({}, reset, first); + // apply new option to all regl components (used on drag) + scene.update = function update(opt) { + 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); + if(scene.error2d) scene.error2d.update(opts.concat(opts)); + if(scene.select2d) scene.select2d.update(opts); + + scene.draw(); + }; + // draw traces in proper order scene.draw = function draw() { var i; diff --git a/test/jasmine/tests/gl2d_plot_interact_test.js b/test/jasmine/tests/gl2d_plot_interact_test.js index 5367347b420..da414dce007 100644 --- a/test/jasmine/tests/gl2d_plot_interact_test.js +++ b/test/jasmine/tests/gl2d_plot_interact_test.js @@ -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); From 47768321e84cdbd40f4a44bda3e7cd1c0f2c2fae Mon Sep 17 00:00:00 2001 From: etienne Date: Tue, 6 Mar 2018 16:46:15 -0500 Subject: [PATCH 5/5] no need to extendFlat attrs that 'only' get overrode by overrideAll --- src/traces/scattergl/attributes.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/traces/scattergl/attributes.js b/src/traces/scattergl/attributes.js index cffa267c25a..784d40bb4a2 100644 --- a/src/traces/scattergl/attributes.js +++ b/src/traces/scattergl/attributes.js @@ -83,7 +83,7 @@ var attrs = module.exports = overrideAll({ marker: scatterAttrs.unselected.marker }, - opacity: extendFlat({}, plotAttrs.opacity) + opacity: plotAttrs.opacity }, 'calc', 'nested');