googlefonts / gftools

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

`NinjaBuilder` doesn't create instance TTFs, only makes (unwanted) instance UFOs #849

Open RickyDaMa opened 7 months ago

RickyDaMa commented 7 months ago

Example config:

buildOTF: false
buildStatic: true
buildWebfont: false
familyName: Comic Sans
instances:
  Comic Sans[GRAD,ROND,opsz,slnt,wdth,wght].ttf:
  - coordinates:
      ROND: 0
      opsz: 144
      wdth: 100
      wght: 100.0
    familyName: Comic Sans
    styleName: Thin
removeOutlineOverlaps: false
reverseOutlineDirection: false
sources:
  - Comic Sans.designspace
stat:
  # -- snip --

With GFBuilder, this works as I expect:

  1. The VF is compiled
  2. The static instances defined in the designspace and in the config.yaml are built

With the NinjaBuilder, the following happens:

  1. The VF is compiled
  2. Instance UFOs are generated and put into the sources/instance_ufos directory (unwanted)

No errors in either case, but very different behaviour


I had a look why the NinjaBuilder wasn't producing static TTFs in the debugger and think I found why:

https://github.com/googlefonts/gftools/blob/6997cbac9fc56043cd0fe109c72398091a598506/Lib/gftools/builder/_ninja.py#L303-L321

Here we can see that NinjaBuilder.build_static unconditionally generates instance UFOs, and then delegates to GFBuilder.build_static, but overriding self as the NinjaBuilder

https://github.com/googlefonts/gftools/blob/6997cbac9fc56043cd0fe109c72398091a598506/Lib/gftools/builder/__init__.py#L462-L469

GFBuilder then delegates to self.instantiate_static_fonts, but self in this context is NinjaBuilder, which...

https://github.com/googlefonts/gftools/blob/6997cbac9fc56043cd0fe109c72398091a598506/Lib/gftools/builder/_ninja.py#L323-L324

Ah. Perhaps this was intended to be GFBuilder.instantiate_static_fonts, instead of NinjaBuilder's stub?

This would be easy to workaround if there was a way to manually override the builder being used, but as far as I know there isn't, meaning I either have to change operating system or the source code to avoid the NinjaBuilder for now

m4rc1e commented 7 months ago

@RickyDaMa Could you try out #726 please? we're hoping to merge it this week.

RickyDaMa commented 7 months ago

Okay, looking at the builder2 branch:

'instances' no longer supported; generate a config with --generate and select the instances you want

So now I can't specify my own instances?

The workflow of creating a config, then doing gftools builder --generate sources/myconfig.yaml > sources/myconfig2.yaml is a bit weird. I'm guessing this has given me the designspace instances, which unfortunately as #850 alludes to, I don't want, and nor do I see a way to specify custom instances if I want to

Is there still a way with builder 2 to specify custom instances?

m4rc1e commented 7 months ago

Actually good point. Imo we should support all the features we had in the original builder.

simoncozens commented 7 months ago

We should support the options from the original builder but I'm unconvinced about the use case of declaring certain instances in the font sources and asking the builder to generate completely different ones. Why is this a good idea?

Hoolean commented 7 months ago

In this case, we have a wide designspace for which we want to build more static instances than we have named instances for in the variable font.

(in particular, we are happy to have a less crowded set of instances in the variable font as the user can reach other areas with sliders manually, but for the statics this is not an option, and so we need to build more for a wider reach)

As instances are shared for VFs and statics in a designspace, we can only store one set in a single source. There are workarounds we could deploy here (e.g. dynamically generating a second designspace) but we were interested to see if we could do it with only the gftools builder first :)

simoncozens commented 7 months ago

OK, I'm on it...

RickyDaMa commented 7 months ago

Thanks Simon, sorry for the hassle! There's no urgency here as we're going to re-use a handrolled approach we know works while we wait for builder2 to stabilise (before hopefully migrating to it)