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

Instantiator support for non-default layer sources #980

Merged
merged 4 commits into from
Feb 21, 2023

Conversation

verbosus
Copy link
Contributor

This PR adds the ability to build from non-default layers. If the default designspace source is set to a non-default layer, then its glyphset is used as reference for all the other sources—glyphs that are not on the layer are discarded. I believe this should also address issue #603

@verbosus
Copy link
Contributor Author

The build failure seems to be related to some codecov “owner” argument I don’t know much about.

Copy link
Member

@anthrotype anthrotype left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hi Antonio! thanks, I left some comments

Lib/fontmake/__main__.py Show resolved Hide resolved
Lib/fontmake/errors.py Outdated Show resolved Hide resolved
Lib/fontmake/instantiator.py Outdated Show resolved Hide resolved
Lib/fontmake/instantiator.py Outdated Show resolved Hide resolved
Lib/fontmake/instantiator.py Outdated Show resolved Hide resolved
Lib/fontmake/instantiator.py Outdated Show resolved Hide resolved
Lib/fontmake/instantiator.py Outdated Show resolved Hide resolved
Lib/fontmake/instantiator.py Outdated Show resolved Hide resolved
Lib/fontmake/instantiator.py Outdated Show resolved Hide resolved
@anthrotype
Copy link
Member

don't worry about codecov, it's flaky at best.. i'm going to make the PR pass even when it fails to upload (which is 50% of the time lately)

@verbosus
Copy link
Contributor Author

Thanks Cosimo: I believe I fixed all your comments, except the one about reporting the non-existing glyphs in non-default masters. I’d rather have it behave like it does right now: it’s a corner case anyway so the main path isn’t affected, and for the way I’m using it I much prefer it to silently ignore the missing glyphs rather than reporting them. Codecov still isn’t happy, but I don’t think there is anything I can do to help there.

@anthrotype
Copy link
Member

this is looking good now, thanks!
Maybe we should change the PR title to better reflect the changes that you made. Only the fontmake.instantiator (which generates static instance UFOs from a DS) has been modified, whereas the PR title suggests building in general (not just static interpolated instances) from non-default layer source isn't supported..

by the way, did it work before this PR to build a VF from the same DS with such non-default layer set as default source?

@anthrotype
Copy link
Member

I'm also curious, why not simply mark the desired non-default layer that you wish to use as the default DS source as the default layer in the font editor or via script?

@verbosus verbosus changed the title Build from non-default layer source Instantiator support for non-default layer sources Feb 16, 2023
@verbosus
Copy link
Contributor Author

In some cases it’s handy to have metrically-compatible sources in the same UFO (think: a full outline and a stencil variant, etc.) but on different layers. And yeah, I tried and it did NOT use to work before the PR: fontmake was crashing saying it couldn’t find a base master.

@anthrotype
Copy link
Member

I tried and it did NOT use to work before the PR: fontmake was crashing saying it couldn’t find a base master.

was that trying to build interpolated static instance font (via instantiator) or building a VF (which does not go through instantiator)?

I'm trying to understand if this PR fixes other things, or if other parts of the pipeline need similar fixes to adjust expectation that a base source may not necessarily come from a default layer

@verbosus
Copy link
Contributor Author

Sorry, I misunderstood your previous question. I only tried building static instances so far. I just checked and building a VF fails either with or without this PR. The failure is the same: fontmake: Error: In 'sources/testfamily.designspace': Generating fonts from Designspace failed: "'name' table not found". Unless it raises any flags for obvious known issues, I guess it needs some deeper investigation.

@anthrotype
Copy link
Member

@verbosus can you try running fontmake with --verbose DEBUG option so it displays the full exception traceback, so you can see where exactly that error is raised?

@anthrotype anthrotype merged commit eef7c40 into googlefonts:main Feb 21, 2023
@anthrotype
Copy link
Member

@verbosus please file an issue about that error when you find out more details, and thanks again for the PR

@verbosus
Copy link
Contributor Author

Thanks for merging and sure, I just filed #983 to cover the rest of the work on the variable path.

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.

2 participants