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

Use d50 white point instead of d65 in HCL/LAB color spaces #146

Merged
merged 1 commit into from
Apr 28, 2023

Conversation

ChrisLoer
Copy link
Contributor

@ChrisLoer ChrisLoer commented Apr 27, 2023

See: maplibre/maplibre-gl-js#2382

The RGB -> HCL code in https://github.com/maplibre/maplibre-gl-js/blob/main/src/style-spec/util/color_spaces.ts forked out of d3-color in 2016. It uses the D65 illuminant in all of its intermediate color spaces. In 2018 d3-color was modified to convert from D65 to D50 (see discussion at d3/d3-color#42). The CSS spec recommends using D50 to convert to LAB (which is the step before converting to HCL).

This PR updates the code using constants derived from the modern d3-color (:wave: thank you @mbostock!), with a link to the helpful explanation at https://observablehq.com/@mbostock/lab-and-rgb.

@kajkal put together a very helpful demo showing the effect of this change (along with many other recent changes they've been working on): https://kajkal.github.io/maplibre-gl-style-spec-color-demo/swipe.html. By swiping up and down, you can see the d65 vs d50. The difference is subtle, but if you look at the "blue->white" section of the "interpolate-hcl" line, you can see that with d65 the whole range of colors looks whiter than with d50.

This does change existing behavior, but in a pretty subtle way, and no APIs/documentation etc. need to change as far as I'm aware. As @kajkal points out, there were much bigger problems (and fixes) in color handling that will go into the 3.x maplibre-gl release, and I hope this will just be an extra bit of tidying up.

Launch Checklist

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Link to related issues.
  • Include before/after visuals or gifs if this PR includes visual changes.
  • Write tests for all new functionality.
  • Document any changes to public APIs.
  • Post benchmark scores.
  • Add an entry to CHANGELOG.md under the ## main section.

@ChrisLoer
Copy link
Contributor Author

Oops, there was one linter change that failed in CI -- should be fixed now

@HarelM HarelM merged commit dcb12f3 into maplibre:main Apr 28, 2023
@HarelM
Copy link
Collaborator

HarelM commented Apr 28, 2023

Thanks!!

@ChrisLoer
Copy link
Contributor Author

@HarelM now that this is merged, how should we move it towards release? Is the plan something like:

  • Merge Migrate expression tests #97 to migrate expression tests into maplibre-style-spec
  • Do a 19.1.0 release (or 19.0.2, might have to read the semver spec...)
  • Make a PR to maplibre-gl-js that simultaneously bumps the package version, migrates the tests, and picks up the new behavior?

We could alternatively try to push this ahead of #97 but it seems like it would add a bunch of extra release/update/migrate steps. cc @birkskyum

@HarelM
Copy link
Collaborator

HarelM commented Apr 28, 2023

Release a new npm package in this repo and bump it in maplibre-gl.
If you have a few minutes to push this forward you can create a PR with the version bump and changelog changes.
See #134 for example.

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.

2 participants