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

Add conversion from Rgb666 and Bgr666 to Grey8 #656

Merged
merged 5 commits into from
Mar 22, 2022

Conversation

brianmay
Copy link
Contributor

This is required for the tinytga library.

Fixes #654.

This is required for the tinytga library.

Fixes embedded-graphics#654.
@jamwaffles jamwaffles requested a review from rfuest March 20, 2022 13:34
Copy link
Member

@jamwaffles jamwaffles left a comment

Choose a reason for hiding this comment

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

LGTM but I'll let @rfuest merge in case I missed anything with the colour conversion code.

Copy link
Member

@rfuest rfuest left a comment

Choose a reason for hiding this comment

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

The change itself looks good, but there are other instances in the same file where Rgb666 and Bgr666 are also missing. At least the gray_to_rgb and rgb_to_gray should be fixed before this gets merged.

The impl_from_binary! and impl_rgb_to_binary! and their test are also missing the Rgb666 and Bgr666 types. But if you don't want to fix these now, because they are outside the scope of this PR, we can fix these later.

@brianmay
Copy link
Contributor Author

I am totally unfamiliar with this code, so not surprising if I missed something.

Looks like I missed updating the tests. Tests are good. Now fixed. Hopefully.

@brianmay
Copy link
Contributor Author

For some reason the CI is unahppy with my new references to Bgr666. Don't understand why, all the tests pass on my box.

This is something I might need help with.

@rfuest
Copy link
Member

rfuest commented Mar 20, 2022

For some reason the CI is unahppy with my new references to Bgr666. Don't understand why, all the tests pass on my box.

Maybe you forgot to commit something? Rgb666 and Bgr666 are missing in the invocations of the impl_from_binary! (line 121andimpl_rgb_to_binary!` (line 147) macros.

@brianmay
Copy link
Contributor Author

I missed those places. Now fixed. Odd this didn't break the tests when I ran them locally.

@brianmay
Copy link
Contributor Author

Tests pass now.

@brianmay brianmay requested a review from rfuest March 21, 2022 00:45
@rfuest
Copy link
Member

rfuest commented Mar 21, 2022

Odd this didn't break the tests when I ran them locally.

Did you run the tests by calling cargo test? Because that only runs the tests in the main embedded-graphics crate and not the embedded-graphics-core crate. You need to run cargo test --workspace or preferably use just build (if you aren't using Windows) to run all tests.

Copy link
Member

@rfuest rfuest left a comment

Choose a reason for hiding this comment

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

Two remaining issues:

  • Rgb666 and Bgr666 are missing in the rgb_to_rgb test (line 220)
  • Please add a changelog entry to /CHANGELOG.md and /core/CHANGELOG.md

@brianmay brianmay requested a review from rfuest March 21, 2022 20:53
Copy link
Member

@rfuest rfuest left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@rfuest rfuest merged commit f9a7b1d into embedded-graphics:master Mar 22, 2022
@brianmay brianmay deleted the greyscale_color_conversion branch March 22, 2022 20:39
brianmay added a commit to brianmay/robotica-remote-rust that referenced this pull request Mar 22, 2022
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.

Add greyscale conversions for Rgb666
3 participants