Centralise color scale function creation #1090
Conversation
- so that contour, heatmap and colorbar can call it to generate their color scale function - as the interpolation logic isn't trivial (in order to support rgba colorscale), reusing the same logic everywhere is a big +
- no more d3.scale.linear in heatmap, contour and all the colorbar modules! - this adds support for rgba colorscales in contour traces.
- the legend items of traces with colorscale are now properly shown.
|
regarding c3577f9, the legend items in the baselines were wrong. They are now on-par their
|
| if(opts.noNumericCheck || isNumeric(v)) { | ||
| var colorArray = _sclFunc(v); | ||
|
|
||
| if(opts.returnArray) return colorArray; |
There was a problem hiding this comment.
I guess returnArray is only valid if noNumericCheck===true - otherwise a non-numeric input would still return a color string. That's fine (assuming we don't use this), but we should probably throw an error if the unsupported combination is ever used.
There was a problem hiding this comment.
good point. thanks for the headsup!
| .range(range) | ||
| .clamp(true); | ||
|
|
||
| var sclFunc = function(v) { |
There was a problem hiding this comment.
🐎 It may not the slowest part of this, but since this is a hot path it might be worth pulling the opts logic out of this function, something like:
var sclNumeric = function(v) {
var colorArray = _sclFunc(v);
var colorObj = {
r: colorArray[0],
g: colorArray[1],
b: colorArray[2],
a: colorArray[3]
};
// may even be worthwhile to rewrite this one to avoid the creation
// of a tinycolor object, but that doesn't have to be now.
return tinycolor(colorObj).toRgbString();
};
if(opts.returnArray) {
// and noNumericCheck, but we should already have checked that this is true
return _sclFunc;
}
else if(opts.noNumericCheck) {
sclFunc = sclNumeric;
}
else {
sclFunc = function(v) {
if(isNumeric(v)) return sclNumeric(v);
else if(tinycolor(v).isValid()) return v;
else return Color.defaultLine;
};
}
// then your domain & range patches and returnThere was a problem hiding this comment.
It may not the slowest part of this,
Yep, that's what I thought too. But you're right, 🐎 always 🐎
| markerLine = marker.line || {}, | ||
| zmin = trace.zmin, | ||
| zmax = trace.zmax, | ||
| scl = getColorscale(trace.colorscale), |
There was a problem hiding this comment.
Ah good catch, we've already done this at the supplyDefaults step so no need to repeat it anywhere else.
There was a problem hiding this comment.
yep, so many getColorscale were 🔪 in this PR.
| @@ -1,6 +1,523 @@ | |||
| { | |||
| "data": [ | |||
| { | |||
| "type": "contour", | |||
There was a problem hiding this comment.
might want to change the title to match what's actually being tested in this image (scatter & contour colorscale opacity)
| * @return {function} | ||
| * | ||
| */ | ||
| module.exports = function makeScaleFunction(scl, cmin, cmax, opts) { |
There was a problem hiding this comment.
this signature is a bit funny, especially when you have calls like
makeScaleFunction(scl, null, null, {domain: domain, range: range})
where scl gets ignored entirely. Maybe better to just put everything into opts?
- options object now includes cmin / cmax - now respect all noNumericCheck + returnArray combination - small perf boost in noNumericCheck version
- pass noNumericCheck option in contour / heatmap (where a clean-z-data pass is already done) and in all the colorbar module (which have clean data)
- into extractScale and makeColorScaleFn
| domain: domain, | ||
| range: range | ||
| }; | ||
| }; |
There was a problem hiding this comment.
OK sure - we could imagine having this one also include opts and call the other one, since I can't see us ever using this without wrapping it in makeColorScaleFunc... but then naming gets confusing. meh, lets leave it as you have it here.
|
💃 nice consolidation! |
fixes #1064
Adding support for rgba colorscales (thus fixing #1064) was easy. It was simply a matter of porting that patch made in #422 for scatter traces in the contour module.
But, by doing I realised that our color scale function creation code wasn't very dry. So, this PR generalise the colorbar component
makeScaleFunctionroutine so that all trace plot and colorbar module that need a color scale can use it.