-
Notifications
You must be signed in to change notification settings - Fork 999
Supports dynamic loading of charcodes #2526
base: master
Are you sure you want to change the base?
Conversation
Sometimes we need to load characters dynamically, especially CJK characters. because they contain huge characters
There is |
Not really sure I should comment in this PR or not as it is beyond my expertise. But I think we need a better glyph caching mechanism for CJK than what we have currently. Having said that, by turning off the cache unconditionally may not be a good answer. Especially if the change would also impact non-CJK font users. |
In my opinion, this effect is limited.This PR doesn't change the font cache actually, it just makes the characters cache as needed. |
I see. I have looked at the code closer now. So, the Just an idea. Instead of setting that flag unconditionally, could it be initialized as per what we have now but we find another way to somehow just load a predefined list of char codes from library user, not hardcoding it like you have proposed. For elementary Chinese app, one may preload 3000 common chars for example. If there is no predefined list then library should still attempt to load all the glyphs as per now. i.e no impact to non-CJK font users. |
Good idea. |
Thanks for the effort. I like the new implementation of the idea. However, just curious why don’t you use hashmap of unsigned to Boolean. I also think it would be better to leave the two non-CJK fonts out of this PR. Perhaps we can add a new free Chinese font from Google Fonts to serve as an example. We could actually use a Chinese font in HelloGUI or somewhere to showcase the IME feature. And lastly, may be we should add a few liners in the online docs describing the “precache” element tag. |
@weitjong Thanks for your reply. I have done what you said. |
If there are no other objections then I will merge this PR over the weekend. Probably I will make a few minor cosmetic changes. About the Google Fonts I thought (IANAL) they are all “free to use”? |
Ok. Thanks. |
Source/Urho3D/UI/Font.h
Outdated
@@ -109,6 +112,8 @@ class URHO3D_API Font : public Resource | |||
FontType fontType_; | |||
/// Signed distance field font flag. | |||
bool sdfFont_; | |||
/// Pre-Cache charaters | |||
HashMap<unsigned, bool> precacheChars_; |
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 don't get when this HashMap
contains false
...
Maybe HashSet
will do?
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. You`re right.I will change it.:)
<precache charset="utf8"> | ||
<![CDATA[ !\"#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`abcdefghijklmnopqrstuvwxyz{|}~]]> | ||
</precache> | ||
</font> |
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.
Can you please add some indentation and newline at the end of file, in both fonts?
If I get the code right, |
Yes, that is intended. I will not merge the two xml files in the PR to ensure the existing fonts are loaded/cached as per before. Instead I will attempt to add a new Chinese font with a precache setting, assuming we are ok to use one of the Google Fonts this way. |
Don't you think that it looks a bit confusing? Perfect solution would be extra flag to turn on/off whole feature (like |
Stale pull request message |
Sometimes we need to load characters dynamically, especially CJK characters. because they contain huge characters