-
-
Notifications
You must be signed in to change notification settings - Fork 428
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
Add conversion for HSB to RGBW and back #3849
Conversation
1bb3ef0
to
2571f77
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution. Looks like a useful addition. I have left some comments.
bundles/org.openhab.core/src/main/java/org/openhab/core/util/ColorUtil.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.core/src/main/java/org/openhab/core/util/ColorUtil.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.core/src/main/java/org/openhab/core/util/ColorUtil.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @genesis81, it is good to introduce RGBW as well.
I have added my thoughts regarding style, but also regarding the interfaces an how this fits to the existing methods for RGB.
bundles/org.openhab.core/src/main/java/org/openhab/core/util/ColorUtil.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.core/src/main/java/org/openhab/core/util/ColorUtil.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.core/src/test/java/org/openhab/core/util/ColorUtilTest.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.core/src/test/java/org/openhab/core/util/ColorUtilTest.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.core/src/test/java/org/openhab/core/util/ColorUtilTest.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.core/src/test/java/org/openhab/core/util/ColorUtilTest.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.core/src/test/java/org/openhab/core/util/ColorUtilTest.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.core/src/main/java/org/openhab/core/util/ColorUtil.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.core/src/main/java/org/openhab/core/util/ColorUtil.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.core/src/main/java/org/openhab/core/util/ColorUtil.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, just a few last comments....
bundles/org.openhab.core/src/main/java/org/openhab/core/util/ColorUtil.java
Show resolved
Hide resolved
bundles/org.openhab.core/src/main/java/org/openhab/core/util/ColorUtil.java
Show resolved
Hide resolved
bundles/org.openhab.core/src/main/java/org/openhab/core/util/ColorUtil.java
Outdated
Show resolved
Hide resolved
No problem, i add the points. Thanks for review. |
bundles/org.openhab.core/src/main/java/org/openhab/core/util/ColorUtil.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only left some comments regarding the variable naming. Please also adjust that in the other direction. Once that is done, we are good to go. Please make sure that all names start with a lower case letter.
bundles/org.openhab.core/src/main/java/org/openhab/core/util/ColorUtil.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.core/src/main/java/org/openhab/core/util/ColorUtil.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.core/src/main/java/org/openhab/core/util/ColorUtil.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.core/src/main/java/org/openhab/core/util/ColorUtil.java
Outdated
Show resolved
Hide resolved
for KNX DPT251.600 to use all 4 colors. With the new feature, the HSBType can converted into RGBW, and also back to HSB. Due to the conversion, some accuracy is lost, but the result is approximately correct. Signed-off-by: Marco Müller <[email protected]>
Signed-off-by: Marco Müller <[email protected]>
Signed-off-by: Marco Müller <[email protected]>
Signed-off-by: Marco Müller <[email protected]>
Signed-off-by: Marco Müller <[email protected]>
Signed-off-by: Marco Müller <[email protected]>
Signed-off-by: Marco Müller <[email protected]>
Signed-off-by: Marco Müller <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hsbToRgbwPercent
looks good now. As I said in my last review: please also apply that to rgbwToHsb
.
Signed-off-by: Marco Müller <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
New functions for RGBW introduced in openhab#3849. Signed-off-by: Holger Friedrich <[email protected]>
New functions for RGBW introduced in #3849. Signed-off-by: Holger Friedrich <[email protected]>
} | ||
|
||
/** | ||
* Transform <a href="https://en.wikipedia.org/wiki/HSL_and_HSV">HSV</a> based {@link HSBType} to RGBW. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just found this PR and was interested because I think this method could be used instead of doing two passes here:
https://github.com/openhab/openhab-addons/blob/a13fd80bfe5cf0b6c55e13756f42abc27b55796b/bundles/org.openhab.binding.hdpowerview/src/main/java/org/openhab/binding/hdpowerview/internal/handler/HDPowerViewRepeaterHandler.java#L264-L265
Anyway, commenting because this description seems to be copied from the method for the other direction. This one is RGBW to HSBType
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, included in #3882
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/hue-bulb-changing-color-wrongly-when-brightness-is-0/154005/8 |
For KNX DPT251.600 to use all 4 colors.
With the new feature, the HSBType can converted into RGBW, and also back to HSB.
Due to the conversion, some accuracy is lost, but the result is approximately correct.