googlefonts / glyphsLib

A bridge from Glyphs source files (.glyphs) to UFOs
Apache License 2.0
178 stars 51 forks source link

Build intermediate layers with non-intermediate components #971

Closed simoncozens closed 5 months ago

simoncozens commented 5 months ago

This is kind of the inverse of #954. If an ~alternate~ intermediate layer references a component, and that component does not have a matching intermediate layer, glyphsLib currently generates a "too sparse" master. In the test file in this PR, Aslash has an intermediate layer but it uses a component glyph A which does not have that intermediate layer. Sometimes the resulting UFO file for the sparse master can't be compiled, because the referenced component A is missing; sometimes it can be compiled but there is no variation:

INFO:ufo2ft.filters:Running DecomposeComponentsFilter on ComponentsWithIntermediates-Regular-{151}
WARNING:ufo2ft.util:dropping non-existent component 'A' in glyph 'Astroke'
...
WARNING:fontTools.varLib:glyph Astroke has incompatible masters; skipping

There are two ways this could be addressed - we could interpolate and decompose the component, or we could make the sparse master a bit less sparse by interpolating the referenced layer. I've taken the second approach because (a) it's simpler, and (b) keeping components where possible can lead to smaller file sizes.

This also introduces a new way of preparing Glyphs files for Glyphs->UFO translation; we now have a preflight_glyphs function which runs a list of translations, progressively "dumbifying" all the smart stuff in the GSFont object. Currently this is only used for the current job (de-sparsifying overly sparse masters), but in the future, we could use this to simplify some of the more gnarly translations we do in compilation, especially those requiring Glyphs-specific ufo2ft plugins. (i.e. EraseOpenCorners, CornerComponents, etc.)

anthrotype commented 5 months ago

If an alternate layer references a component, and that component does not have a matching intermediate layer

just a terminology clarification: IIRC, an "alternate layer" in Glyphs.app has a specific meaning akin to so-called 'bracket' layers (that are swapped within given axis ranges) which is distinct from "intermediate layer" proper (aka 'brace' layers), best not to use the two interchangeably

simoncozens commented 5 months ago

~This doesn't correctly set the advance width for the newly-created intermediate layer, which means that the underlying glyph looks wrong.~

anthrotype commented 5 months ago

@simoncozens is this ready to merge? or you plan to do more work to tackle https://github.com/googlefonts/glyphsLib/issues/954 in here?

anthrotype commented 5 months ago

IIUC this and the other https://github.com/googlefonts/glyphsLib/issues/954 are basically two faces of the same problem, we want to make sure that composite glyphs and their components all have the same set of intermediate layers; in here, you fixed the issue when the composite has intermediates that the component has not; in #954, the component has more intermediate layers than the composite glyph that uses it, so we need to add the missing ones there. It's probably even possible (though unlikely) that the composite and the component have different intermediate layers for whatever reason, which means you need to add the respectively missing ones to both.

anthrotype commented 5 months ago

let's merge this as is for now so I can make a new release

yanone commented 5 months ago

I just tried this, and couldn't get it to work. But I'm not 100% sure this PR is solving the same problem.

I have a font with 3-dimensional design space: weight, kashida, and swash. Layer setup per glyph as shown below. The composite glyph doesn't have any intermediate layers, which I thought this PR addresses.

The width of the affected glyphs doesn't change along the kashida and swash axes, only the paths, hinting at missing intermediate layers in the composite glyph with adjusted width.

Bildschirmfoto 2024-01-30 um 14 04 10 Bildschirmfoto 2024-01-30 um 14 03 57 Bildschirmfoto 2024-01-30 um 14 02 46 Bildschirmfoto 2024-01-30 um 14 03 01

anthrotype commented 5 months ago

The composite glyph doesn't have any intermediate layers, which I thought this PR addresses.

no, this PR only addresses the issue when a composite glyph defines more intermediate layers than the component glyph that it references. You are hitting https://github.com/googlefonts/glyphsLib/issues/954 -- glyph used as component has more intermediate layers than the composite glyph that uses it

yanone commented 5 months ago

I had a feeling I got it mixed up. Thank you, I'll try out the other PR.

anthrotype commented 5 months ago

I'll try out the other PR.

there is not another PR yet..

simoncozens commented 4 months ago

I think I screwed up something here:

DesignSpace sources contain duplicate locations; varLib expects each master to define a unique location. Make sure that you used consistent 'brace layer' names in all the glyph layers that share the same location.
  ['{151, 100}', '{151.0, 100.0}'] => {'Weight': 151.0, 'Width': 100.0}
anthrotype commented 4 months ago

I presume this started happening since v6.6.2 (which includes this PR)? Also is this a Glyphs 3 souce? if so the error message is misleading because it talks about "consistent 'brace layer' names" which only makes sense for Glyphs 2.

simoncozens commented 4 months ago

Yes, it's glyphs 3 source - I think we just didn't update an error message somewhere. And yes, this is directly related to this issue. Specifically it's happening because we set

location = tuple(parent_layer._brace_coordinates())
...
layer.attributes["coordinates"] = location

because I was trying to be clever and abstract out getting the location of an intermediate layer in a way that would kind of serve as documentation.

But _layer_brace_coordinates returns (float(v) for v in self.attributes["coordinates"]); we just want location = parent_layer.attributes["coordinates"].

anthrotype commented 4 months ago

@simoncozens even after we merged https://github.com/googlefonts/glyphsLib/pull/976, I'm still a bit puzzled by that layer._brace_coordinates() returning an (optional) generator expression but only when format_version > 2 (Glyphs 3), otherwise a list (via list comprehension) for Glyphs 2.

https://github.com/googlefonts/glyphsLib/blob/ce69aad9e804b3dfa38023055a7da19af7fa4c09/Lib/glyphsLib/classes.py#L3972-L3982

How can this layer._brace_coordinates() == location condition ever be true if the first operand is of type generator expression and the second is of type tuple?

https://github.com/googlefonts/glyphsLib/blob/ce69aad9e804b3dfa38023055a7da19af7fa4c09/Lib/glyphsLib/builder/transformations/intermediate_layers.py#L55-L56

anthrotype commented 4 months ago

How can this layer._brace_coordinates() == location condition ever be true if the first operand is of type generator expression and the second is of type tuple?

I made it return a list so that comparison should work now (https://github.com/googlefonts/glyphsLib/commit/a944fd3afac8b6e4de6ef5699c89c104874b981f)