Skip to content

Set enabled:false when filter target array is empty #3766

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

Merged
merged 8 commits into from
May 8, 2019
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 @@ -642,7 +642,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
2 changes: 1 addition & 1 deletion src/plots/cartesian/set_convert.js
Original file line number Diff line number Diff line change
Expand Up @@ -403,7 +403,7 @@ module.exports = function setConvert(ax, fullLayout) {
return;
}

if(ax.type === 'date') {
if(ax.type === 'date' && !ax.autorange) {
// check if milliseconds or js date objects are provided for range
// and convert to date strings
range[0] = Lib.cleanDate(range[0], BADNUM, ax.calendar);
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
23 changes: 13 additions & 10 deletions src/plots/plots.js
Original file line number Diff line number Diff line change
Expand Up @@ -2749,17 +2749,13 @@ plots.doCalcdata = function(gd, traces) {
);
}

setupAxisCategories(axList, fullData);

var hasCalcTransform = false;

// transform loop
for(i = 0; i < fullData.length; i++) {
function transformCalci(i) {
trace = fullData[i];
_module = trace._module;

if(trace.visible === true && trace.transforms) {
_module = trace._module;

// we need one round of trace module calc before
// the calc transform to 'fill in' the categories list
// used for example in the data-to-coordinate method
Expand All @@ -2786,9 +2782,6 @@ plots.doCalcdata = function(gd, traces) {
}
}

// clear stuff that should recomputed in 'regular' loop
if(hasCalcTransform) setupAxisCategories(axList, fullData);

function calci(i, isContainer) {
trace = fullData[i];
_module = trace._module;
Expand All @@ -2797,7 +2790,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 Expand Up @@ -2833,6 +2826,16 @@ plots.doCalcdata = function(gd, traces) {
calcdata[i] = cd;
}

setupAxisCategories(axList, fullData);

// 'transform' loop - must calc container traces first
// so that if their dependent traces can get transform properly
for(i = 0; i < fullData.length; i++) calci(i, true);
for(i = 0; i < fullData.length; i++) transformCalci(i);

// clear stuff that should recomputed in 'regular' loop
if(hasCalcTransform) setupAxisCategories(axList, fullData);

// 'regular' loop - make sure container traces (eg carpet) calc before
// contained traces (eg contourcarpet)
for(i = 0; i < fullData.length; i++) calci(i, true);
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
8 changes: 7 additions & 1 deletion src/transforms/filter.js
Original file line number Diff line number Diff line change
Expand Up @@ -138,10 +138,16 @@ exports.supplyDefaults = function(transformIn) {
var enabled = coerce('enabled');

if(enabled) {
var target = coerce('target');

if(Lib.isArrayOrTypedArray(target) && target.length === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

@etpinard Thanks for the PR.
Cool!
What about handling a case like this: [ [ ] ]?
See codepen.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. Thanks for the review!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@archmoj this turned out to be a bit more of a project than anticipated.

See 3a5a4c2 - which makes things work for all trace types except scattercarpet and in some world-calendar scenario. I'll try to get those two cases fixed before 1.48.0, but in the meantime here's "a good chuck" of the solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

which makes things work for all trace types except scattercarpet and in some world-calendar scenario.

Now fixed by b643238 and 40c8abc

Making this PR ready for a second 👁️

transformOut.enabled = false;
return transformOut;
}

coerce('preservegaps');
coerce('operation');
coerce('value');
coerce('target');

var handleCalendarDefaults = Registry.getComponentMethod('calendars', 'handleDefaults');
handleCalendarDefaults(transformIn, transformOut, 'valuecalendar', null);
Expand Down
9 changes: 3 additions & 6 deletions test/jasmine/tests/funnel_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1018,7 +1018,7 @@ describe('A funnel plot', function() {
.then(done);
});

it('should be able to deal with blank bars on transform', function(done) {
it('should be able to deal with transform that empty out the data coordinate arrays', function(done) {
Plotly.plot(gd, {
data: [{
type: 'funnel',
Expand All @@ -1038,14 +1038,11 @@ describe('A funnel plot', function() {
})
.then(function() {
var traceNodes = getAllTraceNodes(gd);
var funnelNodes = getAllFunnelNodes(traceNodes[0]);
var pathNode = funnelNodes[0].querySelector('path');
expect(traceNodes.length).toBe(0);

expect(gd.calcdata[0][0].x).toEqual(NaN);
expect(gd.calcdata[0][0].y).toEqual(NaN);
expect(gd.calcdata[0][0].isBlank).toBe(true);

expect(pathNode.outerHTML).toEqual('<path d="M0,0Z" style="vector-effect: non-scaling-stroke;"></path>');
expect(gd.calcdata[0][0].isBlank).toBe(undefined);
})
.catch(failTest)
.then(done);
Expand Down
93 changes: 93 additions & 0 deletions test/jasmine/tests/transform_filter_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,33 @@ describe('filter transforms defaults:', function() {
expect(traceOut.transforms[2].target).toEqual('x');
expect(traceOut.transforms[3].target).toEqual('marker.color');
});

it('supplyTraceDefaults should set *enabled:false* and return early when *target* is an empty array', function() {
// see https://github.com/plotly/plotly.js/issues/2908
// this solves multiple problems downstream

traceIn = {
x: [1, 2, 3],
transforms: [{
type: 'filter',
target: []
}]
};
traceOut = Plots.supplyTraceDefaults(traceIn, {type: 'scatter'}, 0, fullLayout);
expect(traceOut.transforms[0].target).toEqual([]);
expect(traceOut.transforms[0].enabled).toBe(false, 'set to false!');

traceIn = {
x: new Float32Array([1, 2, 3]),
transforms: [{
type: 'filter',
target: new Float32Array()
}]
};
traceOut = Plots.supplyTraceDefaults(traceIn, {type: 'scatter'}, 0, fullLayout);
expect(traceOut.transforms[0].target).toEqual(new Float32Array());
expect(traceOut.transforms[0].enabled).toBe(false, 'set to false!');
});
});

describe('filter transforms calc:', function() {
Expand Down Expand Up @@ -1292,3 +1319,69 @@ describe('filter transforms interactions', function() {
.then(done);
});
});

describe('filter resulting in empty coordinate arrays', function() {
var gd;

afterEach(function(done) {
Plotly.purge(gd);
setTimeout(function() {
destroyGraphDiv();
done();
}, 200);
});

function filter2empty(mock) {
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) {
it(d[0], function(done) {
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) {
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) {
gd = createGraphDiv();
var fig = filter2empty(d[1]);
Plotly.newPlot(gd, fig).catch(failTest).then(done);
});
});
});
});
9 changes: 3 additions & 6 deletions test/jasmine/tests/waterfall_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -976,7 +976,7 @@ describe('A waterfall plot', function() {
.then(done);
});

it('should be able to deal with blank bars on transform', function(done) {
it('should be able to deal with transform that empty out the data coordinate arrays', function(done) {
Plotly.plot(gd, {
data: [{
type: 'waterfall',
Expand All @@ -993,14 +993,11 @@ describe('A waterfall plot', function() {
})
.then(function() {
var traceNodes = getAllTraceNodes(gd);
var waterfallNodes = getAllWaterfallNodes(traceNodes[0]);
var pathNode = waterfallNodes[0].querySelector('path');
expect(traceNodes.length).toBe(0);

expect(gd.calcdata[0][0].x).toEqual(NaN);
expect(gd.calcdata[0][0].y).toEqual(NaN);
expect(gd.calcdata[0][0].isBlank).toBe(true);

expect(pathNode.outerHTML).toEqual('<path d="M0,0Z" style="vector-effect: non-scaling-stroke;"></path>');
expect(gd.calcdata[0][0].isBlank).toBe(undefined);
})
.catch(failTest)
.then(done);
Expand Down