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

Refactor XYZ color space into its own module. #84

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

DCtheTall
Copy link

For #51

Refactors the implementation of the XYZ color space (i.e. CIE XYZ D50) out of src/lab.js into its own module, src/xyz.js.

@mbostock
Copy link
Member

This looks like a worthwhile refactoring and I’m in favor of exporting d3.xyz. 👍

@DCtheTall DCtheTall marked this pull request as ready for review November 2, 2020 15:15
@DCtheTall
Copy link
Author

@mbostock the PR is ready for review

@mbostock
Copy link
Member

mbostock commented Nov 2, 2020

Per @danburzo’s comment #51 (comment), what do you think about the choice of illuminant (D65 vs. D50)? One possibility is that the XYZ class has a field which stores the illuminant ("d65" or "d50"), and we export constructors for both (d3.xyzd65 and d3.xyzd50).

Also, does this PR affect the performance of converting between Lab and RGB because it now makes explicit the extra chromatic adaptation step rather than combining it into the matrix multiplication?

@DCtheTall
Copy link
Author

DCtheTall commented Nov 2, 2020

what do you think about the choice of illuminant (D65 vs. D50)?

See my comment on #51

Also, does this PR affect the performance of converting between Lab and RGB because it now makes explicit the extra chromatic adaptation step rather than combining it into the matrix multiplication?

Is there a benchmark you use for this? How have you tested performance impact in the past?

@DCtheTall
Copy link
Author

@mbostock friendly ping

@danburzo
Copy link
Contributor

In a recent addition to the css-color-4 spec, @svgeesus has made the following adjustments as per w3c/csswg-drafts#6722:

  • xyz refers to XYZ with the D65 white-point;
  • xyz-d50 refers the D50-relative XYZ;
  • xyz-d65 is an alias for xyz.

These are all predefined color profiles for the color() syntax and are expected to show up as identifiers in the upcoming Color API.

xyzd50 and xyzd65 sound reasonable as method identifiers on the d3 object. (In Culori, I opted a while back for xyz65 and xyz50 and will maintain it for backwards compatibility, although now I prefer the inclusion of the full illuminant name)

@mbostock mbostock mentioned this pull request Mar 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants