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

Fix unnecessarily quantizing current color in color picker #85749

Merged
merged 1 commit into from
Dec 18, 2023

Conversation

bs-mwoerner
Copy link
Contributor

Fixes #85743.

The color picker would update the current color to match the HTML string whenever the parsed value did not match the current color. Since Godot Colors have a 16 bit resolution, this would quantize the current value to the nearest HTML color even when no change was made. And since this happened whenever the HTML input lost focus, just opening and closing the color picker was enough to change a 16-bit value to the nearest 8-bit value.

With this PR, the HTML input only updates the current color when the HTML representation is different from the current color's HTML representation (and not whenever the HTML representation represents a different color than the current one).

@bs-mwoerner bs-mwoerner requested a review from a team as a code owner December 4, 2023 16:01
@akien-mga
Copy link
Member

For the record, your commit seems not to be linked to your GitHub account. See: Why are my commits linked to the wrong user? for more info.

@bs-mwoerner bs-mwoerner force-pushed the color_picker_quantize_fix branch from 170db96 to 1b85b8f Compare December 4, 2023 16:20
@bs-mwoerner
Copy link
Contributor Author

Oh, thanks for the heads-up! Should be linked to the correct account now. Thanks!

@akien-mga akien-mga added bug topic:gui cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release labels Dec 4, 2023
@akien-mga akien-mga added this to the 4.3 milestone Dec 4, 2023
@KoBeWi
Copy link
Member

KoBeWi commented Dec 12, 2023

#83786 seems to be also addressing this problem, in a simpler way.
CC @aXu-AP

@bs-mwoerner
Copy link
Contributor Author

#83786 seems to be also addressing this problem, in a simpler way. CC @aXu-AP

Ah, yes, I missed that PR. to_abgr32() is probably the better way, although I think that other solution still requires a slight modification to actually work. I'll add my comments there.

@aXu-AP
Copy link
Contributor

aXu-AP commented Dec 12, 2023

Good work catching the error in my pr. I think this pr could be better fit to be modified, and I don't currently have time to work on it, so feel free to borrow from my solution.

@bs-mwoerner
Copy link
Contributor Author

Good work catching the error in my pr. I think this pr could be better fit to be modified, and I don't currently have time to work on it, so feel free to borrow from my solution.

I will, thanks! And I only caught that error because I spent quite some time pulling my hair out when I was working on my own PR wondering how on Earth it still managed to get the wrong color value through to the caller despite me having breakpoints everywhere. 😅

@bs-mwoerner bs-mwoerner force-pushed the color_picker_quantize_fix branch from 1b85b8f to 974656f Compare December 13, 2023 00:14
scene/gui/color_picker.cpp Outdated Show resolved Hide resolved
Made the HTML field send change events based on whether the new color's string is different from the previous color's string (instead of whether the new string parses to the current color value).
Previously, updating the color value even when the corresponding string hadn't changed would unnecessarily quantize the color value to 8 bits just by opening/closing the Color Picker.
@bs-mwoerner bs-mwoerner force-pushed the color_picker_quantize_fix branch from 974656f to 21d778b Compare December 17, 2023 14:37
@YuriSizov YuriSizov merged commit 1125235 into godotengine:master Dec 18, 2023
15 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@YuriSizov YuriSizov removed the cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release label Jan 25, 2024
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.2.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Color changes when opening color picker
5 participants