Add gamutSpace to ColorSpace #369
Conversation
✅ Deploy Preview for colorjs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
HSL, HSV and HWB already do this (they check in sRGB) |
types/src/space.d.ts
Outdated
| @@ -37,6 +37,7 @@ export interface Options { | |||
| cssId?: string | undefined; | |||
| referred?: string | undefined; | |||
| formats?: Record<string, Format> | undefined; | |||
| gamutSpace?: string | ColorSpace | null | undefined; | |||
There was a problem hiding this comment.
Shouldn’t we restrict string to the specific supported keywords?
There was a problem hiding this comment.
string can be "self" or a spaceId. I added "self" to the list of types just to make it a bit more clear. Ideally we'd have a type alias for spaceId. Something like:
type SpaceId = string;
And then we could replace string with SpaceId.
src/space.js
Outdated
| // Gamut space | ||
|
|
||
| this.gamutSpace = this; | ||
|
|
||
| if (options.gamutSpace) { | ||
| if (options.gamutSpace !== "self") { | ||
| this.gamutSpace = ColorSpace.get(options.gamutSpace); | ||
| } | ||
| } | ||
| else if (this.isPolar) { | ||
| // Do not check gamut through polar coordinates | ||
| this.gamutSpace = this.base; | ||
| } | ||
|
|
||
| // Optimize inGamut for unbounded spaces | ||
| if (this.gamutSpace.isUnbounded){ | ||
| this.inGamut = (coords, options) => { | ||
| return true; | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
Minor nit: Setting this.gamutSpace to this and then undoing it later makes the logic hard to follow. I'd restructure a bit to avoid this, possibly via having two branches for the two main cases like:
if (options.gamutSpace) {
// Gamut space explicitly specified
...
}
else {
// Gamut space not explicitly specified, fall back to sensible defaults
...
}Note that if the gamut space is unbounded, this.gamutSpace is effectively still this, we just override inGamut as a performance optimization.
There was a problem hiding this comment.
I've changed the code to look more like what you suggested.
src/space.js
Outdated
| // Do not check gamut through polar coordinates | ||
| this.gamutSpace = this.isPolar ? this.base : this; |
There was a problem hiding this comment.
| // Do not check gamut through polar coordinates | |
| this.gamutSpace = this.isPolar ? this.base : this; | |
| // No gamut space specified, calculate a sensible default | |
| if (this.isPolar) { | |
| // Do not check gamut through polar coordinates | |
| this.gamutSpace = this.base; | |
| } | |
| else { | |
| this.gamutSpace = this; | |
| } |
Some colorspaces such as hsluv, okhsl and okhsv need to do gamut checking in a color space that is not their own space or their base space. For example hsluv needs to do gamut checking in the srgb color space. A gamutSpace can now be specified when constructing a ColorSpace and a gamutSpace property has beed added to ColorSpace. If a gamutSpace is not specified the following rules are used to assign the gamutSpace property: - if the color space is polar use the base space - if the color space is not polar use the current space Polar spaces that need to gamut check themselves (hpluv) can set the gamutSpace option to "self" to avoid gamut checking using the base space.
I've made the suggested changes. |
|
Merged, thanks! (Btw you can click "Commit suggestion" to merge it in, you don't have to redo the changes yourself :) ) |
Some colorspaces such as hsluv, okhsl and okhsv need to do gamut checking in a color space that is not their own space or their base space. For example hsluv needs to do gamut checking in the srgb color space.
A gamutSpace can now be specified when constructing a ColorSpace and a gamutSpace property has beed added to ColorSpace.
This PR is backwards compatible with existing color spaces and uses the "least bad" option of specifying "self" for the gamutSpace of polar color spaces that need to do gamut checking in their own color space (hpluv) as discussed here.
In the original
inGamutmethod thecoordswere converted to the base space by directly calling thetoBasemethod. This version of inGamut calls thetomethod ongamutSpacewhich will result inNaNvalues being converted to 0 before the gamut check. Not sure if this matters.I've also added an optimization to override inGamut for unbounded spaces. If you don't think it's appropriate I can remove it.