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

Ensure clipped is always returned and initialized #378

Closed
wants to merge 1 commit into from

Conversation

facelessuser
Copy link
Collaborator

Ensures clipped value is always returned and initialized, removes some unnecessary code, and tests with zero tolerance to ensure value is fully in gamut.

Fixes #376

Copy link

netlify bot commented Dec 16, 2023

Deploy Preview for colorjs ready!

Name Link
🔨 Latest commit 2bcb4aa
🔍 Latest deploy log https://app.netlify.com/sites/colorjs/deploys/657ce9b885dd090008450571
😎 Deploy Preview https://deploy-preview-378--colorjs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@facelessuser
Copy link
Collaborator Author

Gamut tests are broken on the release and here...I'm looking into that to make sure nothing else is broken.

@facelessuser
Copy link
Collaborator Author

Tests are busted on the official site and here. The changes themselves ensure we don't return and undefined color and that the final color is always clipped. We remove implied logic. Lastly, we ensure our gamut checks are done with zero epsilon for better gamut mapping. They should all be safe changes, but they may have to be evaluated manually.

@facelessuser
Copy link
Collaborator Author

facelessuser commented Dec 16, 2023

Okay, since the HTML tests are broken, I did a little sanity check here to demonstrate that no existing things are broken and the targetted issue is fixed. I also explain in a little more detail why I made the other changes. Some existing results may be ever so slightly different due to tighter tolerance.

mport Color from "./dist/color.js"

const colors = [
    'color(display-p3 1 0 0)',
    'color(display-p3 0 1 0)',
    'color(display-p3 0 0 1)',
    'color(display-p3 1 1 0)',
    'color(display-p3 0 1 1)',
    'color(display-p3 1 0 1)'
]

colors.map(c => {
    console.log(new Color(c).toGamut('srgb').toString({precision: 6, inGamut: true}));
})
// Failing case
console.log(new Color('oklab(0.00002 0 0)').toGamut('acescc').toString({precision: 6, inGamut: true}));

This is what color.js did before the fix:

color.js git:(master) ✗ node example.js
color(display-p3 0.91779 0.21072 0.15424)
color(display-p3 0.45147 0.97123 0.33189)
color(display-p3 0.00008 0.00074 0.99984)
color(display-p3 0.99673 0.99892 0.33034)
color(display-p3 0.45667 0.98176 0.97814)
color(display-p3 0.91957 0.26299 0.95178)
file:///Users/facelessuser/Code/github/color.js/dist/color.js:1010
		throw new TypeError("Empty color reference");
		      ^

TypeError: Empty color reference
    at getColor (file:///Users/facelessuser/Code/github/color.js/dist/color.js:1010:9)
    at to (file:///Users/facelessuser/Code/github/color.js/dist/color.js:1769:10)
    at toGamutCSS (file:///Users/facelessuser/Code/github/color.js/dist/color.js:1758:9)
    at toGamut (file:///Users/facelessuser/Code/github/color.js/dist/color.js:1585:19)
    at func (file:///Users/facelessuser/Code/github/color.js/dist/color.js:4339:14)
    at Color.<computed> [as toGamut] (file:///Users/facelessuser/Code/github/color.js/dist/color.js:4366:12)
    at file:///Users/facelessuser/Code/github/color.js/example.js:15:45
    at ModuleJob.run (node:internal/modules/esm/module_job:192:25)
    at async DefaultModuleLoader.import (node:internal/modules/esm/loader:228:24)
    at async loadESM (node:internal/process/esm_loader:40:7)

Node.js v20.5.1

This is after the fix.

color.js git:(bugfix/gamut-fix) ✗ node example.js
color(display-p3 0.91779 0.21072 0.15424)
color(display-p3 0.45147 0.97123 0.33189)
color(display-p3 0.00014 0.00074 0.95959)
color(display-p3 0.99673 0.99892 0.33034)
color(display-p3 0.45667 0.98176 0.97814)
color(display-p3 0.91957 0.26299 0.95178)
oklab(0.39061% 0 0)
  1. Results are the same, but now the aces case doesn't fail due to an uninitialized variable.
  2. There is no longer a risk of an infinite loop by removing the second else if condition. This could only occur in an extreme case with unexpected color space geometry.
  3. A better tolerance check for gamut mapping is used.

@LeaVerou
Copy link
Member

Okay, since the HTML tests are broken, I

They are? Are you visiting the old testsuite or the new one?

@facelessuser
Copy link
Collaborator Author

@LeaVerou Let's ignore this review for a second and I'll instead reference what I think are the official tests, the legacy and new (so I think). There are two and the gamut tests are either failing or not running: https://colorjs.io/tests/ and https://colorjs.io/test/.

The first seems to be broken and all failing:

Screenshot 2023-12-16 at 3 58 49 PM

The second seems broken in a completely different way:

Screenshot 2023-12-16 at 3 59 07 PM

@facelessuser
Copy link
Collaborator Author

I had not realized this issue had not merged: #352. That review should fix most of these issues, so I will close this as a duplicate.

@jamesnw jamesnw mentioned this pull request Dec 17, 2023
@svgeesus
Copy link
Member

Let's ignore this review for a second and I'll instead reference what I think are the official tests, the legacy and new (so I think). There are two and the gamut tests are either failing or not running: https://colorjs.io/tests/ and https://colorjs.io/test/.

The first seems to be broken and all failing:

The link in the footer went to the old (/tests) so I changed it today to link to the new (/test)

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.

Near-blacks in oklab crash the convert utility
3 participants