Skip to content

scatter marker color gradients#1620

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.

Sign up for GitHub

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
alexcjohnson merged 9 commits into from
May 3, 2017
Merged

scatter marker color gradients #1620

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
541460a
scatter marker color gradients
alexcjohnson Apr 22, 2017
ff6c910
Merge branch 'master' into gradient-fill
alexcjohnson May 2, 2017
105ce9b
include gradient arrays in scattercarpet test image
alexcjohnson May 2, 2017
a074259
:hocho: uid and *src from image mocks
alexcjohnson May 2, 2017
1b0a6bb
more robust no-gradient-object fallback
alexcjohnson May 2, 2017
d1c6b51
test gradient restyle behavior
alexcjohnson May 2, 2017
e3af5eb
link gradient to the point, not the color combo
alexcjohnson May 2, 2017
ec4d9ee
store index with each cd item instead of trying to get it from the join
alexcjohnson May 3, 2017
21650c7
ignore cd[i].i in unrelated tests
alexcjohnson May 3, 2017
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
107 changes: 92 additions & 15 deletions src/components/drawing/index.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -11,6 +11,7 @@

var d3 = require('d3');
var isNumeric = require('fast-isnumeric');
var tinycolor = require('tinycolor2');

var Registry = require('../../registry');
var Color = require('../color');
Expand DownExpand Up@@ -202,7 +203,7 @@ drawing.symbolNumber = function(v) {
return Math.floor(Math.max(v, 0));
};

function singlePointStyle(d, sel, trace, markerScale, lineScale, marker, markerLine) {
function singlePointStyle(d, sel, trace, markerScale, lineScale, marker, markerLine, gd) {
// only scatter & box plots get marker path and opacity
// bars, histograms don't
if(Registry.traceIs(trace, 'symbols')) {
Expand DownExpand Up@@ -237,6 +238,8 @@ function singlePointStyle(d, sel, trace, markerScale, lineScale, marker, markerL
});
}

var perPointGradient = false;

// 'so' is suspected outliers, for box plots
var fillColor,
lineColor,
Expand All@@ -256,8 +259,12 @@ function singlePointStyle(d, sel, trace, markerScale, lineScale, marker, markerL
else if(Array.isArray(markerLine.color)) lineColor = Color.defaultLine;
else lineColor = markerLine.color;

if(Array.isArray(marker.color)) {
fillColor = Color.defaultLine;
perPointGradient = true;
}

if('mc' in d) fillColor = d.mcc = markerScale(d.mc);
else if(Array.isArray(marker.color)) fillColor = Color.defaultLine;
else fillColor = marker.color || 'rgba(0,0,0,0)';
}

Expand All@@ -271,24 +278,93 @@ function singlePointStyle(d, sel, trace, markerScale, lineScale, marker, markerL
});
}
else {
sel.style('stroke-width', lineWidth + 'px')
.call(Color.fill, fillColor);
sel.style('stroke-width', lineWidth + 'px');

var markerGradient = marker.gradient;

var gradientType = d.mgt;
if(gradientType) perPointGradient = true;
else gradientType = markerGradient && markerGradient.type;

if(gradientType && gradientType !== 'none') {
var gradientColor = d.mgc;
if(gradientColor) perPointGradient = true;
else gradientColor = markerGradient.color;

var gradientID = 'g' + gd._fullLayout._uid + '-' + trace.uid;
if(perPointGradient) gradientID += '-' + d.i;

sel.call(drawing.gradient, gd, gradientID, gradientType, fillColor, gradientColor);
}
else {
sel.call(Color.fill, fillColor);
}

if(lineWidth) {
sel.call(Color.stroke, lineColor);
}
}
}

drawing.singlePointStyle = function(d, sel, trace) {
var marker = trace.marker,
markerLine = marker.line;
var HORZGRADIENT = {x1: 1, x2: 0, y1: 0, y2: 0};
var VERTGRADIENT = {x1: 0, x2: 0, y1: 1, y2: 0};

// allow array marker and marker line colors to be
// scaled by given max and min to colorscales
var markerScale = drawing.tryColorscale(marker, ''),
lineScale = drawing.tryColorscale(marker, 'line');
drawing.gradient = function(sel, gd, gradientID, type, color1, color2) {
var gradient = gd._fullLayout._defs.select('.gradients')
.selectAll('#' + gradientID)
.data([type + color1 + color2], Lib.identity);

gradient.exit().remove();

gradient.enter()
.append(type === 'radial' ? 'radialGradient' : 'linearGradient')
.each(function() {
var el = d3.select(this);
if(type === 'horizontal') el.attr(HORZGRADIENT);
else if(type === 'vertical') el.attr(VERTGRADIENT);

el.attr('id', gradientID);

var tc1 = tinycolor(color1);
var tc2 = tinycolor(color2);

el.append('stop').attr({
offset: '0%',
'stop-color': Color.tinyRGB(tc2),
'stop-opacity': tc2.getAlpha()
});

el.append('stop').attr({
offset: '100%',
'stop-color': Color.tinyRGB(tc1),
'stop-opacity': tc1.getAlpha()
});
});

sel.style({
fill: 'url(#' + gradientID + ')',
'fill-opacity': null
});
};

/*
* Make the gradients container and clear out any previous gradients.
* We never collect all the gradients we need in one place,
* so we can't ever remove gradients that have stopped being useful,
* except all at once before a full redraw.
* The upside of this is arbitrary points can share gradient defs
*/
drawing.initGradients = function(gd) {

@etpinardetpinardApr 24, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice pattern. Controlling gradient defs in one place should help us in #581 too.

var gradientsGroup = gd._fullLayout._defs.selectAll('.gradients').data([0]);
gradientsGroup.enter().append('g').classed('gradients', true);

gradientsGroup.selectAll('linearGradient,radialGradient').remove();
};

drawing.singlePointStyle = function(d, sel, trace, markerScale, lineScale, gd) {
var marker = trace.marker;

singlePointStyle(d, sel, trace, markerScale, lineScale, marker, markerLine);
singlePointStyle(d, sel, trace, markerScale, lineScale, marker, marker.line, gd);

};

Expand All@@ -298,11 +374,12 @@ drawing.pointStyle = function(s, trace) {
// allow array marker and marker line colors to be
// scaled by given max and min to colorscales
var marker = trace.marker;
var markerScale = drawing.tryColorscale(marker, ''),
lineScale = drawing.tryColorscale(marker, 'line');
var markerScale = drawing.tryColorscale(marker, '');
var lineScale = drawing.tryColorscale(marker, 'line');
var gd = Lib.getPlotDiv(s.node());

@etpinardetpinardMay 3, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I wonder if it might be better to add a ref to those gradient defs in fullData[i] then to look for gd? Or even better refactor those Drawing method to expect fullLayout as argument.

@alexcjohnsonalexcjohnsonMay 3, 2017

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

I'll leave it for now - I agree this is suboptimal but neither of these alternatives seems particularly simpler to me. If I had to pick one I'd probably go for the first actually... stashing the <defs> in each trace could come in handy elsewhere too.


s.each(function(d) {
drawing.singlePointStyle(d, d3.select(this), trace, markerScale, lineScale);
drawing.singlePointStyle(d, d3.select(this), trace, markerScale, lineScale, gd);
});
};

Expand Down
3 changes: 3 additions & 0 deletions src/plot_api/plot_api.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -153,6 +153,9 @@ Plotly.plot = function(gd, data, layout, config) {
makePlotFramework(gd);
}

// clear gradient defs on each .plot call, because we know we'll loop through all traces
Drawing.initGradients(gd);

@etpinardetpinardApr 24, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It would be nice to add jasmine test checking that restyle can update gradient defs properly.


// save initial show spikes once per graph
if(graphWasEmpty) Plotly.Axes.saveShowSpikeInitial(gd);

Expand Down
9 changes: 9 additions & 0 deletions src/traces/scatter/arrays_to_calcdata.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -15,6 +15,9 @@ var Lib = require('../../lib');
// arrayOk attributes, merge them into calcdata array
module.exports = function arraysToCalcdata(cd, trace) {

// so each point knows which index it originally came from
for(var i = 0; i < cd.length; i++) cd[i].i = i;

@alexcjohnsonalexcjohnsonMay 3, 2017

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

@etpinard to my comment earlier today about not using calcdata as much: I needed to put this in because what I first tried to do, getting the index from the selection .each(d, i), is not reliable, as evidenced by the scattercarpet test image: the 0th point on the plot was actually index 1 from the trace (point 0 is cut off), but then the legend thought IT was point 0 so overwrote the other point 0 gradient.

BUT once we have that, we could get rid of the rest of these Lib.mergeArray statements - which, to address your concern earlier, don't do any sanitization anyway, we only do that for the axis data - and just directly access the arrays (if they are arrays) from within the style routines.

I'm not going to do that now, but could be a nice 🚀 at some point.

@etpinardetpinardMay 3, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good 👁️ here and nice fix. But, I think the solution would be to make Lib.mergeArray sanatize those arrays instead of 🔪 it. We can debate this in a future issue / PR 😄


Lib.mergeArray(trace.text, cd, 'tx');
Lib.mergeArray(trace.hovertext, cd, 'htx');
Lib.mergeArray(trace.customdata, cd, 'data');
Expand All@@ -37,5 +40,11 @@ module.exports = function arraysToCalcdata(cd, trace) {
Lib.mergeArray(markerLine.color, cd, 'mlc');
Lib.mergeArray(markerLine.width, cd, 'mlw');
}

var markerGradient = marker.gradient;
if(markerGradient && markerGradient.type !== 'none') {
Lib.mergeArray(markerGradient.type, cd, 'mgt');
Lib.mergeArray(markerGradient.color, cd, 'mgc');
}
}
};
24 changes: 23 additions & 1 deletion src/traces/scatter/attributes.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -311,7 +311,29 @@ module.exports = {
}
},
colorAttributes('marker.line')
)
),
gradient: {
type: {
valType: 'enumerated',
values: ['radial', 'horizontal', 'vertical', 'none'],
arrayOk: true,
dflt: 'none',
role: 'style',
description: [
'Sets the type of gradient used to fill the markers'
].join(' ')
},
color: {
valType: 'color',
arrayOk: true,
role: 'style',
description: [
'Sets the final color of the gradient fill:',
'the center color for radial, the right for horizontal,',
'or the bottom for vertical.',
].join(' ')
}
}
},
colorAttributes('marker')
),
Expand Down
2 changes: 1 addition & 1 deletion src/traces/scatter/defaults.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -50,7 +50,7 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
}

if(subTypes.hasMarkers(traceOut)) {
handleMarkerDefaults(traceIn, traceOut, defaultColor, layout, coerce);
handleMarkerDefaults(traceIn, traceOut, defaultColor, layout, coerce, {gradient: true});
}

if(subTypes.hasText(traceOut)) {
Expand Down
17 changes: 15 additions & 2 deletions src/traces/scatter/marker_defaults.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -15,12 +15,18 @@ var colorscaleDefaults = require('../../components/colorscale/defaults');

var subTypes = require('./subtypes');


/*
* opts: object of flags to control features not all marker users support

@etpinardetpinardApr 24, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for 🔒 ing this pattern. 🎉

* noLine: caller does not support marker lines
* gradient: caller supports gradients
*/
module.exports = function markerDefaults(traceIn, traceOut, defaultColor, layout, coerce, opts) {
var isBubble = subTypes.isBubble(traceIn),
lineColor = (traceIn.line || {}).color,
defaultMLC;

opts = opts || {};

// marker.color inherit from line.color (even if line.color is an array)
if(lineColor) defaultColor = lineColor;

Expand All@@ -33,7 +39,7 @@ module.exports = function markerDefaults(traceIn, traceOut, defaultColor, layout
colorscaleDefaults(traceIn, traceOut, layout, coerce, {prefix: 'marker.', cLetter: 'c'});
}

if(!(opts || {}).noLine) {
if(!opts.noLine) {
// if there's a line with a different color than the marker, use
// that line color as the default marker line color
// (except when it's an array)
Expand All@@ -57,4 +63,11 @@ module.exports = function markerDefaults(traceIn, traceOut, defaultColor, layout
coerce('marker.sizemin');
coerce('marker.sizemode');
}

if(opts.gradient) {
var gradientType = coerce('marker.gradient.type');
if(gradientType !== 'none') {
coerce('marker.gradient.color');
}
}
};
5 changes: 4 additions & 1 deletion src/traces/scatter/plot.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -427,11 +427,14 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition
.style('opacity', 1);
}

var markerScale = showMarkers && Drawing.tryColorscale(trace.marker, '');
var lineScale = showMarkers && Drawing.tryColorscale(trace.marker, 'line');

join.each(function(d) {
var el = d3.select(this);
var sel = transition(el);
Drawing.translatePoint(d, sel, xa, ya);
Drawing.singlePointStyle(d, sel, trace);
Drawing.singlePointStyle(d, sel, trace, markerScale, lineScale, gd);

if(trace.customdata) {
el.classed('plotly-customdata', d.data !== null && d.data !== undefined);
Expand Down
3 changes: 2 additions & 1 deletion src/traces/scattercarpet/attributes.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -107,7 +107,8 @@ module.exports = {
line: extendFlat({},
{width: scatterMarkerLineAttrs.width},
colorAttributes('marker'.line)
)
),
gradient: scatterMarkerAttrs.gradient

@alexcjohnsonalexcjohnsonApr 22, 2017

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

TODO: test image with carpet plots. Somehow imagetest isn't working for me yet with carpets, must be something I still need to update?

@rreusserrreusserApr 23, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

rm -rf node_modules?

@etpinardetpinardApr 24, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

another TODO: looks like this PR broke gl2d.

@rreusserrreusserApr 24, 2017•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you clarify which PR? Do you mean scattercarpet or this PR? Edit: realizing you probably mean these attributes need to be either added+implemented or properly avoided in gl2d.

@etpinardetpinardApr 24, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This as in #1620 You can calm down @rreusser 😄

@alexcjohnsonalexcjohnsonMay 2, 2017

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

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

scattercarpet image test in 105ce9b
gl2d fix still to come.

}, colorAttributes('marker'), {
showscale: scatterMarkerAttrs.showscale,
colorbar: colorbarAttrs
Expand Down
2 changes: 1 addition & 1 deletion src/traces/scattercarpet/defaults.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -62,7 +62,7 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
}

if(subTypes.hasMarkers(traceOut)) {
handleMarkerDefaults(traceIn, traceOut, defaultColor, layout, coerce);
handleMarkerDefaults(traceIn, traceOut, defaultColor, layout, coerce, {gradient: true});
}

if(subTypes.hasText(traceOut)) {
Expand Down
2 changes: 1 addition & 1 deletion src/traces/scattercarpet/plot.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -24,7 +24,7 @@ module.exports = function plot(gd, plotinfoproxy, data) {
plot: plotinfoproxy.plot
};

scatterPlot(plotinfo.graphDiv, plotinfo, data);
scatterPlot(gd, plotinfo, data);

for(i = 0; i < data.length; i++) {
trace = data[i][0].trace;
Expand Down
3 changes: 2 additions & 1 deletion src/traces/scattergeo/attributes.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -96,7 +96,8 @@ module.exports = {
line: extendFlat({},
{width: scatterMarkerLineAttrs.width},
colorAttributes('marker.line')
)
),
gradient: scatterMarkerAttrs.gradient
},
colorAttributes('marker')
),
Expand Down
2 changes: 1 addition & 1 deletion src/traces/scattergeo/defaults.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -41,7 +41,7 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
}

if(subTypes.hasMarkers(traceOut)) {
handleMarkerDefaults(traceIn, traceOut, defaultColor, layout, coerce);
handleMarkerDefaults(traceIn, traceOut, defaultColor, layout, coerce, {gradient: true});
}

if(subTypes.hasText(traceOut)) {
Expand Down
3 changes: 2 additions & 1 deletion src/traces/scatterternary/attributes.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -121,7 +121,8 @@ module.exports = {
line: extendFlat({},
{width: scatterMarkerLineAttrs.width},
colorAttributes('marker'.line)
)
),
gradient: scatterMarkerAttrs.gradient
}, colorAttributes('marker'), {
showscale: scatterMarkerAttrs.showscale,
colorbar: colorbarAttrs
Expand Down
2 changes: 1 addition & 1 deletion src/traces/scatterternary/defaults.js
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -75,7 +75,7 @@ module.exports = function supplyDefaults(traceIn, traceOut, defaultColor, layout
}

if(subTypes.hasMarkers(traceOut)) {
handleMarkerDefaults(traceIn, traceOut, defaultColor, layout, coerce);
handleMarkerDefaults(traceIn, traceOut, defaultColor, layout, coerce, {gradient: true});
}

if(subTypes.hasText(traceOut)) {
Expand Down
Binary file modifiedtest/image/baselines/bubblechart.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modifiedtest/image/baselines/geo_bg-color.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modifiedtest/image/baselines/scattercarpet.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modifiedtest/image/baselines/ternary_multiple.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
6 changes: 2 additions & 4 deletions test/image/mocks/autorange-tozero-rangemode.json
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -36,8 +36,7 @@
"name": "Revenue",
"xaxis": "x",
"yaxis": "y",
"type": "bar",
"uid": "33011d"
"type": "bar"
},
{
"x": [
Expand DownExpand Up@@ -76,8 +75,7 @@
"name": "Take Rate",
"xaxis": "x",
"yaxis": "y2",
"type": "scatter",
"uid": "31ba98"
"type": "scatter"
}
],
"layout": {
Expand Down
Loading