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

hsl(hue, sat, briPercent) is hsb #60

Closed
jaumard opened this issue Oct 11, 2015 · 7 comments
Closed

hsl(hue, sat, briPercent) is hsb #60

jaumard opened this issue Oct 11, 2015 · 7 comments

Comments

@jaumard
Copy link

jaumard commented Oct 11, 2015

Hi,

I found a problem with the API,

If I set hsl(0, 100, 100) it make the light Red but the good value in HSL for red is hsl(0, 100, 50).

Anyone agree with this ?

@peter-murray
Copy link
Owner

Can you give me more information around how you are doing this, i.e. a code snippet?

I have just put together a test for this on my bridge calling using

state.on().hsl(0, 100, 100);
hue.setLightState(lightId, state)

and the light is correctly set to those values.

@jaumard
Copy link
Author

jaumard commented Oct 11, 2015

I use the same code as you :) but

state.on().hsl(0, 100, 100);
hue.setLightState(lightId, state)

Have to turn on the light in white not in red. In fact in your api there a confusion between brightness and luminance. Normally the function have to be rename to hsb and not hsl (witch it could be nice to have).

For example test it here :
http://serennu.com/colour/hsltorgb.php
If you set hsl to 0, 100, 100 it's white color, to have a red color you have to set 0, 100, 50

It's more clear ?

@jaumard
Copy link
Author

jaumard commented Oct 12, 2015

I just create a pull request to fix this. Let me know what do you think about it.

@peter-murray
Copy link
Owner

Thanks for the pull request, (silly refactoring removed similar code some time back, and implemented it as hsb).

I added some tests to exercise the code changes, they are somewhat bound to my own bridge and lights.

@jaumard
Copy link
Author

jaumard commented Oct 13, 2015

You're welcome :) thanks for the merge! In fact in the hsl, I just calcul the sat and brightness from the the given sat and luminosity. The calculated value is not luminosityValue but brightnessValue.
So the hsl function can become this if you prefer :

State.prototype.hsl = function (hue, saturation, luminosity) {
    var temp = saturation * (luminosity < 50 ? luminosity : 100 - luminosity) / 100
        , satValue = Math.round(200 * temp / (luminosity + temp)) | 0
        , brightnessValue = Math.round(temp + luminosity);
    return this.hsb(hue, satValue, brightnessValue);
};

Any idea when you can put this on npm ? :D

@peter-murray
Copy link
Owner

It's in the 1.2.0 release, which went out last night

@jaumard
Copy link
Author

jaumard commented Oct 13, 2015

Ho great ! I didn't see it thanks !

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

No branches or pull requests

2 participants