Need for HueType

[sidred]

I was trying the Hsl type and it seems inconsistent with the rest of the library.

1. All the other colors take floats for new, but hsl needs Hsl::new((240.0).into(), 0.0, 0.0)

2. You input the hue as 240, but you cannot use hsl.hue, you need to use hsl.hue.to_positive_degrees(). I have been looking at this code for past few weeks and I made this mistake. This is going to be a very common issue. There is no compile error, but the code will not work as expected.

3. I don't see the need for hsl + 0.1. I would rather this be a compile time error, because this is most likely a typo. The values are not on the same scale and does not make sense.

What do you think of removing the huetype and storing the hues as 0-360 floats? I understand that HueType avoids some issues in the code, but maybe we can mitigate it by using more test cases?

[Ogeon]

The hue types are mainly there for semantic purposes and the library itself would do just fine without them. I created them to

1. make a difference between linear and angular values (no beginning or end, for example),
2. prevent hues from different systems/standards from being accidentally mixed up,
3. make equality tests work more as expected for angular values (0 == 360),
4. make arithmetic operators work (or not work) more as expected for angular values.

Angles will probably never be 100% intuitive when everything is based on linear types, and removing the hue types will make some thing easier and other things harder. Regarding your points:

1. That could be made simpler with the function signature fn new<H: Into<RgbHue>>(hue: H, ...).
2. Would you mind giving a more specific example of what you are trying to do? That would help me understand the problem better.
3. This will never "make sense", unless the hue is changed to be within 0-1, which may even be more surprising than now. As to why it's even possible to add a number to Hsl; for completeness' sake. It may even help someone.

[sidred]

1. That'll reduce some friction for the users. I saw the compile error and had to look up the docs to convert float to hue, before realizing From is most likely implemented.

2. I was trying to optimize hsl -> rgb conversion and directly accessed the hue as is in the comparison and when there was an error in the output, it took a while to narrow it down.

It's going to be one of those subtle bugs, usually won't cause an error during arithmatic, but comparing or assuming the value is > 180 will be painful to debug. When you input Hsl::new(240,0,0), you tend to assume the value is still the same, even though it is mentioned in the docs.

3. Instead of hsl.hue + 20 I miss the hue part hsl + 20. Instead of showing the error here, the error will show up when I try to use the result of this somewhere. I guess the compiler will still catch it, so its not a major issue.

[Ogeon]

The thing is that every value (except +-inf and NaN) is a valid hue, since it's a point on a circle, so you can never really trust them to be within a certain range. Possibly within -360 - +360, but who knows what they can add up to? Removing the hue types will never help with that. It will, at best, move the normalization responsibility from the library to the user.

Do you think it would be helpful if there was some sort of to_raw(&self) -> T function that lets you extract the actual, untouched value?

[sidred]

RgbHue

Advantages:

• Consistent range.
• Get and set value will auto normalize.

Inconsistencies:

• Add and subtract do not normalize. So internally these values might be beyond the range and partial eq after these operations might fail.

Downsides:

• Needs explicit casts, especially from RgbHue to f32. Literal struct initialization needs into() cast. new() function can be modified to take floats. It'll be a compile error though.
• Performance issues due to a lot of unnecessary normalizations. Might add up when working on a lot of pixels (images etc.)
• Need to standardize on -180 to 180 or 0-360. One additional thing for the user to keep in mind.

Float

Advantages:

• Consistent with the rest of the library's api.
• Can be directly used as float.
• User can normalize when needed.
• No -180 to 180 or 0 -360 issues. You will just have to consider whether the value is normalized or not.

Disadvantages:

• User has to manually normalize the values and it is not apparent when it needs to be done. Easy mistake to make assuming 0-360 after addition or subtraction.

function like normalize, to_radians, from radians can be standalone functions that work on floats.

[sidred]

Here is a example comparing float vs RgbHue benchmark for 1920x1080 pixels https://gist.github.com/sidred/a13b034b39373c6c10ad

test hsl_float   ... bench:   4,538,838 ns/iter (+/- 162,499)
test hsl_rgb_hue ... bench:  14,223,136 ns/iter (+/- 1,126,534)

It feels a bit clumky to use the RgbHue type. A few things:

• Partial Ord needs to be implemented for RgbHue. Currently you cant do hsl.hue > 180.0
• hsl.hue + 30.0 works but 30.0 + hsl.hue does not

[Ogeon]

Looks like the normalization function isn't inlined. I ran it through callgrind after an attempt to make things more even, and RgbHue::into showed up on third place among the functions. It should ideally be optimized away, but that appears to not be the case.

Here is a version where both hues are normalized using non-inlined functions: https://gist.github.com/Ogeon/78f00346489acf95c3d5

It went from

test hsl_float   ... bench:   3,544,680 ns/iter (+/- 71,980)
test hsl_rgb_hue ... bench:   9,078,188 ns/iter (+/- 79,423)

to

test hsl_float   ... bench:   9,104,431 ns/iter (+/- 79,815)
test hsl_rgb_hue ... bench:   9,106,459 ns/iter (+/- 118,469)

on my machine. Maybe we should stick #[inline] hints to some of the small functions to make sure they are inlined. Doing that to both into and normalize_angle resulted in this:

test hsl_float   ... bench:   3,547,799 ns/iter (+/- 157,137)
test hsl_rgb_hue ... bench:   3,552,225 ns/iter (+/- 114,003)

As for the clunkiness:

• You can't order them if there is no beginning or end, so it doesn't really make sense without a range.
• That's an effect of making them generic. We can't implement Add<RgbHue<T>> for T, but we may be able to implement Add<RgbHue<f32>> for f32. I haven't tried. I tried and it works.

[Ogeon]

Just to clarify my view here: It's not that I'm not willing to "kill my darling", but I would rather try to improve it if possible. I think it fills a function, but it's not as good as it can be.

[sidred]

I understand that. I am thinking out loud here too. I am wondering whether it makes it any easier from a usage point of view and whether its worth the added complexity.

[sidred]

I can definitely see a use for partial ord when you only want to change something only on a particular set of hues like reds or blues.

I was mistaken on the add and subtract. When i convert to float it will be normalized, so any comparison should be ok.

[Ogeon]

Seems like we are on the same plane, then. We could make an attempt to improve it and see how it plays out. There are already a few here:

• Make hue based new generic over <H: Into<Hue<T>>>,
• make it possible to extract the raw hue value,
• add #[inline] hints (not only for hues),
• implement Add<RgbHue<f32/64>> for f32/64.

We could also rename to_positive_degrees to be shorter. Naming is hard.

I can also see the use for partial ord, but I'm not sure how it should be defined to make sense. An alternative would be some kind of range check to see if it falls within x..y.

[sidred]

I just moved the normalize and clamp from benchmark to the palette crate and I still see the same results

test hsl_float   ... bench:   4,572,120 ns/iter (+/- 52,408)
test hsl_rgb_hue ... bench:  16,222,436 ns/iter (+/- 2,031,631)

I my case inling also doesn't seem to help.

[Ogeon]

I added #[inline] to normalize_angle, all of the into, and from.

[sidred]

For some reason I am unable to duplicate your results even with all inlines. In any case there will be a slight performance impact on every access of the hue value.

I only see 2 use of atan(), one in lab.rs and one in rgb.rs. Is that the only case where rust's internal [-180, 180] comes into play? If thats the case won't it be easier to normalize that value and store everything internally as [0,360]. Then we can have a single normalize function.

[Ogeon]

For some reason I am unable to duplicate your results even with all inlines.

That's very strange. I wonder why... Have you tried with #[inline(always)], just to be sure?

In any case there will be a slight performance impact on every access of the hue value.

Not necessarily. Only if it's normalized every time, given that the inlining issue gets resolved.

Is that the only case where rust's internal [-180, 180] comes into play?

It's also used when checking for the smallest difference between two hues.

[sidred]

It's also used when checking for the smallest difference between two hues.

Can you point me to the code? I can't find where this calculation is happening.

One advantage of making it [0-360] is that we can use normalize for add and substract and then we can get rid of the normalize for read, we only normalize when the value changes or when it is loaded. I see no code that provides mutable access to the angle.

[Ogeon]

It's in Mix, here. The smallest difference between 0 and 270 is -90, for example.

Moving the normalization to the add/sub operators will make it both mandatory and completely destructive. The "raw" value will be lost. The current method can at least be made opt-in/out (the idea with to_raw), so it can be made even cheaper with the right design.

[sidred]

Wouldn't you still have the same problem even with (-180,180)? difference between 150 and 210 is 60, but becomes 300 ( 150 - (-150))

[Ogeon]

It takes the difference first, and then normalizes, so it's 150 - 210 and then normalize that to (-180,180).

[Ogeon]

The proposed changes for improving the usability of the hue types are simple enough to be doable before 0.3.0, although I realize that no concrete decision has been made yet. I would still like to keep the hue types for a bit longer and see if those improvements makes them usable enough.

I'm scheduling this issue as a reminder to make a decision before the next patch phase.

[Ogeon]

I'm postponing this a bit to get the release out. It's not a show stopper.

[Ogeon]

The hue types stays for now.