-
Notifications
You must be signed in to change notification settings - Fork 103
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 linux fontconfig #1771
Fix linux fontconfig #1771
Conversation
rts/Rendering/Fonts/CFontTexture.cpp
Outdated
@@ -461,7 +460,7 @@ static std::shared_ptr<FontFace> GetFontForCharacters(const std::vector<char32_t | |||
|
|||
if (family) | |||
FcPatternAddString(pattern, FC_FAMILY, family); | |||
if (foundry) | |||
if (foundry && strcmp("UKWN", reinterpret_cast<char*>(foundry)) != 0 && strcmp("ukwn", reinterpret_cast<char*>(foundry)) != 0) |
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 we put something like your comment from the PR description here, perhaps away in some sort of function?
if (foundry && strcmp("UKWN", reinterpret_cast<char*>(foundry)) != 0 && strcmp("ukwn", reinterpret_cast<char*>(foundry)) != 0) | |
if (foundry && isFoundryOkay(foundry)) |
static bool isFoundryOkay(const auto *foundry) {
// guaranteed null-terminated because of blabla
auto *const foundryString = reinterpret_cast <char*> (foundry);
/* (FIXME wants better wording, i just copypasted from PR)
* it's not finding requested glyphs (at least in first matches) when that is set.
* Imo we could just not filter by foundry at all and would be cleaner but
* trying to modify behaviour as little as possible. I know fontconfig should
* be returning matching fonts in order of priority filters, but somehow
* that's breaking it completely. Those foundries are defaults from some
* programs and totally not recommended to have inside font files. */
return strcmp("UKWN", foundryString) != 0 // fixme: consider string_view + starts_with, more idiomatic
&& strcmp("ukwn", foundryString) != 0; // fixme: case sensitivity (can a foundry be "Uknw"?)
}
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.
Changed that.
Regarding UKWN and ukwn case sensitivity, I found this seeming to imply just UKWN and ukwn are possible fonttools/fontbakery#713, also https://github.com/fonttools/fontbakery/blob/c9954809500b6a2fe4b03995594b917167315ecc/Lib/fontbakery/checks/vendorspecific/googlefonts/os2.py#L104 has some other bad codes but not sure we should filter by those too since we're not having any problems related to that and seems complicating the code for nothing atm.
string_view.starts_with instead of strcmp.
Ok, I got the PR tested on windows by @rhys-vdw, discord logs here. He could confirm:
|
What if you remove the foundry check altogether? I don't think its use was taken from any best practices, but rather from some common sense. |
Because.... ? |
Initializing the config object with FcInitLoadConfigAndFonts already loads system fonts and configuration, so loading manually becomes redundant. |
I agree, didn't want to change more than needed but if you think it's ok I'll remove it. |
removed it |
I see what you mean. Probably makes sense. spring/rts/Rendering/Fonts/CFontTexture.cpp Lines 185 to 191 in 2ed31ad
osFontsDir then too, otherwise it's confusing.
|
Done, all uses of osFontsDir removed now. Also, I noticed now fontconfig wasn't really using our defined cache folder, since fontconfig sets it's own general and user caches and it was preferring the user home cache. For now I set FcConfigEnableHome(FcFalse) before loading config since this reverts back to previous behaviour of using fontconfig inside spring dir as cache directory for the engine fontconfig usage (at least on linux where the system one is not editable by the user, I'm guessing that must also be the case nowadays for windows but can't check myself). I also changed the loop that prints the cache dir(s) since it was printing just the first directory, but there can be many. This (setting FcConfigEnableHome to false) might also skip other user folder fonts or font configuration, but I believe this is unlikely to be a problem. The alternative is just letting fontconfig use it's default cache dirs, probably not bad either, not totally sure why we use our own cache but for now just trying to maintain behaviour as much as possible while fixing issues. (other alternative is trying to fight it to use our cache first, but I didn't see a good way to prepend a cache dir ourselves, if not by convoluting initialization quite a bit) |
Is this the |
Hmm not sure maybe that's the system cache, that's a weird path. The user cache should go somewhere in the user folder and that's the one I disabled. If you run this you should see the cache dirs printed on engines infolog.txt. I'll try to get some log from windows as well to check. |
Work done
Filter out UKWN and ukwn foundries.Remarks
Related issues
All these issues are somehow related, at least they are badly affecting linux since it's not loading system fonts at all atm, although just the fixes here may not fix them for everyone as that depends on system fonts. With the proposed changes now emojis and glyphs from 3925 work here (i had to install noto font in ubuntu 24 though).
Looks like on windows also #3925 is solved with this PR, otherwise fontconfig wouldn't find the glyphs even when present in the system. Not sure if this is because of the new initialization method or because of removing the foundry though, this would have to be checked by someone on windows.
The ZorinOS bug is a bit weird but might be related to trying to load /etc/fonts using FcConfigAppFontAddDir on an old distro, since that's not the intended way to use the method. Looks like the method is for loading actual font files from a directory, but /etc/fonts holds config files.
Imo fixing missing emojis can be fixed by distributing NotoEmoji-VariableFont_wght.ttf, glyphs, for beyond-all-reason/Beyond-All-Reason#3925 I found at NotoSansSymbols2-Regular.ttf (1f81a) and DejaVuSerif.ttf (1F525). All our provided fonts inside fonts/ should be loaded and used if the user misses glyphs. I know this is maybe more of a game issue and I can propose including and loading them there to bar. Then how to prioritize app provided fonts can be looked into but imo it should be feasible (there's some talks about this at #537).