Skip to content

fix: Wrong RGB -> HSL convertion - #505

Merged
mloskot merged 3 commits into
boostorg:developfrom
eugene-kulak:patch-1
Jun 24, 2022
Merged

fix: Wrong RGB -> HSL convertion#505
mloskot merged 3 commits into
boostorg:developfrom
eugene-kulak:patch-1

Conversation

@eugene-kulak

@eugene-kulakeugene-kulak commented Jul 3, 2020

Copy link
Copy Markdown
Contributor

Description

Fixes wrong formula used to convert RGB to HSL color space,
check correct algorithm here (https://www.niwa.nu/2013/05/math-behind-colorspace-conversions-rgb-hsl/)

References

Tasklist

  • Add test case(s)
  • Ensure all CI builds pass
  • Review and approve

@mloskot

Copy link
Copy Markdown
Member

Thanks for the patch.

Could you tell how did you discover the issue, for what input, expected output, a MWE?
In other words, it would be helpful if this PR includes a corresponding test case.

@mloskotmloskot added cat/bug But reports and bug fixes ext/toolbox boost/gil/extension/toolbox/ labels Jul 3, 2020
@eugene-kulak

Copy link
Copy Markdown
ContributorAuthor

Hi @mloskot , unfortunately, I don't use boost tests, but here are my GTests:

#definergb2hsl(r, g, b, h, s, l) \
{ \
constfloat abs_error = 0.002; \
using boost::gil::hsl32f_pixel_t; \
using boost::gil::rgb8_pixel_t; \
using boost::gil::hsl_color_space::hue_t; \
using boost::gil::hsl_color_space::lightness_t; \
using boost::gil::hsl_color_space::saturation_t; \
rgb8_pixel_trgb(r, g, b); \
hsl32f_pixel_t hsl; \
\
color_convert(rgb, hsl); \
\
ASSERT_NEAR(h, get_color(hsl, hue_t()), abs_error); \
ASSERT_NEAR(s, get_color(hsl, saturation_t()), abs_error); \
ASSERT_NEAR(l, get_color(hsl, lightness_t()), abs_error); \
}
#definehsl2rgb(h, s, l, r, g, b) \
{ \
constfloat abs_error = 1; \
using boost::gil::blue_t; \
using boost::gil::green_t; \
using boost::gil::hsl32f_pixel_t; \
using boost::gil::red_t; \
using boost::gil::rgb8_pixel_t; \
hsl32f_pixel_thsl(h, s, l); \
rgb8_pixel_t rgb; \
\
color_convert(hsl, rgb); \
\
ASSERT_NEAR(r, get_color(rgb, red_t()), abs_error); \
ASSERT_NEAR(g, get_color(rgb, green_t()), abs_error); \
ASSERT_NEAR(b, get_color(rgb, blue_t()), abs_error); \
}
TEST(ColorSpaceTest, RGBToHSL) {
// blackrgb2hsl(0, 0, 0, 0, 0, 0);
// whitergb2hsl(255, 255, 255, 0, 0, 1);
// redrgb2hsl(255, 0, 0, 0, 1, 0.5);
// limergb2hsl(0, 255, 0, 2 / 6.f, 1, 0.5);
// bluergb2hsl(0, 0, 255, 4 / 6.f, 1, 0.5);
// yellowrgb2hsl(255, 255, 0, 1 / 6.f, 1, 0.5);
// cyanrgb2hsl(0, 255, 255, 3 / 6.f, 1, 0.5);
// magentargb2hsl(255, 0, 255, 5 / 6.f, 1, 0.5);
// silverrgb2hsl(191, 191, 191, 0, 0, 0.75);
// grayrgb2hsl(128, 128, 128, 0, 0, 0.5);
// maroonrgb2hsl(128, 0, 0, 0, 1, 0.25);
// olivergb2hsl(128, 128, 0, 1 / 6.f, 1, 0.25);
// greenrgb2hsl(0, 128, 0, 2 / 6.f, 1, 0.25);
// purplergb2hsl(128, 0, 128, 5 / 6.f, 1, 0.25);
// tealrgb2hsl(0, 128, 128, 3 / 6.f, 1, 0.25);
// navyrgb2hsl(0, 0, 128, 4 / 6.f, 1, 0.25);
}
TEST(ColorSpaceTest, HSLToRGB) {
// blackhsl2rgb(0, 0, 0, 0, 0, 0);
// whitehsl2rgb(0, 0, 1, 255, 255, 255);
// redhsl2rgb(0, 1, 0.5, 255, 0, 0);
// limehsl2rgb(2 / 6.f, 1, 0.5, 0, 255, 0);
// bluehsl2rgb(4 / 6.f, 1, 0.5, 0, 0, 255);
// purplehsl2rgb(5 / 6.f, 1, 0.25, 128, 0, 128);
// tealhsl2rgb(3 / 6.f, 1, 0.25, 0, 128, 128);
// navyhsl2rgb(4 / 6.f, 1, 0.25, 0, 0, 128);
}

@mloskotmloskot self-assigned this Jan 26, 2021

@lpranamlpranam left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good. Would be nice to add those tests now we don't use boost test so should be easy. Let us know if you can add or not. @eugene-kulak

@mloskot

Copy link
Copy Markdown
Member

@lpranam I think we will have to add the tests on our own
I'm merging this, so this can be followed with porting of the GTest tests - #690

@mloskot
mloskot merged commit 5273678 into boostorg:developJun 24, 2022
@mloskotmloskot added this to the Boost 1.80 milestone Jun 24, 2022
@mloskotmloskot changed the title Fix wrong RGB -> HSL convertionfix: Wrong RGB -> HSL convertionJun 24, 2022
mloskot added a commit to mloskot/gil that referenced this pull request Jun 25, 2022
Contributor of PR boostorg#505 posted GTest-based tests in.
This commit ports those tests to Boost.LightweightTest.
Closesboostorg#690
@mloskotmloskot mentioned this pull request Jun 25, 2022
3 tasks
mloskot added a commit that referenced this pull request Jun 25, 2022
Contributor of PR #505 posted GTest-based tests in.
This commit ports those tests to Boost.LightweightTest.
Closes#690
mloskot added a commit that referenced this pull request Jun 27, 2022
* develop:
docs!: Announce plan to require C++17 after Boost 1.80 (#694)
feat: Added apply_rasterizer() free function (#695)
refactor: Ellipse rasterizer according to the comment at (#692)
refactor: Deprecate apply_operation in favor of variant2::visit for any_image (#656)
refactor: Replace deprecated libtiff v4.3 typedefs with C99 fixed-size integers (#685)
fix: Automatic detection of <filesystem> header (#684)
test: Add tiled TIFF test case to simple_all_formats
test: Add tests for RGB to HSL (#691)
refactor: Move RGB to HSL tests to color_convert_rgb.cpp
refactor: Make with_tolerance reusable across other tests
chore: Correct include guard
fix: Add missing #include <array>
fix: Wrong RGB -> HSL convertion (#505)
@mloskotmloskot mentioned this pull request Jul 5, 2022
6 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cat/bugBut reports and bug fixesext/toolboxboost/gil/extension/toolbox/

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@eugene-kulak@mloskot@lpranam