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 conversion of hex color strings in project converter #74026

Merged
merged 1 commit into from
Sep 29, 2023

Conversation

ericliu206
Copy link
Contributor

fixes #73633

@ericliu206
Copy link
Contributor Author

This appears to fix the issue, however, I am not sure if this is the best way to organize the code. Any suggestions will be appreciated. :)

@Maran23
Copy link
Contributor

Maran23 commented Apr 10, 2023

Can you also add tests? (test_conversion_gdscript_builtin).
Also you need to rebase the branch. :)

@YuriSizov YuriSizov modified the milestones: 4.1, 4.2 Jun 20, 2023
@YuriSizov YuriSizov added the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Jun 20, 2023
@akien-mga akien-mga requested a review from YuriSizov June 21, 2023 07:51
@YuriSizov
Copy link
Contributor

Sorry for the wait. So I checked this again and realized my suggestion was rather poor, as it would incorrectly convert your normal 6-character long hex colors.

image

Test set
Color("#dede")
Color("dede")
Color("#7e4e")
Color("8e0d")
Color("#bcbcbc")
Color("fefefe")
Color("#11dd33aa")
Color("11dd33aa")
Color("#9d9g9e")
Color("abch")
Color("#deded")
Color("0909909")
Color("123456789")

So I went ahead and updated your PR on your behalf, adding more tests to catch these cases and splitting the operation into two passes: one explicitly for 4 characters and another explicitly for 8 characters.

@akien-mga akien-mga merged commit 0923b87 into godotengine:master Sep 29, 2023
15 of 16 checks passed
@akien-mga
Copy link
Member

akien-mga commented Sep 29, 2023

Thanks! And congrats for your first merged Godot contribution 🎉

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Oct 24, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.3.

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.

Conversion to 4.0 doesn't change hex string to RGBA in Color constructor
4 participants