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

Color gradient curve #14976

Merged
merged 15 commits into from
Sep 2, 2024
Merged

Conversation

RobWalt
Copy link
Contributor

@RobWalt RobWalt commented Aug 29, 2024

Objective

  • Currently we have the ColorRange trait to interpolate linearly between two colors
  • It would be cool to have:
    1. linear interpolation between n colors where n >= 1
    2. other kinds of interpolation

Solution

  1. Implement ColorGradient which takes n >= 1 colors and linearly interpolates between consecutive pairs of them
  2. Implement Curve intergration for this ColorGradient which yields a curve struct. After that we can apply all of the cool curve adaptors like .reparametrize() and .map() to the gradient

Testing

  • Added doc tests
  • Added tests

Showcase

// let gradient = ColorGradient::new(vec![]).unwrap(); // panic! 💥
let gradient = ColorGradient::new([basic::RED, basic::LIME, basic::BLUE]).expect("non-empty");
let curve = gradient.to_curve();
let brighter_curve = curve.map(|c| c.mix(&basic::WHITE, 0.5));

Kind of related to #14971 (comment)

Copy link
Contributor

@mweatherley mweatherley left a comment

Choose a reason for hiding this comment

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

At a high level, I feel like the interplay between Curve and ColorRange needs a bit of thought (in terms of how exactly we want to work with gradients long-term).

Separately, it seems to me like all of the concrete curvy stuff here is essentially reinventing the logic in curve::cores::EvenCore/SampleCurve.

Maybe the right thing to do would be to use the EvenCore technology in helping define the ColorRanges for these kinds of transitions and separately to have a curve constructor for any ColorRange that spits out a Curve over the unit interval.

Having both ColorRange and Curve feels a bit weird, but I'm not color-y enough to make any pronouncements about whether ColorRange ought to be deprecated or something, so I'll leave that to other people.

crates/bevy_color/src/color_gradient.rs Outdated Show resolved Hide resolved
crates/bevy_color/src/color_gradient.rs Outdated Show resolved Hide resolved
crates/bevy_color/src/color_gradient.rs Outdated Show resolved Hide resolved
crates/bevy_color/src/color_gradient.rs Outdated Show resolved Hide resolved
@bushrat011899 bushrat011899 added C-Enhancement A new feature X-Contentious There are nontrivial implications that should be thought through D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward A-Color Color spaces and color math labels Aug 29, 2024
Copy link
Contributor

@bushrat011899 bushrat011899 left a comment

Choose a reason for hiding this comment

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

I like this feature, but there are some changes I'd like made around trait implementations. I can't comment on interoperability with the Curve side of Bevy as I'm not familiar with that yet.

crates/bevy_color/src/color_gradient.rs Outdated Show resolved Hide resolved
crates/bevy_color/src/color_gradient.rs Outdated Show resolved Hide resolved
crates/bevy_color/src/color_gradient.rs Outdated Show resolved Hide resolved
crates/bevy_color/src/color_gradient.rs Outdated Show resolved Hide resolved
crates/bevy_color/src/color_gradient.rs Outdated Show resolved Hide resolved
crates/bevy_color/src/color_gradient.rs Outdated Show resolved Hide resolved
crates/bevy_color/src/color_gradient.rs Outdated Show resolved Hide resolved
crates/bevy_color/src/color_gradient.rs Outdated Show resolved Hide resolved
crates/bevy_color/src/color_range.rs Outdated Show resolved Hide resolved
@RobWalt
Copy link
Contributor Author

RobWalt commented Sep 1, 2024

@mweatherley @bushrat011899

I took another try to implement it. Now it's backed by a SampleCurve which simplifies it quiet a bit overall. However, unfortunately, because types, we can't get a constructor with a default mix function. Hence the current implementation also needs the user to specify how to mix 2 mixables. This is a bit unfortunate because it allows misuse of the API if the passed mix function doesn't behave like T::mix. Not sure how to resolve it in a better way :/

Copy link
Contributor

@bushrat011899 bushrat011899 left a comment

Choose a reason for hiding this comment

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

This is a much better attempt, thank you for taking our feedback on board and not giving up! I have made some suggestions which should resolve your type parameter issue and make the API much cleaner to work with. I've tested this on my own machine so it should work straight away.

I also experimented with adding a FromIterator implementation, but this cannot be done due to the fallibility of ColorCurve::new, we'll have to wait for TryFromIterator to be stabilised.

Excellent work!

crates/bevy_color/src/color_gradient.rs Show resolved Hide resolved
crates/bevy_color/src/color_gradient.rs Outdated Show resolved Hide resolved
crates/bevy_color/src/color_gradient.rs Outdated Show resolved Hide resolved
crates/bevy_color/src/color_gradient.rs Outdated Show resolved Hide resolved
crates/bevy_color/src/color_gradient.rs Outdated Show resolved Hide resolved
crates/bevy_color/src/color_gradient.rs Outdated Show resolved Hide resolved
crates/bevy_color/src/color_gradient.rs Outdated Show resolved Hide resolved
Co-Authored-By: Zachary Harrold <zac@harrold.com.au>
@RobWalt
Copy link
Contributor Author

RobWalt commented Sep 2, 2024

@bushrat011899 Thanks for guiding me! I almost already had this at some point in time. The deal breaker for me was the

        let mix: fn(&T, &T, f32) -> T = <T as Mix>::mix;

I guess I gave up too early 😅

Anyways: Looks sooooo much cleaner now! Thanks again!

Copy link
Contributor

@bushrat011899 bushrat011899 left a comment

Choose a reason for hiding this comment

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

Excellent work, glad I could help!

@mweatherley
Copy link
Contributor

@bushrat011899 Thanks for guiding me! I almost already had this at some point in time. The deal breaker for me was the

        let mix: fn(&T, &T, f32) -> T = <T as Mix>::mix;

I guess I gave up too early 😅

Anyways: Looks sooooo much cleaner now! Thanks again!

Another approach, if you prefer to avoid function pointers, would be to just use EvenCore directly; the Curve library itself ran into the same issue that you encountered, which is why SampleAutoCurve exists in addition to SampleCurve.

@RobWalt
Copy link
Contributor Author

RobWalt commented Sep 2, 2024

@bushrat011899 Thanks for guiding me! I almost already had this at some point in time. The deal breaker for me was the

        let mix: fn(&T, &T, f32) -> T = <T as Mix>::mix;

I guess I gave up too early 😅
Anyways: Looks sooooo much cleaner now! Thanks again!

Another approach, if you prefer to avoid function pointers, would be to just use EvenCore directly; the Curve library itself ran into the same issue that you encountered, which is why SampleAutoCurve exists in addition to SampleCurve.

That's even better and also solves the serde flaw!

Co-authored-by: Matty <weatherleymatthew@gmail.com>
@alice-i-cecile alice-i-cecile added the C-Needs-Release-Note Work that should be called out in the blog due to impact label Sep 2, 2024
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Useful, and a straightforward implementation. Nice work!

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 2, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 2, 2024
Merged via the queue into bevyengine:main with commit 8a64b76 Sep 2, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Color Color spaces and color math C-Enhancement A new feature C-Needs-Release-Note Work that should be called out in the blog due to impact D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants