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.

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

Better axis range animations #2788

Merged
merged 5 commits into from
Jul 11, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
23 changes: 20 additions & 3 deletions src/plots/cartesian/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -122,15 +122,32 @@ exports.finalizeSubplots = function(layoutIn, layoutOut) {
}
};

/**
* Cartesian.plot
*
* @param {DOM div | object} gd
* @param {array | null} (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
* transition make callback function from Plots.transition
*/
exports.plot = function(gd, traces, transitionOpts, makeOnCompleteCallback) {
var fullLayout = gd._fullLayout;
var subplots = fullLayout._subplots.cartesian;
var calcdata = gd.calcdata;
var i;

// If traces is not provided, then it's a complete replot and missing
// traces are removed
if(!Array.isArray(traces)) {
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 traces is not provided, then it's a complete replot and missing
// traces are removed
traces = [];
for(i = 0; i < calcdata.length; i++) traces.push(i);
}
Expand Down
3 changes: 3 additions & 0 deletions src/plots/plots.js
Original file line number Diff line number Diff line change
Expand Up @@ -2333,6 +2333,9 @@ 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 happens on layout-only (e.g. axis range) animations
transitionedTraces = null;
} else {
traceTransitionOpts = transitionOpts;
}
Expand Down
40 changes: 40 additions & 0 deletions test/jasmine/tests/animate_test.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
var Plotly = require('@lib/index');
var Lib = require('@src/lib');
var Registry = require('@src/registry');
var Plots = Plotly.Plots;

var createGraphDiv = require('../assets/create_graph_div');
Expand Down Expand Up @@ -836,4 +837,43 @@ describe('animating scatter traces', function() {
expect(gd.calcdata[0][0].y).toEqual(3);
}).catch(failTest).then(done);
});

it('should animate axis ranges using the less number of steps', function(done) {
Plotly.plot(gd, [{
y: [1, 2, 1]
}, {
type: 'bar',
y: [2, 1, 2]
}])
.then(function() {
spyOn(gd._fullData[0]._module.basePlotModule, 'transitionAxes').and.callThrough();
spyOn(gd._fullData[0]._module, 'plot').and.callThrough();
spyOn(gd._fullData[1]._module, 'plot').and.callThrough();
spyOn(Registry, 'call').and.callThrough();

return Plotly.animate('graph', {
layout: {
xaxis: {range: [0.45, 0.55]},
yaxis: {range: [0.45, 0.55]}
}
}, {
transition: {duration: 500},
frame: {redraw: false}
});
})
.then(function() {
// the only redraw should occur during Cartesian.transitionAxes,
// where Registry.call('relayout') is called leading to a _module.plot call
expect(gd._fullData[0]._module.basePlotModule.transitionAxes).toHaveBeenCalledTimes(1);
expect(gd._fullData[0]._module.plot).toHaveBeenCalledTimes(1);
expect(gd._fullData[1]._module.plot).toHaveBeenCalledTimes(1);
expect(Registry.call).toHaveBeenCalledTimes(1);

var calls = Registry.call.calls.all();
expect(calls.length).toBe(1, 'just one Registry.call call');
expect(calls[0].args[0]).toBe('relayout', 'called Registry.call with');
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a pretty low-level assertion, not clear to me that it would catch breaks other than the exact change you made here. Would it be possible to do something like:

    var animatePromise = Plotly.animate(...)
    expect(stuff).toBe(nearWhereItStarted)
    return animatePromise;
})
.then(function() {
    expect(stuff).toBe(nearWhereItFinishes);
    expect(theFullDataAndRegistryCallStuff);

ie something about the scatter marker and bar positions being updated async that would have failed before this change?

Copy link
Contributor Author

@etpinard etpinard Jul 9, 2018

Choose a reason for hiding this comment

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

ie something about the scatter marker and bar positions being updated async that would have failed before this change?

The marker and bar position does not change, it's the subplot and clip rect translate transforms that change during axis range transitions via Cartesian.transitionAxes (which mimics cartesian dragbox logic).

Better test added in -> 19300f9

})
.catch(failTest)
.then(done);
});
});