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

scatter marker color gradients #1620

Show file tree
Hide file tree
Changes from 1 commit
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
46 changes: 31 additions & 15 deletions src/components/drawing/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ drawing.symbolNumber = function(v) {
return Math.floor(Math.max(v, 0));
};

function singlePointStyle(d, sel, trace, markerScale, lineScale, marker, markerLine, gd) {
function singlePointStyle(d, sel, trace, markerScale, lineScale, marker, markerLine, gd, i) {
// only scatter & box plots get marker path and opacity
// bars, histograms don't
if(Registry.traceIs(trace, 'symbols')) {
Expand Down Expand Up @@ -238,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 @@ -257,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 @@ -275,10 +281,20 @@ function singlePointStyle(d, sel, trace, markerScale, lineScale, marker, markerL
sel.style('stroke-width', lineWidth + 'px');

var markerGradient = marker.gradient;
var gradientColor = markerGradient && (d.mgc || markerGradient.color);
var gradientType = markerGradient && (d.mgt || markerGradient.type);

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

if(gradientType && gradientType !== 'none') {
sel.call(drawing.gradient, gd, gradientType, fillColor, gradientColor);
var gradientColor = d.mgc;
if(gradientColor) perPointGradient = true;
else gradientColor = markerGradient.color;

var gradientID = 'g' + gd._fullLayout._uid + '-' + trace.uid;
if(perPointGradient) gradientID += '-' + i;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Now we're making either one gradient for the whole trace (if we can) or one per point (if any of the attributes varies per point) and it will update if the attributes change. This means that several traces (or several points in one trace with array attributes) will no longer be able to share a gradient def, but I don't think there are any cases where this can blow up.

Copy link
Contributor

Choose a reason for hiding this comment

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

That sounds right 👍


sel.call(drawing.gradient, gd, gradientID, gradientType, fillColor, gradientColor);
}
else {
sel.call(Color.fill, fillColor);
Expand All @@ -293,12 +309,12 @@ function singlePointStyle(d, sel, trace, markerScale, lineScale, marker, markerL
var HORZGRADIENT = {x1: 1, x2: 0, y1: 0, y2: 0};
var VERTGRADIENT = {x1: 0, x2: 0, y1: 1, y2: 0};

drawing.gradient = function(sel, gd, type, color1, color2) {
var fullLayout = gd._fullLayout;
var gradientID = (
type + fullLayout._uid + '-' + color1 + '-' + color2
).replace(/[^\w\-]+/g, '_');
var gradient = fullLayout._defs.select('.gradients').selectAll('#' + gradientID).data([0]);
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()
Copy link
Contributor

Choose a reason for hiding this comment

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

I suspect this algo would work for adding gradients to bar traces? Would be a cool feature 😏

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep, would be cool to add bar gradients... for another PR

.append(type === 'radial' ? 'radialGradient' : 'linearGradient')
Expand Down Expand Up @@ -345,10 +361,10 @@ drawing.initGradients = function(gd) {
gradientsGroup.selectAll('linearGradient,radialGradient').remove();
};

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

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

};

Expand All @@ -362,8 +378,8 @@ drawing.pointStyle = function(s, trace) {
var lineScale = drawing.tryColorscale(marker, 'line');
var gd = Lib.getPlotDiv(s.node());
Copy link
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.

Copy link
Collaborator Author

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, gd);
s.each(function(d, i) {
drawing.singlePointStyle(d, d3.select(this), trace, markerScale, lineScale, gd, i);
});
};

Expand Down
4 changes: 2 additions & 2 deletions src/traces/scatter/plot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -430,11 +430,11 @@ function plotOne(gd, idx, plotinfo, cdscatter, cdscatterAll, element, transition
var markerScale = showMarkers && Drawing.tryColorscale(trace.marker, '');
var lineScale = showMarkers && Drawing.tryColorscale(trace.marker, 'line');

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

if(trace.customdata) {
el.classed('plotly-customdata', d.data !== null && d.data !== undefined);
Expand Down
75 changes: 48 additions & 27 deletions test/jasmine/tests/drawing_test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,20 +325,31 @@ describe('gradients', function() {

afterEach(destroyGraphDiv);

function checkGradientIds(uid, specs) {
var sortedExpected = specs.map(function(spec) {
return (spec[0] + uid + '-' + spec[1] + '-' + spec[2]).replace(/[^\w\-]+/g, '_');
}).sort();
function checkGradientIds(ids, types, c1, c2) {
var expected = ids.map(function(id) {
return 'g' + gd._fullLayout._uid + '-' + gd._fullData[0].uid + id;
});

var gids = [];
var typesOut = [];
var c1Out = [];
var c2Out = [];
var gradients = d3.select(gd).selectAll('radialGradient,linearGradient');
gradients.each(function() { gids.push(this.id); });
gradients.each(function() {
gids.push(this.id);
typesOut.push(this.nodeName.replace('Gradient', ''));
c1Out.push(d3.select(this).select('stop[offset="100%"]').attr('stop-color'));
c2Out.push(d3.select(this).select('stop[offset="0%"]').attr('stop-color'));
});
gids.sort();

expect(gids.length).toBe(sortedExpected.length);
expect(gids.length).toBe(expected.length);

for(var i = 0; i < Math.min(gids.length, sortedExpected.length); i++) {
expect(gids[i]).toBe(sortedExpected[i]);
for(var i = 0; i < Math.min(gids.length, expected.length); i++) {
expect(gids[i]).toBe(expected[i]);
expect(typesOut[i]).toBe(types[i]);
expect(c1Out[i]).toBe(c1[i]);
expect(c2Out[i]).toBe(c2[i]);
}
}

Expand All @@ -355,41 +366,51 @@ describe('gradients', function() {
}
}])
.then(function() {
checkGradientIds(gd._fullLayout._uid, [
['radial', '#123', '#fff'],
['radial', '#123', '#eee'],
['radial', '#123', '#ddd']
]);
checkGradientIds(
['-0', '-1', '-2'],
['radial', 'radial', 'radial'],
['rgb(17, 34, 51)', 'rgb(17, 34, 51)', 'rgb(17, 34, 51)'],
['rgb(255, 255, 255)', 'rgb(238, 238, 238)', 'rgb(221, 221, 221)']);

return Plotly.restyle(gd, {'marker.color': '#456'});
})
.then(function() {
// simple scalar restyle doesn't trigger a full replot, so
// doesn't clear the old gradients
checkGradientIds(gd._fullLayout._uid, [
['radial', '#123', '#fff'],
['radial', '#123', '#eee'],
['radial', '#123', '#ddd'],
['radial', '#456', '#fff'],
['radial', '#456', '#eee'],
['radial', '#456', '#ddd']
]);
checkGradientIds(
['-0', '-1', '-2'],
['radial', 'radial', 'radial'],
['rgb(68, 85, 102)', 'rgb(68, 85, 102)', 'rgb(68, 85, 102)'],
['rgb(255, 255, 255)', 'rgb(238, 238, 238)', 'rgb(221, 221, 221)']);

return Plotly.restyle(gd, {'marker.gradient.type': [['horizontal', 'vertical', 'radial']]});
})
.then(function() {
// array restyle does replot
checkGradientIds(gd._fullLayout._uid, [
['horizontal', '#456', '#fff'],
['vertical', '#456', '#eee'],
['radial', '#456', '#ddd']
]);
checkGradientIds(
['-0', '-1', '-2'],
['linear', 'linear', 'radial'],
['rgb(68, 85, 102)', 'rgb(68, 85, 102)', 'rgb(68, 85, 102)'],
['rgb(255, 255, 255)', 'rgb(238, 238, 238)', 'rgb(221, 221, 221)']);

return Plotly.restyle(gd, {
'marker.gradient.type': 'vertical',
'marker.gradient.color': '#abc'
});
})
.then(function() {
// down to a single gradient because they're all the same
checkGradientIds(
[''],
['linear'],
['rgb(68, 85, 102)'],
['rgb(170, 187, 204)']);

return Plotly.restyle(gd, {'mode': 'lines'});
})
.then(function() {
// full replot and no resulting markers at all -> no gradients
checkGradientIds(gd._fullLayout._uid, []);
checkGradientIds([], [], [], []);
})
.catch(fail)
.then(done);
Expand Down