Feature: correct small rendering

[Finii]

Description

[why] Some fonts set specific lower resolutions and handle the small rendering very nicely. But fontforge does not copy that information when it opens a font; rather some default values are always written. That can destroy the small font rendering.

The responsible settings are the ppem_to_int flag and the lowestRecPPEM setting, both in the HEAD table.

[how] After successfully patching and generating the new font we copy that settings over from the source font.

Instead of using fontTools (and thereby requiring all users to pull that dependency) we handle the raw font file changes ourselves.

Fixes: 612

Reported-by: @nojus297

Requirements / Checklist

• [x] Read the Contributing Guidelines
• [x] Verified the license of any newly added font, glyph, or glyph set

What does this Pull Request (PR) do?

How should this be manually tested?

Any background context you can provide?

What are the relevant tickets (if any)?

Screenshots (if appropriate or helpful)

All these information is given in Issue #612 but a lot other fonts are also affected.

For example

fontforge font-patcher src/unpatched-fonts/CascadiaCode/Regular/CascadiaCode-Regular.otf
Copyright (c) 2000-2021. See AUTHORS for Contributors.
 License GPLv3+: GNU GPL version 3 or later <http://gnu.org/licenses/gpl.html>
 with many parts BSD <http://fontforge.org/license.html>. Please read LICENSE.
 Version: 20201107
 Based on sources from 2021-01-15 15:55 UTC-ML-D-GDK3.
PythonUI_Init()
copyUIMethodsToBaseTable()
No configfile given, skipping configfile related actions
Nerd Fonts Patcher v2.1.0 executing

Adding 56 Glyphs from Seti-UI + Custom Set 
╢████████████████████████████████████████╟ 100%
Adding 198 Glyphs from Devicons Set 
╢████████████████████████████████████████╟ 100%

Done with Patch Sets, generating font...
Changing flags from 0xB to 0x3
Changing lowestRecPPEM from 8 to 6

Generated: CaskaydiaCoveNerdFont-Regular in './Caskaydia Cove Regular Nerd Font.otf'

(Note messages about 'changing' in the bottom, these values are copied over from the source font into the patched font, because fontforge does not even read them,)

[Finii]

Instead of the hand-made (but more or less trivial) new class TableHEADWriter we could also utilize the excellent fontTools, as @nojus297 suggested here.

It's just the NPM explosion (again) and the inconvenience for end users that let me seek another way to handle it.

Many thanks to @nojus297 whose colaboration made this PR possible.

[vaijab]

Would there be a way to run the build locally to produce patched fonts? I am so keen to get my hands on properly scaled Hack font.

[Finii]

If you have font-patcher already somewhere locally (if not, see here), you can just download the changed script from this branch. Only the font-patcher file is changed.

Or you need https://github.com/ryanoasis/nerd-fonts/blob/feature/correct-small-rendering/font-patcher and all files (recursively) from https://github.com/ryanoasis/nerd-fonts/tree/feature/correct-small-rendering/src/glyphs (in the correct relative directory).

If you use git

git fetch
git checkout feature/correct-small-rendering

Or whatever ;) I do not know where you are. If you use a container or Linux-On-Windows, I have no clue ;)

If you need only a very few specific fonts (and tell me which, and wether --mono or --windows) I might find time to put them somewhere. But please only if you can not do it yourself.

[vaijab]

Or whatever ;) I do not know where you are. If you use a container or Linux-On-Windows, I have no clue ;)

If you need only a very few specific fonts (and tell me which, and wether --mono or --windows) I might find time to put them somewhere. But please only if you can not do it yourself.

This is great, thank you so much. I use linux, no problem. I'll figure it out!

[gnojus]

Thanks for the fix, and the exclusion of fontTools as a dependency is nice. Although this does make me wonder if some tests or checks would help to prevent regression failures in the future.

[Finii]

What kind of checks to you have in mind?

The actual code this PR adds has been tested with the help of fontTools, and I think adding it as test-dependency is quite ok. But I have a problem introducing normal unit tests here; it would require possibly some restructuring of everything. I would also split the code in smaller files as modules, test the modules independently etc, and then 'build' the end-user font-patcher from the individual files.

Some random thoughts following.

But at the moment the concept here is to have everything (also the built artifacts like patched fonts) IN the repo and not only as release artifacts. For easier access by users I assume.

What I also look upon with mixed feeling is that the release is defined only as workflow. Having even some kind of make release Makefile would be more desirable in my opinion, as that allows to produce exactly 'the release' on a local machine easily; and having the github workflow only doing the upload-files-as-release stuff.

And the tests would then be triggered by make test, as usual. THAT can be triggered by a commit hook.

Use of make is just exemplary, usually I use Meson, but even a central shell script would be ok, that calls the appropriate sub-scripts in bin/scripts/.

The question is... is Nerd-Fonts complex enough to change from a github-workflow based release definition to a proper stand alone definition? Hmm.

[gnojus]

I don't doubt that your code is correct and tested. I'm just worried that, as the patcher code is already quite complex, some subtle changes may break it in the future. And it will be hard to notice, as it wasn't easy to understand/reproduce the issue.

Therefore, for this case, I had in mind some sort of CI test to simply check if the flags of generated fonts are correct, e. g. with fontTools. It could be integrated after the patching workflow.

This was inspired by the fact that most recent font builds/releases do not work at all on my machine, they don't show up in font picker. Regression happens easily.

---

Storing patched fonts in the repository is certainly a bad idea, the .git folder is already 7.4G in size, making cloning really slow.

Also separating patching script from github-releasing workflow is probably a good idea, as is restructuring of the whole project.

By the way, also noticed that fontTools is already installed by workflows: https://github.com/ryanoasis/nerd-fonts/blob/877887cac2d6ccc7354a8d95e8c39fff10bf120b/.github/workflows/font-patcher.yml#L52

[Finii]

Therefore, for this case, I had in mind some sort of CI test to simply check if the flags of generated fonts are correct, e. g. with fontTools. It could be integrated after the patching workflow.

Added, utilizing fontforge's own toolset ;-) Thanks for the suggestion.

[jirutka]

It fails if the font file is sfd:

$ font-patcher './src/unpatched-fonts/NerdFontsSymbolsOnly/NerdFontsSymbols 1000 EM Nerd Font Complete Blank.sfd' --complete --ext ttf
...
Done with Patch Sets, generating font...
Traceback (most recent call last):
  File "font-patcher", line 1191, in <module>
    main()
  File "font-patcher", line 1186, in main
    patcher.patch()
  File "font-patcher", line 253, in patch
    source_font = TableHEADWriter(self.args.font)
  File "font-patcher", line 140, in __init__
    self.flags = self.getshort('flags')
  File "font-patcher", line 49, in getshort
    return (ord(self.f.read(1)) << 8) + ord(self.f.read(1))
TypeError: ord() expected a character, but string of length 0 found

[Finii]

It fails if the font file is sfd

Thanks for the report @jirutka! Fixed.