peter-murray / node-hue-api

Node.js Library for interacting with the Philips Hue Bridge and Lights
Apache License 2.0
1.19k stars 145 forks source link

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

Closed jaumard closed 8 years ago

jaumard commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

peter-murray commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

jaumard commented 8 years ago

Ho great ! I didn't see it thanks !