Color picker: show current color instead of black - #1753
Conversation
|
@AnibalPinto Is there a simple way to ensure this transformation is the inverse of the other way? This conversion is just inlined where the picker is opened, can it be moved to a function right above or below the one that does the inverse transformation, or is that conversion just inline somewhere too? ;-) BTW Thanks for looking into this, I rarely use the color picker but when I do it's really annoying that it isn't initialized to the current color! |
|
@phkahler I am using deepseek to help on choosing issues an this was a lucky one. Function It is easy to "transcrive" the functions for an isolated file and some tests, but not sure how ... any ideas? |
|
@phkahler I run some tests and will push the fix and address the comments. Don't have experience on HSV but it appears Solvespace don't use the standard calculus, for ChatGPT evaluation. A doubt on SolveSpace git process, should I address the comments an publish a branch with multiple patches or is preferable to squash everything in only one commit? |
Perfect. And yes, squash all the commits down to one. Also, make sure the change is attributed to you and not an AI agent. For testing, you could write some code to verify round trip conversion RGB->HSV->RGB on a bunch of colors. But we don't want that code in the repository. |
8c5be86 to
2a08a48
Compare
|
I wrote some code to validate RGB->HSV->RGB and didn't fin any issue. |
|
@AnibalPinto This does not work correctly. Open a real model with at least 2 extrusion groups. Edit the color of one group to some blue color. then go edit the other to some green color, be sure to use a different saturation value. Go back to the first group and edit the color. The Saturation slider may well be the wrong color (and wrong value?) In one case I did a blue group and a green group, went back to edit the blue group color and the saturation slider was RED. It's possible that your RGB->HSV conversion is right (the original HSV->RGB code is mind boggling) but something else in the picker initialization is not right. Please do this kind of basic human check before submitting a fix. |
Fixes solvespace#1127. The on-screen color picker stored the incoming RGB in colorPicker.rgb but hardcoded the HSV state to (h=0, s=0, v=1), so the hue/saturation crosshair and value slider always reset to black regardless of the current color. Convert the current RGB to HSV (normalized to the picker's [0,1] convention, with hue in [0,6) as consumed by HsvToRgb) when showing the picker so it reflects the actual color.
2a08a48 to
0a88194
Compare
|
Sorry for disturbing your work flow. My first patch functions HsvToRgb and RgbToHsv were not the inverse due an h /= 6. I removed it and from my tests all appear to be fixed so I pushed, and you found the problem. I look again to code and for what I see all calls to HsvToRgb multiply hue by 6. I modified the code to after call RgbToHsv it divides the hue by 6, I don't understand why this extra step on hue is needed. I also used ChatGPT to improve the function RgbToHsv and reduce the errors on calling rgb = HsvToRgb(RgbToHsv(rgb)) to test the inverse calculation. |
|
@AnibalPinto Thank you for this PR, it's working well now. BTW the factor of 6 doesn't really make sense to me - it's using a strange color conversion. |
Fixes #1127.
The on-screen color picker stored the incoming RGB in colorPicker.rgb but hardcoded the HSV state to (h=0, s=0, v=1), so the hue/saturation crosshair and value slider always reset to black regardless of the current color. Convert the current RGB to HSV (normalized to the picker's [0,1] convention, with hue in [0,6) as consumed by HsvToRgb) when showing the picker so it reflects the actual color.