Merge #344

344: Fix Oklab from Oklch hue conversion r=Ogeon a=tomcur

<!-- Describe your changes as well as possible. What has changed and why? Someone who hasn't followed previous discussions should be able to understand why this change was made and/or be able to find out why. -->

The Oklch -> Oklab conversion currently inverts the hue by swapping the a and b components. This PR corrects the calculation and adds an Oklch -> Oklab -> Oklch roundtrip test.

<!--
## Closed Issues

* Fixes #1234, by doing XYZ.
* Closes #4321, by implementing ABC.
-->

<!--
## Breaking Change

Will this pull request break dependent code? Describe how/why and give an example of how to migrate existing code to use the new version.
-->


Co-authored-by: Thomas Churchman <[email protected]>

palette/src/oklab.rs

@@ -357,13 +357,13 @@ where
     T: RealAngle + Zero + MinMax + Trigonometry + Mul<Output = T> + Clone,
 {
     fn from_color_unclamped(color: Oklch<T>) -> Self {
-        let (sin_hue, cos_hue) = color.hue.into_cartesian();
+        let (a, b) = color.hue.into_cartesian();
         let chroma = color.chroma.max(T::zero());
 
         Oklab {
             l: color.l,
-            a: cos_hue * chroma.clone(),
-            b: sin_hue * chroma,
+            a: a * chroma.clone(),
+            b: b * chroma,
         }
     }
 }

palette/src/oklch.rs

@@ -160,10 +160,82 @@ unsafe impl<T> bytemuck::Pod for Oklch<T> where T: bytemuck::Pod {}
 
 #[cfg(test)]
 mod test {
-    use crate::Oklch;
+    use crate::convert::FromColorUnclamped;
+    use crate::rgb::Rgb;
+    use crate::visual::{VisualColor, VisuallyEqual};
+    use crate::{encoding, LinSrgb, Oklab, Oklch};
 
     test_convert_into_from_xyz!(Oklch);
 
+    #[cfg_attr(miri, ignore)]
+    #[test]
+    fn test_roundtrip_oklch_oklab_is_original() {
+        let colors = [
+            (
+                "red",
+                Oklab::from_color_unclamped(LinSrgb::new(1.0, 0.0, 0.0)),
+            ),
+            (
+                "green",
+                Oklab::from_color_unclamped(LinSrgb::new(0.0, 1.0, 0.0)),
+            ),
+            (
+                "cyan",
+                Oklab::from_color_unclamped(LinSrgb::new(0.0, 1.0, 1.0)),
+            ),
+            (
+                "magenta",
+                Oklab::from_color_unclamped(LinSrgb::new(1.0, 0.0, 1.0)),
+            ),
+            (
+                "black",
+                Oklab::from_color_unclamped(LinSrgb::new(0.0, 0.0, 0.0)),
+            ),
+            (
+                "grey",
+                Oklab::from_color_unclamped(LinSrgb::new(0.5, 0.5, 0.5)),
+            ),
+            (
+                "yellow",
+                Oklab::from_color_unclamped(LinSrgb::new(1.0, 1.0, 0.0)),
+            ),
+            (
+                "blue",
+                Oklab::from_color_unclamped(LinSrgb::new(0.0, 0.0, 1.0)),
+            ),
+            (
+                "white",
+                Oklab::from_color_unclamped(LinSrgb::new(1.0, 1.0, 1.0)),
+            ),
+        ];
+
+        const EPSILON: f64 = 1e-14;
+
+        for (name, color) in colors {
+            let rgb: Rgb<encoding::Srgb, u8> =
+                crate::Srgb::<f64>::from_color_unclamped(color).into_format();
+            println!(
+                "\n\
+            roundtrip of {} (#{:x} / {:?})\n\
+            =================================================",
+                name, rgb, color
+            );
+
+            println!("Color is white: {}", color.is_white(EPSILON));
+
+            let oklch = Oklch::from_color_unclamped(color);
+            println!("Oklch: {:?}", oklch);
+            let roundtrip_color = Oklab::from_color_unclamped(oklch);
+            assert!(
+                Oklab::visually_eq(roundtrip_color, color, EPSILON),
+                "'{}' failed.\n{:?}\n!=\n{:?}",
+                name,
+                roundtrip_color,
+                color
+            );
+        }
+    }
+
     #[test]
     fn ranges() {
         // chroma: 0.0 => infinity