Add colormap option to replace surfaceColors

[coldfix]

Hi,

first, this fixes a few remaining issues with the surfaceColors implementation.

Next, I've wanted to have an option that allows specifying a function that maps a value between 0 and 1 to an RGB value. This makes it possible to e.g. plug in colormaps from chroma.js or use any custom user logic.

I believe it is probably best not to reuse surfaceColors for this, but replace it with a colormap because

• the name surfaceColors implies it works only with surface plots, but I think the colormap should be usable for all plot styles
• the surfaceColors option previously worked only for surface plots, i.e. if we would change it to work with other styles, this might be a (minor) break in backward compatibility
• colormap is the more commonly used name
• the surfaceColors option takes colors in the order from vMax to vMin, whereas I think that it is more intuitive to pass colors in ascending order

If you disagree, it is of course trivial to just add this capability to the existing surfaceColors option.

[Thomaash]

Do you have any examples how to use the function version of colormap?

[coldfix]

Hi, thanks for taking the time to review this.

Do you have any examples how to use the function version of colormap?

let viridis = chroma.scale('viridis');
options.colormap = function(x) {
    let [r, g, b] = viridis(x);
    return {r, g, b};
}

// or e.g.: scale in saturation instead of hue:
options.colormap = function(x) {
  return vis.util.HSVToRGB(200, x, 1);
}

it's explained in the docs, but I could add an example somewhere..

[Thomaash]

Seems to work well, there are still some detail to polish though.

[coldfix]

Btw: while doing this PR, I initially contemplated removing the options.colormap = {hue: {start, end, ...}} form, and replacing it with more generic utilities. These changes can be seen here. However, I think it is probably rather over-engineered relative to the very limited feature set it provides, and so I concluded that colormap utilities are probably better left to dedicated libraries such as chromajs which should be enough as long as the user has the ability to pass a function. What do you think?

[Thomaash]

Some more minor issues in the code.

Also input validation:

options.colormap = {
  hue: {
    start: +"ř",
    end: +"ř",
    saturation: +"ř",
    brightness: +"ř",
    colorStops: +"ř" // How many colour gradients do we want
  }
};

crashes with very unhelpful error message.

With proper inputs it works great, though.

[coldfix]

Some more minor issues in the code.

Also input validation:

options.colormap = {
  hue: {
    start: +"ř",
    end: +"ř",
    saturation: +"ř",
    brightness: +"ř",
    colorStops: +"ř" // How many colour gradients do we want
  }
};

crashes with very unhelpful error message.

With proper inputs it works great, though.

Keep in mind, that I didn't change anything about this syntax... If you want to have better input validation for that, I think it should be done in a separate PR (and I won't do it, because I disagree that it is useful, and I would remove this syntax anyway).

[Thomaash]

Some more minor issues in the code.
Also input validation:

options.colormap = {
  hue: {
    start: +"ř",
    end: +"ř",
    saturation: +"ř",
    brightness: +"ř",
    colorStops: +"ř" // How many colour gradients do we want
  }
};

crashes with very unhelpful error message.
With proper inputs it works great, though.

Keep in mind, that I didn't change anything about this syntax... If you want to have better input validation for that, I think it should be done in a separate PR (and I won't do it, because I disagree that it is useful, and I would remove this syntax anyway).

You had to change something because it doesn't crash upstream. Also, having an option to simply use a range of colors seems really handy to me. What's wrong with that?

[coldfix]

You had to change something because it doesn't crash upstream.

I changed the check

-  if (hues.saturation < 0 || hues.saturation > 100) {
+  if (!(hues.saturation >= 0 && hues.saturation <= 100)) {
     throw new Error('Saturation is out of bounds. Expected range is 0-100.');
   }

by your request. And yes, it is an improvement over the old version. Now you get at least an error that informs you that the variable is out of bounds. Previously it failed quietly without any error message (but of course still didn't work).

Also, having an option to simply use a range of colors seems really handy to me. What's wrong with that?

I'm not argueing against the list-style option that allows specifying a list of colours to be interpolated. I think that's pretty useful. I'm arguing against the {hue: {start: ..., end: ..., saturation: ...}} form of the option. I think it's overly specific. and has a weird format. Why doesn't it allow start/end for saturation and value? It'd make more sense to me to just pass a list of {h, s, v} colors to be interpolated between, and let the user specify whether to interpolate in HSV or RGB space. Then we could unify this form with just passing list of colors and a specification on how to interpolate.

Take a quick look at the changes in the colormodule branch I mentioned before. It includes all the tools to let the user do exactly that.

[coldfix]

Also, I'm personally not a big fan of too much input validation (in non-security critical code of course). In my experience this often prevents uses that would actually make sense but were not anticipated by the developer. I mean if the user wants, they can always break it. My opinion is, let them have the freedom to do that, but when they're doing something that's not documented, they're on their own with that (consenting adults...).

(Edit: I'm of course referring only to arguments that are passed in by library users (i.e. other developers) here, not to arguments passed in directly from the end user)

[Thomaash]

Okay, I see your point now. Do you plan to get the colormap to production too? Anyway thank you very much.

[vis-bot]

🎉 This PR is included in version 5.8.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[coldfix]

Okay, I see your point now. Do you plan to get the colormap to production too?

Not sure.. While I think that branch has structural improvements, it currently doesn't provide an good user-level API with thought-through names. For now I'm happy with being able to specify a function and personally use that to plug in chroma.js.

Anyway thank you very much.

Thanks as well!