Qix-/color-convert

add useful constants / scales #78

oclyke posted onGitHub

Hello - thanks for your code!

I used 'color-convert' to create an array of colors sweeping through hues like so:

const elements = 16;
var colors = [...Array(elements)].map((_, index) => convert.hsv.rgb([360*(index/elements), 100, 100]))

However figuring out the values of 360 and 100 felt a tad hacky (even if 360 is a fairly well accepted range for hue - my first guess was 60 by glancing at the source code). I hail from a more embedded-oriented land and the fast HSV -> RGB conversion code that I admire provides constants to indicate what scale is used for the hue:

#define HSV_HUE_SEXTANT        256
#define HSV_HUE_STEPS        (6 * HSV_HUE_SEXTANT)

#define HSV_HUE_MIN        0
#define HSV_HUE_MAX        (HSV_HUE_STEPS - 1)
#define HSV_SAT_MIN        0
#define HSV_SAT_MAX        255
#define HSV_VAL_MIN        0
#define HSV_VAL_MAX        255

This keeps me from ever guessing what scale I should use.

I would definitely enjoy making a PR that adds some of these useful landmarks to the 'color-convert' landscape. They can serve not only in HSV but also RGB, HSL, and maybe more. With these kinds of additions we would enjoy re-writing my lines as:

const elements = 16;
var colors = [...Array(elements)].map((_, index) => convert.hsv.rgb([convert.hsv.h_scale*(index/elements), convert.hsv.s_scale, convert.hsv.v_scale]))

Naming of these constants could change of course if something else seems more appropriate. Maybe:

  • 'max'
  • 'scale'
  • 'unit'

So, is this something you are interested in? If so do you have a preference for how these constants are delivered to users?

Regards, O


IMO, that code is wildly less readable than reading the docs and using agreed upon values.

I understand the case for preprocessor defines since they are no-cost (using them enables your compiler to do constant folding) but that's unfortunately not the case in Javascript.

One woe I do have that I agree with you about: the 0..100-domain parameters should have been 0..1 in my opinion - however, that's now a compatibility thing (I wasn't the initial designer of this library).

Feel free to open a PR but I'm not keen on the idea, personally.

posted by Qix- about 5 years ago

Alright, thanks for letting me know.

Such a method does seem to add size to the package, but it would also provide a way for users to future-proof code. Say, for example, that one day the s and v channels should get set to the range [0, 1]. Users who rely on a named constant value would not have to worry.

Of course I don't expect this to change. The PR I submitted is an attempt to make the information more obvious in the documentation.

Cheers!

posted by oclyke about 5 years ago

Fund this Issue

$0.00
Funded

Pull requests