Merge #158

158: Make `Take` iterator for gradient inclusive of both end colors, add tests r=Ogeon a=okaneco

As mentioned in the third point of #156, this PR includes the bounds of the gradient along with more equal distribution of colors returned from the iterator.

In the current version of the iterator, `(self.from_head) / (self.len)` will approach 1 but never equal 1 which results in the upper gradient bound being excluded. The current behavior of `take()` is surprising if one expects the last color step to be the max color of the gradient. This PR increases step distance so the user receives an iterator of colors that evenly traverses from the minimum bound of the gradient to the maximum. The step is increased by subtracting 1 from `self.len` in the calculation of `i`. The calculation for `i` will then include
- `0` when `self.from_head` is `0`
- `1` when `self.from_head == self.len - 1`. 

The assignment of `i` has a conditional for when `self.len` is `1`. This avoids division by 0 which results in NaN parameters for the iterator color returned in the case of `take(1)`. The check needed to be added to the `DoubleEndedIterator`, otherwise it would NaN on `1` as well. The behavior of `take(1)` before this PR is to supply the minimum color in the gradient.

Pictured below is an example of the difference between current behavior and this PR in 3 sets of Lch gradients. The 1st, 3rd, and 5th rows are the current implementation of the iterator and the others are from the PR with the final step being inclusive of the maximum gradient color specified. The gradients are no longer skewed towards the beginning. The penultimate steps are very close to the current behavior's final step.
![image of 6 stepped gradients](https://raw.githubusercontent.com/okaneco/images/master/inclusive_grad.png)

---

`gradient::test::simple_slice` fails as a result of this PR.
```
---- gradient::test::simple_slice stdout ----
thread 'gradient::test::simple_slice' panicked at 'assert_relative_eq!(t1, t2)

    left  = Rgb { red: 0.8888888888888888, green: 0.0, blue: 0.1111111111111111, standard: PhantomData }
    right = Rgb { red: 0.875, green: 0.0, blue: 0.125, standard: PhantomData }

', palette/src/gradient.rs:453:13
```
```rust
#[test]
    fn simple_slice() {
        let g1 = Gradient::new(vec![
            LinSrgb::new(1.0, 0.0, 0.0),
            LinSrgb::new(0.0, 0.0, 1.0),
        ]);
        let g2 = g1.slice(..0.5);

        let v1: Vec<_> = g1.take(10).take(5).collect();
        let v2: Vec<_> = g2.take(5).collect();
        for (t1, t2) in v1.iter().zip(v2.iter()) {
            assert_relative_eq!(t1, t2);
        }
    }
```

Co-authored-by: okaneco <[email protected]>

README.md

@@ -99,7 +99,7 @@ This results in the following three colors:
 
 There is also a linear gradient type which makes it easy to interpolate between a series of colors. This gradient can be used in any color space and it can be used to make color sequence iterators.
 
-The following example shows two gradients between the same two endpoints, but one is in RGB and the other in is HSV space.
+The following example shows three gradients between the same two endpoints, but the top is in RGB space while the middle and bottom are in HSV space. The bottom gradient is an example of using the color sequence iterator.
 
 ```Rust
 extern crate palette;
@@ -116,7 +116,7 @@ let grad2 = Gradient::new(vec![
 ]);
 ```
 
-The RGB gradient goes through gray, while the HSV gradients changes only the hue:
+The RGB gradient goes through gray, while the HSV gradients only change hue:
 
 ![Gradient Comparison](gfx/readme_gradients.png)
 

palette/examples/readme_examples.rs

@@ -93,7 +93,7 @@ fn display_gradients<A: Mix<Scalar = f32> + Clone, B: Mix<Scalar = f32> + Clone>
     LinSrgb: From<A>,
     LinSrgb: From<B>,
 {
-    let mut image = RgbImage::new(256, 64);
+    let mut image = RgbImage::new(256, 96);
     {
         let mut sub_image = image.sub_image(0, 0, 256, 32);
         let (width, height) = sub_image.dimensions();
@@ -130,6 +130,26 @@ fn display_gradients<A: Mix<Scalar = f32> + Clone, B: Mix<Scalar = f32> + Clone>
         }
     }
 
+    {
+        let mut sub_image = image.sub_image(0, 64, 256, 32);
+        let swatch_size = 32;
+        let mut v1 = Vec::new();
+        for color in grad2.take(8) {
+            let pix: [u8; 3] = Srgb::from_linear(LinSrgb::from(color))
+                .into_format()
+                .into_raw();
+            v1.push(pix);
+        }
+        for (s, color) in v1.into_iter().enumerate() {
+            for x in (s * swatch_size)..((s + 1) * swatch_size) {
+                for y in 0..swatch_size {
+                    let pixel = sub_image.get_pixel_mut(x as u32, y as u32);
+                    *pixel = image::Rgb(color);
+                }
+            }
+        }
+    }
+
     match image.save(filename) {
         Ok(()) => println!("see '{}' for the result", filename),
         Err(e) => println!("failed to write '{}': {}", filename, e),

palette/src/gradient.rs

@@ -92,7 +92,34 @@ impl<C: Mix + Clone> Gradient<C> {
         min_color.mix(max_color, factor)
     }
 
-    ///Take `n` evenly spaced colors from the gradient, as an iterator.
+    ///Take `n` evenly spaced colors from the gradient, as an iterator. The
+    ///iterator includes both ends of the gradient, for `n > 1`, or just
+    ///the lower end of the gradient for `n = 0`.
+    ///
+    ///For example, `take(5)` will include point 0.0 of the gradient, three
+    ///intermediate colors, and point 1.0 spaced apart at 1/4 the distance
+    ///between colors 0.0 and 1.0 on the gradient.
+    /// ```
+    /// #[macro_use] extern crate approx;
+    /// use palette::{Gradient, LinSrgb};
+    ///
+    /// let gradient = Gradient::new(vec![
+    ///     LinSrgb::new(1.0, 1.0, 0.0),
+    ///     LinSrgb::new(0.0, 0.0, 1.0),
+    /// ]);
+    ///
+    /// let taken_colors: Vec<_> = gradient.take(5).collect();
+    /// let colors = vec![
+    ///     LinSrgb::new(1.0, 1.0, 0.0),
+    ///     LinSrgb::new(0.75, 0.75, 0.25),
+    ///     LinSrgb::new(0.5, 0.5, 0.5),
+    ///     LinSrgb::new(0.25, 0.25, 0.75),
+    ///     LinSrgb::new(0.0, 0.0, 1.0),
+    /// ];
+    /// for (c1, c2) in taken_colors.iter().zip(colors.iter()) {
+    ///     assert_relative_eq!(c1, c2);
+    /// }
+    /// ```
     pub fn take(&self, n: usize) -> Take<C> {
         let (min, max) = self.domain();
 
@@ -142,9 +169,14 @@ impl<'a, C: Mix + Clone> Iterator for Take<'a, C> {
 
     fn next(&mut self) -> Option<C> {
         if self.from_head + self.from_end < self.len {
-            let i = self.from + (self.diff / cast(self.len)) * cast(self.from_head);
-            self.from_head += 1;
-            Some(self.gradient.get(i))
+            if self.len == 1 {
+                self.from_head += 1;
+                Some(self.gradient.get(self.from))
+            } else {
+                let i = self.from + (self.diff / cast(self.len - 1)) * cast(self.from_head);
+                self.from_head += 1;
+                Some(self.gradient.get(i))
+            }
         } else {
             None
         }
@@ -160,9 +192,14 @@ impl<'a, C: Mix + Clone> ExactSizeIterator for Take<'a, C> {}
 impl<'a, C: Mix + Clone> DoubleEndedIterator for Take<'a, C> {
     fn next_back(&mut self) -> Option<Self::Item> {
         if self.from_head + self.from_end < self.len {
-            let i = self.from + (self.diff / cast(self.len)) * cast(self.len - self.from_end - 1);
-            self.from_end += 1;
-            Some(self.gradient.get(i))
+            if self.len == 1 {
+                self.from_end += 1;
+                Some(self.gradient.get(self.from))
+            } else {
+                let i = self.from + (self.diff / cast(self.len - 1)) * cast(self.len - self.from_end - 1);
+                self.from_end += 1;
+                Some(self.gradient.get(i))
+            }
         } else {
             None
         }
@@ -439,7 +476,7 @@ mod test {
         ]);
         let g2 = g1.slice(..0.5);
 
-        let v1: Vec<_> = g1.take(10).take(5).collect();
+        let v1: Vec<_> = g1.take(9).take(5).collect();
         let v2: Vec<_> = g2.take(5).collect();
         for (t1, t2) in v1.iter().zip(v2.iter()) {
             assert_relative_eq!(t1, t2);
@@ -456,5 +493,29 @@ mod test {
         let v1: Vec<_> = g.take(10).collect::<Vec<_>>().iter().rev().cloned().collect();
         let v2: Vec<_> = g.take(10).rev().collect();
         assert_eq!(v1, v2);
+
+        //make sure `take(1).rev()` doesn't produce NaN results
+        let v1: Vec<_> = g.take(1).collect::<Vec<_>>().iter().rev().cloned().collect();
+        let v2: Vec<_> = g.take(1).rev().collect();
+        assert_eq!(v1, v2);
+    }
+
+    #[test]
+    fn inclusive_take() {
+        let g = Gradient::new(vec![
+            LinSrgb::new(1.0, 1.0, 0.0),
+            LinSrgb::new(0.0, 0.0, 1.0),
+        ]);
+
+        //take(0) returns None
+        let v1: Vec<_> = g.take(0).collect();
+        assert_eq!(v1.len(), 0);
+        //`Take` produces minimum gradient boundary for n=1
+        let v1: Vec<_> = g.take(1).collect();
+        assert_eq!(v1[0], LinSrgb::new(1.0, 1.0, 0.0));
+        //`Take` includes the maximum gradient color
+        let v1: Vec<_> = g.take(5).collect();
+        assert_eq!(v1[0], LinSrgb::new(1.0, 1.0, 0.0));
+        assert_eq!(v1[4], LinSrgb::new(0.0, 0.0, 1.0));
     }
 }