Skip to content
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.

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

Add color spaces for Luv, LCHuv, HSLuv and HPLuv. #339

Closed
wants to merge 1 commit into from

Conversation

lloydk
Copy link
Collaborator

@lloydk lloydk commented Oct 19, 2023

This is a draft PR for adding the Luv, LCHuv, HSLuv and HPLuv color spaces.

It's probably to big for single commit so I plan on breaking it up into four smaller pull requests after it has been reviewed:

  1. Allow color spaces that are parsed and serialized using the color() function to have a coord grammer.
  2. Allow color spaces to specify which color space to use when checking to see if a color is inGamut.
  3. Luv and LCHuv color spaces
  4. HSLuv and HPLuv color spaces

The Javascript reference implementation for HSLuv and HPLuv can be found here. The HSLuv and HPLuv color spaces are based on the reference implementation so I've add the license file from that repository to the source code for those two color spaces. I'm assuming that's ok.

Gamut Checking

I wasn't sure how to do gamut checking for HSLuv and HPLuv. The ColorSpace inGamut function uses the base space if the current space is a polar space. This doesn't work for HSLuv and HPLuv because the base space has a larger gamut.

I added a new property to the ColorSpace class called gamutCheck which is either a color space that should be used for gamut checking (sRGB in the case of HSLuv) or "self" which means use the current color space for gamut checking. HPLuv uses "self" because it has a gamut that is a subset of sRGB.

@Myndex
Copy link
Contributor

Myndex commented Oct 19, 2023

Hi @lloydk

... how to do gamut checking for HSLuv and HPLuv..

HSLuv and HPLuv are both clamped to sRGB, which is a thing that is useful-but they need a different set of constraints for other spaces. So, for sRGB, no gamut check should be required—but if it's to be used for P3, then the m_r0.... numbers need to be recalculated, the method using Maxima is in the math folder.

HSLuv allows for saturated colors, and therefore the saturation correlate is relative.

HPLuv is clamped so that the saturation correlate is absolute (aka intended to be orthogonally independent) but as a result the clamp only allows pastel, desaturated colors.

The hue is pretty stable and orthogonally independent.
The lightness is except for the lack of providing for HK and other contextual effects.

CIE

Luv*, LChuv, Lshuv on the other hand are essentially unbounded, and therefore definitely do need the gamut check. LChuv is particularly a nuisance because C is all over the place relative to various hue settings—max C for red is ~179, but that is far out of gamut for all other hues.

@LeaVerou
Copy link
Member

LeaVerou commented Oct 19, 2023

This is a draft PR for adding the Luv, LCHuv, HSLuv and HPLuv

Wow, thanks for this amazing work!! 😍

It's probably to big for single commit so I plan on breaking it up into four smaller pull requests after it has been reviewed:

The thing is, the bigger the PR, the harder it is to review it, so that’s a bit cyclical 😕
I know it's extra work, but is there a chance you might consider breaking it up before review? 🙏🏼

  1. Allow color spaces that are parsed and serialized using the color() function to have a coord grammar.

Hmm, the issue is in the spec only predefined color() spaces have a grammar. Not sure if serializing these as color() or as a custom function would be more compatible. @svgeesus , thoughts?

@svgeesus now I even wonder if @color-profile should even allow specifying syntax for the coordinates.

The Javascript reference implementation for HSLuv and HPLuv can be found here. The HSLuv and HPLuv color spaces are based on the reference implementation so I've add the license file from that repository to the source code for those two color spaces. I'm assuming that's ok.

Of course!

I wasn't sure how to do gamut checking for HSLuv and HPLuv. The ColorSpace inGamut function uses the base space if the current space is a polar space. This doesn't work for HSLuv and HPLuv because the base space has a larger gamut.

I added a new property to the ColorSpace class called gamutCheck which is either a color space that should be used for gamut checking (sRGB in the case of HSLuv) or "self" which means use the current color space for gamut checking. HPLuv uses "self" because it has a gamut that is a subset of sRGB.

Note that since color spaces are just instances of ColorSpace, they could define their own inGamut() class (@MysteryBlokHed @jgerigmeyer would that mess with TS?).

If we introduce a new property to make this simpler, I'd rather not have something that's either a keyword or a ColorSpace object, that’s a bit weird. I propose:

  • gamutSpace: ColorSpace? property that can be part of the color space config.
  • gamutSpace getter on ColorSpace which returns the passed gamutSpace if set, or this.base / null accordingly if a gamutSpace has not been passed.
  • inGamut modified to use gamutSpace if not null, rather than the ad hoc logic around polar spaces that it has now.

@lloydk
Copy link
Collaborator Author

lloydk commented Oct 19, 2023

The thing is, the bigger the PR, the harder it is to review it, so that’s a bit cyclical 😕 I know it's extra work, but is there a chance you might consider breaking it up before review? 🙏🏼

Instead of review I probably should have said discussion. I mostly wanted to show how the changes for points 1 & 2 are being used by the new color spaces. Consider a thorough review as optional at this point.

@lloydk
Copy link
Collaborator Author

lloydk commented Oct 19, 2023

If we introduce a new property to make this simpler, I'd rather not have something that's either a keyword or a ColorSpace object, that’s a bit weird. I propose:

  • gamutSpace: ColorSpace? property that can be part of the color space config.
  • gamutSpace getter on ColorSpace which returns the passed gamutSpace if set, or this.base / null accordingly if a gamutSpace has not been passed.
  • inGamut modified to use gamutSpace if not null, rather than the ad hoc logic around polar spaces that it has now.

How about:

  • gamutSpace: ColorSpace?
  • no getter.
  • Polar spaces that want to use the base space set gamutSpace to the base space. This would require changes to existing spaces.
  • Spaces that want to use some other space for gamut checking would set the appropriate space
  • inGamut modified to use gamutSpace if not null otherwise use the space we're in.

@LeaVerou
Copy link
Member

How about:

  • gamutSpace: ColorSpace?
  • no getter.
  • Polar spaces that want to use the base space set gamutSpace to the base space. This would require changes to existing spaces.
  • Spaces that want to use some other space for gamut checking would set the appropriate space
  • inGamut modified to use gamutSpace if not null otherwise use the space we're in.

So as you point out this requires wider changes across more spaces, and adds to the burden of specifying a polar space. I'm missing a discussion on the advantages of this alternative approach?

@lloydk
Copy link
Collaborator Author

lloydk commented Oct 19, 2023

So as you point out this requires wider changes across more spaces, and adds to the burden of specifying a polar space. I'm missing a discussion on the advantages of this alternative approach?

  • gamutSpace getter on ColorSpace which returns the passed gamutSpace if set, or this.base / null accordingly if a gamutSpace has not been passed.

By accordingly I'm assuming you mean isPolar. If that's the case then I don't think we have enough information to handle the HPLuv color space which is polar and will have a null gamutSpace because it's a subset of sRGB. So it's not clear to me how the getter would be implemented.

I'll try your proposed changes to see if I can make it work.

@svgeesus
Copy link
Member

So, lets first examine the goals and desirability of adding these spaces.

CIE LUV has the advantage over CIE Lab that it provides a 2D chromaticity space, which CIE Lab does not, and that the uv chromaticity diagram is less bad than the older xy chromaticity diagram. But both are bad, they are not at all perceptually uniform, and since 1976 computers have become a bit more advanced so we don't need to calculate light mixtures on a printed chart and measuring with a ruler; we can calculate them in 3D in either a linear-light space or a perceptually uniform space depending on what problem we are solving.

CIE LUV has all the disadvantages of CIE Lab in terms of radial hue uniformity, chroma stretching for higher chroma colors, and hue curvature. It also has an additional disadvantage, in that the results of chromatic adaptation in LUV are especially bad, often predicting non-physical colors. There are lots of better ways to predict corresponding colors nowadays.

None the less, CIE LUV is, or was, fairly widely used and we already have utility functions to calculate uv chromaticity coordinates so we should probably add it. For one thing, if using a dataset where the colors are given in LUV or in Yuv, it would make transforming to XYZ and thence to some other color space easier.

Moving on to CIE LCHuv - this has all the disadvantages of LCHab, and then some, which is why it is rarely used in practice nowadays. It might be worthwhile to add it but frankly I am in no rush.

Both CIE LUV and CIE LCHuv have no gamut restrictions.

HSL (and HSV, and HWB) are terrible formulations that promise (but do not deliver) usability benefits and perceptual uniformity. They are implemented in color.js solely because HSL is widely used in practice, notably in CSS preprocessors such as SASS, and they are part of CSS Color. There is usually a better way to solve a given problem than using HSL.

HSL used to be restricted to the sRGB gamut but in CSS Color we changed that, relatively recently, avoiding a gamut mapping step to prioritize lossless round-tripping of colors outside the sRGB gamut.

HSLuv is an unholy union of CEI LUV and HSL, combining the disadvantages of both by adding a non-perceptual saturation to LCHuv, just so that it forms a square shape which is convenient for color pickers. It is also restricted to the sRGB gamut and thus a strange anachronism in a world where the major RGB color spaces are Display P3 and BT.2020/2100.

I would want to see some more justification for adding this, especially if doing so involves refactoring other color spaces.

@LeaVerou
Copy link
Member

So as you point out this requires wider changes across more spaces, and adds to the burden of specifying a polar space. I'm missing a discussion on the advantages of this alternative approach?

  • gamutSpace getter on ColorSpace which returns the passed gamutSpace if set, or this.base / null accordingly if a gamutSpace has not been passed.

By accordingly I'm assuming you mean isPolar. If that's the case then I don't think we have enough information to handle the HPLuv color space which is polar and will have a null gamutSpace because it's a subset of sRGB. So it's not clear to me how the getter would be implemented.

get gamutSpace() {
	return this.isPolar ? this.base : this;
}

But if a gamutSpace config option is passed, it shoild override the getter entirely so that this code doesn't run at all.

So HPLuv can just provide gamutSpace: sRGB when it's registered and it should just work.

@svgeesus
Copy link
Member

@LeaVerou wrote:

Not sure if serializing these as color() or as a custom function would be more compatible. @svgeesus , thoughts?

Using a custom color function and thus, a dashed ident as the colorspace name, is probably the way to go.

@svgeesus now I even wonder if @color-profile should even allow specifying syntax for the coordinates.

I have wondered that for a while, currently in CSS Color 5 pointing to an ICC profile is the only way to add an implementation of a custom color space which is certainly needed for CMYK and suchlike spaces; for additive RGB spaces it is needless overhead and what is needed is primary and white chromaticities plus a transfer curve (EOTF) formula. We might prototype that in color.js and it might even make it's way into CSS Color or the Color API as an extensibility point.

@svgeesus
Copy link
Member

It doesn't help that the website for HSLuv not only uses the term "RGB" to mean sRGB but also requires that the conversion function names do so as well:

Every implementation of HSLuv must provide the following four functions:

hsluvToRgb([H, S, L]) -> [R, G, B]
hpluvToRgb([H, S, L]) -> [R, G, B]
rgbToHsluv([R, G, B]) -> [H, S, L]
rgbToHpluv([R, G, B]) -> [H, S, L]

@lloydk
Copy link
Collaborator Author

lloydk commented Oct 19, 2023

I would want to see some more justification for adding this, especially if doing so involves refactoring other color spaces.

I think that's a fair question to ask. I'm fine with maintaining these color spaces in my fork.

@facelessuser
Copy link
Collaborator

It doesn't help that the website for HSLuv not only uses the term "RGB" to mean sRGB but also requires that the conversion function names do so as well:

Every implementation of HSLuv must provide the following four functions:

hsluvToRgb([H, S, L]) -> [R, G, B] hpluvToRgb([H, S, L]) -> [R, G, B] rgbToHsluv([R, G, B]) -> [H, S, L] rgbToHpluv([R, G, B]) -> [H, S, L]

I wonder how literal this is meant...to have these exact functions and names as such. As an implementer of these spaces in my library, if it is super literal, I'm in violation 🙃. It doesn't fit into my API at all, though the spirit of it is there, the ability to convert from these spaces to sRGB and back.

I don't see these requirments explicitly stated in the license, but yeah, it does say this on their website. I guess if the author hunts me down and says I must have these exact functions with these exact signatures, I guess I could reconsider and add functions that no one will ever use, or just remove HSLuv and HPLuv 🤷🏻.

With all of that said, I can definitely see the argument that as we move into a wider gamut world that many of these HSL-ish spaces seem less useful. Though, I can also understand that there will always be applications stuck with sRGB and people wanting something more perceptually uniform, even if those solutions are imperfect. Regardless, it doesn't mean every color library is obligated to directly support and maintain them.

@LeaVerou
Copy link
Member

LeaVerou commented Oct 20, 2023

@svgeesus The only reason I can think of for not adding a color space is that it's not popular enough. These color spaces are very popular, and their lacking in color.js has been noticed by many. Color.js is a tool where we can experiment with every popular color space, and has plenty of other spaces we don't plan to add natively to CSS. It's useful for comparisons, as well as to see what needs different color spaces have so that the Color API is flexible enough. It's the same reason why you implemented a bunch of contrast algorithms, some of which are very clearly suboptimal: so we can use them in comparisons. 😊

@lloydk
Copy link
Collaborator Author

lloydk commented Oct 20, 2023

So as you point out this requires wider changes across more spaces, and adds to the burden of specifying a polar space. I'm missing a discussion on the advantages of this alternative approach?

There are currently six color spaces that are polar:

  • hsl
  • hsv
  • hwb
  • jzczhz
  • lch
  • oklch

Only three of those spaces (hsl, hsv, hwb) have a base space with coordinates that have range values. The inGamut method only checks coordinates with range values. For the other spaces (jzczhz, lch and oklch) the fact that they're polar is irrelevant as they don't have base spaces with coordinates that have range values so the base space inGamut check will always return true.

Of the three new polar spaces in this PR none of them would do any gamut checks using the base space:

  • lchuv has a base space with no range values
  • hsluv would have a gamutSpace of srgb
  • hpluv is a subset of srgb and has range values so it would check itself

I don't think it's a burden when defining polar spaces. It's extra line for new polar spaces that have a base space with range values that aren't gamut checked in some other space. For the three new spaces in this PR there is no burden as none of the spaces are gamut checked in the base space. Same for OKhsl and OKhsv should those spaces get added. If there are other unbounded polar spaces that get added there would be no extra work to specify those spaces.

Only three of the existing color spaces would need to specify a gamutSpace (hsl, hsv, hwb).

I think checking isPolar to determine if the base space should be used to gamut check is misleading and explicitly specifying the gamutSpace for those polar spaces that need to do gamut checking in their base space makes the code more understandable.

@facelessuser
Copy link
Collaborator

As someone who has implemented both HSLuv and HPLuv, the only way to provide a default gamut check/gamut map for HPLuv is to have its default be itself, so if HPLuv is meant to be in color.js, there would have to be a way for it to do that if no gamut space is provided as the base conversion space is simply not sufficient for a default gamut mapping.

I personally agree that it would be cleaner to use something like gamutSpace to explicitly specify a needed gamut space. If nothing is defined, the default gamut space is itself, if something is defined, then gamutSpace is used instead. It doesn't even have to be polar-specific even if that is likely the only way it will be used. It makes things more explicit and less magical.

@LeaVerou
Copy link
Member

LeaVerou commented Oct 20, 2023

@lloydk

There are currently six color spaces that are polar:

  • hsl
  • hsv
  • hwb
  • jzczhz
  • lch
  • oklch

Only three of those spaces (hsl, hsv, hwb) have a base space with coordinates that have range values. The inGamut method only checks coordinates with range values. For the other spaces (jzczhz, lch and oklch) the fact that they're polar is irrelevant as they don't have base spaces with coordinates that have range values so the base space inGamut check will always return true.

The current heuristic works across all six of them though 😄 For the first three because they have a base space with coordinate ranges, and for the last three trivially, because they have no gamut, and their base space has no ranges. The base space always returns true, because that's the correct answer!

I think checking isPolar to determine if the base space should be used to gamut check is misleading and explicitly specifying the gamutSpace for those polar spaces that need to do gamut checking in their base space makes the code more understandable.

I think there may be a misunderstanding here. The design I proposed only checks isPolar if no gamutSpace is specified. It's a smart default, not an alternative to gamutSpace. I agree we need gamutSpace, and if specified, it should always take precedence. But there needs to be a default, because we cannot guarantee gamutSpace being there (nor would we want to make it mandatory). So what would be a better default? Using this, even when it's polar? That covers zero use cases, since we never check gamut with polar coordinates, whereas my proposed approach requires zero changes to existing spaces and covers ~2/3 of use cases of polar spaces out of the box, without requiring a (redundant) gamutSpace.

Reading @facelessuser's comment I'm even more inclined to think that you both misunderstood what I was proposing. Hopefully now that I explained it better it's clearer?

@facelessuser
Copy link
Collaborator

It is likely I misunderstood and didn't read close enough.

@LeaVerou
Copy link
Member

Actually, it's possible I did! I think I just realized what you were saying @facelessuser: there is no way to override the smart default if the gamut space is the space itself, but the space is also polar, because we don't have a reference to the space until after it's created, so nothing to pass to gamutSpace. Essentially a chicken and egg problem.

Hmm hmm. So the options are:

  1. @lloydk’s original idea of having a special "self" value that resolves to this internally
  2. Ditching the isPolar heuristic when no gamutSpace is provided
  3. Dropping the getter idea (or using a different name) so that authors can just do:
let foo = new ColorSpace(...);
foo.gamutSpace = foo;

I think out of those, (1) is the least bad. Not sure if there are any other options?

@Myndex
Copy link
Contributor

Myndex commented Oct 20, 2023

As gently as possible (that's my vain attempt to not be controversial):

...we can calculate them in 3D in either a linear-light space or a perceptually uniform...

Absolutely agree, I also agree that LChuv has a wacky-way-out Chroma correlate. And we have much better color spaces now, and fairly mature LUT pipelines, so does LUV/LChuv/Lshuv have relevance? I'll suggest it does for certain applications.

...LUV has all the disadvantages of CIE Lab in terms of radial hue uniformity...

Lshuv is markedly better in hue distribution and freedom from the blue/purple shift.

image

CIELUV is substantially more uniform in hue than CIELAB. This comparison is using polar on the top row, cartisian on the bottom row.

Screen Shot 2021-09-07 at 6 47 18 AM

Top row second from left is LUV polar with the saturation instead of chroma . Lshuv with sat correlate substantially tames the chroma bronco-bull.

There are modifications that can be applied to reign in some issues. E.g. I don't think L* is the right correlate for lightness to build everything around either, and accounts for a healthy chunk of LUV's MacAdams ellipse issues. L* is Munsell value, created around diffuse reflecting tiles in a defined env.

...results of chromatic adaptation in LUV are especially bad...

LUV should never be used for chromatic adaptation, it's not fit for that. As it happens, the things LUV is good for should not require that: start and end in D65, and working space should be the destination space when practical.

...HSL (and HSV, and HWB) are terrible formulations...

No disagreement here... though I don't think @lloydk is doing anything with those?

...HSL used to be restricted to the sRGB gamut but in CSS Color we changed that, relatively recently,...

Probably inevitable... is there a flag/meta for the color space?

...HSLuv is an unholy union of CEI LUV and HSL, combining the disadvantages of both by adding a non-perceptual saturation to LCHuv, just so that it forms a square shape which is convenient for color pickers...

Well, the main reason to employ HSLuv (or Lshuv), is for the purpose of picking colors, and making gradients, etc. In this role, it performs well. It's computationally inexpensive, intuitive to use, much more uniform than HSL, no significant purple shift like LAB, and can be clamped to a given space, preventing out of gamut clipping.

...It is also restricted to the sRGB gamut and thus a strange anachronism in a world where the major RGB color spaces are Display P3 and BT.2020/2100...

sRGB is still the default, that's "major" yea? And there are some accessibility reasons that sRGB and Adobe98 are preferred over BT.2100 etc (those issues can be remedied with universal user personalization).

The means to calculate the constraints for HSLuv to be clamped to a particular space is in the repo. It does not look terribly difficult to generate the constraint matrix for any given space.

...for additive RGB spaces it is needless overhead and what is needed is primary and white chromaticities plus a transfer curve (EOTF) formula....

OpenColorIO and LUTs...

We use LUTs in film/TV because they are fast, easy, stackable, and in addition to transformations from a space to a space or device, it's easy to create a look and apply it across media, easy(er) to do gamut mapping, etc.

Moreover: easy to create. For instance Davinci Resolve has a free version that is usefully capable. Once you get a look or grade you like, Resolve can bake it into a LUT, or bake several LUTs together, and send it wherever needed, in a click.

But then, there is Open Color IO, open source complete color management system. Highly configurable, the full configuration can output many flavors of LUT or other transform methods, including ICC profiles.

My thought is not to use the full implementation, just enough of the API for connecting a LUT for in and out. Sort of an OCIO lite—with all the new color spaces, and the many to come, and the fact that OCIO LUTs work well with 32bit float and half float, it feels like the direction to examine....

Y 4 R There LUV

...I would want to see some more justification for adding this,...

Well, HSLuv is popular in DataViz, in part because of the saturation correlate, even if relative, it is still somewhat stable (better than the C). As I mentioned, Hsluv is best for color picking, gradients, etc. It may not be perfect, but it's well suited to the job of color selector/setter.

@lloydk
Copy link
Collaborator Author

lloydk commented Oct 20, 2023

Actually, it's possible I did! I think I just realized what you were saying @facelessuser: there is no way to override the smart default if the gamut space is the space itself, but the space is also polar, because we don't have a reference to the space until after it's created, so nothing to pass to gamutSpace. Essentially a chicken and egg problem.

Hmm hmm. So the options are:

  1. @lloydk’s original idea of having a special "self" value that resolves to this internally
  2. Ditching the isPolar heuristic when no gamutSpace is provided
  3. Dropping the getter idea (or using a different name) so that authors can just do:
let foo = new ColorSpace(...);
foo.gamutSpace = foo;

I think out of those, (1) is the least bad. Not sure if there are any other options?

Still not a big fan of option 1. If feels like we're having to jump through hoops in order to avoid adding this line to three color spaces:

gamutSpace: sRGB,

With option 2:

  • everything is explicit
  • the code in the Colorspace class is super simple.
    • In the constructor: this.gamutSpace = options.gamutSpace
    • The inGamut method is simply if(this.gamutSpace) { // convert and check this.gamutSpace } else { // check self }
  • the gamut check for lch, jzczhz and oklch avoids an unnecessary conversion to the base space
  • avoids the isPolar check which I think is confusing because the method name doesn't give any clue as to why we're using it. For the longest time I thought there was some funky math reason not to do gamut checks in polar spaces.

@lloydk
Copy link
Collaborator Author

lloydk commented Oct 27, 2023

Not sure where we're at as far as proceeding with adding these new color spaces. If they're not going to be added then I'll just close this PR.

If they're going to be added the only thing I have to add regarding the gamutSpace discussion is that with the "self" option there will be two ways to specify self, not specifying a gamutSpace (the default) and the special self value. Once a decision is made on which approach to take I'll create a PR. If seeing some code would make the decision easier I can create two PRs one for the "self" option and one for the option I'm advocating and then you can merge which ever one you think is best.

For the coord grammar change I know that it's not spec compliant but even if you restrict the grammar to numbers and percentages a grammar is still required on the Luv color space to handle negative reference range values. If angles aren't allowed then it's not really clear to me how Hue should be defined in the LCHuv HSLuv and HPLuv color spaces (and OKhsl/OKhsv if they were to be added)

@LeaVerou
Copy link
Member

Not sure where we're at as far as proceeding with adding these new color spaces. If they're not going to be added then I'll just close this PR.

I think the consensus among maintainers is that they should be added, but @svgeesus has some reservations, that may require more discussion. Definitely don't close the PR though!

If they're going to be added the only thing I have to add regarding the gamutSpace discussion is that with the "self" option there will be two ways to specify self, not specifying a gamutSpace (the default) and the special self value.

These are not the same. Not specifying it may or may not resolve to self, depending on isPolar. Specifying "self" explicitly will always resolve to self.

Once a decision is made on which approach to take I'll create a PR. If seeing some code would make the decision easier I can create two PRs one for the "self" option and one for the option I'm advocating and then you can merge which ever one you think is best.

The option you are advocating is not backwards compatible, requires changes to existing spaces, and adds more boilerplate to defining a new space. I'm happy to discuss alternative designs, but making the gamut space default cover fewer use cases than it currently does and requiring more overrides is a non-starter.

For the coord grammar change I know that it's not spec compliant but even if you restrict the grammar to numbers and percentages a grammar is still required on the Luv color space to handle negative reference range values. If angles aren't allowed then it's not really clear to me how Hue should be defined in the LCHuv HSLuv and HPLuv color spaces (and OKhsl/OKhsv if they were to be added)

@svgeesus This is actually excellent feedback for how the color() function itself should work, since in L5 we allow custom color spaces, without currently covering this author need. I wonder if we could instead allow all types (<number>, <percentage>, <angle> etc) and simply make the color invalid at computed value time if the arguments provided don't match the grammar specified.

@LeaVerou
Copy link
Member

Hi @lloydk,

Thank you for your patience!

I think the consensus among maintainers is that they should be added, but @svgeesus has some reservations, that may require more discussion. Definitely don't close the PR though!

As an update, @svgeesus and I just discussed it, and we have consensus that:

  1. We do want to add these color spaces 🎉
  2. We should move towards non-native color spaces being serialized using dashed idents in color(), but that can be done at a later stage.
  3. The change should not require edits to other color spaces, see my comments above wrt API.

@LeaVerou
Copy link
Member

Oh, and for tests you'd likely want to use the new format: https://github.com/LeaVerou/color.js/blob/main/tests/conversions.js (would love feedback on it too!)

"name": "Luv",
"id": "luv",
"url": "https://en.wikipedia.org/wiki/CIELUV",
"description": ""
Copy link
Member

Choose a reason for hiding this comment

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

It does need a description, but that can be added later

refRange: [0, 100],
name: "Lightness"
},
c: {
Copy link
Member

Choose a reason for hiding this comment

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

@LeaVerou LCHuv, unlike LCHab has a saturation as well. Saturation is chroma / lightness. How should that be exposed, as a utility method? Or can lchuv.s be made to work somehow?

Copy link
Member

Choose a reason for hiding this comment

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

The MVP won't have it, and you (or someone else) needs to open a new issue to discuss how to handle this in the API. 🥲

@LeaVerou
Copy link
Member

Hi everyone, I just got reminded of this from this tweet.
What are the current blockers? @lloydk are you still interested in working on this PR?

@lloydk
Copy link
Collaborator Author

lloydk commented Jan 18, 2024

Hi everyone, I just got reminded of this from this tweet. What are the current blockers? @lloydk are you still interested in working on this PR?

I'm waiting for #369 and #370 to be merged before proceeding with PR's for the color spaces. Was also potentially waiting for the parsing tests to be ported to the new format.

@LeaVerou
Copy link
Member

Hi everyone, I just got reminded of this from this tweet. What are the current blockers? @lloydk are you still interested in working on this PR?

I'm waiting for #369 and #370 to be merged before proceeding with PR's for the color spaces. Was also potentially waiting for the parsing tests to be ported to the new format.

Somehow these fell completely off our radar, so sorry @lloydk!! I just reviewed one and asked a clarifying question on the other.

In general, I'm personally very keen to add support for these color spaces, so if you see any delays on our part, please don't be afraid to ping us, we're not ghosting you, we're just spread very very thin. 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants