Skip to content

Conversation

@praliedutzel
Copy link
Contributor

Problem

When converting from HSL to RGB, the RGB values do not always come out as expected. This is only an issue when the original color's hue value is less than 60, which, when divided by 60.0 in the remainder function, results in a value less than 1 but greater than or equal to 0.

In the example below, converting the color to RGB changes it from yellow to red:

iex> color = Chameleon.HSL.new(59, 91, 57)
%Chameleon.HSL{h: 59, l: 57, s: 91}

iex> Chameleon.convert(color, Chameleon.RGB)
%Chameleon.RGB{b: 46, g: 49, r: 245}

# Expected Output: %Chameleon.RGB{b: 48, g: 242, r: 245}

Example1

The following example uses the same color, but the hue has been shifted to 60, making the value of h / 60.0 a whole number and resulting in the correct conversion:

iex> color = Chameleon.HSL.new(60, 91, 57)
%Chameleon.HSL{h: 60, l: 57, s: 91}

iex> Chameleon.convert(color, Chameleon.RGB)
%Chameleon.RGB{b: 46, g: 245, r: 245}

# Expected Output: %Chameleon.RGB{b: 48, g: 245, r: 245}

Example2

Solution

Leverage Erlang's :math.fmod function in the remainder function to properly handle cases where dividing the hue by 60.0 results in a non-whole number.

Using the examples above, we can see the comparison of running the remainder function before and after this change:

# Example 1 - Before
iex> remainder(59)
1.9833333333333334

# Example 1 - After
iex> remainder(59)
0.9833333333333333

# Example 2 - Before
iex> remainder(60)
1.0

# Example 2 - After
iex> remainder(60)
1.0

@supersimple
Copy link
Owner

@praliedutzel Thank you for your contribution. Would you mind adding tests to verify the conversion?

@supersimple supersimple merged commit acb33f1 into supersimple:main Dec 30, 2021
@supersimple
Copy link
Owner

@praliedutzel I had some time, so I added the tests. Thanks a lot for pointing this bug out!! ❤️ 🐛 💀

@praliedutzel
Copy link
Contributor Author

@praliedutzel I had some time, so I added the tests. Thanks a lot for pointing this bug out!! ❤️ 🐛 💀

@supersimple Awesome! Thanks for adding the tests, and glad to help! ❤️

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