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

Generalizing gradients and add constant gradients #205

Merged
merged 6 commits into from Mar 30, 2021

Conversation

NicolasKlenert
Copy link
Contributor

@NicolasKlenert NicolasKlenert commented Mar 17, 2021

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).

@Ogeon
Copy link
Owner

Ogeon commented Mar 18, 2021

Oh, wow! This looks really good! I'll of course have to let this rest here for a bit, at least until const generics are on stable, but I think the solution looks good and it's definitely a nice extension to the way it works now.

I just want to say that if there is anything you had in mind that would have been a breaking change, don't hesitate to let me know. Its current shape and form isn't necessarily the optimal solution (probably far from it). I have to admit that I even considered throwing it out at some point and just recommend finding a separate spline crate...

palette/src/gradient/mod.rs Outdated Show resolved Hide resolved
Comment on lines 135 to 139
pub fn with_domain(colors: T) -> Gradient<C, T> {
assert!(!colors.is_empty());

//Maybe sort the colors?
Gradient(colors)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub fn with_domain(colors: T) -> Gradient<C, T> {
assert!(!colors.is_empty());
//Maybe sort the colors?
Gradient(colors)
pub fn with_domain(colors: T) -> Option<Gradient<C, T>> {
if !colors.is_empty() {
Gradient(colors)
} else {
None
}

Since we can make breaking changes, it might be more idiomatic and friendly to users to return an Option here and in Gradient::{new, get, take, domain} by removing the expects and asserts.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be nice, but I'm not sure what's the better option. On one hand it removes panics from the code, on the other hand it pushes them over to the user's code instead. It's not a complicated validation so I'm still leaning towards not returning an Option and sparing people the need to always check it themselves.

Co-authored-by: Collyn O'Kane <47607823+okaneco@users.noreply.github.com>
Copy link
Owner

@Ogeon Ogeon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so I have taken another look at this, just to check if I missed anything, and added a couple of comments. I'm thinking we can merge it as it is after those have been addressed and think about any breaking changes as a larger makeover effort.

I have restarted the build to see what it says, but it looks like it's going to fail on nightly due to this:

error: derive helper attribute is used before it is introduced

   --> palette/src/convert.rs:535:7

    |

535 |     #[palette(

    |       ^^^^^^^

...

543 |     #[derive(FromColorUnclamped, WithAlpha)]

    |              ------------------ the attribute is introduced here

    |

    = note: `-D legacy-derive-helpers` implied by `-D warnings`

    = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!

    = note: for more information, see issue #79202 <https://github.com/rust-lang/rust/issues/79202>

error: derive helper attribute is used before it is introduced

   --> palette/src/convert.rs:604:7

    |

604 |     #[palette(

    |       ^^^^^^^

...

612 |     #[derive(Copy, Clone, FromColorUnclamped, WithAlpha)]

    |                           ------------------ the attribute is introduced here

    |

    = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!

    = note: for more information, see issue #79202 <https://github.com/rust-lang/rust/issues/79202>

error: aborting due to 2 previous errors

Would you mind going in and just reordering those attributes?

palette/build/named.rs Outdated Show resolved Hide resolved
palette/src/gradient/mod.rs Outdated Show resolved Hide resolved
@NicolasKlenert
Copy link
Contributor Author

Would you mind going in and just reordering those attributes?

I don't mind, however if I'm not mistaken, these errors don't have anything to do with this PR. This error was previously a warning and already existed in the crate. Such a seperate commit would be more idomatic.

@Ogeon
Copy link
Owner

Ogeon commented Mar 27, 2021

I don't mind, however if I'm not mistaken, these errors don't have anything to do with this PR. This error was previously a warning and already existed in the crate. Such a seperate commit would be more idomatic.

It's a surprise change from the nightly channel, so giving it its own commit sounds perfect! 🙂 I'm most grateful for the help!

bors bot added a commit that referenced this pull request Mar 30, 2021
207: Reorder attributes r=Ogeon a=NicolasKlenert

As mentioned in #205 [(in this comment)](#205 (review)) using derive attributes before them being introduced results in an error in the new nightly build. This PR reorders the attributes.

Co-authored-by: Nicolas Klenert <klenert.nicolas@gmail.com>
@Ogeon
Copy link
Owner

Ogeon commented Mar 30, 2021

It looks great with the new changes, thank you!

@Ogeon
Copy link
Owner

Ogeon commented Mar 30, 2021

Alright, I'm assuming this is good to go! Thank you and thanks for adding in the attribute fix! This is a good step towards a better gradient.

bors r+

@bors
Copy link
Contributor

bors bot commented Mar 30, 2021

Build succeeded:

@bors bors bot merged commit 00f8e6a into Ogeon:master Mar 30, 2021
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 this pull request may close these issues.

Add convenience gradients
3 participants