-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
FSUI: Load all language fonts at all times #13589
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
base: master
Are you sure you want to change the base?
Conversation
47467db to
0422418
Compare
2f63b04 to
03f7350
Compare
TheLastRar
left a comment
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.
translation.cpp needs to be formatted, any sections where you disagree with clang-format probably should have // clang-format off/on comments added.
I've not reviewed the added freetype code or the patches to imgui.
| void SetFontPath(std::string path); | ||
| struct FontInfo | ||
| { | ||
| std::span<const u8> data; | ||
| std::span<const u32> exclude_ranges; | ||
| const char* face_name; | ||
| bool is_emoji_font; | ||
| }; | ||
|
|
||
| /// Sets a list of fonts to use. | ||
| void SetFonts(std::vector<FontInfo> info); | ||
|
|
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.
Given how we have to tread emoji fonts separately, would it make sense to pass those fonts as a second parameter instead of having to check is_emoji_font eveytime we iterate info?
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.
Hopefully in the future we can treat emoji fonts less special, and it'll just be an is_emoji_font ? color : grayscale.
I'd like to keep them together.
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.
Aside from the Glyph*AdvanceX setting, the main special treatment needed would be filtering the Variation Selector codepoints.
I'm suspecting that will probably be needed for as long as we use ImGui for text rendering.
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.
Ahh true
I'd still like them together since it allows the font selection code to have minimal extra stuff to work around imgui's brokenness.
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.
Side note, I'd like to get rid of the forced advance at some point, since it affects the spacing when used in normal text. If we want them to use a fixed amount of space when being used as icons, we should do that by separately rendering the icon and giving it a fixed amount of space, not by editing the font (which is shared by normal text rendering) to make it monospace.
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'd still like them together since it allows the font selection code to have minimal extra stuff to work around imgui's brokenness.
I guess it's a question where you think it's best to handle it.
I'm still inclined to think in translation is better, where you are already filtering the emoji font due to ImGui's broken bitmap and missing COLv1 support.
But I guess that is up to you.
Side note, I'd like to get rid of the forced advance at some point.
We should, outside of the settings interfaces I think there are a few other locations relying on forced advance, but outside the scope here.
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'm still inclined to think in translation is better, where you are already filtering the emoji font due to ImGui's broken bitmap and missing COLv1 support.
0001-FSUI-Separate-font-and-emoji-font-lists.patch
Is this what you were thinking? Because it doesn't really help specialize that filtering for only emoji fonts or anything.
(The filtering isn't even applied to all emoji fonts, since it wouldn't properly allow a non-SVG COLRv0 or B&W emoji 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.
I mentioned the filtering as more of an example of translations working around ImGui issues, rather then anything to be address. (It looks fine to me).
Your patch does look reasonable, when I to a poke at it I had looked at TryLoadFonts/DownloadFontIfMissing instead TheLastRar@122c5fb
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.
Not a huge fan of either patch tbh, but I'd prefer mine if we have to use one
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 guess it's fine
Maybe a 3rd opinion would serve as a deciding factor.
Protects against all the other stuff we load eating up all the address space that we want
e35ef57 to
b1ba002
Compare
b1ba002 to
c22d397
Compare
c716ff3 to
2618ccc
Compare
2618ccc to
a228248
Compare
|
I have a question: instead of relying on Noto Color Emoji, can’t it instead just take the OS’s color emoji font and work with that? Then there is no longer a need to include Noto just for some emojis. |
It does that on Windows. Apple Color Emoji isn't currently renderable, since it's bitmap based, which isn't supported by imgui+Freetype. Most Linux distros ship Noto Color Emoji, but it comes in Bitmap, SVG, and COLRv1 versions and SVG is the only one imgui supports. That's the biggest of the three, so most Linux distros do not ship it. Fedora recently switched from Bitmap to COLRv1, neither of which we can render. |
TheLastRar
left a comment
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.
Had a quick look at the achievement FSUI and spacing looked okay.
Code changes look fine, Pending another reviewers opinion on #13589 (comment)
ocornut responded to your request ocornut/imgui#8857
Can you perhaps respond to them given it's relevance to your patch?
Done |
Description of Changes
Fixes #12804
Rationale behind Changes
Suggested Testing Steps
Notes
Did you use AI to help find, test, or implement this issue or feature?
No