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

[css-color-4] Gamut mapping pseudocode was changed incorrectly #9715

Closed
svgeesus opened this issue Dec 16, 2023 · 5 comments
Closed

[css-color-4] Gamut mapping pseudocode was changed incorrectly #9715

svgeesus opened this issue Dec 16, 2023 · 5 comments
Assignees
Labels
css-color-4 Current Work

Comments

@svgeesus
Copy link
Contributor

svgeesus commented Dec 16, 2023

@svgeesus I'm a bit confused by the change in 0674822 - I think you may have added the early return in the wrong place, and then removed some needed steps that had become unreachable.

The change we were looking to make in # 2 above was to insert after line 10:

  • set |current| to |origin_Oklch|
  • set |clipped| to clip(|current|)
  • set |E| to delta(|clipped|, |current|)
  • if |E| < |JND| return |clipped| as the gamut mapped color

This is identical to the logic within the while, but crucially is checked before the chroma is adjusted.

Originally posted by @jamesnw in color-js/color.js#352 (comment)

@svgeesus svgeesus added the css-color-4 Current Work label Dec 16, 2023
@svgeesus svgeesus self-assigned this Dec 16, 2023
@svgeesus
Copy link
Contributor Author

Reverted changes

The actual change is still to be done, and should be sanity-checked here first

@svgeesus
Copy link
Contributor Author

Ok so, what we currently have (after backing out the incorrect change) is

<div algorithm="to CSS gamut map a color">
	To <dfn export>CSS gamut map</dfn> a color |origin|
	in color space |origin color space|
	to be in gamut of a destination color space |destination|:

<ol>
	<li>if |destination| has no gamut limits (XYZ-D65, XYZ-D50, Lab, LCH, Oklab, Oklch) convert |origin| to |destination| and return it as the gamut mapped color
	<li>let |origin_Oklch| be |origin| converted
		from |origin color space| to the Oklch color space</li>
	<li>if the Lightness of |origin_Oklch| is greater than or equal to 100%,
		return { 1 1 1 origin.alpha } in |destination|</li>
	<li>if the Lightness of |origin_Oklch| is less than than or equal to 0%,
			return { 0 0 0 origin.alpha } in |destination|</li>
	<li>let inGamut(|color|) be a function which returns true if, when passed a color,
		that color is inside the gamut of |destination|.
		For HSL and HWB, it returns true if the color is inside the gamut of sRGB.
	</li>
	<li>if inGamut(|origin_Oklch|) is true, convert |origin_Oklch| to |destination| and return it as the gamut mapped color</li>
	<li>otherwise, let delta(|one|, |two|) be a function which returns the deltaEOK of color |one| compared to color |two|</li>
	<li>let |JND| be 0.02</li>
	<li>let |epsilon| be 0.0001</li>
	<!-- we already excluded spaces with no gamut limits in the first step, so this is fine -->
	<li>let clip(|color|) be a function which converts |color| to |destination|,
		converts all negative components to zero,
		converts all components greater that one to one,
		and returns the result
	</li>
	<li>set |min| to zero</li>
	<li>set |max| to the Oklch chroma of |origin_Oklch|</li>
	<li> let |min_inGamut| be a boolean that represents when |min| is still in gamut, and set it to true
	<li>while (|max| - |min| is greater than |epsilon|) repeat the following steps
		<ol>
			<li>set |chroma| to (|min| + |max|) /2</li>
			<li>set |current| to |origin_Oklch| and then set the chroma component to |chroma|</li>
			<li>if |min_inGamut| is true and also if inGamut(|current|) is true, set |min| to |chroma| and continue to repeat these steps</li>
			<li>otherwise, if inGamut(|current|) is false carry out these steps:
				<ol>
					<li>set |clipped| to clip(|current|)</li>
					<li>set |E| to delta(|clipped|, |current|)</li>
					<li>if |E| < |JND|
					<ol>
						<li>if (|JND| - |E| < |epsilon|) return |clipped| as the gamut mapped color</li>
						<li>otherwise,
						<ol>
							<li>set |min_inGamut| to false</li>
							<li>set |min| to |chroma|</li>
						</ol>
						</li>
					</ol>
					</li>
					<li>otherwise, set |max| to |chroma| and continue to repeat these steps</li>
				</ol>
			</li>
		</ol>
	</li>
	<li>return |clipped| as the gamut mapped color</li>
</ol>

@svgeesus
Copy link
Contributor Author

and what we actually want is (some comments added for better documentation):

<div algorithm="to CSS gamut map a color">
	To <dfn export>CSS gamut map</dfn> a color |origin|
	in color space |origin color space|
	to be in gamut of a destination color space |destination|:

<ol>
	<!-- check if we need gamut mapping at all -->
	<li>if |destination| has no gamut limits (XYZ-D65, XYZ-D50, Lab, LCH, Oklab, Oklch) convert |origin| to |destination| and return it as the gamut mapped color
	<!-- we do, so convert to Oklch -->
		<li>let |origin_Oklch| be |origin| converted
		from |origin color space| to the Oklch color space</li>
	<!-- constrain to SDR lightness range, which gamut maps to black or white -->
	<li>if the Lightness of |origin_Oklch| is greater than or equal to 100%,
		return { 1 1 1 origin.alpha } in |destination|</li>
	<li>if the Lightness of |origin_Oklch| is less than than or equal to 0%,
			return { 0 0 0 origin.alpha } in |destination|</li>
	<li>let inGamut(|color|) be a function which returns true if, when passed a color,
		that color is inside the gamut of |destination|.
		For HSL and HWB, it returns true if the color is inside the gamut of sRGB.
	</li>
	<!-- are we already in gamut? -->
	<li>if inGamut(|origin_Oklch|) is true, convert |origin_Oklch| to |destination| and return it as the gamut mapped color</li>
	<!-- now start to gamut map -->
	<li>otherwise, let delta(|one|, |two|) be a function which returns the deltaEOK of color |one| compared to color |two|</li>
	<li>let |JND| be 0.02</li>
	<li>let |epsilon| be 0.0001</li>
	<!-- we already excluded spaces with no gamut limits in the first step, so this is fine -->
	<li>let clip(|color|) be a function which converts |color| to |destination|,
		converts all negative components to zero,
		converts all components greater that one to one,
		and returns the result
	</li>

	<!-- is clipped already indistinguishable from origin, and in gamut? -->
	<li>set |current| to |origin_Oklch|</li>
	<li>set |clipped| to clip(|current|)</li>
	<li>set |E| to delta(|clipped|, |current|)</li>
	<li>if |E| < |JND|
		<ol>
			<li>return |clipped| as the gamut mapped color</li>
		</ol>
	</li>

	<!-- reduce chroma -->
	<li>set |min| to zero</li>
	<li>set |max| to the Oklch chroma of |origin_Oklch|</li>
	<li> let |min_inGamut| be a boolean that represents when |min| is still in gamut, and set it to true
	<li>while (|max| - |min| is greater than |epsilon|) repeat the following steps
		<ol>
			<li>set |chroma| to (|min| + |max|) /2</li>
			<li>set the chroma component of |current| to |chroma|</li>
			<li>if |min_inGamut| is true and also if inGamut(|current|) is true, set |min| to |chroma| and continue to repeat these steps</li>
			<li>otherwise, if inGamut(|current|) is false carry out these steps:
				<ol>
					<li>set |clipped| to clip(|current|)</li>
					<li>set |E| to delta(|clipped|, |current|)</li>
					<li>if |E| < |JND|
					<ol>
						<li>if (|JND| - |E| < |epsilon|) return |clipped| as the gamut mapped color</li>
						<li>otherwise,
						<ol>
							<li>set |min_inGamut| to false</li>
							<li>set |min| to |chroma|</li>
						</ol>
						</li>
					</ol>
					</li>
					<li>otherwise, set |max| to |chroma| and continue to repeat these steps</li>
				</ol>
			</li>
		</ol>
	</li>
	<li>return |clipped| as the gamut mapped color</li>
</ol>

@svgeesus
Copy link
Contributor Author

Incorporating the changes from #9651 (comment) and #9651 (comment) too, we have

<div algorithm="to CSS gamut map a color">
	To <dfn export>CSS gamut map</dfn> a color |origin|
	in color space |origin color space|
	to be in gamut of a destination color space |destination|:

<ol>
	<!-- check if we need gamut mapping at all -->
	<li>if |destination| has no gamut limits (XYZ-D65, XYZ-D50, Lab, LCH, Oklab, Oklch) convert |origin| to |destination| and return it as the gamut mapped color
	<!-- we do, so convert to Oklch -->
		<li>let |origin_Oklch| be |origin| converted
		from |origin color space| to the Oklch color space</li>
	<!-- constrain to SDR lightness range, which gamut maps to black or white -->
	<li>if the Lightness of |origin_Oklch| is greater than or equal to 100%,
		convert `oklab(1 0 0 / origin.alpha)` to |destination| and return it as the gamut mapped color</li>
	<li>if the Lightness of |origin_Oklch| is less than than or equal to 0%,
		convert `oklab(0 0 0 / origin.alpha)` to |destination| and return it as the gamut mapped color</li>
	<li>let inGamut(|color|) be a function which returns true if, when passed a color,
		that color is inside the gamut of |destination|.
		For HSL and HWB, it returns true if the color is inside the gamut of sRGB.
	</li>
	<!-- are we already in gamut? -->
	<li>if inGamut(|origin_Oklch|) is true, convert |origin_Oklch| to |destination| and return it as the gamut mapped color</li>
	<!-- now start to gamut map -->
	<li>otherwise, let delta(|one|, |two|) be a function which returns the deltaEOK of color |one| compared to color |two|</li>
	<li>let |JND| be 0.02</li>
	<li>let |epsilon| be 0.0001</li>
	<!-- we already excluded spaces with no gamut limits in the first step, so this is fine -->
	<li>let clip(|color|) be a function which converts |color| to |destination|,
		clamps each component to the bounds of the reference range for that component
		and returns the result</li>

	<!-- is clipped already indistinguishable from origin, and in gamut? -->
	<li>set |current| to |origin_Oklch|</li>
	<li>set |clipped| to clip(|current|)</li>
	<li>set |E| to delta(|clipped|, |current|)</li>
	<li>if |E| < |JND|
		<ol>
			<li>return |clipped| as the gamut mapped color</li>
		</ol>
	</li>

	<!-- reduce chroma -->
	<li>set |min| to zero</li>
	<li>set |max| to the Oklch chroma of |origin_Oklch|</li>
	<li> let |min_inGamut| be a boolean that represents when |min| is still in gamut, and set it to true
	<li>while (|max| - |min| is greater than |epsilon|) repeat the following steps
		<ol>
			<li>set |chroma| to (|min| + |max|) /2</li>
			<li>set the chroma component of |current| to |chroma|</li>
			<li>if |min_inGamut| is true and also if inGamut(|current|) is true, set |min| to |chroma| and continue to repeat these steps</li>
			<li>otherwise, if inGamut(|current|) is false carry out these steps:
				<ol>
					<li>set |clipped| to clip(|current|)</li>
					<li>set |E| to delta(|clipped|, |current|)</li>
					<li>if |E| < |JND|
					<ol>
						<li>if (|JND| - |E| < |epsilon|) return |clipped| as the gamut mapped color</li>
						<li>otherwise,
						<ol>
							<li>set |min_inGamut| to false</li>
							<li>set |min| to |chroma|</li>
						</ol>
						</li>
					</ol>
					</li>
					<li>otherwise, set |max| to |chroma| and continue to repeat these steps</li>
				</ol>
			</li>
		</ol>
	</li>
	<li>return |clipped| as the gamut mapped color</li>
</ol>

@jamesnw does that look correct?

@jamesnw
Copy link

jamesnw commented Dec 20, 2023

@svgeesus That all looks correct to me. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
css-color-4 Current Work
Projects
None yet
Development

No branches or pull requests

2 participants