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

bar transitions #4180

Merged
merged 6 commits into from
Sep 11, 2019
Merged
Show file tree
Hide file tree
Changes from all 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
2 changes: 1 addition & 1 deletion src/components/drawing/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -442,7 +442,7 @@ drawing.singlePointStyle = function(d, sel, trace, fns, gd) {
fill: 'none'
});
} else {
sel.style('stroke-width', lineWidth + 'px');
sel.style('stroke-width', (d.isBlank ? 0 : lineWidth) + 'px');
Copy link
Contributor

@etpinard etpinard Sep 11, 2019

Choose a reason for hiding this comment

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

Great - tests are passing. Looks like this didn't break @archmoj's isBlank work from #4056 🎉


var markerGradient = marker.gradient;

Expand Down
1 change: 1 addition & 0 deletions src/traces/bar/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ module.exports = {
name: 'bar',
basePlotModule: require('../../plots/cartesian'),
categories: ['bar-like', 'cartesian', 'svg', 'bar', 'oriented', 'errorBarsOK', 'showLegend', 'zoomScale'],
animatable: true,
meta: {
description: [
'The data visualized by the span of the bars is set in `y`',
Expand Down
51 changes: 41 additions & 10 deletions src/traces/bar/plot.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,28 @@ function getXY(di, xa, ya, isHorizontal) {
return isHorizontal ? [s, p] : [p, s];
}

function plot(gd, plotinfo, cdModule, traceLayer, opts) {
function transition(selection, opts, makeOnCompleteCallback) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Clean wrapper!

It's a little different than what scatter/plot.js does, in particular this piece:

transition.each(function() {
// Must run the selection again since otherwise enters/updates get grouped together
// and these get executed out of order. Except we need them in order!
scatterLayer.selectAll('g.trace').each(function(d, i) {
plotOne(gd, i, plotinfo, d, cdscatterSorted, this, transitionOpts);
});
});

@antoinerg have you noticed "out of order" transitions?

Your wrapper plays better with Lib.makeTraceGroups, so if there are no side-effects, maybe we could try this pattern in scatter/plot.js and maybe fix #3931 while at it.

Copy link
Contributor

Choose a reason for hiding this comment

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

One drawback from your pattern:

  • we have to pass opts and makeOnCompleteCallback around in all the transition calls

Copy link
Contributor

@etpinard etpinard Sep 10, 2019

Choose a reason for hiding this comment

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

scatter/plot.js uses this trick:

function transition(selection) {
return hasTransition ? selection.transition() : selection;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is non-blocking

if(hasTransition(opts)) {
var onComplete;
if(makeOnCompleteCallback) {
onComplete = makeOnCompleteCallback();
}
return selection
.transition()
.duration(opts.duration)
.ease(opts.easing)
.each('end', function() { onComplete && onComplete(); })
.each('interrupt', function() { onComplete && onComplete(); });
} else {
return selection;
}
}

function hasTransition(transitionOpts) {
return transitionOpts && transitionOpts.duration > 0;
}

function plot(gd, plotinfo, cdModule, traceLayer, opts, makeOnCompleteCallback) {
var xa = plotinfo.xaxis;
var ya = plotinfo.yaxis;
var fullLayout = gd._fullLayout;
Expand Down Expand Up @@ -96,7 +117,6 @@ function plot(gd, plotinfo, cdModule, traceLayer, opts) {
// clipped xf/yf (2nd arg true): non-positive
// log values go off-screen by plotwidth
// so you see them continue if you drag the plot

var xy = getXY(di, xa, ya, isHorizontal);

var x0 = xy[0][0];
Expand All @@ -118,8 +138,11 @@ function plot(gd, plotinfo, cdModule, traceLayer, opts) {
}
di.isBlank = isBlank;

if(isBlank && isHorizontal) x1 = x0;
if(isBlank && !isHorizontal) y1 = y0;

// in waterfall mode `between` we need to adjust bar end points to match the connector width
if(adjustPixel) {
if(adjustPixel && !isBlank) {
if(isHorizontal) {
x0 -= dirSign(x0, x1) * adjustPixel;
x1 += dirSign(x0, x1) * adjustPixel;
Expand Down Expand Up @@ -178,12 +201,18 @@ function plot(gd, plotinfo, cdModule, traceLayer, opts) {
y1 = fixpx(y1, y0);
}

Lib.ensureSingle(bar, 'path')
var sel = transition(Lib.ensureSingle(bar, 'path'), opts, makeOnCompleteCallback);
sel
.style('vector-effect', 'non-scaling-stroke')
.attr('d', isBlank ? 'M0,0Z' : 'M' + x0 + ',' + y0 + 'V' + y1 + 'H' + x1 + 'V' + y0 + 'Z')
.attr('d', 'M' + x0 + ',' + y0 + 'V' + y1 + 'H' + x1 + 'V' + y0 + 'Z')
.call(Drawing.setClipUrl, plotinfo.layerClipId, gd);

appendBarText(gd, plotinfo, bar, cd, i, x0, x1, y0, y1, opts);
if(hasTransition(opts)) {
var styleFns = Drawing.makePointStyleFns(trace);
Drawing.singlePointStyle(di, sel, trace, styleFns, gd);
}

appendBarText(gd, plotinfo, bar, cd, i, x0, x1, y0, y1, opts, makeOnCompleteCallback);

if(plotinfo.layerClipId) {
Drawing.hideOutsideRangePoint(di, bar.select('text'), xa, ya, trace.xcalendar, trace.ycalendar);
Expand All @@ -197,10 +226,10 @@ function plot(gd, plotinfo, cdModule, traceLayer, opts) {
});

// error bars are on the top
Registry.getComponentMethod('errorbars', 'plot')(gd, bartraces, plotinfo);
Registry.getComponentMethod('errorbars', 'plot')(gd, bartraces, plotinfo, opts);
}

function appendBarText(gd, plotinfo, bar, calcTrace, i, x0, x1, y0, y1, opts) {
function appendBarText(gd, plotinfo, bar, calcTrace, i, x0, x1, y0, y1, opts, makeOnCompleteCallback) {
var xa = plotinfo.xaxis;
var ya = plotinfo.yaxis;

Expand All @@ -212,7 +241,6 @@ function appendBarText(gd, plotinfo, bar, calcTrace, i, x0, x1, y0, y1, opts) {
.text(text)
.attr({
'class': 'bartext bartext-' + textPosition,
transform: '',
'text-anchor': 'middle',
// prohibit tex interpretation until we can handle
// tex and regular text together
Expand Down Expand Up @@ -325,9 +353,12 @@ function appendBarText(gd, plotinfo, bar, calcTrace, i, x0, x1, y0, y1, opts) {
(textPosition === 'outside') ?
outsideTextFont : insideTextFont);

var currentTransform = textSelection.attr('transform');
textSelection.attr('transform', '');
textBB = Drawing.bBox(textSelection.node()),
Copy link
Contributor

Choose a reason for hiding this comment

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

So here, you trying to get the "untransformed" bounding box, correct? Nice catch!

I guess that's what we have to do in the current bar text svg structure:

image

where the transform is attached to the <text> element.

Compare this to scatter text:

image

which then can benefit from this piece of logic in Drawing.bBox:

if(!transform) {
// in this case, just varying x and y, don't bother caching
// the final bBox because the alteration is quick.
var innerBB = drawing.bBox(innerNode, false, hash);
if(x) {
innerBB.left += x;
innerBB.right += x;
}
if(y) {
innerBB.top += y;
innerBB.bottom += y;
}
return innerBB;
}

and not have to worry about removing and adding back the transform.


Oh well, @antoinerg nothing to do here for now, I just wanted to write this down for future reference.

Copy link
Contributor

Choose a reason for hiding this comment

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

This comment is non-blocking

textWidth = textBB.width,
textHeight = textBB.height;
textSelection.attr('transform', currentTransform);

if(textWidth <= 0 || textHeight <= 0) {
textSelection.remove();
Expand Down Expand Up @@ -360,7 +391,7 @@ function appendBarText(gd, plotinfo, bar, calcTrace, i, x0, x1, y0, y1, opts) {
}));
}

textSelection.attr('transform', transform);
transition(textSelection, opts, makeOnCompleteCallback).attr('transform', transform);
}

function getRotateFromAngle(angle) {
Expand Down
Binary file added test/image/baselines/animation_bar.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
22 changes: 22 additions & 0 deletions test/image/mocks/animation_bar.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
{
"data": [{
"type": "bar",
"x": ["A", "B", "C"],
"y": [24, 5, 8],
"error_y": {"array": [3, 2, 1]}
}],
"layout": {
"width": 400,
"height": 400,
"yaxis": {"range": [0, 30]}
},
"frames": [
Copy link
Contributor

Choose a reason for hiding this comment

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

For reference, calling Plotly.animate(gd) off this mock is very fun:

Peek 2019-09-10 11-50

{"data": [{"y": [12, 15, 10]}]},
{"data": [{"error_y": {"array": [5, 4, 1]}}]},
{"data": [{"marker": {"color": ["red", "blue", "green"]}}]},
{"data": [{"width": 0.25}]},
{"data": [{"marker": {"line": {"width": 10}}}]},
{"data": [{"marker": {"line": {"color": ["orange", "yellow", "blue"]}}}]},
{"layout": {"yaxis": {"range": [0, 20]}}}
]
}
133 changes: 133 additions & 0 deletions test/jasmine/assets/check_transitions.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,133 @@
'use strict';

var Plotly = require('@lib/index');
var Lib = require('@src/lib');
var d3 = require('d3');
var delay = require('./delay.js');

var reNumbers = /([\d\.]+)/gm;

function promiseSerial(funcs, wait) {
return funcs.reduce(function(promise, func) {
return promise.then(function(result) {
return func().then(Array.prototype.concat.bind(result)).then(delay(wait));
});
}, Promise.resolve([]));
}

function clockTick(currentNow, milliseconds) {
Date.now = function() {
return currentNow + milliseconds;
};
d3.timer.flush();
}

// Using the methodology from http://eng.wealthfront.com/2017/10/26/testing-d3-transitions/
module.exports = function checkTransition(gd, mock, animateOpts, transitionOpts, tests) {
if(!transitionOpts) {
transitionOpts = {
transition: {
duration: 500,
easing: 'linear'
},
frame: {
duration: 500
}
};
}
// Prepare chain
var now = Date.now;
var startTime;
var currentTime;
var p = [
function() {
return Plotly.newPlot(gd, mock)
.then(function() {
// Check initial states if present
for(var i = 0; i < tests.length; i++) {
if(tests[i][0] === 0) assert(tests[i]);
}
});
},
function() {
// Hijack Date.now
startTime = Date.now();
currentTime = 0;
clockTick(startTime, 0);
Plotly.animate(gd, animateOpts, transitionOpts);
return Promise.resolve(true);
}
];

var checkTests = tests.map(function(test) {
return function() {
if(test[0] === 0) return Promise.resolve(true);
if(test[0] !== currentTime) {
clockTick(startTime, test[0]);
currentTime = test[0];
}
return assert(test);
};
});

// Run all tasks
return promiseSerial(p.concat(checkTests))
.catch(function(err) {
Date.now = now;
return Promise.reject(err);
})
.then(function() {
etpinard marked this conversation as resolved.
Show resolved Hide resolved
Date.now = now;
});
};

// A test array is made of
// [ms since start of transition, selector, (attr|style), name of attribute, array of values to be found]
// Ex.: [0, '.point path', 'style', 'fill', ['rgb(31, 119, 180)', 'rgb(31, 119, 180)', 'rgb(31, 119, 180)']]
function assert(test) {
var msg = 'at ' + test[0] + 'ms, selection ' + test[1] + ' has ' + test[3];
var cur = [];
d3.selectAll(test[1]).each(function(d, i) {
if(test[2] === 'style') cur[i] = this.style[test[3]];
if(test[2] === 'attr') cur[i] = d3.select(this).attr(test[3]);
});
switch(test[3]) {
case 'd':
assertEqual(cur, test[4], round, msg);
break;
case 'transform':
assertCloseTo(cur, test[4], 3, extractNumbers, msg);
break;
default:
assertEqual(cur, test[4], Lib.identity, msg);
}
return Promise.resolve(true);
}

function assertEqual(A, B, cb, msg) {
var a = cb(A);
var b = cb(B);
expect(a).withContext(msg + ' equal to ' + JSON.stringify(a)).toEqual(b);
}

function assertCloseTo(A, B, tolerance, cb, msg) {
var a = cb(A).flat();
var b = cb(B).flat();
expect(a).withContext(msg + ' equal to ' + JSON.stringify(A)).toBeWithinArray(b, tolerance);
}

function extractNumbers(array) {
return array.map(function(d) {
return d.match(reNumbers).map(function(n) {
return parseFloat(n);
});
});
}

function round(array) {
return array.map(function(cur) {
return cur.replace(reNumbers, function(match) {
return Math.round(match);
});
});
}
Loading