[basicUI] Wrong behaviour of the colorpicker

[lolodomo]

I see a big bug with the colorpicker in BasicUI. When you use it to select a color near the limit of the circle for a hue color light and then wait for the state update, you see the selector moving near the center of the circlie which maps the real color of the light (not the expected color). So the scolorpicker allows to select a color which is not correctly handled by the hue system.

Even with a color item not linked to any device, I can see that if I select a color at the limit of the circle and then close/reopen the popup, the selector can be outside the circle ! This probably explains the strange behaviour with the hue light.

The popup after moving the selector to the limit of the circle:

Then after I close and reopen the popup:

Here are associated logs:

12:27:45.918 [INFO ] [openhab.event.ItemCommandEvent      ] - Item 'MagicColor' received command 305,19,100
12:27:47.447 [INFO ] [openhab.event.ItemCommandEvent      ] - Item 'MagicColor' received command 305,19,100
12:27:47.449 [INFO ] [openhab.event.ItemCommandEvent      ] - Item 'MagicColor' received command 305,19,100
12:27:47.449 [INFO ] [openhab.event.ItemCommandEvent      ] - Item 'MagicColor' received command 312,32,100
12:27:47.449 [INFO ] [openhab.event.ItemCommandEvent      ] - Item 'MagicColor' received command 312,32,100
12:27:47.449 [INFO ] [openhab.event.ItemCommandEvent      ] - Item 'MagicColor' received command 312,32,100
12:27:47.449 [INFO ] [openhab.event.ItemStateChangedEvent ] - Item 'MagicColor' changed from 305,19,100 to 312,32,100
12:27:47.734 [INFO ] [openhab.event.ItemCommandEvent      ] - Item 'MagicColor' received command 312,32,100

Sometimes, the selector is moved near the center of the circle after reopening the popup.

I have the strange feeling that the selector allows to select invalid color ?

[lolodomo]

It might be a HSB <-> RGB conversion problem, as I see that the state is converted to a RGB value when opening the page: https://github.com/openhab/openhab-webui/blob/main/bundles/org.openhab.ui.basic/src/main/java/org/openhab/ui/basic/internal/render/ColorpickerRenderer.java#L74 This is the first time I am looking to the JavaScript code of the colorpicker. It looks like it works internally with RGB.

[lolodomo]

When you move at the limit of the circle (saturation around 100%), there is certainly a RGB=>HSB conversion error because I can see saturation moving to 2 !

12:22:07.194 [INFO ] [openhab.event.ItemCommandEvent      ] - Item 'DemoColor' received command 315,93,100
12:22:07.201 [INFO ] [openhab.event.ItemStateChangedEvent ] - Item 'DemoColor' changed from 314,88,100 to 315,93,100
12:22:07.499 [INFO ] [openhab.event.ItemCommandEvent      ] - Item 'DemoColor' received command 314,95,100
12:22:07.500 [INFO ] [openhab.event.ItemStateChangedEvent ] - Item 'DemoColor' changed from 315,93,100 to 314,95,100
12:22:20.353 [INFO ] [openhab.event.ItemCommandEvent      ] - Item 'DemoColor' received command 314,95,100
12:22:20.662 [INFO ] [openhab.event.ItemCommandEvent      ] - Item 'DemoColor' received command 312,2,100
12:22:20.667 [INFO ] [openhab.event.ItemStateChangedEvent ] - Item 'DemoColor' changed from 314,95,100 to 312,2,100

[lolodomo]

The code checks that the saturation value is not greater than 100 using a modulo 100. I can imagine that color.s was 1.02 in my case, instead of 1.00, leading to considering 2% of saturation instead of 100%.

The problem could be in the function updateValue at this line when computing the saturation: https://github.com/openhab/openhab-webui/blob/main/bundles/org.openhab.ui.basic/web-src/smarthome.js#L1114

[lolodomo]

If I set the value to 312,100,100 using MainUI: And then open the color picker in Basic UI, the cursor is at the right place: So the initial HSB to RGB conversion is correctly done. The problem seems to be the conversion to HSB when releasing the mouse at the limit of the circle or even outside the circle.

[andrewfg]

Continuing from here https://github.com/openhab/openhab-webui/issues/427#issuecomment-1320873817

@lolodomo Yes, I am pretty sure that the problem is the mouse hit test java script code. I am working on it. And will come back with a proposed code change (which hopefully you can test).

[lolodomo]

The problem seems to be the conversion to HSB when releasing the mouse at the limit of the circle or even outside the circle.

If you click around the limit of the circle, the computed saturation will be finally very small. Something like 1.02 instead of 1.00, leading to 2% saturation.

If you click at the top left of the square (so clearly ouside the circle), this leads to 40% as saturation, instead of 100%, even if the circle is still on the border of the circle. I think computed value for saturation is probably 1.40 leading to a final 40% value (instead of 100%).

[lolodomo]

@lolodomo Yes, I am pretty sure that the problem is the mouse hit test java script code. I am working on it. And will come back with a proposed code change (which hopefully you can test).

Great.

[lolodomo]

When I re-read my initial message, it can even happen that the small circle is outside the big circle when you open the color picker. I did not reproduce that case today but it is probably a bug at a different place in the JavaScript but still in function Colorpicker of course.

[lolodomo]

Here is the place where the final modulo 100 is done just before sending the updated value to the bus: https://github.com/openhab/openhab-webui/blob/main/bundles/org.openhab.ui.basic/web-src/smarthome.js#L1405

[lolodomo]

The problem could be in the function updateValue at this line when computing the saturation: https://github.com/openhab/openhab-webui/blob/main/bundles/org.openhab.ui.basic/web-src/smarthome.js#L1114

Maybe we just have to be sure there that the value will be 1.0 max. I will check that.

[andrewfg]

Yes the problem is definitely in updateValue; it produces 'munged' Hue values; and as you say, the modulo 100 conversion is therefore needed separately to 'un-munge' it again. And furthermore the hit test logic is completely wrong. I am currently writing some code that I can run in a JUnit test so I can produce the correct HSB values in updateValue. And then in a second step I think we would need to check the 'un-munge' code does not munge it again..

[andrewfg]

@lolodomo I think the following should be the correct JavaScript code for 'updateValue()'. I did not actually test in JavaScript, but I wrote the same code in Java and tested it in a JUnit with all possible mouse hit (x,y) positions of 0<=x/y<=300, and assertion tested to confirm that 0.0<=HSV:hue<=1.0 and 0.0<=HSV:sat<=1.0 under all input circumstances.

EDIT: and I also tested that color wheel does correctly produce a hue value in degrees that rotates anticlockwise starting from the bottom centre of the color wheel to produce red:0, yellow:60, green:120, cyan:180, blue:240, and magenta:300 degrees.

function updateValue(event) {
    var
        pos;

    if (event.touches !== undefined) {
        pos = {
            x: event.touches[0].pageX - _t.colorpicker.offsetLeft,
            y: event.touches[0].pageY - _t.colorpicker.offsetTop
        };
    } else {
        pos = {
            x: event.pageX - _t.colorpicker.offsetLeft,
            y: event.pageY - _t.colorpicker.offsetTop
        };
    }

    var
        maxRadius = _t.image.clientWidth / 2,
        offsetX = pos.x - maxRadius,
        offsetY = pos.y - maxRadius,
        radius = Math.sqrt((offsetY * offsetY) + (offsetX * offsetX)),
        angle = Math.atan2(offsetX, offsetY) / Math.PI / 2;

    if (radius > maxRadius) {
        var
            ratio = 1 - Math.abs(maxRadius / radius);
            pos.x -= (offsetX * ratio);
            pos.y -= (offsetY * ratio);
    }

    _t.handle.style.left = (pos.x / _t.image.clientWidth) * 100 + "%";
    _t.handle.style.top = (pos.y / _t.image.clientWidth) * 100 + "%";

    var
        hsv = {
            h: Math.min(1.0, Math.max(0.0, (offsetX >= 0 ? angle : 1 + angle)),
            s: Math.min(1.0, radius / maxRadius),
            v: 1
        },
        hsl = hsv2hsl(hsv);

    _t.hsvValue = {
        h: hsv.h,
        s: hsv.s,
        v: _t.slider.value / 100
    };

    hsl.l = hsl.l < 0.5 ? 0.5 : hsl.l;
    _t.background.style.background = "hsl(" + hsl.h * 360 + ", 100%, " + (Math.round(hsl.l * 100)) + "%)";
}

[lolodomo]

I am going to do it just a little differently for the saturation. But something else must be fixed because the new code leads to 1 (as expected) which is then converted to 0 due to the module 100. But I think you do not need this modulo 100 anymore. I have to check your changes regarding the hue to understand if they are required. I am going to submit a PR.

[andrewfg]

the new code leads to 1 (as expected) which is then converted to 0 due to the module 100

I think it is true also in HSBType. Hue 0 is pure red, and hue 1.0 (aka 360 degrees) is also pure red.

But I think you do not need this modulo 100 anymore.

Agreed. Especially if you change the logic above from hue <= 1.0 to hue <1.0

I have to check your changes regarding the hue to understand if they are required.

FYI I changed the code from the old (IMHO very messy) angle calculation based on the Math.atan() function to use the now officially recommended new Math.atan2() function..

    angle = Math.atan2(offsetX, offsetY) / Math.PI / 2;

[lolodomo]

I think it is true also in HSBType. Hue 0 is pure red, and hue 1.0 (aka 360 degrees) is also pure red.

I was talking about saturation, not hue. Saturation is between 0 and 100.

Look at my change, it is working.

FYI I changed the code from the old (IMHO very messy) angle calculation based on the Math.atan() function to use the now officially recommended new Math.atan2() function..

I have not yet integrated any change regarding hue (angle calculation). Will do it.

Thank you for your help to fix a MAJOR bug in Basic UI.