Add `Okhsv`, `Okhsl` and `Okhwb` #282

NeverGivinUp · 2022-05-12T12:25:05Z

update OKLab-XYZ transformation coefficients
add Okhsl and Okhsv color spaces

Closed Issues

Closes Add Okhsv, Okhsl and Okhwb color spaces #258, by implementing Okhsl and Okhsv

Breaking Change

The updated matrix will change colors, if those colors have been selected visually (e.g. using a hsl color picker) and stored as Oklab. If they have been stored in any other format, a transformation to Oklab should produce more correct colors.

* added OKhsl and Okhsv color spaces

NeverGivinUp · 2022-05-12T14:41:57Z

I don't know much about color in general and palette's structure in particular. So I expect this PR to require very, very serious corrections and refactoring. Be nice, but please be clear. Don't sugar coat.

Ogeon

Thanks a lot for opening this PR! I think it looks good from a first look-through but I need to wrap my head around more of the details to be able to give a deeper review. I have at least added some surface level comments to begin with. Note that I didn't comment on every occasion of, for example, a print that will has to be removed before merging, but it applies for everything.

You pointed out that you wanted more info on the conversion macro. The macro is essentially a substitute for specialization, and connects all color types with each other. The manual implementations make up a tree of "closest neighbors", with Xyz as the root, that are used as the foundation. It then figures out the appropriate conversion implementations to delegate to for all other cases. For example to go from Rgb to Lch it finds Rgb -> Xyz -> Lab -> Lch.

There is a list of all the color types and their "closest neighbors" towards Xyz in palette_derive/src/lib.rs. You will have to add these two types and say that they should be connected to Oklab. The Ok* colors are also a bit special since they only use the D65 white point, so you will have to add them as exceptions in a few places. You can look at Oklch as a reference and search for it in the macro files. I hope it helps, but let me know otherwise.

It would also be super if we can have a counterpart to HWB, which is just a different form of HSV. I know the original blog post doesn't mention it but I think we can add it as a custom extension since it's such an easy thing to implement. It's just a bit tedious. You can also skip it if you don't feel like it and just not close the the issue this time around. I or someone else can add it afterwards.

Another thing that we can keep as extra credits is to make the implementation SIMD friendly. This means replacing if/if else/else with lazy_select! and friends. If you don't want to get into that, you can just add HasBoolMask<Mask=bool> to every bounds that has PartialOrd or PartialEq. That signals that we don't expect SIMD types (for now).

Let me know if you need some more info or help! I'll be able to do more during the weekend but can always answer questions.

Ogeon · 2022-05-12T16:21:35Z

palette/src/okhsl.rs

@@ -0,0 +1,626 @@
+use approx::AbsDiffEq;
+use std::fmt::Debug;


Just a reminder to change this to core or gate behind the std feature.

Ogeon · 2022-05-12T16:25:59Z

palette/src/okhsl.rs

+    /// * 240° to a kind of blue (RBG #00aefe).
+    ///
+    /// For s == 0 or v == 0, the hue is irrelevant.
+    pub h: T,


I would prefer if we can use a hue type here. I have to check what exactly is going on in the conversion but it looks like it's just the oklab hue, but rotated 180 degrees. Unless the negation of a and b is an error or I'm misunderstanding.

Also, prefer full names over single letters, so hue rather than h, etc.

There is something strange about Okhsl's hue.

Björn Ottosson's blog starts

To derive Okhsv, we will start with OkLCh, use its estimate for hue, hhh, as is

which sounds like Okhsv should have the same hue as OklCh, which is defined as h∘=atan2(b,a)

The reference implementation however computes and returns the hue as
float h = 0.5f + 0.5f * atan2f(-lab.b, -lab.a) / pi;
The unit here is degrees/360 and there is the 180° rotation, you mentioned.

I changed that to normal degrees, but didn't know what to make of the rotation. Online color pickers, that just copied the Javascript implementation of the interactive demo stick to the rotation. The CSS Color Modul Level 4 draft defines Oklab, but not Okhsl or Okhsv.

What format do you suggest for the hue?

That actually explains it all, it's all the same hue. The addition of 0.5 corrects it all by adding back the missing 180 degrees. It's probably related to the scaling. I also found this where they don't negate a and b: https://github.com/Evercoder/culori/blob/main/src/okhsl/convertOklabToOkhsl.js

But that means we should use OklabHue<T> as the hues.

Ogeon · 2022-05-12T16:33:01Z

palette/src/okhsl.rs

+    T: Real + Zero + One + PartialOrd + Debug + Copy + Arithmetics,
+{
+    /// Create an Okhsl color.
+    // FIXMe: cannot make constructor constant because zero and one are not constant


Constructors in this lib are usually simple and don't perform any logic, since there are some cases where having "strange" values is useful. Arithmetic calculations, for example. The IsWithinBounds trait is meant for validating the values.

You can remove all the corrections and asserts. And the print. 🙂

Ogeon · 2022-05-12T16:38:07Z

palette/src/okhsl.rs

+
+/// # See
+/// See [`okhsl_to_srgb`](https://bottosson.github.io/posts/colorpicker/#hsl-2)
+impl<T> FromColorUnclamped<Okhsl<T>> for Oklab<T>


I prefer to have the implementations for the types in their files, so it would be great if you can move this to oklab.rs.

* integrate Okhsv, Okhsl and Okhwb in conversion macro * move FromColorUnclamped implementations to module containing struct * remove println * remove assertions and corrections from constructors * refactor functions and types used in several Ok* modules in ok_ultils module * use OklabHue type for hues of Ok* * rename fields of Ok* to full names * replace std by core

* convert variables to snake case * added missing comments * commented out unused assignment

Ogeon · 2022-05-14T10:44:24Z

palette/src/oklab.rs

+//FIXME:https://colorjs.io/docs/spaces.html#oklab uses different values to the documentation above and these min- and max- values
+// From my understanding Chroma in Oklab is unlimited, as it is only limited, when used in reference to the gamut of a specific  display technology.
+// So here, a and b should be unlimited. Only in Okhsl and Okhsv should chroma be limited, as they reference the sRGB gamut.
+// For a different Okhsl and Okhsv, which references HDR (https://bottosson.github.io/posts/colorpicker/#ideas-for-future-work) they'd need to be different again.


A note on HDR is that Palette doesn't try to handle it in any special way at the moment. It's a plus if it happens to work but not necessary for now. Let's start with sRGB for now. A future extension could be to look into supporting other gamuts, maybe by generalizing over RgbSpace if that's sufficient.

So should I just remove this fixme and be done with it or should I add a comment somewhere? And if so, what should it say and where would I put it?

You can mention in the implementation, where it adjusts to the gamut, that we assume sRGB. Maybe also in the descriptions of Okhsv, Okhsl and Okhwb so it's clear what they represent. Otherwise, it's enough to refer to the reference implementation so there's something to compare to when debugging or reworking something. I appreciate that you have already added comments with clarifications in some places! It helps a lot!

Ogeon · 2022-05-14T10:48:30Z

palette/src/xyz.rs

+impl<T> FromColorUnclamped<Okhsv<T>> for Xyz<D65, T>
+where
+    Okhsv<T>: IntoColorUnclamped<Oklab<T>>,
+    Self: FromColorUnclamped<Oklab<T>>,
+{
+    fn from_color_unclamped(color: Okhsv<T>) -> Self {
+        let oklab: Oklab<T> = color.into_color_unclamped();
+        Self::from_color_unclamped(oklab)
+    }
+}
+
+impl<T> FromColorUnclamped<Okhsl<T>> for Xyz<D65, T>
+where
+    Okhsl<T>: IntoColorUnclamped<Oklab<T>>,
+    Self: FromColorUnclamped<Oklab<T>>,
+{
+    fn from_color_unclamped(color: Okhsl<T>) -> Self {
+        let oklab: Oklab<T> = color.into_color_unclamped();
+        Self::from_color_unclamped(oklab)
+    }
+}
+
+impl<T> FromColorUnclamped<Okhwb<T>> for Xyz<D65, T>
+where
+    Okhwb<T>: IntoColorUnclamped<Okhsv<T>>,
+    Self: FromColorUnclamped<Okhsv<T>>,
+{
+    fn from_color_unclamped(color: Okhwb<T>) -> Self {
+        let okhsv: Okhsv<T> = color.into_color_unclamped();
+        Self::from_color_unclamped(okhsv)
+    }
+}


These should not be necessary. This is precisely the type of code the macro should generate. Did you run into any issues with them? There may very well be a hole in its logic.

I didn't run into issues. I just added stuff without any understanding until the compiler was satisfied. Then I stopped. Still without understanding :(
I will remove them.

Let me know if the compiler complains without them (and without mentioning them in the exclusion list above), and we can figure it out. 🙂

I've removed them locally, and the compiler is fine.

Nice! Then all should be fine if it's still possible to convert to Xyz. It's a bit tricky that it doesn't complain until you try to use it somewhere. There are a bunch of tests in convert.rs that checks this but it looks like the Ok* colors are not there yet.

I shouldn't have counted our chickens before they hatched. The compiler is fine, but when running tests, 3 doc tests fail. So I will leave them in.

Can you paste the compiler output here, including hints and notes? I would like to see what it complains about.

error[E0308]: mismatched types
--> src\cast.rs:39:42
|
6 | let color = cast::from_array::<Srgb>([23u8, 198]); // Too few components.
| ---------------------------- ^^^^^^^^^^^ expected an array with a fixed size of 3 elements, found one with 2 elements
| |
| arguments to this function are incorrect
|
note: function defined here
--> C:\Users\Navid\Softwareentwicklung\rust_projects\palette\palette\src\cast\array.rs:200:8
|
200 | pub fn from_array(array: T::Array) -> T
| ^^^^^^^^^^

error: aborting due to previous error

For more information about this error, try rustc --explain E0308.
warning: derive helper attribute is used before it is introduced
--> src\convert.rs:220:3
|
11 | #[palette(
| ^^^^^^^
...
15 | #[derive(PartialEq, Debug, FromColorUnclamped, WithAlpha)]
| ------------------ the attribute is introduced here
|
= note: #[warn(legacy_derive_helpers)] on by default
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
= note: for more information, see issue #79202 rust-lang/rust#79202

error[E0277]: the trait bound Xyz: FromColorUnclamped<Okhwb> is not satisfied
--> src\convert.rs:224:28
|
15 | #[derive(PartialEq, Debug, FromColorUnclamped, WithAlpha)]
| ^^^^^^^^^^^^^^^^^^ the trait FromColorUnclamped<Okhwb> is not implemented for Xyz
|
= help: the following other types implement trait FromColorUnclamped<T>:
<Xyz as FromColorUnclamped>
<Xyz<D65, T> as FromColorUnclamped<Oklab>>
<Xyz<D65, T> as FromColorUnclamped<Oklch>>
<Xyz<Wp, T> as FromColorUnclamped<Alpha<_C, _A>>>
<Xyz<Wp, T> as FromColorUnclamped<Hsl<_S, T>>>
<Xyz<Wp, T> as FromColorUnclamped<Hsluv<Wp, T>>>
<Xyz<Wp, T> as FromColorUnclamped<Hsv<_S, T>>>
<Xyz<Wp, T> as FromColorUnclamped<Hwb<_S, T>>>
and 8 others
= note: required because of the requirements on the impl of FromColorUnclamped<Okhwb> for Rgb
= help: see issue #48214
= help: add #![feature(trivial_bounds)] to the crate attributes to enable
= note: this error originates in the derive macro FromColorUnclamped (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: the trait bound Xyz: FromColorUnclamped<Okhsl> is not satisfied
--> src\convert.rs:224:28
|
15 | #[derive(PartialEq, Debug, FromColorUnclamped, WithAlpha)]
| ^^^^^^^^^^^^^^^^^^ the trait FromColorUnclamped<Okhsl> is not implemented for Xyz
|
= help: the following other types implement trait FromColorUnclamped<T>:
<Xyz as FromColorUnclamped>
<Xyz<D65, T> as FromColorUnclamped<Oklab>>
<Xyz<D65, T> as FromColorUnclamped<Oklch>>
<Xyz<Wp, T> as FromColorUnclamped<Alpha<_C, _A>>>
<Xyz<Wp, T> as FromColorUnclamped<Hsl<_S, T>>>
<Xyz<Wp, T> as FromColorUnclamped<Hsluv<Wp, T>>>
<Xyz<Wp, T> as FromColorUnclamped<Hsv<_S, T>>>
<Xyz<Wp, T> as FromColorUnclamped<Hwb<_S, T>>>
and 8 others
= note: required because of the requirements on the impl of FromColorUnclamped<Okhsl> for Rgb
= help: see issue #48214
= help: add #![feature(trivial_bounds)] to the crate attributes to enable
= note: this error originates in the derive macro FromColorUnclamped (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: the trait bound Xyz: FromColorUnclamped<Okhsv> is not satisfied
--> src\convert.rs:224:28
|
15 | #[derive(PartialEq, Debug, FromColorUnclamped, WithAlpha)]
| ^^^^^^^^^^^^^^^^^^ the trait FromColorUnclamped<Okhsv> is not implemented for Xyz
|
= help: the following other types implement trait FromColorUnclamped<T>:
<Xyz as FromColorUnclamped>
<Xyz<D65, T> as FromColorUnclamped<Oklab>>
<Xyz<D65, T> as FromColorUnclamped<Oklch>>
<Xyz<Wp, T> as FromColorUnclamped<Alpha<_C, _A>>>
<Xyz<Wp, T> as FromColorUnclamped<Hsl<_S, T>>>
<Xyz<Wp, T> as FromColorUnclamped<Hsluv<Wp, T>>>
<Xyz<Wp, T> as FromColorUnclamped<Hsv<_S, T>>>
<Xyz<Wp, T> as FromColorUnclamped<Hwb<_S, T>>>
and 8 others
= note: required because of the requirements on the impl of FromColorUnclamped<Okhsv> for Rgb
= help: see issue #48214
= help: add #![feature(trivial_bounds)] to the crate attributes to enable
= note: this error originates in the derive macro FromColorUnclamped (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to 3 previous errors; 1 warning emitted

For more information about this error, try rustc --explain E0277.
Couldn't compile the test.

error[E0277]: the trait bound Xyz: FromColorUnclamped<Okhwb> is not satisfied
--> src\convert.rs:91:28
|
7 | #[derive(PartialEq, Debug, FromColorUnclamped)]
| ^^^^^^^^^^^^^^^^^^ the trait FromColorUnclamped<Okhwb> is not implemented for Xyz
|
= help: the following other types implement trait FromColorUnclamped<T>:
<Xyz as FromColorUnclamped>
<Xyz<D65, T> as FromColorUnclamped<Oklab>>
<Xyz<D65, T> as FromColorUnclamped<Oklch>>
<Xyz<Wp, T> as FromColorUnclamped<Alpha<_C, _A>>>
<Xyz<Wp, T> as FromColorUnclamped<Hsl<_S, T>>>
<Xyz<Wp, T> as FromColorUnclamped<Hsluv<Wp, T>>>
<Xyz<Wp, T> as FromColorUnclamped<Hsv<_S, T>>>
<Xyz<Wp, T> as FromColorUnclamped<Hwb<_S, T>>>
and 8 others
= help: see issue #48214
= help: add #![feature(trivial_bounds)] to the crate attributes to enable
= note: this error originates in the derive macro FromColorUnclamped (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: the trait bound Xyz: FromColorUnclamped<Okhsl> is not satisfied
--> src\convert.rs:91:28
|
7 | #[derive(PartialEq, Debug, FromColorUnclamped)]
| ^^^^^^^^^^^^^^^^^^ the trait FromColorUnclamped<Okhsl> is not implemented for Xyz
|
= help: the following other types implement trait FromColorUnclamped<T>:
<Xyz as FromColorUnclamped>
<Xyz<D65, T> as FromColorUnclamped<Oklab>>
<Xyz<D65, T> as FromColorUnclamped<Oklch>>
<Xyz<Wp, T> as FromColorUnclamped<Alpha<_C, _A>>>
<Xyz<Wp, T> as FromColorUnclamped<Hsl<_S, T>>>
<Xyz<Wp, T> as FromColorUnclamped<Hsluv<Wp, T>>>
<Xyz<Wp, T> as FromColorUnclamped<Hsv<_S, T>>>
<Xyz<Wp, T> as FromColorUnclamped<Hwb<_S, T>>>
and 8 others
= help: see issue #48214
= help: add #![feature(trivial_bounds)] to the crate attributes to enable
= note: this error originates in the derive macro FromColorUnclamped (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: the trait bound Xyz: FromColorUnclamped<Okhsv> is not satisfied
--> src\convert.rs:91:28
|
7 | #[derive(PartialEq, Debug, FromColorUnclamped)]
| ^^^^^^^^^^^^^^^^^^ the trait FromColorUnclamped<Okhsv> is not implemented for Xyz
|
= help: the following other types implement trait FromColorUnclamped<T>:
<Xyz as FromColorUnclamped>
<Xyz<D65, T> as FromColorUnclamped<Oklab>>
<Xyz<D65, T> as FromColorUnclamped<Oklch>>
<Xyz<Wp, T> as FromColorUnclamped<Alpha<_C, _A>>>
<Xyz<Wp, T> as FromColorUnclamped<Hsl<_S, T>>>
<Xyz<Wp, T> as FromColorUnclamped<Hsluv<Wp, T>>>
<Xyz<Wp, T> as FromColorUnclamped<Hsv<_S, T>>>
<Xyz<Wp, T> as FromColorUnclamped<Hwb<_S, T>>>
and 8 others
= help: see issue #48214
= help: add #![feature(trivial_bounds)] to the crate attributes to enable
= note: this error originates in the derive macro FromColorUnclamped (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to 3 previous errors

For more information about this error, try rustc --explain E0277.
Couldn't compile the test.

error[E0369]: cannot add Rgb<Srgb, {float}> to Rgb<Srgb, {float}>
--> src\lib.rs:26:37
|
9 | let whatever_it_becomes = orangeish + blueish; // Does not compile
| --------- ^ ------- Rgb<Srgb, {float}>
| |
| Rgb<Srgb, {float}>

error: aborting due to previous error

For more information about this error, try rustc --explain E0369.
error[E0277]: the trait bound Xyz: FromColorUnclamped<Okhwb> is not satisfied
--> src\lib.rs:1772:10
|
12 | #[derive(FromColorUnclamped, WithAlpha)]
| ^^^^^^^^^^^^^^^^^^ the trait FromColorUnclamped<Okhwb> is not implemented for Xyz
|
= help: the following other types implement trait FromColorUnclamped<T>:
<Xyz as FromColorUnclamped>
<Xyz<D65, T> as FromColorUnclamped<Oklab>>
<Xyz<D65, T> as FromColorUnclamped<Oklch>>
<Xyz<Wp, T> as FromColorUnclamped<Alpha<_C, _A>>>
<Xyz<Wp, T> as FromColorUnclamped<Hsl<_S, T>>>
<Xyz<Wp, T> as FromColorUnclamped<Hsluv<Wp, T>>>
<Xyz<Wp, T> as FromColorUnclamped<Hsv<_S, T>>>
<Xyz<Wp, T> as FromColorUnclamped<Hwb<_S, T>>>
and 8 others
= note: required because of the requirements on the impl of FromColorUnclamped<Okhwb> for Rgb
= help: see issue #48214
= help: add #![feature(trivial_bounds)] to the crate attributes to enable
= note: this error originates in the derive macro FromColorUnclamped (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: the trait bound Xyz: FromColorUnclamped<Okhsl> is not satisfied
--> src\lib.rs:1772:10
|
12 | #[derive(FromColorUnclamped, WithAlpha)]
| ^^^^^^^^^^^^^^^^^^ the trait FromColorUnclamped<Okhsl> is not implemented for Xyz
|
= help: the following other types implement trait FromColorUnclamped<T>:
<Xyz as FromColorUnclamped>
<Xyz<D65, T> as FromColorUnclamped<Oklab>>
<Xyz<D65, T> as FromColorUnclamped<Oklch>>
<Xyz<Wp, T> as FromColorUnclamped<Alpha<_C, _A>>>
<Xyz<Wp, T> as FromColorUnclamped<Hsl<_S, T>>>
<Xyz<Wp, T> as FromColorUnclamped<Hsluv<Wp, T>>>
<Xyz<Wp, T> as FromColorUnclamped<Hsv<_S, T>>>
<Xyz<Wp, T> as FromColorUnclamped<Hwb<_S, T>>>
and 8 others
= note: required because of the requirements on the impl of FromColorUnclamped<Okhsl> for Rgb
= help: see issue #48214
= help: add #![feature(trivial_bounds)] to the crate attributes to enable
= note: this error originates in the derive macro FromColorUnclamped (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0277]: the trait bound Xyz: FromColorUnclamped<Okhsv> is not satisfied
--> src\lib.rs:1772:10
|
12 | #[derive(FromColorUnclamped, WithAlpha)]
| ^^^^^^^^^^^^^^^^^^ the trait FromColorUnclamped<Okhsv> is not implemented for Xyz
|
= help: the following other types implement trait FromColorUnclamped<T>:
<Xyz as FromColorUnclamped>
<Xyz<D65, T> as FromColorUnclamped<Oklab>>
<Xyz<D65, T> as FromColorUnclamped<Oklch>>
<Xyz<Wp, T> as FromColorUnclamped<Alpha<_C, _A>>>
<Xyz<Wp, T> as FromColorUnclamped<Hsl<_S, T>>>
<Xyz<Wp, T> as FromColorUnclamped<Hsluv<Wp, T>>>
<Xyz<Wp, T> as FromColorUnclamped<Hsv<_S, T>>>
<Xyz<Wp, T> as FromColorUnclamped<Hwb<_S, T>>>
and 8 others
= note: required because of the requirements on the impl of FromColorUnclamped<Okhsv> for Rgb
= help: see issue #48214
= help: add #![feature(trivial_bounds)] to the crate attributes to enable
= note: this error originates in the derive macro FromColorUnclamped (in Nightly builds, run with -Z macro-backtrace for more info)

error: aborting due to 3 previous errors

For more information about this error, try rustc --explain E0277.
Couldn't compile the test.

error: cannot find macro lazy_select in this scope
--> src\macros\lazy_select.rs:4:14
|
3 | let result = lazy_select! {
| ^^^^^^^^^^^

error: aborting due to previous error

Couldn't compile the test.

Thanks! Too bad it doesn't say a lot, but I hoped it would give a hint. Anyway, I can see if I can fix it later. You gave me access after all 👍

Damn, I forgot to investigate why these are needed... 😬

Ogeon · 2022-05-14T10:56:46Z

That unused macro rule warning is new. Feel free to comment out that rule if you don't happen to need it.

NeverGivinUp · 2022-05-14T11:47:53Z

That unused macro rule warning is new. Feel free to comment out that rule if you don't happen to need it.

You mean

Error: --> palette/src/macros/casting.rs:341:5
|
341 | ($self_ty: ident < $($ty_param: ident),+ > $($rest: tt)*) => {
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
= note: -D unused-macro-rules implied by -D warnings

I am not getting that locally. When I set Rustflatgs -D warnings, the compiler is fine. Maybe because of different rust versions? I'm on a not totally current nightly by default. Oh, that test was on nightly as well. But a fresher nightly I guess

Ogeon · 2022-05-14T11:57:01Z

Exactly, this is on the latest nightly, so it's on the horizon. I probably just copy-pasted that macro, so the rule was either necessary for something else or no longer in use.

NeverGivinUp · 2022-05-14T12:00:09Z

There is a more general problem with the Ok colors:

Instead of Bjorn Ottoson's oklab_to_linear_srgb functions, I'm using palette's conversions. In each Oklab -> Okhsl/v conversion, there is an additional conversion in the find_cusp function to find the maximum combined lightness and chroma of the hue in sRGB. Before the code reaches that conversion, it is absolutely equal to the Ottoson's C-code. But that conversion introduces a difference in the order of 1e-4, which will be non-negligible for some users.

Would you like to investigate the source of the difference? Should I replace pallete's conversions by oklab_to_linear_srgb?

Ogeon · 2022-05-14T12:13:16Z

It should be possible to replace it, so it's more optimal. If you exclude Rgb from the conversion macro for Oklab (and vice versa) and implement it yourself, you can look specifically for Srgb as the rgb space, similar to this:

impl<S, T> FromColorUnclamped<Rgb<S, T>> for Oklab<T>
where
    // ...
{
    fn from_color_unclamped(rgb: Rgb<S, T>) -> Self {
        if TypeId::of::<<S as RgbStandard>::Space>() == TypeId::of::<Srgb>() {
            // Use direct sRGB to Oklab conversion
        } else {
            // Convert via XYZ
        }
    }
}

It's not beautiful but it's as good as it gets without specialization. Alternatively, it could be generalized to work with any RGB gamut, unless the process gets super complicated.

NeverGivinUp · 2022-05-14T12:19:25Z

Exactly, this is on the latest nightly, so it's on the horizon. I probably just copy-pasted that macro, so the rule was either necessary for something else or no longer in use.

With the current nightly I see the error, too. And I've commented the macro rule out. But when running the tests one fails with

error: cannot find macro lazy_select in this scope
--> src\macros\lazy_select.rs:4:14
|
3 | let result = lazy_select! {
| ^^^^^^^^^^^

error: aborting due to previous error

Couldn't compile the test.

What should I do about that?

Ogeon · 2022-05-14T12:35:17Z

Ouch, I don't think that macro is exported. You can add ignore after the backticks for now: ```ignore

NeverGivinUp · 2022-05-14T13:05:10Z

It should be possible to replace it, so it's more optimal. If you exclude Rgb from the conversion macro for Oklab (and vice versa) and implement it yourself, you can look specifically for Srgb as the rgb space, similar to this:
impl<S, T> FromColorUnclamped<Rgb<S, T>> for Oklab<T>
where
    // ...
{
    fn from_color_unclamped(rgb: Rgb<S, T>) -> Self {
        if TypeId::of::<<S as RgbStandard>::Space>() == TypeId::of::<Srgb>() {
            // Use direct sRGB to Oklab conversion
        } else {
            // Convert via XYZ
        }
    }
}
It's not beautiful but it's as good as it gets without specialization. Alternatively, it could be generalized to work with any RGB gamut, unless the process gets super complicated.

I've never worked with TypeId like that.

How do I cast?

With the following I get a non-primitive cast error:

impl<S, T> FromColorUnclamped<Rgb<S, T>> for Oklab<T>
where
    T: Real + Cbrt + Arithmetics + FromScalar + Copy,
    T::Scalar: Recip
        + IsValidDivisor<Mask = bool>
        + Arithmetics
        + FromScalar<Scalar = T::Scalar>
        + Real
        + Zero
        + One
        + Clone,
    S: RgbStandard,
    S::TransferFn: IntoLinear<T, T>,
    S::Space: RgbSpace<WhitePoint = D65> + 'static,
    <S::Space as RgbSpace>::Primaries: Primaries<T::Scalar>,
{
    fn from_color_unclamped(rgb: Rgb<S, T>) -> Self {
        if TypeId::of::<<S as RgbStandard>::Space>() == TypeId::of::<Srgb>() {
            // Use direct sRGB to Oklab conversion
            linear_srgb_to_oklab(rgb as LinSrgb<T>)
        } else {
            // Convert via XYZ
            Xyz::from_color_unclamped(rgb).into_color_unclamped()
        }
    }
}

Ogeon · 2022-05-14T13:20:23Z

It's not exactly what TypeId is typically used for, but very handy in cases like this. The compiler should also optimize out the unused branch, since it's all constant values in the comparison. You can use rgb.into_linear().reinterpret_as(). First it makes sure the color is linear (no-op for already linear colors), and then the internal .reinterpret_as() performs the actual cast and it's also a no-op. You will just have to make it public for the crate. You can have a peek at Rgb's conversion from itself where this trick is used.

…th reference implementation (and possibly higher precision) * replace macro implementations of equality with tolerances (abbs_diff_eq, relative_eq, ulps_eq) by manual implementations that care about semantic equality (e.g. black is black no matter the unused hue number) * comment out broken doc-test in lazy_select.rs, and unnecessary branches of impl_uint_casts_other and impl_eq macros

NeverGivinUp · 2022-05-18T12:28:03Z

This push highlights a problem with ´f32:´ At least for the color blue (#0000ff) the precision is insufficient to detect the correct path for the conversion to Okhsv. See ´okhsv::tests::blue´

I have contacted Björn Ottoson about this via twitter on how to handle this.

Ogeon · 2022-05-18T13:08:22Z

Thanks a lot for looking deeper into it!

# Conflicts: # palette/src/oklab.rs

NeverGivinUp · 2022-05-20T19:06:43Z

@bottosson I have written to you via Twitter about the issue with f32. (I used @projekt_kultur there). If you happen to have the time, please take a look.

@Ogeon Alternatively we could do all computations that might have accuracy problems in f64, no matter the formal type. What do you think?

NeverGivinUp · 2022-05-21T09:29:10Z

The tests failing in CI now, are because of the f32 issue.

Ogeon · 2022-05-21T10:00:41Z

Yes and I see it's not a small error. I would of course prefer if we can keep it fully generic if possible, but f64 could be an option if we can't find any other way. Although with the reference implementation using float, my interpretation is that it's meant to work with 32 bit floats too. I haven't had a good moment to get into the nitty gritty details yet, due to having had a cold and been generally busy, but there may also be some implementation difference that throws it off.

NeverGivinUp · 2022-05-21T10:25:54Z

I get the exact same value as the reference implementation (when I set const MAX_ITER: usize = 1 in ok_utils.rs line 370) . But I can't rule out an implementation error in my code.

In my (very limited understanding) the max_saturation algorithm may be mathematically correct, but is isn't numerically robust. It sequentially compares two linear equation of the normalized hue to 1.0, assuming the comparison will be clear cut and if the first two (red and green) are not clearly > 1.0 it assumes the third (blue) will be. But this assumption holds only with infinite precision.

To cope with limited precision the algorithm would either need to compare >=, or use an epsilon tolerance (like the abs_diff_eq macro) or actually compute all three equations for all three colors and go with the largest. I think the last option would be preferable, but I don't know enough about color-math to compute the equation. Neither do I know enough to tell if using an epsilon-tolerance wouldn't break anything.

I believe the reference implementation hides the error unintentionally, by doing just one optimization round. When you do more than one (my implementation currently does 2), the error becomes very visible, as the error does not lie with the optimization but with the optimization goal, which is decided before, when choosing the critical color.

Ogeon · 2022-05-21T11:19:28Z

I see, but that's promising! I think we can go with one iteration for now, so it matches the reference, and return to it later if there's any response to your questions. I'm more than happy with something that just replicates the reference/specification as a first version, since it's usually also easier to verify if it doesn't diverge. Have you seen any particular issues that using 2 iterations over 1 solved? Other than a generally larger error.

NeverGivinUp · 2022-05-21T12:03:05Z

A comment in the reference implementation is

Do one step Halley's method to get closer
this gives an error less than 10e6, except for some blue hues where the dS/dh is close to infinite
this should be sufficient for most applications, otherwise do two/three steps

I think Björn Ottoson saw the problem, but attributed it erroneously to the optimization process. Going by his recommendation I added iterations. But now I believe it is not an optimization problem and the single optimization step accidentally just hides the root problem.

I do believe 2 iterations do warrant the additional computational expense over a single iteration (40 multiplications+1 division), even for benign hues. But of course that is a matter of opinion and depends on the use case.

Personally I prefer using f64 to get a correct solution. But I can reduce the number of iterations to 1, to match the reference implementation. What should I do with the tests? Should I just comment out / remove the failing cases to make CI happy?

Ogeon · 2022-05-21T12:32:16Z

Going with what the reference does now doesn't prevent us from improving it later. But that said, I'm also open for other workarounds.

Are the reference values from the C++ implementation? Or the online color picker? The color picker uses 64 bit floats, since it's JavaScript, so some small differences are expected. But it's also running only one iteration. We can run those test with f64 anyway. That's fine!

Ogeon · 2022-05-21T12:40:06Z

...and tests with both f64 and f32, where the f32 version has a larger error can have its f32 error tolerance increased. That way it's still a guard against large divergences.

Ogeon

I didn't get as much time as I wanted, so I will have to look at the rest tomorrow. But so far, so good and thanks for organizing things into smaller files. The file lengths have been a bit annoying but I never got around to do much to shorten them (only adding macros here and there).

For Oklab, it seem like its ranges were originally based on a misunderstanding and I think it's good to be explicit about the lack of well defined bounds. I'm fine with removing some random generation and validation that assume otherwise.

I'm a bit indecisive regarding what I would like to be public or not, so I'm just letting things pass for now. I tend to avoid exposing circumstantial or single-use parts, but I'll look through that later. Probably after merging.

I'll continue looking through this tomorrow. Again feel free to remind me if I forget. Also, don't hesitate to comment here in the code if I have missed anything you have questions for or want me to have a closer look at.

palette/src/macros/lazy_select.rs

Ogeon · 2022-08-22T19:15:22Z

palette/src/num.rs

+
+    /// Returns the negative infinity (−∞).
+    #[must_use]
+    fn neg_inf() -> Self;
+
+    /// Returns the infinity (∞).
+    #[must_use]
+    fn inf() -> Self;


The idea with this trait is to not be float specific, so it may allow types that can't represent infinity. Would it be an issue to either rename these methods to something like get_min/get_max or move them to a separate trait for types with infinity representations?

If these are just for the Oklab limits, I'm more willing to drop the limits.

It's also the upper limit for the chroma of Oklch (technically they are also for CIELab and CIELch -- "The a* and b* axes are unbounded, and depending on the reference white they can easily exceed ±150 to cover the human gamut. Nevertheless, software implementations often clamp these values for practical reasons." ). The failed test in CI is exactly about the chroma of Oklch. A last-minute change I made and forgot to rerun the test for.

The doc of Real says "Numbers that belong to the real number set." How is that not float specific? Do you plan to include complex numbers? Or special kinds of real numbers that are finite only? In the later case, a trait like the following would make sense

/// Methods for the finite limits of the numbers. pub trait Limits{ /// Returns a finite number, for which `x <= Self::max_value()` holds for all comparable `x`. fn max_value() -> Self; /// Returns a finite number, for which `x >= Self::min_value()` holds for all comparable `x`. fn min_value() -> Self; }

Oklch and Oklab are more or less just different coordinate systems in the same space, so I suppose what goes for one goes for the other. Unless there's some definition that says otherwise. I think CIELab has a bit of an exception in that each axis is expected to fit in an i8 when rounded. CIELch is a bit more difficult to nail down, but the radii are based on the CIELab square.

The doc of Real says "Numbers that belong to the real number set." How is that not float specific? Do you plan to include complex numbers?

I'm referring to the mathematical real number set, which doesn't include infinity as far as I know. But it's less about number sets and more about widening the set of allowed types to anything that can be used as a substitute for floats. Not all of them has a representation of infinity, so this particular trait is very minimal.

A Limits trait could be useful but I prefer to first question why an unbounded color space has bounds. 🤔

palette/src/okhsv/alpha.rs

palette/src/okhwb/alpha.rs

Ogeon · 2022-08-22T19:45:58Z

palette/src/oklab.rs

    pub fn min_a() -> T {
-        T::from_f64(-1.0)
+        T::neg_inf()
    }

    /// Return the `a` value maximum.
    pub fn max_a() -> T {
-        T::from_f64(1.0)
+        T::inf()
    }

    /// Return the `b` value minimum.
    pub fn min_b() -> T {
-        T::from_f64(-1.0)
+        T::neg_inf()
    }

    /// Return the `b` value maximum.
    pub fn max_b() -> T {
-        T::from_f64(1.0)
+        T::inf()
    }


I'm thinking of dropping these completely if there's no well defined range for them. The idea is mostly to use them for validation or calculations that need a min/max, so infinity doesn't give much anymore.

Ogeon · 2022-08-22T19:49:56Z

palette/src/oklab.rs

+    T: RealAngle + Zero + Arithmetics + Trigonometry + Clone + PartialEq,
+{
+    /// Returns the hue, if at least one of `a` and `b` are non-zero  
+    pub fn try_hue(&self) -> Option<OklabHue<T>> {


This could be neat to have in the Hue trait. I'll think about that one. 🤔

palette/src/oklab/properties.rs

Ogeon · 2022-08-22T20:04:09Z

palette/src/oklab/random.rs

+    // `a` and `b` both range from (-1.0, 1.0)
+    // TODO: the choice for a and b is random and rather bad:
+    //  1. a and b are unlimited. Oklab can express the whole electro-magnetic spectrum
+    //  2. Oklab is a perceptual color space. It would make sense to limit random
+    //  values to the limits of human perception.
+    //  https://bottosson.github.io/posts/oklab/#luo-rigg-dataset-and-full-gamut
+    //  shows, that at least for some L, a should not be greater than 0.5, to
+    //  avoid leaving the perceivable gamut. Though it could go to -2.5.
+    //  3. If people want random sRGB values: Expressing the sRGB bounds in Oklab is
+    //  beyond my abilities. However, it is not necessary either. It can be done in
+    //  Okhsv, Okhsl or Okhwb. Maybe we should  not offer the ability in Oklab and
+    //  encourage sampling in the color spaces that are limited to sRGB gamut.


These are very good points. I don't have it fresh in mind at the moment but there is an option to sample a specific range. I think that still makes sense to keep, since I don't think there's any reason to prevent that. If a default alternative is still necessary (again, I would have to take a look later), this could be kept as is, since it allows easy scaling to other ranges by multiplication. Similar to a random function returning 0.0-1.0.

* Use impl_eq macro for Oklab instead of impl_eq_hue

Ogeon

Ok, I hope I got all of it. Let me know if I missed anything. So the general points of improvements are:

See if we can avoid pretending that Lab and Lch colors have bounds in the chroma plane. May make the (de)saturate traits a bit sad.
Remove any trait bounds that aren't strictly necessary for their impl blocks and functions. Makes things more flexible and allows const functions.
Trim/postpone any extra traits (and functions I suppose), if possible, to focus on the Ok* types in this PR. Helps with reviewing and getting this off the ground.

After that, and this has been merged, it's easier to focus on expanding the API where it's useful.

palette/src/oklch.rs

Ogeon · 2022-08-23T17:55:28Z

palette/src/rgb/rgb.rs

+            // fixme: why does the direct conversion produce relevant differences
+            // to the conversion via XYZ?


Do you think we need to investigate this before pushing the button? My guess is that it's just due to it being better code for the situation. Probably fewer steps too.

Its entirely possible, that it is just due to rounding errors, but I don't have more than a guess here, either.

That's likely a contributing factor. The conversion via XYZ doesn't use pre-defined matrices (yet), so that's a difference I know of.

Well, I'm happy that it's better, regardless. I think this bit of the comment can be removed. I also prefer to have remaining todos/fixmes in the issue tracker, rather than in code, so I would like to make sure there are none left.

I also prefer to have remaining todos/fixmes in the issue tracker, rather than in code, so I would like to make sure there are none left.

There are two issues, that I can see in the TODOs/FIXMEs

The range of colors created for distributions of Oklab and Oklch. We should resolve this before we publish the PR. And the FIXMEs/TODOs won't be necessary then.

The erroneous conversion using f32 on some CPUs (or maybe just on my machine, maybe all AMD CPUs) , that I need @bottosson 's guidance for. This has one FIXME/TODO in the code and two in the tests. I think they need to stay in the tests to avoid a misleading appearance of the tests and to be able to find the relevant tests, if and when we find a way to correct the error. As long as there is no fix, maybe we should elevate the FIXME to a warning in the documentation or even a trait bound, that allows only f64, as the error might be catastrophic for some library users.

Yes we definitely need to do something about point 1. In the (likely, it seems) case that we don't get a response for point 2, we should add a note in the documentation about it being sensitive to precision. It could be that it's not important for everyone, but it's a good idea to be a bit proactive with a warning that explains the phenomenon a bit.

Ogeon · 2022-08-23T18:00:38Z

palette/src/xyz.rs

+impl<T> FromColorUnclamped<Okhsv<T>> for Xyz<D65, T>
+where
+    Okhsv<T>: IntoColorUnclamped<Oklab<T>>,
+    Self: FromColorUnclamped<Oklab<T>>,
+{
+    fn from_color_unclamped(color: Okhsv<T>) -> Self {
+        let oklab: Oklab<T> = color.into_color_unclamped();
+        Self::from_color_unclamped(oklab)
+    }
+}
+
+impl<T> FromColorUnclamped<Okhsl<T>> for Xyz<D65, T>
+where
+    Okhsl<T>: IntoColorUnclamped<Oklab<T>>,
+    Self: FromColorUnclamped<Oklab<T>>,
+{
+    fn from_color_unclamped(color: Okhsl<T>) -> Self {
+        let oklab: Oklab<T> = color.into_color_unclamped();
+        Self::from_color_unclamped(oklab)
+    }
+}
+
+impl<T> FromColorUnclamped<Okhwb<T>> for Xyz<D65, T>
+where
+    Okhwb<T>: IntoColorUnclamped<Okhsv<T>>,
+    Self: FromColorUnclamped<Okhsv<T>>,
+{
+    fn from_color_unclamped(color: Okhwb<T>) -> Self {
+        let okhsv: Okhsv<T> = color.into_color_unclamped();
+        Self::from_color_unclamped(okhsv)
+    }
+}


Damn, I forgot to investigate why these are needed... 😬

palette/src/visual.rs

palette/src/macros/equality.rs

palette/src/okhsl/alpha.rs

palette/src/okhsl.rs

* limit VisualColor and VisuallyEqual traits to testing, move their implementations to separate files and also limit them to testing * remove neg_inf and inf methods from Real trait and add separate traits LowerBounded and UpperBounded * Remove more superfluous trait bounds -- specially Real

Ogeon · 2022-08-24T07:43:01Z

I think the questions we need to answer when it comes to the Lab and Lch colors are these:

How should the saturation trait(s) work? Will they work?
What to do with the random generation?

For clamping and validation, we can just not check the upper bound for Lch, so I see those as more or less non-issues. If there is some kind of conventional limit, we could lean on those (like with the CIE colors). Another idea is to use something like the sRGB gamut as a limit, but I don't know if that's a feasible idea.

NeverGivinUp · 2022-08-24T15:24:30Z

* How should the saturation trait(s) work?
* What to do with the random generation?

These questions and problems share common sources in implicit assumptions: Let me illustrate the problem with the current saturation trait :

A call to saturate(x) , where 0.0 < x < 1.0, returns a saturation of saturation + x * (max_saturation - saturation).

This formula has the assumptions:

max_saturation is finite, so that results are finite
max_saturation is independent of the type of number used (unlike the upper bound of the number), so that results do not depend on the type of the number
max_saturation is independent of hue and lightness, as it is not a function of hue and lightness.

These assumptions hold for (Ok)hsv and (Ok)hsl, but not for Ok/CIELab and Ok/CIELch. Ok)hsv and (Ok)hsl refer to sRGB -- a display-dependent color space and thus bounded color space, that has a non-linear, but bijective conversion. Ok/CIELab and Ok/CIELch are display-independent. Formally they are infinite. Materially they refer to the perceivable gamut. The common solution for CIELab is to limit the color space to the perceivable gamut and even ignoring some part of it, as as that part is rarely used anyway.

Limiting the color space to the perceivable gamut makes sense. Also, assumption 3 does not hold. max_saturation must be a function of hue and lightness. However, I don't know how to express that gamut in Oklab.

More generally, I think the traits themselves have the problematic, implicit assumption, that

the color space, that implements the trait already is minimal and users do not want to limit the colors to a smaller color space.

This assumption becomes most obvious when the material color space is display-independent: Some users will want to generate random colors in sRGB, others in Display P3. CSS Color 4 also suggests Prophoto, REC.2020 and Adobe 1998 RGB.

Thus from the perspective of the ideal API (not from the perspective of what I am able to implement) there should not be a single saturation trait(s) and a single random generator or a single clamp or gamut-map trait. There should be one for each display-dependent color space and every color space should implement all traits for its maximum as well as all smaller color spaces. I guess the smallest being sRGB.

Obviously I have strayed from the question of how to implement Oklab to the general library design. But the two are connected.

Ogeon · 2022-08-24T20:26:59Z

You are absolutely right and hit the nail on the head when it comes to what makes this difficult. The fundamental assumptions do not match these spaces. These traits are simplistic tools, so let's not try to make them do more than they are made for. Let's cut anything that requires substantial re-design of the API. That's for another time. If it doesn't fit the trait's model, it doesn't implement it.

The (de)saturation traits assume a finite range for the relative method. The absolute method could perhaps be implemented if we settle on a reasonable scale. Their visual uniformity helps here too. For the sake of this PR, we can just say that we don't implement them for now, and later add them again if there's a practical way of doing it. Probably by splitting the traits into relative and absolute variants.

The idea with the random generation is to be a bit better than just randomly sampling each parameter, just so whatever they get looks somewhat evenly distributed (pretty much a non-issue for Cartesian spaces, though). I think something like sampling a well specified space by default (regardless of how much it covers) has some value on its own. It's a foundation for something more advanced, like, perhaps rejection sampling. A tool, rather than a definition. That's why I suggested sampling 0 to 1 or -1 to 1 and letting the user scale them to their liking. For CIELab (and CIELch), I'm still inclined to keep it as it was, just because it has its conventional i8 compatible scale. It's also slightly out of scope for this PR.

NeverGivinUp · 2022-08-25T10:29:25Z

So my remaining todo list is

remove saturate and desaturate implementations from Oklab/ch
feel free to add more

		// fixme: why does the direct conversion produce relevant differences
		// to the conversion via XYZ?

Add Okhsv, Okhsl and Okhwb #282

Add Okhsv, Okhsl and Okhwb #282

Conversation

NeverGivinUp commented May 12, 2022

Closed Issues

Breaking Change

NeverGivinUp commented May 12, 2022

Ogeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ogeon commented May 14, 2022

NeverGivinUp commented May 14, 2022 • edited Loading

Ogeon commented May 14, 2022 • edited Loading

NeverGivinUp commented May 14, 2022

Ogeon commented May 14, 2022

NeverGivinUp commented May 14, 2022

Ogeon commented May 14, 2022

NeverGivinUp commented May 14, 2022

Ogeon commented May 14, 2022

NeverGivinUp commented May 18, 2022 • edited Loading

Ogeon commented May 18, 2022

NeverGivinUp commented May 20, 2022

NeverGivinUp commented May 21, 2022

Ogeon commented May 21, 2022

NeverGivinUp commented May 21, 2022

Ogeon commented May 21, 2022 • edited Loading

NeverGivinUp commented May 21, 2022 • edited Loading

Ogeon commented May 21, 2022

Ogeon commented May 21, 2022 • edited Loading

Ogeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ogeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ogeon commented Aug 24, 2022

NeverGivinUp commented Aug 24, 2022

Ogeon commented Aug 24, 2022

NeverGivinUp commented Aug 25, 2022

Ogeon commented Aug 25, 2022

NeverGivinUp commented Aug 31, 2022

Ogeon commented Sep 1, 2022

qmathe commented Nov 28, 2022 • edited Loading

Ogeon commented Nov 28, 2022

qmathe commented Nov 29, 2022 • edited Loading

Ogeon commented Dec 2, 2022 • edited Loading

Ogeon commented Dec 2, 2022

bors bot commented Dec 2, 2022

qmathe commented Dec 3, 2022

qmathe commented Dec 23, 2022

Add `Okhsv`, `Okhsl` and `Okhwb` #282

Add `Okhsv`, `Okhsl` and `Okhwb` #282

NeverGivinUp commented May 14, 2022 •

edited

Loading

Ogeon commented May 14, 2022 •

edited

Loading

NeverGivinUp commented May 18, 2022 •

edited

Loading

Ogeon commented May 21, 2022 •

edited

Loading

NeverGivinUp commented May 21, 2022 •

edited

Loading

Ogeon commented May 21, 2022 •

edited

Loading

qmathe commented Nov 28, 2022 •

edited

Loading

qmathe commented Nov 29, 2022 •

edited

Loading

Ogeon commented Dec 2, 2022 •

edited

Loading