-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Added TrueType default font to allow for different sizes #7354
Conversation
Let's add this to release notes. |
Unfortunately, this breaks BC. We ( I really like this change, because the old default font was ugly AF (not complaining; atleast there was a default font). Would it be possible to expose the old font so? IIUC, it is currently only possible to load it with |
Hi. Given that you like the change, would it not be simpler to update the torchvision tests? Even if we provide a way to switch back to the old font, wouldn't you want to add torchvision tests for the new font as well? I personally think of backwards compatibility in terms of errors and functionality, not individual pixels. As another example, in Pillow 9.0.0, we switched to libjpeg-turbo in our wheels, which lead to users reporting that JPEG pixel values were different - but I don't think that means the Pillow wheels were backwards incompatible. Not trying to shift the problem onto you, just looking to understand the motivation here. |
The keyword in my statement is I. But I don't know if there is consensus for this amongst the other maintainers. I'll get back to you on that.
For the stuff that we are testing, the actual font is irrelevant. We just want to make sure that some bounding boxes drawn onto an image are labelled. Meaning, regardless with which font we ultimately end up with, we'll never have multiple reference images for different fonts.
That is fair, although I would still classify a change in default value BC breaking. However, the case here is not just a change of default value, but the old value is no longer accessible. Meaning, it is impossible for the user to get the old behavior back. I'm not familiar with Pillows BC policy, but since the versioning scheme is semver, I would only expect BC breaks on major versions. And even there I would expect a deprecation cycle where possible. Doesn't work for switching from |
FWIW different FreeType versions sometimes render fonts with minor differences, so you might still prefer a bitmap font. However, IIRC it was more of an issue with older versions and not that common anymore. |
The torchvision concern was later discarded
|
@radarhere This change broke some of the tests for #7244. |
Wouldn't a simple fix be to just create an instance of the ImageFont class directly in the test? I've created #7647 |
#7647 has now been merged. |
It also caused our tests to fail and decided to include and specifically load the old builtin font to restore failing tests, instead of having to redo each test for the new builtin font (rm-hull/luma.core#274) |
Resolves #6622
At the moment, Pillow's builtin font,
ImageFont.load_default()
, has a static size.This PR checks if FreeType support is available, and if so, uses Aileron Regular instead, a CC0 font from https://dotcolon.net/font/aileron
Because it is CC0, we are free to change it, so I removed various characters using FontForge to reduce the file size, and converted it to TTF using https://convertio.co. This increases the size of ImageFont.py from 42kb in main to 60kb in this PR.
You might ask why I didn't convert it to WOFF or WOFF2. WOFF2 requires at least FreeType 2.10.2 which not all of our Docker jobs have, and WOFF doesn't load on some of our Docker jobs.
With a TrueType font, the size is able to be changed. In terms of our API, the following arguments are added.
This PR designates all of the new ImageDraw arguments as keyword arguments.