Add feature "random" for random color generation using rand crate

[okaneco]

Implement Distribution<T> for Standard from rand for all color types, LabHue, and RgbHue
Implement SampleUniform for all color types, LabHue, and RgbHue
Color sampling is supported for gen, gen_range, and Uniform.

See #174 for design discussion
Color models are sampled based on their geometric space with special consideration for HSV (cone) and HSL (bicone).

Geometry of each color model:
Hsv and Hwb - Cone. Hwb is expressed in terms of Hsv.
Hsl - Bicone. Sampled as a random selection between two cones.
Lab, Rgb, Xyz and Yxy - Cubes and blocks. Each component can be sampled independently.
Luma - A line. A single value.
Alpha - Samples the contained color and the alpha channel independently.

Closes #174

[okaneco]

I don't know how to implement Distribution for Alpha.
I implemented Distribution for Hwb from Hsv but wasn't sure if we wanted to do that.

It was a struggle to get the UniformSampler to work with the type parameters but I guess this is one way it can be done.

[Ogeon]

This is a really good start!

I'm still trying to wrap my head around HWB. I wonder if it would make sense to sample it as a bicone, similar to HSL, but with W and B along the outer diagonals. It would be like taking the color picker triangle and making the gray scale edge the center axis. That may be closer to what one would expect from it, despite what the original paper says about its relationship with HSV.

For Alpha, I think this should work:

impl<C, T> Distribution<Alpha<C, T>> for Standard
where
    T: Component,
    Standard: Distribution<C> + Distribution<T>,
{
    fn sample<R: Rng + ?Sized>(&self, rng: &mut R) -> Alpha<C, T> {
        Alpha {
            color: rng.gen(),
            alpha: rng.gen(),
        )
    }
}

Overall, I think it may be a good idea to write short comments in the code to explain the cone sampling a bit, just for future reference. Doesn't need to be more than maybe a link to the SO page and a short explanation of how we eliminated some of the parameters.

[okaneco]

I added documentation, required Standard: Distribution<T>, implemented for Alpha, and was able to remove some of the from_f64 👍

He did say it required a small stretch of the imagination to view it as a hemisphere. The paper was interesting and enlightening about the space. Also a good reminder how we often gloss over the hue circle really being a hexagon with RGBCMY primary edges. The plates clarified a lot, especially on the last page of the paper (pg. 14). The space seems much more like a cone which can be thought of as spherical unlike HSV. We might be able to sample as a cone with whiteness as radius and blackness as height?

hue = 2 * pi * random()
whiteness =  1 - sqrt(random()) /* white is placed at center of hexagon */
blackness = random() ** (1/3)

I'm going to try working on some more of the uniform samplers which should be easier after the Rgb impl.

[Ogeon]

The documentation comments are really good! 👍 The only thing I had to comment on at the moment is the use of from_f64. I added comments with my reasoning to one of the files, but it applies to more of them.

The reason why I started to consider sampling a bicone for HWB is that whiteness and blackness have the same "importance". Taking a step back and looking at the metaphor of HWB, the idea is that you have a saturated color with some hue and mix it with a bit of white and a bit of black. That gives us a triangle between color C, white and black. In addition to that we have the restrictions that both are in the range [0, 1], but whiteness + blackness <= 1. Expressing that geometrically could look like this:

whiteness = 1
|\
| \
|  \
|   \
|-o  \ <-- our color
| |   \
-------- blackness = 1

The x and y of the point are the blackness and whiteness of the color. x + y is always <= 1 within the triangle. The problem here is that the fully saturated point is at (0, 0), so we can't just spin it around either of the axes and make a cone. The diagonal is the gray scale, so that one has to be in the center of the solid. If it's turned into a with either whiteness or blackness as the radius, the other can't be the height unless we do some additional gymnastics. If it's sampled as a bicone, we will still have to do the gymnastics, but it's at least the same for both components. I.e. find the distance from the top and bottom surfaces. This does not seem to be an easy one.

[okaneco]

Ah, I wasn't sure what you meant in reference to "diagonals" before but that makes sense now. The hardest part thinking about this the entire time is that the surface is the greyscale and the most saturated point is (0,0). You're right, it doesn't make sense to sample as a single cone but I'm struggling with the idea of a bicone for it so I'll have to think about that more.

I was thinking of each section as 1/4 of a unit circle, and you can integrate that unit circle from [0,2π) and get a hemisphere. Then black and white are equally important and the angle of whiteness and blackness always adds up to 90º. The flaw is that the saturated colors at (0,0) and not the outside. The paper mentions W is actually the complement during the description of the HWB solid. Could we model it as a hemisphere like I'm describing but instead of X being whiteness, it can be (1 - whiteness)? Y can remain blackness with no adjustment.

I changed the calculations to do sampling in T now.

I've run into a roadblock with doing the samplers for Hue types. A straight port of the other implementations to hue types looks like this but it has a few problems. Naively, I would like to get the T from a LabHue (eventually from RgbHue as well) but the value is private and there's no way to cast to a primitive it seems. There's also no PartialOrd impl which I understand why but for this use case in particular it makes generating between the range supplied very difficult since I cannot assert low < high or get Uniform<T>.

[Ogeon]

Yes, let's think some more regarding HWB. I'll keep this response somewhat short and come back to it tomorrow.

Getting the hue value should not be a problem. You have all the options. 🙂 to_raw_degrees gives the value as it is. It's the fastest option, but may create weird ranges if the difference is not within 360 degrees. to_positive_degrees normalizes it to [0, 360). There's also from/into or to_degrees for [-180, 180), but it has a more complicated normalization. Speaking of, I forgot to check that the correct unit is used. 🤦‍♂️ Hues are in degrees for familiarity's sake.

[okaneco]

I completely blanked out regarding those Hue accessors. to_positive_degrees probably makes the most sense.

Regarding the asserts, that's probably right. It would be a place to specify what value failed so I can just get rid of them in general since it's a check for all. rand will panic if those asserts aren't true. "Should we wrap it in an error?" is what I meant to get around to asking eventually.

Those suggested changes look good.

[Ogeon]

I vote for removing them until we see a need to add them with details.

[okaneco]

My naive thoughts for HWB are with the hemisphere modeling. The sampling area would be the positive quadrant of the unit circle.

We generate a radius like we did for saturation with a magnitude between 0 and 1.

magnitude = sqrt(random())

Next, we generate an angle [0,90º].

theta = random() * pi/2

Finally we take the sin() and cos() of the angle and multiply it by the magnitude to get the whiteness and blackness.

(sin, cos) = sin_cos(theta)
whiteness = magnitude * cos
blackness = magnitude * sin
hue = 2 * pi * random()

[Ogeon]

The challenge with that approach is to uphold the whiteness + blackness <= 1 constraint. sin(45deg) = cos(45deg) = 0.70710678119, so it would need to be compensated for. Except for the question if that's the shape we want to model it as.

I started writing out this long argument for using a bicone because of how color pickers work, when I realized that the sampling function we are using doesn't care about the shape of the cone. What I mean is that the probability of picking a point along the radius isn't affected by the position along the height, and vice versa.

I'm now in such a deep mathematical rabbit hole that I'm not sure what's up or down, but it seems like we can use plain cone sampling and affine transforms to sample all three of HSV, HSL and HWB. The difference between them is how we transform the sampled height and radius into different triangle shapes before extracting the components.

With that in mind, sampling a bicone for HSL becomes this:

let h = Float::powf(rng.gen(), 1.0 / 3.0);
let saturation = h * Float::sqrt(rng.gen());
// Reduces the lightness towards 0.5 if we are closer to full saturation. Now, it's a bicone!
let lightness = h - saturation * 0.5; 

Quite similar to how HSV is converted into HSL. Makes me wonder if I'm missing something or if it's just because we are starting out with an anonymous cone.

I made a quick and dirty plot and immediately lost it when I tried to resize it, but it looked like it worked. I'll come up with something better later.

By the same logic, we can just straight up use the HSV cone for HWB and convert it as usual, because no matter which cone or bicone shape we sample, they can all be transformed into the same cone. It should be simple enough to do that and hopefully also to invert it when we want to sample ranges.

Either way, I'm out of brain juice for now.

[okaneco]

I'm not sure what makes sense for the uniform samplers in HSV and HSL. Should they be sampled as a straight range between [low, high] or take into account the cone sampling (or even a frustum)? gen uses Standard as the distribution and gen_range uses Uniform.

Current provisional implementation is treating each parameter as a uniform range. The UniformHsv holds three ranges and implements UniformSampler's new, new_inclusive, and sample.

palette/palette/src/hsv.rs

Lines 717 to 726 in 3b19028

	pub struct UniformHsv<S, T>
	where
	T: FloatComponent + SampleUniform,
	S: RgbSpace + SampleUniform,
	{
	hue: Uniform<T>,
	saturation: Uniform<T>,
	value: Uniform<T>,
	space: PhantomData<S>,
	}

// Uniform::new() -> UniformHsv
 UniformHsv {
    hue: Uniform::new::<_, T>(
        low.hue.to_positive_degrees(),
        high.hue.to_positive_degrees(),
    ),
    saturation: Uniform::new::<_, T>(low.saturation, high.saturation),
    value: Uniform::new::<_, T>(low.value, high.value),
    space: PhantomData,
}

// Uniform::new_inclusive() -> UniformHsv
UniformHsv {
    hue: Uniform::new::<_, T>(
        low.hue.to_positive_degrees(),
        high.hue.to_positive_degrees(),
    ),
    saturation: Uniform::new_inclusive::<_, T>(low.saturation, high.saturation),
    value: Uniform::new_inclusive::<_, T>(low.value, high.value),
    space: PhantomData,
}

// Uniform::sample() -> Hsv
Hsv {
    hue: (self.hue.sample(rng)).into(),
    saturation: self.saturation.sample(rng),
    value: self.value.sample(rng),
    space: PhantomData,
}

We can store a tuple of the lower and upper bounds, and one range which can be high-exclusive or inclusive.

pub struct UniformHsv<S, T> {
    hue: Uniform<T>,
    saturation: (T, T),
    value: (T, T),
    range: Uniform<T>,
    space: PhantomData<S>,
}

// Uniform::new(), inclusive would change the range field to be new_inclusive
UniformHsv {
    hue: Uniform::new::<_, T>(
        low.hue.to_positive_degrees(),
        high.hue.to_positive_degrees(),
    ),
    saturation: (low.saturation, high.saturation),
    value: (low.value, high.value),
    range: Uniform::new(from_f64::<T>(0.0), from_f64::<T>(1.0)),
    space: PhantomData,
}

Then sampling would be performed inside a smaller cone. The logic is

• sample from a cone as in Standard sample
• multiply the sample by the difference between high and low
• add that value to low

fn sample<R: Rng + ?Sized>(&self, rng: &mut R) -> Hsv<S, T> {
    let saturation = Float::sqrt(self.range.sample(rng))
        * (self.saturation.1 - self.saturation.0)
        + self.saturation.0;
    let value = Float::powf(self.range.sample(rng), from_f64::<T>(1.0) / from_f64::<T>(3.0),)
        * (self.value.1 - self.value.0)
        + self.value.0;
    Hsv {
        hue: (self.hue.sample(rng)).into(),
        saturation,
        value,
        space: PhantomData,
    }
}

By a similar logic for HSL, use the bicone sampling to sample between the low and high HSL points.

// Uniform::sample()
fn sample<R: Rng + ?Sized>(&self, rng: &mut R) -> Hsl<S, T> {
    let h = Float::powf(self.range.sample(rng), from_f64::<T>(1.0) / from_f64::<T>(3.0));
    let saturation =
        h * Float::sqrt(self.range.sample(rng)) * (self.saturation.1 - self.saturation.0)
            + self.saturation.0;
    let lightness = (h - saturation * from_f64::<T>(0.5))
        * (self.lightness.1 - self.lightness.0)
        + self.lightness.0;

    Hsl {
        hue: (self.hue.sample(rng)).into(),
        saturation,
        lightness,
        space: PhantomData,
    }
}

HWB is still an annoying case here which I assume will use the HSV implementation.

[Ogeon]

I feel like the least surprising behavior is to sample each component as independently as possible for Uniform. It becomes weird because there are so many ways of doing it with three dimensions, but I'm thinking that if someone want a specific behavior that is not covered by this, the Mix trait or gradients, it's still possible to make custom implementations. This doesn't close the door to making smarter alternatives later, but I think for this one it may be better to keep it as simple as possible. Except for the cone sampling, that is.

Getting evenly spaced colors on a line between two other colors is as simple as color_a.mix(&color_b, rng.gen()) and gradients give even more control.

It's going to be tricky enough as it is, because I'm pretty sure we have to invert the sampling weights to find the real limits for each component. For example, when we do x = random.pow(1/3) (cube root of random) we have to invert it to become random = x.pow(3) before plugging it into Uniform::new. So we find the limits for the random variables by doing the cone transform backwards, and then do it forwards again on the new random values. Otherwise it's going to become linear anyway. There could probably be some code sharing there. Something like hsv_from_random_values(r1, r2, r3).

I have thought some more about why it seem to work and I think it's the same as when sampling a triangle. It can be done as a deformed unit triangle. Based on https://stackoverflow.com/q/4778147 and https://math.stackexchange.com/q/18686, but picking A = (0, 0), B = (0, 1), C = (1, 1) gives us:

(  sqrt(r1) * r2  ,  sqrt(r1) * (1 - r2) + sqrt(r1) * r2  ) =
(  sqrt(r1) * r2  ,  sqrt(r1) - sqrt(r1) * r2 + sqrt(r1) * r2  ) =
(  sqrt(r1) * r2  ,  sqrt(r1)  )

I picked A = (0, 0), B = (0, 1), C = (1, 1) because it's the same shape as the cone we are sampling. sqrt(r1) is the scale of the triangle and has to be the square root because the area grows with scale * scale. r2 is the distance from the y axis, so let's call it radius. Substituting them to make it more generic, gives us x = scale * radius, y = scale. It's the same recipe as for the cone sampling, but a cone is three dimensional and circular, which make scale = powf(r1, 1.0/3.0) and radius = sqrt(r2).

What's happening is basically that we are sampling points on a disc that has been offset from the center by 1 along one axis. Then we sample a random scale for that "scene", making the infinite number of discs describe a cone. That's also why what I described above, with shearing it into a bicone, works. Making h = scale - radius * 0.5 makes it sample a cone/funnel surface instead of a disc.

So, the correct (I made mistakes above) way to get HSL becomes

let r1 = rng.gen();
let r2 = rng.gen();
let r3 = rng.gen();
let scale = powf(r1, 1.0/3.0); // It's the cube root because the volume grows by the scale^3
let radius = scale * sqrt(r2);
let h = scale - radius * 0.5;

// To get the saturation we need to find out what the max radius is at `h`.
// We do know `scale - scale * sqrt(r2) * 0.5 = x - x * 0.5`, where `x - x * 0.5` comes
// from substituting `scale -> x, sqrt(r2) -> 1.0` in
// `scale - radius * 0.5 = scale - scale * sqrt(r2) * 0.5`. This describes a scenario
// where the RNG gives us a point at the surface of the bicone at height `h`.
// The fact that `x` is both the scale and the radius makes it really easy for us:
// `2 * scale - scale * sqrt(r2) = scale * (2 - sqrt(r2)) = x`

let maxRadius = scale * (2 - sqrt(r2));

let hue = 360.0 * r3; // Remember, we are working with degrees. :p
let saturation = radius / maxRadius;
let lightness = h;

The next trick is to do it backwards, to find r1 and r2 for Uniform::new. scale and radius are mixed together in both saturation and lightness, so it's not super straight forward. Expanding radius and maxRadius gives us;

saturation = scale * sqrt(r2) / scale * (2 - sqrt(r2)) = sqrt(r2) / (2 - sqrt(r2));
sqrt(r2) = saturation * (2 - sqrt(r2))
sqrt(r2) + sqrt(r2) * saturation = saturation * 2
sqrt(r2) * (1 + saturation) = saturation * 2
sqrt(r2) = saturation * 2 / (1 + saturation)

Now that we know that, we can get scale from lightness = scale - scale * sqrt(r2) * 0.5:

lightness = scale  - scale * sqrt(r2) * 0.5 = scale * (1 - sqrt(r2) * 0.5)
scale = lightness / (1 - sqrt(r2) * 0.5)

then we can get r1 = scale * scale * scale and r2 = sqrt(r2) * sqrt(r2) and use them as u1 = Uniform::new(r1_min, r1_max) and so on.

Everything could also be optimized to skip a few steps, but it was easier to show this way. Please double check that I got it right, though.

[okaneco]

let hue = 360.0 * r3; // Remember, we are working with degrees. :p

🤦‍♂ I fixed the other hue types with the same error

It's going to be tricky enough as it is, because I'm pretty sure we have to invert the sampling weights to find the real limits for each component.

I knew we probably had to do this but was at a loss for the proper "expansion" factors.

I double-checked the math on paper and it looked good. But I'm not sure I got the calculations for Uniform::new() and reverse-solving for r1 and r2 correct.

I solved for scale with low.lightness and r2 = 0 for r1_min. Then solved again for high.lightness and r2 = 1 for r1_max.

// scale = lightness / (1 - sqrt(r2) * 0.5), with r2 = 0 denominator drops out
let scale_low = low.lightness;
let r1_min = scale_low * scale_low * scale_low;
let scale_high = high.lightness * from_f64::<T>(2.0);
let r1_max = scale_high * scale_high * scale_high;

[Ogeon]

r2 = 0 and r2 = 1 are special cases where the linear and square root curves meet. sqrt(0) = 0 and sqrt(1) = 1, so those will look simple. Any point between them will require squaring to compensate for the square root. Either way, the R2 random variable may be any value between 0 and 1, as it's connected to the saturation, so we can't make any assumption for min and max. It's also not linear in relation to the saturation, according to the formula I found.

Although, playing around with wolframalpha (I have never had this much use for it before!), I realize there's something off with the solutions. I decided to let it solve the equations for me:

• It seem to be correct for finding the max radius, but it's written differently.
• Same thing with saturation -> sqrt(r2). x is for scale and r is for sqrt(r2) there.
• lightness -> scale is on a different form, but seem to check out. s is for scale and r is once again for sqrt(r2) there.

Still, the result for scale, when putting it all together, isn't what it should be. It goes as high as 2 when lightness = 1 and saturation = 1. Turns out the max radius is different when lightness > 0.5.

I/we determined that h = max_radius - max_radius * 0.5 = max_radius * 0.5 (that equation could have been solved in an easier way...), meaning that max_radius = h * 2. You can probably see the problem here. When h goes above 0.5, the max radius goes above 1. So... max radius should really be:

let max_radius = if h <= 0.5 {
    h * 2.0
} else {
    (1.0 - h) * 2.0
};

That's familiar. Luckily lightness = h, still, so we can use the exact same logic for the inverted version, but swapping h for lightness. A side note here is that this whole thing is going to break down for any lightness outside the [0, 1] span, so we may have to start it all by clamping the input.

Unfortunately, trying to pry sqrt(r2) out of scale * sqrt(r2) / ((1.0-h) * 2.0) is not as easy. scale is not magically eliminated this time. However, plugging sqrt(r2) = saturation *(2 - 2 * lightness) / scale into lightness = scale - scale * sqrt(r2) * 0.5 gives us scale = saturation + lightness - saturation * lightness for when saturation * lightness != saturation + lightness. We should not hit that edge case as long as lightness is within (0.5, 1] and saturation is within [0, 1]. The scale is also always >= 0.5, so all is good for substituting it back and finding sqrt(r2).

So, assuming I got the math right this time, it would have to be something like this:

fn random_values_from_sl<T>(saturation: T, lightness: T) -> (T, T) {
    let (sqrt_r2, scale) = if lightness <= 0.5 {
        let sqrt_r2 = saturation * 2.0 / (1.0 + saturation);
        let scale = lightness / (1.0 - sqrt_r2 * 0.5);
        (sqrt_r2, scale)
    } else {
        let scale = saturation + lightness - saturation * lightness;
        let sqrt_r2 = saturation * 2.0 * (1.0 - lightness) / scale;
        (sqrt_r2, scale)
    };

    (scale * scale * scale, sqrt_r2 * sqrt_r2)
}

Here is a plot of scale for lightness <= 0.5.
Here is a plot of scale for lightness >= 0.5. Note that the lowest point in this one is 0.5, not 0.

For code structure, I would suggest encapsulating it in a function, like this. I think that's going to help with verifying too, as random_values_from_sl(random_values_to_sl(r1, r2)) == (r1, r2) (with rounding errors), so it would be possible to feed it with a bunch of numbers and check that it properly inverts them.

It would also make it easy to check that some input gives the right output by just factoring out the RNG. It would be similar to the assert_ranges macro for clamping. It could be used for inspiration, I guess.

Phew, I hope this is it! The idea for verifying, like above, should catch these types of problems. It would possibly make sense to go back to the double cone variant we had at the beginning, for the symmetry. No idea if it's easier or not, and I don't have the energy to check the math at the moment.

[Ogeon]

Ugh, I couldn't let it go. I had to give it a try. The fact that we can't get away from branching makes me think it's pointless to be clever.

Let's encapsulate the cone sampling in a function like this:

fn sample_cone<T>(r1: T, r2: T) -> (T, T) {
    let h = Float::cbrt(r1);
    let r = h * Float::sqrt(r2);

    (h, r)
}

Note that I found a dedicated function for the cube root. The inverse of this is

fn invert_cone_sample<T>(h: T, r: T) -> (T, T) {
    let sqrt_r2 = if h > 0.0 { r / h } else { 0.0 }; // Who knows what we had...
    (h * h * h, sqrt_r2 * sqrt_r2)
}

Easy! Now for the HSL trickery. I'm going to use r1 to also determine if it's upper or lower cone. r1 <= 0.5 is lower and r1 > 0.5 is upper.

fn sample_hsl<T>(r1: T, r2: T, r3: T) -> Hsl {
    let (saturation, lightness) = if r1 <= 0.5 {
        let r1 = r1 * 2.0; // Scale it up to [0, 1]
        let (h, r) = sample_cone(r1, r2);
        (r / h, h * 0.5) // Scale the lightness back to [0, 0.5]
    } else {
        let r1 = (1.0 - r1) * 2.0; // Scale and shift it to [0, 1).
        let (h, r) = sample_cone(r1, r2);
        (r / h, (2.0 - h) * 0.5) // Turn the cone upside-down and scale the lightness back to (0.5, 1.0]
    };

    //...
}

Since the cone is sampled with the tip at h = 0 and base at h = 1, r1 is reversed in the second branch to not sample the base (lightness = 0.5) in both branches. Probably negligible, but still wanted to try. Now, time to invert it:

fn invert_hsl_sample<T>(Hsl { hue, saturation, lightness, ... }) -> (T, T, T) {
    let (r1, r2) = if lightness <= 0.5 {
        let h = lightness * 2.0; // Scale it up to [0, 1]
        let r = saturation * h;
        let (r1, r2) = invert_cone_sample(h, r);
        (r1 * 0.5, r2) // Scale r1 back to [0, 0.5]
    } else {
        let h = 2.0 - lightness * 2.0; Turn the cone the right way up and scale to [0, 1).
        let r = saturation * h;
        let (r1, r2) = invert_cone_sample(h, r);
        (1.0 - r1 * 0.5, r2) // Scale and shift r1 back to [0, 1).
    };

    // ...
}

There's a lot of potential for further simplification here, like once again eliminating h from saturation, but this is essentially how I reasoned about it. Here's the inlined and simplified revert:

fn invert_hsl_sample<T>(Hsl { hue, saturation, lightness, ... }) -> (T, T, T) {
    let r1 = if lightness <= 0.5 {
        // ((x * 2)^3) / 2 = x^3 * 4.
        // lightness is multiplied by 2 to scale it up to [0, 1], becoming h.
        // h is cubed to make it r1. r1 is divided by 2 to take it back to [0, 0.5].
        lightness * lightness * lightness * 4.0
    } else {
        let h = 2.0 - lightness * 2.0; Turn the cone the right way up and scale to [0, 1).
        let r1 = h * h * h;
        1.0 - r1 * 0.5 // Scale and shift r1 back to [0, 1).
    };

    // saturation is first multiplied, then divided by h before squaring.
    // h can be completely eliminated, leaving only the saturation.
    let r2 = saturation * saturation;

    // ...
}

It's much easier to follow, even if the upper cone branch can't be simplified as much. Not from what I can see, anyway. I like this one much more than the monstrosity in my previous post.

[Ogeon]

Sorry for spamming. The upper cone branch (lightness > 0.5) can be written as

let x = lightness - 1.0;
x * x * x * 4.0 + 1.0

It's the same as the lower cone branch, but in negative space. It works partly because a negative number cubed is still negative, so when it's brought up to positive space, it's already upside-down as I wanted it. Se also my wolframalpha check that they are the same.

That's it from me today.

[okaneco]

It took a little while to get back in the mindset of this after a few days. I moved the cone sampling into its own random sampling module and added tests to verify sampling HSL and inverting it would return the same values. I used the functions for cone sampling in the Hsv and Hsl UniformSamplers.

[Ogeon]

Neat! There are a couple of operations that can easily be removed, as mentioned. It's nice if we can avoid dividing by values that may be 0, or even avoid dividing at all.

For HWB, I think it could be implemented in terms of HSV. I.e. sample HSV and convert. It would be the same operations, as far as I understand it.

[okaneco]

I removed those two cone sampling/inverting helper functions and inlined the operations to their call locations. The extra h multiplication was a special case for the Hsl sampling and not necessary for Hsv if I remember correctly.

Changed Lch chroma to be sampled like Saturation in Hue types where the radius is squared for Uniform::new and then square-rooted to sample in Uniform::sample.

Did a first pass of the UniformSampler for Hwb in terms of Hsv.

[Ogeon]

Nice to see that this is getting close to completion! I would just like to suggest that the uniform sampler for HWB could wrap the uniform sampler for HSV, instead of picking it apart.

The h cancels out in both cases, as I understand it. In both cases, the saturation goes to 0 closer to the tip of the cones. That's what multiplying the radius with h would do, but since the saturation range doesn't actually shrink towards the tips, we have to divide by h in both cases to normalize it to the [0, 1] range. That's when it cancels out.

Also, I think since this covers all of the rand integration, you could mark this PR as closing the issue for it.

[okaneco]

I think that's closer to how the Hwb sampler should be. I've marked the opening comment to mention it closes the rand issue.

[Ogeon]

Looks good! It has been quite the ride. Turns out that the motivation for the algorithms are more complicated than the algorithms themselves. Thanks for seeing it through, this far.

I don't know how much more verification it need. It would be with the fake RNG, if anything. Having random unit tests is not the best. The way it uses the random values are quite predictable, so it would perhaps be possible to make the RNG return all zeros or all ones to test the boundaries, for example.

[okaneco]

It was quite an effort but I'm glad to have worked through it all.

I'm not inclined to add any random unit tests either since I've seen it cause spurious failures/errors that end up (having to be) ignored. I added a unit test for the sample_hsv/hsl functions with full 0s and 1s. It is fairly straight forward so not sure what other verification is necessary other than covering the custom sampling functions we had to make for cones.

[Ogeon]

There's probably not much more to it now. I just thought it may be useful to have them as sanity checks, just to make sure they and the other color spaces stay within the expected ranges. In case they are refactored.

I really don't want to pile anything more onto this, but something I just realized had slipped my mind is that the hue types should have their own random sampling. Would you mind adding that as a final feature addition? If you would rather want to get this off your shoulders, we can split that off as a separate improvement.

[Ogeon]

I like to keep things more bite sized, just to avoid it becoming exhausting and eating too much of people's free time. Hence the checklist and recommendation to start small earlier. 🙂 So I just want to say that I can definitely sympathize if you would prefer to call it done for now with what we have.

[okaneco]

I don't mind adding that. What do you have in mind for the hue sampling? Specific functions forRgbHue and LabHue?

[Ogeon]

Just implementing Distribution and SampleUniform for them. That would also make it possible to replace any (simplified) instance of hue: rng.gen() * 360 with just hue: rng.gen().

The hue types are created in a macro that duplicates the implementations, but I think they can probably just share the same uniform sampler. Something like this may work:

// Outside the macro. H will be the hue,
// but I think we need a place for the float type T too.
impl<H, T> UniformSampler for UniformHue<H, T>
where
    T: Float + SampleUniform,
    H: From<T>,
{
    type X = H;
    // ...
}

// Inside the macro
impl<T> SampleUniform for $name<T>
where
    T: Float + SampleUniform,
{
    type Sampler = UniformAlpha<Self, T>;
}

[okaneco]

I'll try working on that.

---

Unrelated to this PR but going through all of this makes me wonder if in the future it'd make sense to organize the colors into modules. Put all the hue types together and group the Rgb family together. It's slightly more overhead but conceptually helps differentiate the "kind" of the color you're working with. It's easy to overlook some of the characteristics of types when you're implementing something for all of them at once. A rough sketch:

cie
    lab
    lch
    xyz
    yxy
rgb
    hue
        hsl
        hsv
        hwb
    rgb

or

cie
    lab
    xyz
    yxy
hues
    cie
        lch
    rgb
        hsl
        hsv
        hwb
// doesn't change
rgb 
    rgb

[Ogeon]

Yeah, it may be time to group a few more things. Preferably by "family" rather than some property of them. The file structure doesn't have to be the way the public modules are exposed, so it could still be more flat from the user's perspective. Not sure what's best when trying to find things. But that's a separate topic. 🙂

[Ogeon]

Great! I think it looks good to be merged, except for the small mistake I pointed out.

[okaneco]

Fixed the new_inclusive, ran cargo fmt again, and left a comment on the unit test to explain what's being checked.

[Ogeon]

I got sidetracked by work related stuff, so sorry for the delay. This looks good! I like the little test macro too. Tank you, once again, for this. I hope you get some well deserved brain (and body) rest after this.

bors r+

[bors]

Build succeeded

• continuous-integration/travis-ci/push