Skip to content

Fix Plotly.animate on graphs with multiple basePlotModules #3860

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 4 commits into from
May 14, 2019
Merged
Show file tree
Hide file tree
Changes from 3 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
9 changes: 2 additions & 7 deletions src/plots/cartesian/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -125,10 +125,9 @@ exports.finalizeSubplots = function(layoutIn, layoutOut) {
* Cartesian.plot
*
* @param {DOM div | object} gd
* @param {array | null} (optional) traces
* @param {array (optional)} traces
* array of traces indices to plot
* if undefined, plots all cartesian traces,
* if null, plots no traces
* @param {object} (optional) transitionOpts
* transition option object
* @param {function} (optional) makeOnCompleteCallback
Expand All @@ -140,11 +139,7 @@ exports.plot = function(gd, traces, transitionOpts, makeOnCompleteCallback) {
var calcdata = gd.calcdata;
var i;

if(traces === null) {
// this means no updates required, must return here
// so that plotOne doesn't remove the trace layers
return;
} else if(!Array.isArray(traces)) {
if(!Array.isArray(traces)) {
// If traces is not provided, then it's a complete replot and missing
// traces are removed
traces = [];
Expand Down
30 changes: 17 additions & 13 deletions src/plots/plots.js
Original file line number Diff line number Diff line change
Expand Up @@ -2309,7 +2309,7 @@ plots.extendLayout = function(destLayout, srcLayout) {
*/
plots.transition = function(gd, data, layout, traces, frameOpts, transitionOpts) {
var opts = {redraw: frameOpts.redraw};
var transitionedTraces = [];
var transitionedTraces = {};
var axEdits = [];

opts.prepareFn = function() {
Expand All @@ -2319,16 +2319,18 @@ plots.transition = function(gd, data, layout, traces, frameOpts, transitionOpts)
for(var i = 0; i < traceIndices.length; i++) {
var traceIdx = traceIndices[i];
var trace = gd._fullData[traceIdx];
var module = trace._module;
var _module = trace._module;

// There's nothing to do if this module is not defined:
if(!module) continue;
if(!_module) continue;

// Don't register the trace as transitioned if it doesn't know what to do.
// If it *is* registered, it will receive a callback that it's responsible
// for calling in order to register the transition as having completed.
if(module.animatable) {
transitionedTraces.push(traceIdx);
if(_module.animatable) {
var n = _module.basePlotModule.name;
if(!transitionedTraces[n]) transitionedTraces[n] = [];
transitionedTraces[n].push(traceIdx);
}

gd.data[traceIndices[i]] = plots.extendTrace(gd.data[traceIndices[i]], data[i]);
Expand Down Expand Up @@ -2427,19 +2429,21 @@ plots.transition = function(gd, data, layout, traces, frameOpts, transitionOpts)
if(hasAxisTransition) {
traceTransitionOpts = Lib.extendFlat({}, transitionOpts);
traceTransitionOpts.duration = 0;
// This means do not transition traces,
// This means do not transition cartesian traces,
// this happens on layout-only (e.g. axis range) animations
transitionedTraces = null;
delete transitionedTraces.cartesian;
} else {
traceTransitionOpts = transitionOpts;
}

for(i = 0; i < basePlotModules.length; i++) {
// Note that we pass a callback to *create* the callback that must be invoked on completion.
// This is since not all traces know about transitions, so it greatly simplifies matters if
// the trace is responsible for creating a callback, if needed, and then executing it when
// the time is right.
basePlotModules[i].plot(gd, transitionedTraces, traceTransitionOpts, makeCallback);
// Note that we pass a callback to *create* the callback that must be invoked on completion.
// This is since not all traces know about transitions, so it greatly simplifies matters if
// the trace is responsible for creating a callback, if needed, and then executing it when
// the time is right.
for(var n in transitionedTraces) {
var traceIndices = transitionedTraces[n];
var _module = gd._fullData[traceIndices[0]]._module;
_module.basePlotModule.plot(gd, traceIndices, traceTransitionOpts, makeCallback);
}
};

Expand Down
1 change: 1 addition & 0 deletions src/traces/sunburst/attributes.js
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ module.exports = {
level: {
valType: 'any',
editType: 'plot',
anim: true,
role: 'info',
description: [
'Sets the level from which this sunburst trace hierarchy is rendered.',
Expand Down
Binary file added test/image/baselines/sunburst_pie_cartesian.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
121 changes: 121 additions & 0 deletions test/image/mocks/sunburst_pie_cartesian.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

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

Nicely done.
Could we reuse this mock instead?
test/image/baselines/display-text_zero-number.png

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What's wrong with the mock I used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ha I see, that mock also has a sunburst along with cartesian traces. Yep, good call. Thanks!

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 d5064ff

"data": [
{
"name": "pie",
"type": "pie",
"labels": [
"A",
"B",
"C"
],
"values": [
3,
2,
1
],
"domain": {
"x": [
0,
0.48
],
"y": [
0.52,
1
]
}
},
{
"name": "sunburst",
"type": "sunburst",
"parents": [
"",
"A",
"B"
],
"labels": [
"A",
"B",
"C"
],
"values": [
3,
2,
1
],
"domain": {
"x": [
0.52,
1
],
"y": [
0,
0.48
]
}
},
{
"name": "bar",
"type": "bar",
"x": [
"A",
"B",
"C"
],
"y": [
3,
2,
1
]
},
{
"name": "waterfall",
"type": "waterfall",
"x": [
"A",
"B",
"C"
],
"y": [
3,
-2,
1
],
"xaxis": "x2",
"yaxis": "y2"
}
],
"layout": {
"width": 600,
"height": 600,
"dragmode": "pan",
"xaxis": {
"domain": [
0,
0.48
]
},
"xaxis2": {
"anchor": "y2",
"domain": [
0.52,
1
]
},
"yaxis": {
"domain": [
0,
0.48
]
},
"yaxis2": {
"anchor": "x2",
"domain": [
0.52,
1
]
},
"title": {
"text": "Sunburst + Pie + Cartesian"
}
}
}
54 changes: 54 additions & 0 deletions test/jasmine/tests/sunburst_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1160,4 +1160,58 @@ describe('Test sunburst interactions edge cases', function() {
})
.then(done);
});

it('should transition sunburst traces only', function(done) {
var mock = Lib.extendDeep({}, require('@mocks/sunburst_pie_cartesian.json'));
mock.data[0].visible = false;

function _assert(msg, exp) {
var gd3 = d3.select(gd);
expect(gd3.select('.cartesianlayer').selectAll('.trace').size())
.toBe(exp.cartesianTraceCnt, '# of cartesian traces');
expect(gd3.select('.pielayer').selectAll('.trace').size())
.toBe(exp.pieTraceCnt, '# of pie traces');
expect(gd3.select('.sunburstlayer').selectAll('.trace').size())
.toBe(exp.sunburstTraceCnt, '# of sunburst traces');
}

Plotly.plot(gd, mock)
.then(function() {
_assert('base', {
cartesianTraceCnt: 2,
pieTraceCnt: 0,
sunburstTraceCnt: 1
});
})
.then(click(gd, 2))
.then(delay(constants.CLICK_TRANSITION_TIME + 1))
.then(function() {
_assert('after sunburst click', {
cartesianTraceCnt: 2,
pieTraceCnt: 0,
sunburstTraceCnt: 1
});
})
.catch(failTest)
.then(done);
});

it('should be able to transition sunburst traces via `Plotly.react`', function(done) {
var mock = Lib.extendDeep({}, require('@mocks/sunburst_pie_cartesian.json'));
mock.layout.transition = {duration: 200};

spyOn(Plots, 'transitionFromReact').and.callThrough();

Plotly.plot(gd, mock)
.then(function() {
gd.data[1].level = 'B';
return Plotly.react(gd, gd.data, gd.layout);
})
.then(delay(202))
.then(function() {
expect(Plots.transitionFromReact).toHaveBeenCalledTimes(1);
})
.catch(failTest)
.then(done);
});
});