Add hue traits #12399
Add hue traits #12399
Conversation
|
Welcome, new contributor! Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨ |
TrialDragon
added
C-Enhancement
crates/bevy_color/src/color_ops.rs
Outdated
| /// Return a new version of this color with the hue channel set to the given value. | ||
| fn with_hue(&self, hue: f32) -> Self; | ||
|
|
||
| /// Return the hue of this color (0.0 - 360.0). |
There was a problem hiding this comment.
| /// Return the hue of this color (0.0 - 360.0). | |
| /// Return the hue of this color [0.0, 360.0]. |
There was a problem hiding this comment.
At the same time, I think we shouldn't keep the interval here. What if we find a colorspace that for some reason will go by another interval?
There was a problem hiding this comment.
Certainly, it's not guaranteed that the range of hue values will fit within this range in every color space.
Thank you for your comment!
There was a problem hiding this comment.
I think the range here should be consistent across all hue implementations, hence defined here.
The implementations need to perform mapping from a predefined range to their hidden representation.
Let's say you have a color that uses [0, 1] and another that uses [0, 360].
You are refactoring your code from one to the other,
suddenly the .rotate_hue(0.5) which used to work 'stops' working.
It's influence decreased from 1/2 rotation to 1/720 rotation.
I personally like [0, 1] range, but [0, 360] or [0, 2pi] will work just fine.
If there is a consistent value range for current implementors, we should probably go with that.
([0, 360] seems to be it)
There was a problem hiding this comment.
Yeah, I agree that we don't have many colorspaces out there with some odd behavior, but at the same time, this trait is made to be generic, and I think that should reflect on docs
There was a problem hiding this comment.
Generic implies consistent behavior across implementations, so a consistent range should be in place.
Having read the PR now, all existing colorspaces are consistent so this behavior (and docs) is not necessary yet at the very least
Co-authored-by: Pablo Reinhardt <126117294+pablo-lua@users.noreply.github.com>
examples/3d/animated_material.rs
Outdated
| let mut i = 0; | ||
| for x in -1..2 { | ||
| for z in -1..2 { | ||
| commands.spawn(PbrBundle { | ||
| mesh: cube.clone(), | ||
| material: materials.add(Color::WHITE), | ||
| material: materials.add(Color::hsl((i as f32 * 2.345) * 100.0 % 360.0, 1.0, 0.5)), | ||
| transform: Transform::from_translation(Vec3::new(x as f32, 0.0, z as f32)), | ||
| ..default() | ||
| }); | ||
| i += 1; |
There was a problem hiding this comment.
Not important, but you can use the rotate_hue and a cool property of rotating by golden ratio here for showcase:
(aka golden angle)
// Not a stable f32 constant yet
const PHI: f32 = 1.6180339;
let mut c = Color::hsl(0.0, 1.0, 0.5);
for x in -1..2 {
for z in -1..2 {
// Pbr spawning
c = c.rotate_hue(PHI);
}
}this also declutters the random color calculations
There was a problem hiding this comment.
As you suggested, rotating at a constant ratio makes for a smarter implementation indeed. I rotated it at the golden angle to ensure that the colors of adjacent materials diverge!
8fde8f6
crates/bevy_color/src/color_ops.rs
Outdated
| /// Return a new version of this color with the hue channel rotated by the given degrees. | ||
| fn rotate_hue(&self, degrees: f32) -> Self { | ||
| let degrees = degrees.rem_euclid(360.); | ||
| self.with_hue((self.hue() + degrees) % 360.) |
There was a problem hiding this comment.
Why not call .rem_euclid() on the result after the addition?
pablo-lua
added
the
S-Ready-For-Final-Review
|
@oyasumi731 I've done my best to resolve merge conflicts: going to try and merge this :) If it fails, please clean up my changes and get CI passing and then ping me to retry the merge. |
Head branch was pushed to by a user without write access
|
@alice-i-cecile Thank you. |
|
Thank you very much :) I really appreciate when authors are able to fix up things like that for us. |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Objective
Fixes #12200 .
Solution
I added a Hue Trait with the rotate_hue method to enable hue rotation.
Additionally, I modified the implementation of animations in the animated_material sample.
Changelog
Huetrait tobevy_color/src/color_ops.rs.Huetrait implementation toHsla,Hsva,Hwba,Lcha, andOklcha.Migration Guide
Users of Oklcha need to change their usage to use the with_hue method instead of the with_h method.