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

[Gamut Mapping App] Always display delta #458

Merged
merged 1 commit into from
Feb 27, 2024

Conversation

jamesnw
Copy link
Contributor

@jamesnw jamesnw commented Feb 27, 2024

See conversation at #438 (comment).

The result of a gamut mapping algorithm may change in-gamut colors, so it is useful to see the deltas in those cases as well.

Copy link

netlify bot commented Feb 27, 2024

Deploy Preview for colorjs ready!

Name Link
🔨 Latest commit dfbaf62
🔍 Latest deploy log https://app.netlify.com/sites/colorjs/deploys/65de46f9a6dad400087373ae
😎 Deploy Preview https://deploy-preview-458--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.

@jamesnw jamesnw changed the title [Gamut Mapping App] Always show delta display to help confirm it is in gamut [Gamut Mapping App] Always display delta Feb 27, 2024
@facelessuser
Copy link
Collaborator

I like this.

@jgerigmeyer jgerigmeyer merged commit 0d6b31b into color-js:main Feb 27, 2024
5 checks passed
@jgerigmeyer jgerigmeyer deleted the delta-display branch February 27, 2024 21:06
@LeaVerou
Copy link
Member

The result of a gamut mapping algorithm may change in-gamut colors

As I mentioned in the other PR, gamut mapping should never change in-gamut colors. When this happens, it's a bug.

@jgerigmeyer
Copy link
Member

The result of a gamut mapping algorithm may change in-gamut colors

As I mentioned in the other PR, gamut mapping should never change in-gamut colors. When this happens, it's a bug.

@LeaVerou Even if that's correct (and this comment makes it sound like not everyone agrees with that), isn't it helpful to surface it?

@facelessuser
Copy link
Collaborator

@LeaVerou Even if that's correct (and this comment makes it sound like not everyone agrees with that), isn't it helpful to surface it?

That was my thinking. It isn't that gamut mapping should alter already in gamuts, but it lets you see whether they are.

@LeaVerou
Copy link
Member

I don’t have a very strong opinion, but it seems to me that it's a bigger UI distinction to not show deltas when they're 0. It goes through the image processing circuit of your brain, whether displaying them but having them be 0 requires you to read and process the numbers to understand that the color is the same.

That said, @svgeesus brought up a very good point about certain GMA that affect colors near the gamut boundary. So my weakly held opinion is that we should show deltas iff the color is different than the original (whether or not they are in gamut).

We could potentially get the best of both worlds by displaying the deltas but fading them out.

@facelessuser
Copy link
Collaborator

we should show deltas iff the color is different than the original (whether or not they are in gamut).

I think that would be a fine alternative. It solves the original concern but also filters out the useless noise.

@jamesnw
Copy link
Contributor Author

jamesnw commented Feb 28, 2024

I like that as an alternative, and @jgerigmeyer has offered to implement. Thanks!

jgerigmeyer added a commit that referenced this pull request Feb 28, 2024
* main:
  Audit function parameter and return types (2/2) (#457)
  Always show delta display to help confirm it is in gamut (#458)
  Final clip is required in Ray tracing
  [apps/gamut-mapping] Add edge seeker algorithm (#448)
  Audit function parameter and return types (1/2) (#456)
  Rework raytracing limiting hue shift to no more than 3 degrees at worst
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.

4 participants