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

[arrowtype]

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]

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

[m4rc1e]

@arrowtype Please fix the lint issues and I'll take another look.

[arrowtype]

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

[m4rc1e]

There's no way of enabling and disabling this flag inside a config file, You need to add expandFeaturesToInstances to the builder's argument schema, https://github.com/googlefonts/gftools/blob/main/Lib/gftools/builder/schema.py#L66.

It is enabled by default in this pr. I'd prefer it if it was set to false and then the user has to enable it in their config files.

[arrowtype]

Okay, thanks for taking a look and for suggesting that update! Fair enough. I’ll try to make those adjustments soon.

[arrowtype]

@m4rc1e Oh wait, isn’t it already there in the schema? It’s also described in the docs. This PR merely makes it work as already described.

gftools/Lib/gftools/builder/schema.py

Lines 102 to 104 in d23ad21

	Optional("removeOutlineOverlaps"): Bool(),
	Optional("expandFeaturesToInstances"): Bool(),
	Optional("version"): Str(),

gftools/docs/gftools-builder/README.md

Lines 170 to 172 in d23ad21

	- `expandFeaturesToInstances`: Resolve all includes in the sources\'
	features, so that generated instances can be compiled without
	errors. Defaults to `true`.

[arrowtype]

In the past, @anthrotype has suggested that this option should be the default:

We should make that the default...

The builder seems like a place it might make sense to do that, and that’s why I figured it was documented as such. Or, do you think it’s important to keep it non-default? I don’t think it would be a breaking chance to make it default, as I don’t think it blocks features that are already flattened into each source.

[m4rc1e]

Any reason why you're using self.original here? It is breaking the tests since expandFeaturesToInstances isn't in self.original. If I change it to vars it seems to be working but I could be wrong.

Personally, I would use a dict's get method if I cannot guarantee that a key exists. It also allows you to set a value incase it doesn't exist so in your case self.original.get("expandFeaturesToInstances", False) will return False if the key doesn't exist, or it will simply return the value if it does.

[arrowtype]

First, thank you for that tip on .get() – that is news to me, but really helpful to learn!

Any reason why you're using self.original here?

As far as I can remember, I think I just tried to use a similar format as what is a few lines below:

gftools/Lib/gftools/builder/operations/instantiateUfo.py

Line 69 in cc8617e

if self.original.get("glyphData") is not None:

...and when it seemed to work, I thought I had solved it.

My impression was that self.original is the GOOGLEFONTS_SCHEMA, but I now realize that, even if that is true, there are other possible schemas.

[m4rc1e]

Are we 100% sure that this doesn't work? If I was implementing such a feature then I'd simply chuck it here.

[arrowtype]

This is the approach in the release as of when I made the PR, right? If so, then yes, I’m 100% sure it wasn’t working. I can test it again, though, if it seems likely that it may have changed since then.

[simoncozens]

That line feels right - I think I'd like to understand why it's not working before trying something else, because understanding why not might lead us to the right answer.

[arrowtype]

Okay, returning later because the issue persists, and I’ve been working around it a lot, recently. To respond to the comments above:

I think I'd like to understand why it's not working

The reason it is currently not working is that the FontMake arg --expand-features-to-instances is being used too late in the build process.

It is being used during the final font binary compilation, but it needs to be used during the UFO instantiation process, as this PR makes happen.

The arg is used by FontMake to get feature includes, and expand them into instance UFOs. That way, those instance UFOs can be built into final fonts. If the arg is used later, during the TTF generation process, the feature include statements will no longer have the correct relative path, so they will be unusable. And now, FontMake will now throw the error fontmake: error: "--expand-features-to-instances" option invalid for UFO source.

[arrowtype]

The issue persists, but in a slightly different way. I’ll close this PR as stale.

[arrowtype]

Actually, looking again, I think this PR is probably the right solution. I’ll respond inline to the conversation that stalled out, before.