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

Optimize Color::find_named_color() #75860

Merged
merged 1 commit into from
May 23, 2024

Conversation

dalexeev
Copy link
Member

@dalexeev dalexeev commented Apr 9, 2023

Use binary search instead of linear.

Note that GDScript already caches constant values, unlike RichTextLabel. If we still decide to use a HashMap, then we need to store two names (for example, ALICE_BLUE and ALICEBLUE), since the name with underscore is needed for constant binding.

@dalexeev dalexeev requested a review from a team as a code owner April 9, 2023 17:12
@RedworkDE
Copy link
Member

This and your unicode pr makes me think that we should maybe introduce a proper binary_search function that takes a three way comparison and return the complement of the would be index when not found, to for cases like these where information about the existence of the value is also reqired and the existing bsearch function is not useful.

@YuriSizov YuriSizov added this to the 4.1 milestone Apr 10, 2023
@YuriSizov YuriSizov modified the milestones: 4.1, 4.2 Jun 14, 2023
@AThousandShips AThousandShips modified the milestones: 4.2, 4.3 Oct 26, 2023
@dalexeev dalexeev force-pushed the optimize-find-named-color branch from 04bf446 to e129c85 Compare February 22, 2024 09:21
@dalexeev dalexeev force-pushed the optimize-find-named-color branch from e129c85 to 200ace4 Compare May 23, 2024 07:18
@lawnjelly
Copy link
Member

lawnjelly commented May 23, 2024

This should probably just use some kind of HashMap as said imo. I don't see the advantage of binary search here, perhaps I'm missing something.

You could probably have a collision free hash even with the duplicates.

Also this kind of stuff should likely be parsed once then reused on subsequent frames, so there's only a cost if the text is constantly changing. I'm assuming this is the case, but I haven't read any of this code.

@dalexeev dalexeev force-pushed the optimize-find-named-color branch from 200ace4 to dd82146 Compare May 23, 2024 08:22
@dalexeev
Copy link
Member Author

This should probably just use some kind of HashMap as said imo. I don't see the advantage of binary search here, perhaps I'm missing something.

Done.

Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haven't tested but LGTM

core/math/color.cpp Outdated Show resolved Hide resolved
@dalexeev dalexeev force-pushed the optimize-find-named-color branch from dd82146 to 49594d8 Compare May 23, 2024 12:19
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The optimization looks good.


I'm a bit puzzled at the general implementation of that feature though, it feels pretty convoluted, especially with the "normalization" done to support apparently arbitrary variants like "yel_lo.w'G R'-een"...

Seems like unnecessary complexity, but well, changing it now would break compat.
With a simpler design, we could just have a hashmap for the colors directly instead of storing an intermediate map of indices.

@akien-mga akien-mga merged commit 7fd6eb3 into godotengine:master May 23, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks!

@dalexeev dalexeev deleted the optimize-find-named-color branch May 23, 2024 23:00
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.

6 participants