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

rp2040 zero rainbow using palette #82

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ChocolateLoverRaj
Copy link

Rather than do math with rgb numbers directly, it's much simpler to just use HSL and a color conversion library to convert HSL into RGB. Imo this makes the code simpler and easier to understand.

Comment on lines +87 to +89
// 0 is off and 1 is max brightness
let brightness = 0.05 as f64;
(brightness * u8::MAX as f64) as u8
Copy link
Member

Choose a reason for hiding this comment

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

The previous code uses 32 as brightness which is 12.5% not 5%.
Why not leaving the value 32 and only adding a comment like /* 12.5% */

(wheel_pos * 3, 255 - (wheel_pos * 3), 0).into()
ws.write(brightness(
once({
let hsl = Hsl::<Rgb, _>::new(hue as f64, 1.0, 0.5);
Copy link
Member

Choose a reason for hiding this comment

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

hue is already an f64 from its declaration. No need to cast it here.

}),
{
// 0 is off and 1 is max brightness
let brightness = 0.05 as f64;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let brightness = 0.05 as f64;
let brightness = 0.05f64;

},
))
.unwrap();
hue += 360.0 / 6.0 / u8::MAX as f64;
Copy link
Member

Choose a reason for hiding this comment

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

All these values are constants. What’s the intent in that math?

The current code turns the wheel by (1/255)rad (~1.41°) per iteration.
We don’t need this to match strictly. This should be enough:

Suggested change
hue += 360.0 / 6.0 / u8::MAX as f64;
hue += 1.0; // advance by 1°

Copy link
Author

Choose a reason for hiding this comment

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

The intent is that there are 256 steps of changes in a value of either r, g, b in every 1/6 of 360 degrees, so adding the hue by 360/6/256 makes sure that there will actually be a change in pixels every iteration of the loop. This can be changed to 1 if that's what's prefered by this project.

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.

2 participants