-
Notifications
You must be signed in to change notification settings - Fork 82
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 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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this!
So sorry for the delay, somehow these PRs completely slipped through the cracks, I don't recall ever seeing them before!
Overall looks good, see comments for a few minor issues.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn’t we restrict string
to the specific supported keywords?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, see suggestions for clarity
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
inGamut
method thecoords
were converted to the base space by directly calling thetoBase
method. This version of inGamut calls theto
method ongamutSpace
which will result inNaN
values 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.