Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build intermediate layers with non-intermediate components #971

Merged
merged 12 commits into from
Jan 30, 2024

Conversation

simoncozens
Copy link
Collaborator

@simoncozens simoncozens commented Jan 10, 2024

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
Copy link
Member

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
Copy link
Collaborator Author

simoncozens commented Jan 11, 2024

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

@simoncozens simoncozens force-pushed the interpolate-components-with-intermediate branch from 148a990 to 60eb268 Compare January 25, 2024 12:46
@simoncozens simoncozens force-pushed the interpolate-components-with-intermediate branch from 60eb268 to 8b758e6 Compare January 25, 2024 12:57
@anthrotype
Copy link
Member

@simoncozens is this ready to merge? or you plan to do more work to tackle #954 in here?

@anthrotype
Copy link
Member

IIUC this and the other #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
Copy link
Member

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

@anthrotype anthrotype merged commit d345113 into main Jan 30, 2024
10 checks passed
@anthrotype anthrotype deleted the interpolate-components-with-intermediate branch January 30, 2024 11:23
@yanone
Copy link
Contributor

yanone commented Jan 30, 2024

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
Copy link
Member

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 #954 -- glyph used as component has more intermediate layers than the composite glyph that uses it

@yanone
Copy link
Contributor

yanone commented Jan 30, 2024

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

@anthrotype
Copy link
Member

I'll try out the other PR.

there is not another PR yet..

@simoncozens
Copy link
Collaborator Author

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
Copy link
Member

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
Copy link
Collaborator Author

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
Copy link
Member

@simoncozens even after we merged #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.

def _brace_coordinates(self):
if not self._is_brace_layer():
return None
if self.parent.parent.format_version > 2:
return (float(v) for v in self.attributes["coordinates"]) # Glyphs 3
# Glyphs 2
name = self.name
coordinates = name[name.index("{") + 1 : name.index("}")]
return [float(c) for c in coordinates.split(",")]

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?

if "coordinates" in layer.attributes and layer._brace_coordinates() == location:
return

anthrotype added a commit that referenced this pull request Feb 2, 2024
it was returning a generator expression for format_version>2 or a list for format_version=2 which doesn't make any sense

#971 (comment)
@anthrotype
Copy link
Member

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 (a944fd3)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants