Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Redesign and improve the Gradient type and friends #156

Closed
1 of 4 tasks
Ogeon opened this issue Nov 26, 2019 · 2 comments · Fixed by #301
Closed
1 of 4 tasks

Redesign and improve the Gradient type and friends #156

Ogeon opened this issue Nov 26, 2019 · 2 comments · Fixed by #301

Comments

@Ogeon
Copy link
Owner

Ogeon commented Nov 26, 2019

It should have about the same API as today, where it still makes sense, but overall modernized. Also fulfill this:

  • make it #[no_std] enabled
  • support more than just linear interpolation
  • the Take iterator should be inclusive, meaning both ends are part of the output for n > 1
  • support slicing with at least x..=y, but other ranges may also make sense

The first two points may go hand in hand, by replacing the internal Vec with some input collection.

bors bot added a commit that referenced this issue Dec 15, 2019
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 <47607823+okaneco@users.noreply.github.com>
@okaneco
Copy link
Contributor

okaneco commented Dec 19, 2019

To learn more about [no_std], I looked into making gradient [no_std] enabled and built it successfully for thumbv6m-none-eabi. Since the alloc crate has been stabilized, you can use some collections like Vec with no_std. That required changing instances of std::vec to core::vec in gradient.rs, configuring the alloc crate behind a feature flag in lib.rs, and adding some helper code in the no_std_test.rs like the alloc error handler. For data structures and no_std, it seems like the choices are use alloc and make users specify their own memory allocator in their crates/runtimes, or bring in a crate like heapless for your internal data structures (I didn't want to add any new dependencies). I'm very inexperienced with this though, this is what I can gather from the current information I could search.

The no_std_test fails to compile for x86_64-unknown-linux-gnu target in the same CI build. It doesn't seem like there's a solution currently for the undefined reference to '_Unwind_Resume'. It seems to be an issue with the linker and I couldn't get the rust-ld linker to work as I believe you'd have to specify library locations in the VM image. eh_unwind_resume doesn't seem to matter for helping linux-gnu build and thumbv6m didn't need it to build. I don't have Linux installed at the moment so can't test it locally, but I can build it locally for x86_64-apple-darwin on nightly.

Here are the changed files.

@Ogeon
Copy link
Owner Author

Ogeon commented Dec 20, 2019

I have never worked with the alloc crate, so I have no idea off the top of my head what this could be. Part of the reason why making the user provide the storage solution is an interesting idea is to not have to deal with supporting allocations at all. It would be up to the user to decide if a Vec or a slice or an array, or whatever else is the most suitable. As long as it's compatible with some minimal API (basically something like something.color_at(x)).

That and the idea of having support for more than just linear interpolation makes me wonder if it's worth the effort to get alloc working, rather than changing the gradient API to not need it.

@okaneco okaneco mentioned this issue Mar 20, 2020
22 tasks
bors bot added a commit that referenced this issue Mar 30, 2021
205: Generalizing gradients and add constant gradients r=Ogeon a=NicolasKlenert

This pull request contains two commits:

### 1. Generalization of  `Gradient` 

This allows creation of gradients from arrays. The inner collection type only has to be `ArrayLike`. For backward compatibility the default is still vector. This allows constant gradients and should also allow gradients to be supported for the next `#[no-std]` build. Other options to enable `#[no-std]` for gradients were briefly disscused in #156.

### 2. Adding some constant gradients

This commit adds constant gradients. To be precise, it adds all 4 new matplotlib color gradients, which are perfectly perceptually-uniform, both in regular form and also when converted to black-and-white and therefore one of the most useful gradients. These are build the same way the named colors are built. A new feature-flag `named_gradients` is added to toggle said constants. This closes #62.

### Alterantives

- The generalization of gradients can be achieved in a multiple of ways. Using a trait is just one of them and may be the wrong way to go. 
   - However I think because this pull request doesn't have any breaking changes and gradients should be supporting arrays in future versions of the crate, it doesn't seem like this update of  `Gradient` will cause more breaking API changes in the future than otherwise. Also constant gradients may be the only interaction with gradients a user needs, such that introducing them could reduce the number of users which actually relies on the generation of `Gradient` itself.
- At the moment the 4 constant gradients are using linear interpolation but in nature these gradients are Spline-Interpolations of exaclty two points (and 2 controlpoints). If `Gradient` will support Spline-Inerpolation in the future and the exact controlpoints of these gradients can be found (I only found the colormaps), the gradients could be implemented more memory efficient.

### Remark

These commits depend on const generics, which is a feature in beta but is planned to be stable on 2021-03-25 onwords (see [#79135](rust-lang/rust#79135)).

Co-authored-by: Nicolas Klenert <klenert.nicolas@gmail.com>
Co-authored-by: NicolasKlenert <Nicolas_Klenert@web.de>
Co-authored-by: NicolasKlenert <klenert.nicolas@gmail.com>
@bors bors bot closed this as completed in 1be0fb3 Jan 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants