Implement relative and absolute methods for Lighten/Darken, Saturate

[okaneco]

Add lighten_fixed and darken_fixed methods to the Shade trait. The
_fixed functions inherit similar behavior to the old lighten/darken.
The new implementation scales the color toward the maximum or minimum
value for lightness or saturation. This should be more intuitive to
adjust as a small factor could have drastically different results based
on the color space. The same change is made for saturate/desaturate with
corresponding _fixed methods.

Add doc tests showing new behavior of Saturate and Shade
Differentiate between the absolute and relative lighten/darken/saturate
by using amount to refer to the fixed/absolute functions and factor
when using the relative versions (the new default version is relative).

Account for negative factors in the saturate and shade functions.

closes #215

[Ogeon]

Nice, thank you! Good to see that we can most likely get this with the release. Apart from my comments in the code, I have two things that I came to think of:

1. We need too handle the case when the current value is beyond the limit it's moving towards. I would suggest adding a .max(T::zero()) to make it do nothing, but I have no idea what would be the "proper" way to do it. Now it will just move towards the limit value, so lighten may darken a value above the max limit.
2. I would like to normalize the "amount" and "factor" terms here. Maybe just "amount" is good, unless something else is more correct.

[okaneco]

Yes, my thoughts were that this seemed like a nice addition to try getting in for the new release.

Good catch on the Saturate impls. I saw the T::one nearby in the saturate_fixed methods so I think my mind stuck on using that instead of using T::max_intensity.

There's some shuffling in the Saturate docs because I had a formatting error due to duplicating the code originally. I removed "fraction" from the Saturate and Shade docs because it doesn't seem necessary to include and I felt it made the description a bit harder to read. I can re-add it if you think that or "fractional" if you think it should be there to describe the amount. I think amount makes more sense to normalize toward. Mix still uses factor but I think that is the correct use of factor because it's a mixing factor between 0.0 and 1.0.

Luma already had the max(T::zero()) on the original implementation. I added it to the others, that seems like the best thing to do for now.

---

I added the clamping to saturate_fixed as well since I don't think negative saturation makes sense either.

[Ogeon]

This seems like the least weird option for handling "out-of-bounds" values without complicating it. Let's go with it for now.

I just know I/we have been using different terminology (sometimes interchangeably) for factors and amounts, so it would be nice to set a proper standard or pattern that makes sense for what they are. I don't have anything against using different names if they match the different semantics, so if "factor" is correct for relative changes, we could use it for the relative methods and use "amount" for the fixed/absolute methods. I mean since they are also part of the documentation. Does that sound good?

And I would also like to encourage having a couple of small examples in the documentation to really show how things are intended to work. Something simple like showing that Hsl::new(0.0, 1.0, 0.5).lighten(0.5).lightness == 0.75 (with nice formatting), while Hsl::new(0.0, 1.0, 0.5).lighten_fixed(0.5).lightness == 1.0 could help with demonstrating the difference.

Aside from those details, there seem to be a test that need to be changed. It's probably based on an incorrect assumption, so it can either be changed tor removed and replaced with the above mentioned examples.

[okaneco]

I just know I/we have been using different terminology (sometimes interchangeably) for factors and amounts, so it would be nice to set a proper standard or pattern that makes sense for what they are. I don't have anything against using different names if they match the different semantics, so if "factor" is correct for relative changes, we could use it for the relative methods and use "amount" for the fixed/absolute methods. I mean since they are also part of the documentation. Does that sound good?

That makes the most sense and clarifies a lot semantically. I've made it so the _fixed versions use amount and the relative versions use factor.

I've modified the doc tests for Shade and Saturate to show the fixed and relative differences. I removed the old examples but can add them back in. I'm not sure if you want more text in the trait description to detail the relative/fixed types or if the examples are good enough.

The tests for Saturate threw me off because the implementation is actually relative for both and might be surprising. I'm not sure what's more expected.

// Shade
(self.value + amount).max(T::zero())
// Saturate
self.saturation * (T::one() + amount).max(T::zero())

let a = Hsv::new(0.0, 0.5, 1.0);
assert_relative_eq!(a.saturate(0.5).saturation, 0.625);
assert_relative_eq!(a.desaturate_fixed(0.5).saturation, 0.25);

So the calculation for saturate looks this:

let amount = (T::max_intensity() - self.value) * factor;
self.saturation * (T::one() + amount).max(T::zero())

and worked out, it becomes

0.5 * (1 + (1.0 - 0.5) * 0.5)
0.5 * (1 + 0.25)
0.625

[Ogeon]

Ohhh, wait, no, that's not what I had in mind. I must have blinked while reading that earlier, sorry. 😬 Or I may need to sleep. An example of what I'm going for is the color module for the SASS CSS pre-processor: https://sass-lang.com/documentation/modules/color. I don't know how familiar you are with it, but it has some functions for manipulating colors, including the ones from these traits. The darken/lighten/saturate/desaturate (ex. https://sass-lang.com/documentation/modules/color#darken) functions are basically legacy and are using fixed amounts. The scale function is what I had in mind for the new defaults in Palette. So I would say that it should mimic the SASS counterparts. That is, fixed amount is basically just current_value + max * amount.

For the docs, I'm usually not requiring people to write a lot because I know not everyone is too keen on doing it and I don't want to drag things out. And I know there can be a feeling of "being done" once someone has submitted a PR. But if I get to go into full wish list mode, I would say that it's good if both the text and the examples can demonstrate the functions fully. It should be clear from the text, but it should be fine to also only look at the examples. So feel free to make them completely exhaustive if you want to (while not drowning the reader in text 😅). It's going to be helpful as a guide for both using and implementing the traits. But I think that level of complexity (or simplicity) is good for the examples. They could perhaps just be a bit more verbose and explicit with the variables, like let saturated = original.saturate(0.5);.

[okaneco]

I wasn't familiar but I understand the intention a lot better now. I re-read the original issue and see where I went off track keeping the old calculation methods. I think this should be closer to the goal.

I added documentation and examples for Shade and Saturate which was challenging to phrase. I removed the examples from the main trait description since they seemed redundant with each function getting its own example.

[Ogeon]

I'm back with some rest and a fresher mind. The documentation looks great and I think you explained it really well! 😁 There's just a couple of things regarding the algorithm that I think we need to correct. I'll take Hsl and lighten as an example, but it applies everywhere.

First, I think there was a small miscommunication with where I thought the .max(T::zero()) should be necessary. I think it has to be like this to avoid going backwards with a positive factor: self.lighten_fixed((T::max_intensity() - self.lightness).max(T::zero()) * factor). I suppose it's still nice to clamp the end result too, where it applies, since unexpected negative numbers has shown to cause chaos in some cases.

But we need to take care of the negative factors too. Now it won't do anything if lightness is 1.0 and a negative factor is passed. Comparing to the SASS counterparts, our lighten and saturate are the same as their scale for specific parameters. We just make them look specific to positive factors, but they still need to act like darken and desaturate for negative factors. I hope this makes sense.

And, finally, since we are using an amount in the *_fixed functions that is meant to range from 0.0 to 1.0 (I think this is good, since it makes it more uniform between spaces), we can't rely on it to be possible to reuse *_fixed here. What if max intensity is 100? 😬

I would suggest changing the algorithm to this with all of that in mind:

fn lighten(&self, factor: Self::Scalar) -> Self {
    let difference = if factor >= T::zero() {
        T::max_intensity() - self.lightness
    } else {
        self.lightness
    };

    let change = difference.max(T::zero()) * factor;

    Hsl {
        hue: self.hue,
        saturation: self.saturation,
        lightness: (self.lightness + change).max(T::zero()),
        standard: PhantomData,
    }
}

I think that should give us the expected symmetry and work with the default implementation for darken.

[okaneco]

That makes sense in accounting for negative factors, the difference is self.lightness - T::zero() when the factor is negative. And I caught an error in Lab/Lch where I used max_intensity instead of from_f64(100.0) due to you mentioning it in your 4th paragraph.

I've changed the lighten and saturate methods to that algorithm. Are the *_fixed variants okay in their current implementations?

I've been wondering throughout this: should we clamp the components to the min and max before returning the color, or is max(T::zero()) enough? I'm fuzzy on what the guarantees or expectations are for library operations on colors being valid/clamped. I wouldn't want to add more overhead than necessary.

[Ogeon]

Great! Good catch with Lab and Lch. I sort of skimmed through them after realizing it had to be made a bit more complicated than it was.

Are the *_fixed variants okay in their current implementations?

Yes, I think they will be good as they are. 👍 They are just normalized addition and subtraction.

I've been wondering throughout this: should we clamp the components to the min and max before returning the color, or is max(T::zero()) enough? I'm fuzzy on what the guarantees or expectations are for library operations on colors being valid/clamped. I wouldn't want to add more overhead than necessary.

I have actually been wondering the same, so that's why I haven't maybe been so clear about my standpoint. Prepare for a brain dump!

I like to keep things open for "out-of-range" values because they could be useful as deltas (like my_color += delta;, where delta and my_color have the same type), and there's also the whole HDR thing and other cases where there is no specific limit. That's why I generally leave it to the user to clamp their values or offer escape hatches such as from_color_unclamped. But I can't say I have it 100% figured out yet.

For these operations, I see the output as more of a display value than a delta value, so I think some minimal clamping is fine. Values that correspond to some kind of brightness should probably just not be negative (what does that even represent? 😄), while values above 100% has their uses. Saturation could perhaps be fully clamped in some of the cases, but I think the conservative option is to only keep them positive as well.

The relative methods (lighten, saturate, etc.) have the extra tricky aspect of moving the value to one of two specified extremes. They are basically undefined for out-of-range input. I see a number of options there but all seem very weird and I still don't know if I picked the best one. Here are some:

• Never move backwards (the current option). The good thing is that it stays somewhat true to the semantics. Lighten doesn't darken for positive input, etc. The downside is that out-of-range values may seem to be stuck when they don't change and it doesn't work for HDR in SDR scale.
• Find some way to continue scaling them. This is complicated because we no longer have a reference point. Does it just use the current value, i.e. self.lightness += self.lightness * factor? What about negative values?
• Ignore the out-of-range case. Definitely not suitable for HDR in SDR scale, so may be surprising, but is at least cheap and simple.

I don't think any of them stand out as a clear winner, just that the current option doesn't do anything too weird without trying to be particularly clever. But the last option could be perfectly valid, with the disclaimer that in-range values are assumed and clamping is recommended. I expect the "pickier" users to roll their own algorithms when they need full control and this is more for "casual" users, for lack of better terms.

What's your view on it all? Am I lost in my own thoughts?

[okaneco]

The brain dump on this is welcome. There's a lot going on below the surface that isn't obvious at first glance, so it's good to talk about it especially when taking into account the future.

I like to keep things open for "out-of-range" values because they could be useful as deltas (like my_color += delta;, where delta and my_color have the same type), and there's also the whole HDR thing and other cases where there is no specific limit. That's why I generally leave it to the user to clamp their values or offer escape hatches such as from_color_unclamped. But I can't say I have it 100% figured out yet.

For these operations, I see the output as more of a display value than a delta value, so I think some minimal clamping is fine. Values that correspond to some kind of brightness should probably just not be negative (what does that even represent? ), while values above 100% has their uses. Saturation could perhaps be fully clamped in some of the cases, but I think the conservative option is to only keep them positive as well.

I agree with all of this. I think there's only a problem if there's a disconnect between user expectations and what the library does or expects of colors. That can be solved by the library stating its goals for the operations. If there's enough friction from use cases then the behavior can be improved or new methods added.

Unclamped-max values have uses for arithmetic and other cases, the upper clamp can be left to the user instead of adding more machinery and technical debt to churn through at a later date. Unclamped-min values for lightness, value, saturation, chroma, etc. don't make much sense to me.

I don't think any of them stand out as a clear winner, just that the current option doesn't do anything too weird without trying to be particularly clever. But the last option could be perfectly valid, with the disclaimer that in-range values are assumed and clamping is recommended. I expect the "pickier" users to roll their own algorithms when they need full control and this is more for "casual" users, for lack of better terms.

Agreed, I think for now this is the best default for the casual/average user. More advanced users can roll their own or contribute their methods back to the library if it's clearly better or general purpose enough. I can imagine some type of scaling maximum limits supplied as a parameter or generic parameter, but other problems like how HDR is handled should probably be solved first then solutions for relative operations will follow. When there are so many options on the table and with no clear-cut best, choosing what seems to be the least surprising option and not over-complicating the API surface seems reasonable until proven otherwise.

[Ogeon]

The brain dump on this is welcome. There's a lot going on below the surface that isn't obvious at first glance, so it's good to talk about it especially when taking into account the future.

I'll try to do it more often. Don't be afraid to remind me in the future!

I agree with all of this. I think there's only a problem if there's a disconnect between user expectations and what the library does or expects of colors. That can be solved by the library stating its goals for the operations. If there's enough friction from use cases then the behavior can be improved or new methods added.

Absolutely, that's simply a trade-off that need to be communicated in a good place. I suspect it's not so up-front at the moment. We'll have to try and see how it's received.

I can imagine some type of scaling maximum limits supplied as a parameter or generic parameter, but other problems like how HDR is handled should probably be solved first then solutions for relative operations will follow.

It's always possible to scale down to a 0.0-1.0 range while working with the color, and that may even be what they do with proper HDR if I'm not mistaken. My impression is that that's part of why there's a need to differ between HDR and SDR values on a type level. But yes, as you say, it's better to keep it simple. That way it will be easier to reason about and compose with other operations.

But I think this PR is pretty much done, unless you have anything to add. 🙂 I'm happy with the result and especially to have something more consistent that should be familiar when comparing to other software. Something you could do, by the way, is to run the saturate and shade examples and see how the output looks. I would expect them to be a bit different now, so probably good to make sure they look ok.

[okaneco]

I have nothing else to add so I'll squash the commits.

Yes, the results are a bit different now. Current behavior is on the left, this PR on the right. The Lch example for saturate on the right is a bit extreme at 0.8.

[okaneco]

That should be good from my end now 🙂

[Ogeon]

Oh, wow, it's surely saturated! They both need to be changed a bit, but I'll do in my housekeeping PR instead, while messing around with the other examples. I might edit them a bit more than just restoring them to the old results, so I think it's better to do it all together and look over the rest of them too.

Thank you for the help here and for sharing your thoughts! It's always appreciated!

bors r+

[bors]

Build succeeded:

• continuous-integration/travis-ci/push