-
Notifications
You must be signed in to change notification settings - Fork 60
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
Conversation
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
pub fn with_domain(colors: T) -> Gradient<C, T> { | ||
assert!(!colors.is_empty()); | ||
|
||
//Maybe sort the colors? | ||
Gradient(colors) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 expect
s and assert
s.
There was a problem hiding this comment.
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>
There was a problem hiding this 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?
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! |
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>
It looks great with the new changes, thank you! |
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+ |
Build succeeded: |
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
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 ofGradient
itself.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).