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

Animatable for colors #12614

Merged
merged 10 commits into from
Mar 22, 2024
Merged

Conversation

lynn-lumen
Copy link
Contributor

Objective

Solution

  • Implements Animatable for all color types implementing arithmetic operations.
    • the colors returned by Animatables methods are already clamped.
  • Adds a color_animation.rs example.
  • Implements the *Assign operators for color types that already had the corresponding operators. This is just a 'nice to have' and I am happy to remove this if it's not wanted.

Changelog

  • bevy_animation now depends on bevy_color.
  • LinearRgba, Laba, Oklaba and Xyza implement Animatable.

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen A-Animation Make things move and change over time labels Mar 21, 2024
@alice-i-cecile alice-i-cecile added this to the 0.14 milestone Mar 21, 2024
examples/README.md Outdated Show resolved Hide resolved
lynn-lumen and others added 2 commits March 21, 2024 15:56
Co-Authored-By: Alice Cecile <alice.i.cecile@gmail.com>
@lynn-lumen
Copy link
Contributor Author

Do I need to do anything about these errors? A lot of them seem to be unrelated to this PR

@alice-i-cecile
Copy link
Member

No, those appear to be the result of the new Rust version that just shipped this morning. I'll make an issue.

@viridia
Copy link
Contributor

viridia commented Mar 21, 2024

Looks awesome! Can't wait to try it out.

@mockersf
Copy link
Member

I think I would prefer the implementation to be in bevy_color. It makes more sense to me for the impl to be on the crate declaring the type, like third party adding types will also impl the trait, they won't pr it in bevy_animation

@alice-i-cecile
Copy link
Member

I think I would prefer the implementation to be in bevy_color. It makes more sense to me for the impl to be on the crate declaring the type, like third party adding types will also impl the trait, they won't pr it in bevy_animation

The problem with this is that it explodes bevy_color's dependency count. While it was previously a nice quasi-independent crate, it now needs bevy_ecs, bevy_app and so on. If we decide to go this route, we need to feature flag it (probably off by default).

@viridia
Copy link
Contributor

viridia commented Mar 21, 2024

Could Animatable live somewhere else?

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Mar 21, 2024

Animatable is the core trait of bevy_animation: it makes sense there. We could add a bevy_ecs feature flag on bevy_animation instead though, which would make it so we can rely on it in bevy_color (and make it more useful to the ecosystem as a whole).

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.

Just some minor suggestions, really nice!

examples/animation/color_animation.rs Outdated Show resolved Hide resolved
examples/animation/color_animation.rs Outdated Show resolved Hide resolved
examples/animation/color_animation.rs Outdated Show resolved Hide resolved
lynn-lumen and others added 2 commits March 22, 2024 00:33
Co-authored-by: Zachary Harrold <zac@harrold.com.au>
Co-Authored-By: Zachary Harrold <zac@harrold.com.au>
@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 21, 2024
@alice-i-cecile
Copy link
Member

@solis-lumine-vorago once you can appease CI I'll merge this in :) Looks like some clippy failures.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 22, 2024
Merged via the queue into bevyengine:main with commit 887bc27 Mar 22, 2024
30 checks passed
@BD103 BD103 added the A-Color Color spaces and color math label May 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Animation Make things move and change over time A-Color Color spaces and color math A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add color animation support
6 participants