googlefonts / gftools

Misc tools for working with the Google Fonts library
Apache License 2.0
243 stars 70 forks source link

Add '--expand-features-to-instances' arg to fontmake UFO generation #985

Open arrowtype opened 4 months ago

arrowtype commented 4 months ago

UFOs can use relative include statements to import feature files, which is very helpful in the case of a family that has many UFO sources, but just one feature file required. Or, it’s helpful if there are many features, and several complex ones, so they are split into multiple files (example: a Recursive UFO source and its features directory).

Currently, the Builder just brings the include statement directly into the instance UFOs, but then the relative paths are broken, so the fonts don’t build.

fontmake: Error: Compiling UFO failed: <features>:1:8: The following feature file should be included but cannot be found: ./features/features.fea

However, it is very easy to support by adding the arg --expand-features-to-instances to the UFO instantiation step, as I have done in this PR.

I’ve tested this on a UFO-based family (with includes) and a Glyphs-based family (with normal Glyphs-based features), and it seems to work well in both cases. The features are built into the fonts, as expected.

I’m not aware of any downsides or hidden issues with adding this, but they might exist. If any are known, I would be very interested in learning about such cases.

Please let me know if you have any questions or suggestions here! Thanks so much.

arrowtype commented 1 week ago

Oh, I’ve also just realized that the current config option for this seems to activately prevent a build from working.

After I tested the above solution on a config that included expandFeaturesToInstances: true, the build failed with this:

[229/1410] buildTTF
FAILED: /var/folders/nb/mc9zt2n930vd_jkbg0kn2d680000gn/T/tmp8ertvdij 
/Users/stephennixon/venv/bin/python -m gftools.builder.jobrunner fontmake --output-path /var/folders/nb/mc9zt2n930vd_jkbg0kn2d680000gn/T/tmp8ertvdij -o ttf -u sources/ufo/instance_ufos/FamilynameExtraCompressed-Hairline.ufo.json --filter ...  --filter FlattenComponentsFilter --filter DecomposeTransformedComponentsFilter --expand-features-to-instances 

Command failed:
fontmake --output-path /var/folders/nb/mc9zt2n930vd_jkbg0kn2d680000gn/T/tmp8ertvdij -o ttf -u sources/ufo/instance_ufos/FamilynameExtraCompressed-Hairline.ufo.json --filter ... --filter FlattenComponentsFilter --filter DecomposeTransformedComponentsFilter --expand-features-to-instances

usage: fontmake [-h] [--version] [-g GLYPHS | -u UFO [UFO ...] | -m
                DESIGNSPACE] [--glyph-data GLYPHDATA] [-o FORMAT [FORMAT ...]]
                # etc etc etc

fontmake: error: "--expand-features-to-instances" option invalid for UFO source

With that arg removed from the static font step, the build does complete.

~However, the next challenge is adding the config option to the instantiateUfo process. I need to figure out how to access the config at that stage (assuming I can, somehow... I may not yet properly understand the logic here).~

Okay! I’ve figured out how to access and pass that config option to the instantiateUfo step, and I’ve made it default to true, as is written in the builder docs. I’ve tested the config without the option, and with the option set to true and then set to false. It acted as expected, in each case. It inserts that arg into the UFO instantiation, making those features work in the final build:

▶ gftools builder 'Familyname/config.yaml'
[1/1410] instantiateUfo
fontmake -i Familyname Extra Compressed Extra Bold -o ufo -m sources/ufo/FamilynameRoman.designspace --ufo-structure=json --expand-features-to-instances --output-dir sources/ufo/instance_ufos
[2/1410] instantiateUfo
fontmake -i Familyname Extra Compressed Thin -o ufo -m sources/ufo/FamilynameRoman.designspace --ufo-structure=json --expand-features-to-instances --output-dir sources/ufo/instance_ufos
arrowtype commented 1 week ago

Thanks, @m4rc1e! I believe it’s now formatted as desired. It seems to need approval to run the workflow to verify that, though.