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

Dashboard color picker #945

Merged
merged 4 commits into from
Dec 14, 2020

Conversation

atrovato
Copy link
Contributor

Color-picker on dashboard.

Add auto-close when mouse leave.
Set picker as absolute div to avoid scrollbar.

Next steps:

  • temperature slider
  • saturation slider

@codecov
Copy link

codecov bot commented Nov 11, 2020

Codecov Report

Merging #945 (5f0ee87) into master (7f43cf3) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #945   +/-   ##
=======================================
  Coverage   94.65%   94.66%           
=======================================
  Files         494      495    +1     
  Lines        6552     6564   +12     
=======================================
+ Hits         6202     6214   +12     
  Misses        350      350           
Impacted Files Coverage Δ
...r/services/philips-hue/lib/light/light.setValue.js 89.47% <100.00%> (+1.97%) ⬆️
...rver/services/tasmota/lib/features/colorChannel.js 100.00% <100.00%> (ø)
server/utils/colors.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7f43cf3...5f0ee87. Read the comment docs.

@atrovato
Copy link
Contributor Author

Ok, not really fan of the "auto-close" setup, I'll work more on this.

@atrovato atrovato marked this pull request as draft November 14, 2020 06:52
@atrovato atrovato marked this pull request as ready for review November 14, 2020 13:39
@Pierre-Gilles
Copy link
Contributor

Thanks for the PR ! It's something that will help lots of people 😃

I have a question on this, in which format do you get/save the color ? We had a huuuuge debate on this during the v4 specification. The topic is partly here: https://community.gladysassistant.com/t/parlons-de-gladys-v4/3578/179?u=pierre-gilles

I think we were talking about using the HSL format as "source of truth" format in Gladys, and then each service would convert the HSL to the color format he needs.

@atrovato
Copy link
Contributor Author

I simply convert rgb hex to int, as feature consumes int.

@Pierre-Gilles
Copy link
Contributor

Are we sure that storing colors as integer is not losing information ? For example, Philips Hue is handling 16 millions colors. Are we able to represent that in integer ?

We want that the data format we choose in Gladys to represent colors is able to handle all kind of lights and we don't want to lose information during those conversions.

Feature is not necessarily an integer, it can be a string too with last_value_string in the device_feature table, we use this for camera for example. So we can store HSL, RGB, HSB, whatever we want.

Too me this is an important choice, I remember that in the v3 we chose the integer type just because it was easier for us as developers, but people were complaining that colors in Gladys were not the same as colors displayed on their lamp.

We need to think about this end-to-end with integrations.

@atrovato
Copy link
Contributor Author

You're right, and I think we should have different type of color feature.
Some devices are RGB, some HSL...
Some devices can handle color / saturation / brightness in one request, some needs 3 separated commands.

But according to docs, max INT value (32 bits) is 2.147.483.647. So int can be enough to handle 16.000.000 colors.

For more, in every cases, we'll have to check all current services to change it (to use new HSL / RBG / HUE color feature, or to handle string value).

Ok topic is still open.

@Pierre-Gilles
Copy link
Contributor

Let's take real world examples!

On the Philips Hue documentation, I see that:

Screenshot 2020-11-23 at 15 39 21

Name Type Description
bri uint8 Brightness of the light. This is a scale from the minimum brightness the light is capable of, 1, to the maximum capable brightness, 254.
hue uint16 Hue of the light. This is a wrapping value between 0 and 65535. Note, that hue/sat values are hardware dependent which means that programming two devices with the same value does not garantuee that they will be the same color. Programming 0 and 65535 would mean that the light will resemble the color red, 21845 for green and 43690 for blue.
sat uint8 Saturation of the light. 254 is the most saturated (colored) and 0 is the least saturated (white).
xy list 2..2 of float 4 The x and y coordinates of a color in CIE color space.The first entry is the x coordinate and the second entry is the y coordinate. Both x and y are between 0 and 1. Using CIE xy, the colors can be the same on all lamps if the coordinates are within every lamps gamuts (example: “xy”:[0.409,0.5179] is the same color on all lamps). If not, the lamp will calculate it’s closest color and use that. The CIE xy color is absolute, independent from the hardware.
ct uint16 The Mired Color temperature of the light. 2012 connected lights are capable of 153 (6500K) to 500 (2000K).

But this is theory, on the Philips Hue Node.js implementation documentation, I can see that:

Screenshot 2020-11-23 at 15 24 55

I know this can looks like really precise stuff, but I remember that in the v3 people were complaining that Gladys color picker was wrong and not really usable (example: white is too white, how to set the warm white?), so I don't want to make the same mistake !

Maybe this RGB / Int implementation is enough, I just want to make sure this is working as expected :)

Did you test this PR with real lights ?

@atrovato
Copy link
Contributor Author

Wow...
I tested it with incoming AwoX service, looks fine.
Now we have to check if other color services handle it well, but I don't have anu Philips or Tasmota RBG...

@VonOx
Copy link
Contributor

VonOx commented Nov 23, 2020

I will test your PR on philips devices

@VonOx
Copy link
Contributor

VonOx commented Nov 25, 2020

Something weird

The preview color square is changing when colorpicker popup

colorpicker

And my lamp color is still white

IMG_20201125_112742

@atrovato
Copy link
Contributor Author

Thank you for your feedbacks, I'm going to check for the color picker behavior (1st picture).
For PhilipsHue lights, the setValue method only manages binary feature, no color.
I can try to change it, but I won't be able to test it (no device).

@atrovato
Copy link
Contributor Author

Ok I change current version and add Tasmota / PhilipsHue color management.
I don't understand how Xiaomi works...

Can you please try again?

@Pierre-Gilles Pierre-Gilles marked this pull request as draft December 3, 2020 10:04
@Pierre-Gilles
Copy link
Contributor

While waiting for more feedbacks on this, I put this PR as draft so it's more clear for me in the PR list.

@VonOx let us know when you tested this :)

@Terdious
Copy link
Contributor

Terdious commented Dec 3, 2020

Hi,
i'm also testing that on my end this weekend. Saturday or Sunday. I make a return as soon as possible.

@VonOx
Copy link
Contributor

VonOx commented Dec 4, 2020

@atrovato , it's ok with my philips devices. Colors set in dashboard seems to match real lamp color ( just a feeling )

@VonOx VonOx added the Feature label Dec 4, 2020
@atrovato atrovato force-pushed the bashboard-color-picker branch 2 times, most recently from 270d06f to a4dc466 Compare December 5, 2020 06:19
@atrovato atrovato marked this pull request as ready for review December 8, 2020 19:28
Copy link
Contributor

@Pierre-Gilles Pierre-Gilles left a comment

Choose a reason for hiding this comment

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

Thanks for this PR, I really like the color picker and the integration: it works perfectly 👌

Good job! :)

I tested it with MQTT and it worked very smoothly: https://streamable.com/siqiyd

Just one feedback, I have a weird TypeError in the UI:

Screenshot 2020-12-11 at 12 15 05

Screenshot 2020-12-11 at 12 15 01

It's the only bug I had. Otherwise I'm all in to merge this!

@atrovato
Copy link
Contributor Author

Oww... the click listener was not well removed, even if the component is unmount.
I fixed it on last commit.
Thanks for feedbacks.

Copy link
Contributor

@Pierre-Gilles Pierre-Gilles left a comment

Choose a reason for hiding this comment

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

A really great PR as always, I'm happy to merge it 🥳

Thanks for the changes!

@Pierre-Gilles Pierre-Gilles merged commit 8a6ad94 into GladysAssistant:master Dec 14, 2020
atrovato added a commit to atrovato/Gladys that referenced this pull request Feb 21, 2021
* Dashboard color picker

* Tasmota color

* PhilipsHue color

* Fix color picker removeEventListener
@atrovato atrovato deleted the bashboard-color-picker branch November 28, 2021 20:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants