-
-
Notifications
You must be signed in to change notification settings - Fork 152
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
Improve SysFont name resolution on Windows #3093
base: main
Are you sure you want to change the base?
Conversation
Is it possible to create a test that fails on our current Windows CI testing, but then passes after this PR? I.e. it tries to load a default font with a specific style and currently returns the wrong one. I think that would help ensure that we don't backslide in this area and improve the 'what is this for?' aspect of this PR. |
I'm not as bothered about exact historical backwards compatibility here because this function has always been 'fuzzy' in that it is entirely environment dependant what you get back when using SysFont. Two different versions of windows/Mac/Linux don't have the same set of system fonts. If you wanted fine precision in your font choice in pygame you were already shipping and loading your own precisely selected 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.
See above comments about test. I think we should also add a note to the documentation here if we are going to change the behaviour of this function.
I added a test case which tries to create "Arial" and "Arial Narrow" sysfonts and checks their name/styles. Though I'm not sure if this is reliable enough across all Windows systems; maybe the test should be ignored/skipped if the expected font files don't exist? And IDK if hard coding the name/style_name is a good idea. |
Hmm Arial Narrow isn't listed on the official list of Windows fonts: https://learn.microsoft.com/en-us/typography/fonts/windows_10_font_list |
Alternatively, "Calibri"/"Calibri Light" are also affected and on that list so we can test that instead. |
As for documenting the change: if it's documented then we should also probably address the related Unix issue too (#3092 (comment)). But I'm not sure quite how we would want to handle that. |
Thank you for your PR, I've now looked it over (took me a bit) I wanted a way to generate comparisons, so I wrote this script that reaches into the sysfont internals and outputs a json serialized dictionary. import json
import pygame
pygame.init()
f = pygame.font.SysFont("Arial", 32)
adjusted = {name: [[list(designation), path] for designation, path in content.items()] for name, content in pygame.sysfont.Sysfonts.items()}
adjusted_sorted = dict()
for key in sorted(list(adjusted.keys())):
adjusted_sorted[key] = adjusted[key]
print(json.dumps(adjusted_sorted)) I then used VS code to format it and quickdiff.net to compare the two line by line. Here is the comparison: https://quickdiff.net/?unique_id=D8C69C31-12B1-0700-8F27-1554E0E065AA (will expire after 14 days) Going through the changes there are some that are good, and some that are questionable:
On the bright side I also notice:
On another note, I'd like to think about approach. The sysfont api allows a way of requesting bold and a way of requesting italic. Bold is a font "weight", see https://developer.mozilla.org/en-US/docs/Web/CSS/font-weight#common_weight_name_mapping. So bold = font weight 700, black = font weight 900. Do we really want Arial Black to be seen in the system as its own font. If we do that, why not give Arial Bold it's own font entry too? I think the system should be more font weight aware, perhaps even exposing that as an API to the user in the future. A thought experiment: if this runs on a system that has Arial Demi Bold (font weight 600) but no normal bold and the user asks for "Arial" and bold, should the system give them demi bold as a close enough? My suggestion is for you to split some of the smaller changes you've made here into another smaller PR, that will be easier to get merged and into the codebase. Then this PR can be more focused on the light -> normal issue. |
All of those changes were intentional. The "MT" is part of the font's name, as it is listed in MS Word and recognized in a web browser, and we don't remove other similar suffixes like "MS" or "FB". Ideally we would only have one version of those font names to avoid confusion, but I decided to keep the existing normalized names where possible to avoid breaking existing users. As for "demibold" and "light", yes these are font weights and not separate font families, but since they can't otherwise be selected by sysfont I think it makes most sense to treat them as independent. (Interestingly, Word does list it as "Berlin Sans FB Demi". But the font's name in the registry contains the two words "Demi Bold", whereas other fonts have a single word "Demibold" or "Semibold", and it seems that these should be treated identically and different from plain "Bold".) |
Alright, I'm receptive to the MT change with your reasoning. From googling MT means monotype. MS presumably means Microsoft. Do you know what FB means?
Well, we could change that in sysfont. I don't like a situation where normal and bold are shunted into one bucket and everything else is shunted into another bucket with no plan to remedy the disparity. |
Apparently FB is a company Font Bureau: https://en.wikipedia.org/wiki/Font_Bureau. As for the different weights, yeah but it would probably require a much more extensive redesign which I think is out of scope here. Also both MS Word and Chrome seem to recognize most of the font weight variations as separate fonts (although Chrome for some reason doesn't seem to recognize any "Semilight" fonts; Light, Demibold, etc. are mostly recognized). I think making them available separately doesn't necessarily preclude also having them as weight variants of the base font in the future. |
Sure, but I don't want to start adding something to our API surface if we don't plan on offering it in the future. I feel that this breaks the existing sysfont paradigm. |
I mean we could offer both. There's no reason we couldn't have e.g. Calibri Light and Calibri (font_weight= 50%) or whatever map to the same thing. |
Yes, we could. But I see this as a paradigm altering change to sysfont, but it doesn't have a consistent plan for full realization (what happens with bold?), and more importantly if it shifts how things work on Windows, what about Mac and Linux? I'm not comfortable approving this PR in this state because I'm unsure about some of the changes, and I feel that it will make sysfont less consistent between platforms. |
I mentioned in the bug that Linux probably also needs a similar change, due to similar collision issues with multiple font styles. Regarding platform consistency however, the normalization done on Windows seems much more arbitrary and ad-hoc then on the other platforms and isn't really consistent at all. If you're still skeptical of making all styles separate, a shorter-term strategy might be to just attempt to keep only the "regular" variant of each font. But this seems more iffy: 1) there's no obviously complete set of what should be treated as "weights" or variants verses separate fonts; what about Black, Medium, Thin, Hairline, Condensed, Book, Roman... (all labels l found in the font "style" fields in Linux); 2) if we did this more consistently in Windows it would remove fonts that are currently available, including some commonly known ones like Arial Black; and 3) what would we do if there is no "regular" version of a family? Just fall back to a random style like we do now? |
Well, I think there's potential to separate out the "font discovery" and "font name parsing" components so each platform feeds into the same font name parsing machinery. I've also floated the idea of making the underlying system font weight aware, and then having the existing sysfont API select on top of that. #2760 (comment). But I was thinking this would be a pygame-ce 3.0 thing to minimize compatibility worries.
There's no complete set of what we should do for any of this stuff!
Your PR right now eliminates fonts:
I guess so, or not give the user it entirely? |
Which seems to me to point in the direction of doing as little as possible and leaving it more up to the user. Someone could even implement their own font weight system if they want, if we provided all the font variants.
Yeah it does eliminate a few names, but only very obscure and mostly broken cases, and they are still available under a different name. It's worth pointing out that in the current status quo, Arial itself (probably the most common font) gives the wrong font, at least when Arial Narrow is installed (I'm guessing that comes bundled with MS Office or some other MS software, not sure). So any change that fixes that would be at least a good start. |
This solves the problem in #3092 with font style variants being stripped and an arbitrary style being picked. It also handles some additional cases such as "Regular" and "Oblique" in font names, with Oblique being treated the same as Italic.
I tried to maintain backwards compatibility by generating two name variants for each font: a full name that only strips Regular, Bold, and Italic/Oblique, which should be unambiguous; and a legacy name that strips the somewhat arbitrary set of suffixes: "demibold", "narrow", "light", "unicode", "bt", and "mt". The legacy name doesn't strip Regular or Oblique. If a full name and legacy name collide then the full name will be used, and for multiple legacy names only the first one will be used.
Note that I didn't attempt to create any explicit precedence between legacy names, so they may still resolve to an unexpected style if they don't match the full regular font name.
Some other slight changes: