Make color spaces white point aware

[sidred]

This is a breaking change.

• All colors now have an additional trait bound on WhitePoint.
• All "new" methods on colors now assume a default white point of D65. Use "with_wp" method to get color with another white point.
• Delete tristimulus module.
• Add Xyz values for standard observers and illuminants (D65, D50 etc).
• Implement methods for chromatic adaptation using Bradford, VonKries and Xyz scaling.
• Add Srgb primaries for Rgb <-> Xyz converison.
• Make Rgb to luma conversion to convert via xyz.

closes #14

[sidred]

To-Do:

• Add basic white points
• Add chromatic adaptation basics
• Add white point to color spaces
• Make color enum white_point aware
• Make conversions to be white point aware
• Add chromatic adaptation comversions
• Add all white points from cie list

[Ogeon]

Looks like a good start 👍 Just a few things I thought about:

• The white point macro seems simple enough to not really be necessary. Will it become more complex?
• The Rust conventions says that type names that are abbreviations should be camel cased as one word (COW -> Cow). Maybe we should do the same with the various variants of WP.
• It's probably a good idea to make the white_point module public to not clutter the crate root too much.

[sidred]

The white point macro seems simple enough to not really be necessary. Will it become more complex?

Not really. Its just that I have to add a few more and right now its not clear what the correct x,y values are. I''l remove it in the end when I add the remaining white points.

The Rust conventions says that type names that are abbreviations should be camel cased as one word (COW -> Cow). Maybe we should do the same with the various variants of WP.

D65Fov10 is already camel case. Are you talking about something else? Oh and if you can think of a better name, let me know :)

It's probably a good idea to make the white_point module public to not clutter the crate root too much.

Ok

[Ogeon]

Not really. Its just that I have to add a few more and right now its not clear what the correct x,y values are. I''l remove it in the end when I add the remaining white points.

Ok, I was just checking. I have been thinking about phasing out some macros, in general, but that's a separate topic.

D65Fov10 is already camel case. Are you talking about something else?

Those are correctly cased. I'm talking about the type parameters, like WP -> Wp, SWP -> Swp, and so on.

Oh and if you can think of a better name, let me know :)

I'll see what I can come up with, but you have most likely witnessed how bad I am at naming things. The name "Palette" is a masterpiece among my creations ;) Speaking of which, you are always welcome to suggest alternate names for things I contribute with.

[sidred]

Those are correctly cased. I'm talking about the type parameters, like WP -> Wp, SWP -> Swp, and so on.

Yeah, those need to be changed. I thought trait bounds were all caps, but they need to be camel case too.

[Ogeon]

They are temporary types, so it's the same rule for them as well, but they are usually only one letter long, so it's easy to forget :)

[sidred]

I have implemeted the white point for lab and changed the into iterator from

pub trait IntoColor<T>: Sized{

to

pub trait IntoColor<WP = D65, T = f32>: Sized
    where T: Float,
     WP: WhitePoint<T>
{

I get an error for converting xyz to rgb

        let rgb: Rgb<T> = xyz.into_rgb();
        Self::from_rgb(rgb)

error

src/hsl.rs:63:31: 63:39 error: unable to infer enough type information about `_`; type annotations or generic parameter binding required [E0282]
src/hsl.rs:63         let rgb: Rgb<T> = xyz.into_rgb();
                                            ^~~~~~~~

This works

        let rgb = <Rgb<T>  as FromColor<WP, T>>::from_xyz(xyz);
        <Self as FromColor<WP, T>>::from_rgb(rgb)

Is there any other altenative?

[Ogeon]

Is the white point of xyz known?

[sidred]

For now I am not implementing the White Point for Xyz and Yxy. I'll implement it for everything else and only then add it in for Yxy and Xyz if it makes sense.

I've been using the convert as notation for now. Let's see how it shapes up.

[Ogeon]

Well, it can't possibly know what white is if it's not specified, so that's most likely the reason :)

[sidred]

Tests still fail due to color enum. I will update it next.

[Ogeon]

Why are you renaming almost every color to *Space?

[sidred]

Why are you renaming almost every color to *Space?

I am expecting only a small subset of users to care about white point, so i was trying to make it simple for them. This also has the effect of being compatible with the earlier version of the library.

I have defined the original colors to use the default white point

pub type Lab<T> = LabSpace<D65, T>;
pub type Laba<T> = LabSpaceAlpha<D65, T>;

I am expecting this to work as usual before and after the white point change

   let lab1 = Lab::new(0.5, 0.5, 0.5)

   let lab2 = Lab {l: 0.5, a: 0.5, b:0.5}

For users who need a custom white point will need to use

   let lab2 = LabSpace::<D50, f64> {l: 0.5, a: 0.5, b:0.5}

Is it unnecessary since we are setting the defaults in the struct definition?

[Ogeon]

Oh, ok. Well, most users should not be affected by the change, unless most users uses an other float type than f32. Adding the extra white point shouldn't be a huge step from changing the precision, and they can always make their own custom aliases when they need to.

I see the point and I understand that it may become tedious in some situations, but the fact that users can make their own aliases makes me think that it's probably not such a big deal, after all.

[sidred]

I'll revert to using the short names. Having the defaults pretty much makes it unnecessary.

[Ogeon]

We can do something about it later if it turns out to be a problem. There may even be a better solution then. Who knows?

You have probably noticed, but type aliases and the documentation don't really play nice together. You can't see any implementations for aliases, which is why I made the links for the alpha aliases. You can write impl MyAlias { ... } and it works fine, but it won't show up anywhere in the documentation. It's a bit annoying and only somewhat related to this, so I'll stop there :)

[sidred]

Tests still fail for Srgb, Gamma rgb and color enum conversion.

[Ogeon]

I have mostly skimmed through the changes, but I think it looks good so far. The only things I can really comment on at the moment is that I'm leaning towards making the white point fields public, to allow Rgb { red: ..., green: ... blue: ..., _wp: PhantomData } style construction, and that I'm quite sure Xyz and Yxy should have a white point parameter.

You can't know what XYZ for white is if you haven't specified a white point, and the requirement of <xyz as as FromColor<WP, T>>::... is an effect of that problem. We can't just throw away the reference to how the values are supposed to be interpreted. It would actually make more sense to do this with RGB, since we know that rgb(1, 1, 1) will always represent white, but the perception of an XYZ value differs depending on the reference white.

[sidred]

The project builds, but tests fails due to private _wp field.

The only things I can really comment on at the moment is that I'm leaning towards making the white point fields public

I was just thinking the same thing. I can't think of a downside to this. Is wp a good enough name or should I make it white_point (instead os _wp).

I'm quite sure Xyz and Yxy should have a white point parameter.

I am not convinced that Xyz and Yxy need a white point.
Consider the white point (0.31271,0.32902, 1.0). This represents the D65 Illuminant for 2 degree observer. If we define the white point for this Yxy, then all following will be the same. The white point is essentially meaningless here.

Yxy<D65,_> == Yxy<D50,_> == Yxy<A,_> == (0.31271,0.32902, 1.0)

But the same is not the case for the other colors.

Rgb<D65,_> != Rgb<D50,_> != Rgb<A,_>

Also, The conversions Color -> Xyz will depend only on the source white_point and Xyz -> Color will only depend on the target white point and we should not run into any issues in the conversions.

[Ogeon]

I was just thinking the same thing. I can't think of a downside to this. Is wp a good enough name or should I make it white_point (instead os _wp).

white_point looks nicer to me.

then all following will be the same. The white point is essentially meaningless here

We must not mix up perceptive and numeric equality. The Yxy values will have the same energy values, but they will look different under different illuminants, while the Rgb values will look the same, but will have different energy levels under different illuminants.

Take this snowy house, for example. The snow looks mostly white because the colors are adapted to sunlight, but it's blue in the shadows, where it's only illuminated by the sky. We could adapt the colors to make the shadows look white/gray, but the rest would then look yellow. Both the sunlit and shadowy snow is still "white", though, since the snow will reflect more or less equal amounts of every wavelength.

[Ogeon]

I'll continue my example with some code and numbers. Let's pretend that the sky itself is a D50 illuminant and that the sun is a D65 illuminant. Then the shadowy snow would be Rgb::<D50, _>::new(1.0, 1.0, 1.0), which looks like purest white if our eyes adapt to the D50 light, and the sunlit snow would be Rgb::<D65, _>::new(1.0, 1.0, 1.0). The camera in the photo is adapted to the sunlight, or D65 in this example, so we have to translate the color of the shadowy snow to D65 to know how it will look in the photo:

let snow_shadow: Rgb<D50, _> = Rgb::new(1.0, 1.0, 1.0);
let snow_shadow_xyz: Xyz = snow_shadow.into();

//This is no longer numerically the same as Rgb::new(1.0, 1.0, 1.0),
//meaning that it will look different.
let snow_shadow_photo: Rgb<D65, _> = snow_shadow_xyz.into(); 

Let's say that we want to know if the shadowy snow reflects the same amount of light as the sunlit snow, and therefore should look the same under the same light. That's where we have to do a chromatic adaption:

let snow_shadow: Rgb<D50, _> = Rgb::new(1.0, 1.0, 1.0);
let snow_shadow_xyz: Xyz = snow_rgb.into();

//Chromatically adapt from D50 to D65,
//or move snow from the shadow into the sunlight
let snow_photo_xyz: Xyz = snow_shadow_xyz.adapt::<D50, D65>();

//This should now be equal to Rgb::new(1.0, 1.0, 1.0)
let snow_photo: Rgb<D65, _> = snow_photo_xyz.into();

The white point does matter to XYZ, but not in the same way as to RGB. It's still important for knowing how the signals are perceived, while RGB needs it to know what the signals should be. This is the first example again, but with a white point aware Xyz:

let snow_shadow: Rgb<D50, _> = Rgb::new(1.0, 1.0, 1.0);
let snow_shadow_xyz: Xyz<D50, _> = snow_shadow.into();

//Adapt our brain to the sunlight, so the same numbers, but different white point.
let snow_shadow_photo_xyz: Xyz<D65, _> = snow_shadow_xyz.with_white_point();

//This is no longer numerically the same as Rgb::new(1.0, 1.0, 1.0),
//meaning that it will look different.
let snow_shadow_photo: Rgb<D65, _> = snow_shadow_photo_xyz.into(); 

...and the second example:

let snow_shadow: Rgb<D50, _> = Rgb::new(1.0, 1.0, 1.0);
let snow_shadow_xyz: Xyz<D50, _> = snow_rgb.into();

//Chromatically adapt from D50 to D65,
//or move snow from the shadow into the sunlight
let snow_photo_xyz: Xyz<D65, _> = snow_shadow_xyz.adapt();

//This should now be equal to Rgb::new(1.0, 1.0, 1.0)
let snow_photo: Rgb<D65, _> = snow_photo_xyz.into();

It's still possible to do the same calculations without the white point on Xyz, but you have to keep track if it yourself. Letting Xyz keep track of it instead will require an explicit transition between white points, but there's not the same risk of mixing things up. I feel like it just completes the whole model.

[sidred]

Yeah, Xyz also needs an illuminant. For chromatic adaptation needing to do xyz_src to xyz_dst pretty makes this necessary. I'll make the change.

Right now the Color conversions do not take chromatic adaptation into account. They only convert from and into the same white point.

What I am concerned about needing white point annotation in conversions. For example, rgb -> hsb should just work assuming the same white point without having to specify it. White Point annotation is only necessary when chormatic adpatation is needed.

// assumes default D65
let rgb = Rgb::new(1.0, 0.0,0.0);

// Should automatically use D65 white point
let hsl = rgb.into_hsl() 
let lab = rgb.into_lab() // or even rgb.into()

// Instead of Manually having to specify whtie point
let hsl: Lab<D65,_> = rgb.into();
let lab: Lab<D65,_> = rgb.into();

Can we use the existing FromColor and IntoColor to make this happen?
Should I add this in this PR or in a new PR?

[sidred]

just run into a usablility issue. This code fails

let lab = Lab::new(0.4623,-0.517,0.499);
let lab = Lab {l: 0.4623, a: -0.517, b: 0.499, white_point: PhantomData };

with the following error even though the default (D65 and f32) are defined for the lab space.

 unable to infer enough type information about `_`; type annotations or generic parameter binding required

This works

    let lab = Lab::<D65,f64> {l: 0.4623, a: -0.517, b: 0.499, white_point: PhantomData };

Can we do anything to infer the default white points?

[Ogeon]

Right now the Color conversions do not take chromatic adaptation into account. They only convert from and into the same white point.

I think that's good. Chromatic adaption can be more destructive than normal conversion, so I think it should be done explicitly.

Can we use the existing FromColor and IntoColor to make this happen?
Should I add this in this PR or in a new PR?

Those conversions should just work. The white point becomes a part of the type, so it should be restricted in the same way as the float type. I wrote them out in my example to make sure it was clear, but it should only be necessary if something other than D65 is used, just as with the float type.

Can we do anything to infer the default white points?

I don't know. It my depend on the context. Is it just the struct syntax? Is it only when the type isn't specified anywhere else?

[sidred]

Those conversions should just work. The white point becomes a part of the type, so it should be restricted in the same way as the float type

So FromColor<Wp, T> becomes FromColor<Swp, Dwp, T> and same with the IntoColor trait.

I don't know. It my depend on the context. Is it just the struct syntax? Is it only when the type isn't specified anywhere else?

You can check the last commit for all the places I has to explicitly specify the type. It's pretty much everywhere.

[Ogeon]

So FromColor<Wp, T> becomes FromColor<Swp, Dwp, T> and same with the IntoColor trait.

That's not what I meant. I think it's good that chromatic adaption is explicit and not a part of FromColor or IntoColor. It fits better as its own trait, in my opinion.

You can check the last commit for all the places I has to explicitly specify the type. It's pretty much everywhere.

Oh, no! It looks like we have stumbled upon an unfinished feature. See this playpen: http://is.gd/qFU2o5

I'm not sure what to do here... This problem will also be present in the planned Rgb reform.

Edit: this is the closest to a workaround I have found, but I don't like it: http://is.gd/myfW21

[sidred]

How is HashMap default hasher impelmentation working then?

[Ogeon]

Looks like new is only implemented for HashMap<K, V, RandomState>: https://doc.rust-lang.org/src/std/collections/hash/map.rs.html#506

We could do something similar: http://is.gd/AIkZ09 But the situation is different, because the white point is never passed to a function.

[sidred]

We can use something like this http://is.gd/do9sEJ

[Ogeon]

Also from the link below, there are 2 types of white points ICC D65 and ASTM D65 (same for all other D series). www.brucelindbloom.com uses ASTM, ICC profiles use the ICC values. There are slight differences between them. I've used the ASTM values.

It never ends, does it? 😖 We should make sure we use the correct one for sRGB, and maybe provide the others as well. Maybe group them in modules. I don't know. At least make sure sRGB conforms to its standard for this PR, and maybe improve the rest later. We must draw the line somewhere.

[sidred]

Does everything look good. As as far as I can see I have made all the recommended changes.

[sidred]

I don't know. At least make sure sRGB conforms to its standard for this PR, and maybe improve the rest later. We must draw the line somewhere.

There are very small differences between, like in the 4th decimal place 0..3271 and 0.3272. We should be ok.

[Ogeon]

I suppose it's fine, then. I hope nobody thinks this is suitable for super high precision life-at-stake applications 😉

I'm still checking some of the new stuff, but I'm quite sure there's nothing more. Just that typo.

[Ogeon]

I can't find the d65_to_d50 test. Did you skip it?

[sidred]

Yup, not sure if it makes sense

[Ogeon]

It does, as a regression test, to check that nothing odd is going on. That, and the typo fix, should be all.

[sidred]

Let me merge all commit and take another look

[Ogeon]

Ok, just tell me when you feel ready. I'll be busy with all sorts of other things, so no hurry.

[sidred]

Fixed up a few typos and changed the docs for chromatic adaptation, Xyz( referenced only D65 ) and Yxy.

Should be good to go!

[Ogeon]

Nice! I'll, however, have to postpone my final review until tomorrow.

[Ogeon]

"It's" should be "Its". Same for z.

[Ogeon]

I found a couple of typos that I couldn't ignore, so I took the liberty to point out a couple of strange indentations as well. It's otherwise great and I would have merged it straight away.

[sidred]

Done!

[Ogeon]

Great! Thank you! It's exciting how this library has grown from the original idea of just being a simple color calculations library. It's still a color calculations library, and it's still about as simple to use, but it has so much more power. And more is coming.

@homu r+

[homu]

📌 Commit 2ed7fd2 has been approved by Ogeon

[homu]

⚡ Test exempted - status