Skip to content

Commit

Permalink
treat trace w/ _length===0 as if they were visible:false
Browse files Browse the repository at this point in the history
... after transform _module.calc loop

- that way filter transforms that remove all data coordinates
  don't result in errors
- of all the mocks listed in mock_lists.js, only 'scattercarpet' and
  'world-cals' still error out (wip)
  • Loading branch information
etpinard committed Apr 26, 2019
1 parent 1e57928 commit 3a5a4c2
Show file tree
Hide file tree
Showing 9 changed files with 83 additions and 10 deletions.
4 changes: 3 additions & 1 deletion src/components/fx/hover.js
Original file line number Diff line number Diff line change
Expand Up @@ -370,10 +370,12 @@ function _hover(gd, evt, subplot, noHoverEvent) {
cd = searchData[curvenum];

// filter out invisible or broken data
if(!cd || !cd[0] || !cd[0].trace || cd[0].trace.visible !== true) continue;
if(!cd || !cd[0] || !cd[0].trace) continue;

trace = cd[0].trace;

if(trace.visible !== true || trace._length === 0) continue;

// Explicitly bail out for these two. I don't know how to otherwise prevent
// the rest of this function from running and failing
if(['carpet', 'contourcarpet'].indexOf(trace._module.name) !== -1) continue;
Expand Down
3 changes: 2 additions & 1 deletion src/lib/filter_visible.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ function baseFilter(item) {
}

function calcDataFilter(item) {
return item[0].trace.visible === true;
var trace = item[0].trace;
return trace.visible === true && trace._length !== 0;
}

function isCalcData(cont) {
Expand Down
2 changes: 1 addition & 1 deletion src/plot_api/subroutines.js
Original file line number Diff line number Diff line change
Expand Up @@ -674,7 +674,7 @@ exports.redrawReglTraces = function(gd) {
for(i = 0; i < fullData.length; i++) {
var trace = fullData[i];

if(trace.visible === true) {
if(trace.visible === true && trace._length !== 0) {
if(trace.type === 'splom') {
fullLayout._splomScenes[trace.uid].draw();
} else if(trace.type === 'scattergl') {
Expand Down
6 changes: 4 additions & 2 deletions src/plots/get_data.js
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,10 @@ exports.getModuleCalcData = function(calcdata, arg1) {
for(var i = 0; i < calcdata.length; i++) {
var cd = calcdata[i];
var trace = cd[0].trace;
// N.B. 'legendonly' traces do not make it past here
if(trace.visible !== true) continue;
// N.B.
// - 'legendonly' traces do not make it past here
// - skip over 'visible' traces that got trimmed completely during calc transforms
if(trace.visible !== true || trace._length === 0) continue;

// group calcdata trace not by 'module' (as the name of this function
// would suggest), but by 'module plot method' so that if some traces
Expand Down
7 changes: 4 additions & 3 deletions src/plots/gl3d/scene.js
Original file line number Diff line number Diff line change
Expand Up @@ -502,7 +502,7 @@ proto.plot = function(sceneData, fullLayout, layout) {

for(i = 0; i < sceneData.length; ++i) {
data = sceneData[i];
if(data.visible !== true) continue;
if(data.visible !== true || data._length === 0) continue;

computeTraceBounds(this, data, dataBounds);
}
Expand All @@ -526,7 +526,7 @@ proto.plot = function(sceneData, fullLayout, layout) {
// Update traces
for(i = 0; i < sceneData.length; ++i) {
data = sceneData[i];
if(data.visible !== true) {
if(data.visible !== true || data._length === 0) {
continue;
}
trace = this.traces[data.uid];
Expand All @@ -551,7 +551,8 @@ proto.plot = function(sceneData, fullLayout, layout) {
traceIdLoop:
for(i = 0; i < traceIds.length; ++i) {
for(j = 0; j < sceneData.length; ++j) {
if(sceneData[j].uid === traceIds[i] && sceneData[j].visible === true) {
if(sceneData[j].uid === traceIds[i] &&
(sceneData[j].visible === true && sceneData[j]._length !== 0)) {
continue traceIdLoop;
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/plots/plots.js
Original file line number Diff line number Diff line change
Expand Up @@ -2778,7 +2778,7 @@ plots.doCalcdata = function(gd, traces) {

var cd = [];

if(trace.visible === true) {
if(trace.visible === true && trace._length !== 0) {
// clear existing ref in case it got relinked
delete trace._indexToPoints;
// keep ref of index-to-points map object of the *last* enabled transform,
Expand Down
3 changes: 3 additions & 0 deletions src/plots/polar/set_convert.js
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,9 @@ function setConvertAngular(ax, polarLayout) {
var catLen = ax._categories.length;
var _period = ax.period ? Math.max(ax.period, catLen) : catLen;

// fallback in case all categories have been filtered out
if(_period === 0) _period = 1;

c2rad = t2rad = function(v) { return v * 2 * Math.PI / _period; };
rad2c = rad2t = function(v) { return v * _period / Math.PI / 2; };

Expand Down
2 changes: 1 addition & 1 deletion src/traces/scattermapbox/convert.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ var convertTextOpts = require('../../plots/mapbox/convert_text_opts');
module.exports = function convert(calcTrace) {
var trace = calcTrace[0].trace;

var isVisible = (trace.visible === true);
var isVisible = (trace.visible === true && trace._length !== 0);
var hasFill = (trace.fill !== 'none');
var hasLines = subTypes.hasLines(trace);
var hasMarkers = subTypes.hasMarkers(trace);
Expand Down
64 changes: 64 additions & 0 deletions test/jasmine/tests/transform_filter_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1319,3 +1319,67 @@ describe('filter transforms interactions', function() {
.then(done);
});
});

describe('filter resulting in empty coordinate arrays', function() {
afterEach(destroyGraphDiv);

function filter2empty(mock) {

This comment has been minimized.

Copy link
@etpinard

etpinard Apr 26, 2019

Author Contributor

cc #3215 - where we could probably use a similar technique to test typed arrays everywhereTM.

var fig = Lib.extendDeep({}, mock);
var data = fig.data || [];

data.forEach(function(trace) {
trace.transforms = [{
type: 'filter',
target: [null]
}];
});

return fig;
}

describe('svg mocks', function() {
var mockList = require('../assets/mock_lists').svg;

mockList.forEach(function(d) {
if(d[0] === 'scattercarpet' || d[0] === 'world-cals') {
// scattercarpet don't work with transforms
// world-cals mock complains during a Lib.cleanDate()
return;
}

it(d[0], function(done) {
var gd = createGraphDiv();
var fig = filter2empty(d[1]);
Plotly.newPlot(gd, fig).catch(failTest).then(done);
});
});
});

describe('gl mocks', function() {
var mockList = require('../assets/mock_lists').gl;

mockList.forEach(function(d) {
it('@gl ' + d[0], function(done) {
var gd = createGraphDiv();
var fig = filter2empty(d[1]);
Plotly.newPlot(gd, fig).catch(failTest).then(done);
});
});
});

describe('mapbox mocks', function() {
var mockList = require('../assets/mock_lists').mapbox;

Plotly.setPlotConfig({
mapboxAccessToken: require('@build/credentials.json').MAPBOX_ACCESS_TOKEN
});

mockList.forEach(function(d) {
it('@noCI ' + d[0], function(done) {
var gd = createGraphDiv();
var fig = filter2empty(d[1]);
Plotly.newPlot(gd, fig).catch(failTest).then(done);
});
});
});
});

0 comments on commit 3a5a4c2

Please sign in to comment.