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

Add AlphaColor::from_kelvin / OpaqueColor::from_kelvin #137

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

waywardmonkeys
Copy link
Collaborator

These are feature-flagged (feature = "temperature").

These are feature-flagged (`feature = "temperature"`).
288.122_169_528_3 * (kelvin - 60.).powf(-0.075_514_849_2)
};

let blue = if kelvin >= 66. {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The writing on the post about this says that blues above 6500K are 255, but the math here and in other implementations is checking against 6600K. I think this should be fixed.

I also leave things as they are in the sample code as calculating a value between 0 and 255 and then at the end dividing by 255. This should probably do better.

fn rgb_from_kelvin() {
// These expected values are in linear RGB
assert_temperature(2700., [1., 0.6538038, 0.3427666]);
assert_temperature(6600., [1., 1., 1.]);
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to add more tests but also need to make sure we get values from a good source of truth.

impl<CS: ColorSpace> AlphaColor<CS> {
/// Convert a temperature in Kelvin to a color.
///
/// The Kelvin temperature will be clamped to a range of 1000-40,000.
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The docs should be improved here and would be better with some color swatches and a table.

Also, need to credit the original blog post and link to Wikipedia about color temperature.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it is suspect to me that the calculations clamp to the sRGB gamut, but go to 1000 kelvin. I believe temperatures lower than roughly 2000 kelvin are outside sRGB's natural gamut, and for e.g. 1667 kelvin, it should be roughly color(srgb 1.0 0.46 -0.12), based on the chromaticities here https://en.wikipedia.org/w/index.php?title=Planckian_locus&oldid=1245078623#Approximation.

impl<CS: ColorSpace> AlphaColor<CS> {
/// Convert a temperature in Kelvin to a color.
///
/// The Kelvin temperature will be clamped to a range of 1000-40,000.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it is suspect to me that the calculations clamp to the sRGB gamut, but go to 1000 kelvin. I believe temperatures lower than roughly 2000 kelvin are outside sRGB's natural gamut, and for e.g. 1667 kelvin, it should be roughly color(srgb 1.0 0.46 -0.12), based on the chromaticities here https://en.wikipedia.org/w/index.php?title=Planckian_locus&oldid=1245078623#Approximation.

}
}

/// Convert a Kelvin temperature into a linear RGB color.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this is actually calculating sRGB (i.e. non-linear), but would need to double check.

@tomcur
Copy link
Member

tomcur commented Jan 25, 2025

sRGB doesn't necessarily need to be blessed here, users working with color temperatures may be working in color spaces like Adobe RGB, ProPhoto RGB, ACEScg, …. Wanting to chromatically adjust based on the color temperature, I think you'd probably go through XYZ. Maybe it makes sense to calculate the chromaticities (x,y) and use XYZ (D65?) as the base color space for this; there's more prior art we can base this on then. I'm not sure yet how Y could be best determined, though. Some sources use the peak luminance of the target gamut (for example, Wikipedia's Color Temperature template does that https://en.wikipedia.org/wiki/Template:Color_temperature).

Comment on lines +28 to +35
impl<CS: ColorSpace> OpaqueColor<CS> {
/// Convert a temperature in Kelvin to a color.
///
/// The Kelvin temperature will be clamped to a range of 1000-40,000.
pub fn from_kelvin(kelvin: f32) -> Self {
Self::new(CS::from_linear_srgb(from_kelvin(kelvin)))
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps the constructor should just be defined on the base color space used for this. If we have OpaqueColor::<CS>::from_kelvin(), we need to make sure the colors aren't chromatically adapted. As in, say we calculate the color temperature in sRGB as done here (sRGB having a D65 white point), and convert that to, say, ProPhoto RGB (with a D50 white point), the color is chromatically adapted, and now represents a different absolute color, with a different color temperature.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was discussed during office hours. The path forward is probably to put this behind a feature flag that also introduces a new wrapper type, AbsoluteColor, between which conversions can be done that don't do (or undo) the implicit chromatic adaptation performed by ColorSpace::{to_linear_srgb,from_linear_srgb,convert}. This requires storing the white point chromaticities on ColorSpace as an associated const (probably without a feature flag to maintain compatibility of the type between builds with different flags), and expose the white point on DynamicColor as well.

github-merge-queue bot pushed a commit that referenced this pull request Feb 13, 2025
This adds absolute color conversions and chromatic adaptation, and is,
more or less, a prerequisite for #137.

Conversion methods are added to `ColorSpace`, `ColorSpaceTag`, and
`DynamicColor`, that convert between color spaces while keeping the same
absolute color (i.e., if that color were to be reproduced, it would be
the same physical color, but a perceptually different color under the
intended reference white of the color space). These methods are suffixed
`_absolute`. This also adds a `chromatically_adapt` method for manual
chromatic adaptation (useful for, e.g., "white balancing" pictures).

The white points are represented as CIE `xy` chromaticities. Calculation
of chromatic adaptation matrices is `const` where possible.

A follow-up to this would be to manually implement
`ColorSpace::{to_linear_srgb_absolute, from_linear_srgb_absolute,
convert}` for the color spaces we provide. Those can in most cases just
lift directly from their non-`_absolute` counterparts, with the
exceptions being XYZ-D50, ACEScg and ACES2065-1. Those methods will get
somewhat simpler matrices, as the `{D50, ACES}<->D65` adaptation
transforms can be dropped.
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