-
Notifications
You must be signed in to change notification settings - Fork 51
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
UFO Rountrip #244
UFO Rountrip #244
Conversation
aae464a
to
1e09c3f
Compare
c303736
to
607f3be
Compare
Here is an update on this work: the simple parts do roundtrip on GlyphsUnitTestSans, i.e. the outlines, features, kerning, classes, glyphs, layers, font attributes, custom parameters, alignment zones, guidelines, anchors, hints, annotations. There are still some small problems here and there, that you can see in this diff between the original GlyphsUnitTestSans () and the same after conversion to UFO and back to Glyphs () The tricky parts that are not done yet are:
As for the testing, I rely on the builder being thoroughly tested to be sure that the UFO output is usable by UFO application and on the writer being thoroughly tested to ensure that the textual diff between .glyphs files is an accurate representation of what is in memory. If those two components are indeed well tested, then the roundtrip tests can just be textual diffs on files that cover all the features. I have a few questions:
|
Thanks for the update, Jany! I wouldn't worry too much about the smart components or bracket layers at this stage. Let's get the basic stuff working first.
sgtm
No idea. @schriftgestalt ?
that's because the way Glyphs.app orders things without an explicit glyphOrder custom parameter is different from the way ufo2ft would order if glyphOrder is not present. The latter simply places .notdef at the beginning and sorts the rest alphabetically. I believe Glyphs.app uses the order in which the glyphs are written out in the *.glyphs file itself, which in turn seems to follow the order of the "Categories" lists on the left panel. Should we ask Georg to share that list? I don't know. Maybe it's ok if after roundtripping, one always gets an extra glyphOrder custom parameter (provided that the UFO libs has glyphOrder value).
actually I don't know. Is it not alphabetic?
that's ok. explicit is better than implicit
Sure, thanks! |
it is a subclass of GSLayer. The most important difference is the handling of .background and .foreground.
Glyphs always writes the
Master layers don't have a name on there own. They get the name from the master. For other layers, it is possible but not advised.
I would not like to change that just like that. But I see your point.
Glyphs stores font.customParameters in |
what's the difference between the two?
if not alphabetical, what's the order then? The glyphOrder's order?
yes, that's why for filters, for example, we write it as a list of dicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here’s what I noted GlyphsApp does:
.glyphs -> .ufo:
- the glyph order in the font is written to
public.glyphOrder
. - if present, the customParameter
glyphOrder
is written tocom.schriftgestaltung.glyphOrder
.ufo -> .glyphs:
public.glyphOrder
defines the order of the glyphs in the font.com.schriftgestaltung.glyphOrder
is written to the customParameterglyphOrder
if it is not false and if there was nonepublic.glyphOrder
is written to the customParameterglyphOrder
instead.
It’s seems hard to roundtrip with the same logic since .ufo -> .glyphs -> .ufo will change public.glyphOrder
and add com.schriftgestaltung.glyphOrder
.
We probably want to rething the logic so we can roundtrip but preserve the extra information.
register(ParamHandler( | ||
glyphs_name, ufo_name, glyphs_long_name=ufo_name, | ||
value_to_ufo=lambda value: -abs(value), | ||
value_to_glyphs=abs, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In UFO openTypeOS2WinDescent must be non-negative, this should probably be
value_to_ufo=abs,
value_to_glyphs=abs,
@@ -16,151 +16,497 @@ | |||
unicode_literals) | |||
|
|||
import re |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re
is not used here anymore.
Lib/glyphsLib/builder/components.py
Outdated
for c in layer.components: | ||
value = getattr(c, key) | ||
# FIXME: (jany) bad idea to skip none because it shifts indices | ||
# TODO: (jany) switch to a dict structure: {componentId: {data}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Storing the locked state of components as a dict {componentId: {data}}
would be a lot better than a list of booleans indeed.
@schriftgestalt Would it make sense for GlyphsApp to use that for UFO3 since components can have identifiers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it necessary to distinguish between the public.glyphOrder and the con.schriftgestaltung.glyphOrder? Isn’t the former enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicating the glyphOrder is not only confusing but may lead to the two values getting out of sync with each other
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense for GlyphsApp to use that for UFO3 since components can have identifiers.
I’ll follow any working solution.
Why is it necessary to distinguish between the public.glyphOrder and the con.schriftgestaltung.glyphOrder? Isn’t the former enough?
They mean different things. The public.glyphOrder
stores the order of the glyphs as the happened to be at that moment. con.schriftgestaltung.glyphOrder
is just the custom parameter, It doesn't need to be in sync with the actual order and can contain names that are not in the font and doesn’t need to have all glyphs. So you should mostly be using public.glyphOrder
and just keep con.schriftgestaltung.glyphOrder
around for round tripping.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way you described the com.schriftgestaltung.glyphOrder sounds to me very much like the way the UFO spec defines the public.glyphOrder. The fact is the UFO glyph sets don’t have any “actual order” as contents.plist is just a mapping without any internal order (it’s alphabetically sorted in most implementations but doesn’t imply anything).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are differences.
Normally, Glyphs will sort the glyphs automatically. That can be deactivated and manually controlled by the parameters. As I said, the list can be much shorter that the number of glyphs in the font. The remaining glyphs will be sorted by the internal rules.
If Glyphs opens a .ufo it looks for the ‘com.schriftgestaltung.glyphOrder’ entry. If it finds one, it set the glyphOrder parameter (if it finds a list) or does nothing if it is set to None. If the entry isn’t found, it looks for the ‘keep glyph names from imported fonts’ setting from the user defaults and if it is set, it puts the ‘public.glyphOrder’ into the custom parameter. Otherwise does nothing again. In those later cases, the glyphs will be sorted automatically.
So I need both parameters to be able to round trip properly. But as I said, glyphsLib doesn’t need to look at the prefix list.
Lib/glyphsLib/builder/guidelines.py
Outdated
@@ -15,6 +15,10 @@ | |||
from __future__ import (print_function, division, absolute_import, | |||
unicode_literals) | |||
|
|||
from glyphsLib.types import point | |||
|
|||
LOCKED_NAME_SUFFIX = ' [locked]' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to store locked guidelines in the glyph.lib or for global guidelines in the font.lib like it is done for locked components, or better, as a dictionary as you propose?
/cc @schriftgestalt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t have an opinion about this.
bef2814
to
3f3a23c
Compare
Quick status updateAt that point, the only remaining diff on GlyphsUnitTestSans is the instance data, as shown in the following screenshot. My plan is now to:
Questions: how to store instance data in the designspace documentI'm currently trying to store this data into the designspace document and I feel that I'm not doing it right, because the current code I have for Glyphs -> UFO (that is based on the existing When I want to read back the instance data for UFO -> Glyphs, I'm using mutatorMath DesignSpaceDocumentReader and its All those disk operations for instance data are to be compared to the fully in-memory process that I had for everything else while doing Glyphs->UFO->Glyphs. In a way it's good because writing the intermediary UFOs to disk highlighted some problems, so for testing it's nice to write to the disk, but I think that in general the software should be able to operate in memory. Is that indeed considered a problem or is it just me? Would that other library for DesignSpaceDocument improve the situation? https://github.com/LettError/designSpaceDocument |
thanks Jany for the update, and for the great work you're doing!
Have you seen this? you shouldn't need to build instance UFOs and save them to disk just to read back from them the instance data. The instance data (including the custom parameters) should be stored inside the designspace document, and anything that doesn't already fit should be placed in a residual new
yes, I agree.
probably. I ping'ed Erik about it LettError/designSpaceDocument#28
would you like to do that? I mean, work on copying (along with its git history) the |
I agree and that's what I wanted to do, but the mutatorMath APIs don't seem to allow it. OK then, first, I will use designSpaceDocument as a dependency here to keep moving, and I will open a PR in fontTools to move the file and its git history. |
a43f1d8
to
5defdea
Compare
c7823a2
to
ca35f8b
Compare
8e4781a
to
fd147ef
Compare
42e7852
to
b4a0e99
Compare
200d54e
to
bc5f5b9
Compare
e0c8f78
to
db88a07
Compare
WIP UFO round-trip WIP Glyphs and layers WIP kerning, hints, annotations WIP kerning, features WIP path, components WIP paths, features, kerning, glyph order WIP features, alignment zones, guidelines WIP Fix layer ordering WIP restore backround layers, anchor positions, path node order WIP custom parameters WIP Glyph, Layer, Node user data WIP layers WIP background image WIP fix some tests Fixup custom_params Fixup builder.glyph Fixup builder.guidelines WIP GSBackgroundLayer, glyphOrder WIP small fixes, refactor types.py WIP small fixes WIP smart component data WIP instances WIP roundtrip instances using designspace HELP IT'S AWFUL WIP use designSpaceDocument for round-trip WIP implement InMemorySourceDescriptor and test for writing on disk WIP instances
c1f9722
to
adf9155
Compare
This branch is getting ready for merge: I won't add other features, but I will cleanup some TODOs/FIXMEs that are not relevant anymore, update some docstrings and try to spot regressions. Also I'm waiting on fonttools/fonttools#1175 to drop my copy-pasted version of DesignSpaceDocument :) About regressions, I have a question: I have tested manually the generation of instance TTFs using fontmake on one of the Noto fonts, to check that the TTFs come out the same as with the current master. To be more confident, I would like to build all of Noto with master, get all the files as TTXs, do the same with this branch, and compare the TTXs (they should be the same except for the timestamp). So, question: is there already some automated regression testing for the Noto fonts? Apart from that, here are a few examples of how to use the new UFO->Glyphs feature. I have added those to the readme:
import glob
from defcon import Font
from glyphsLib import to_glyphs
ufos = [Font(path) for path in glob.glob('*Italic.ufo')]
# Sort the UFOs because glyphsLib will create masters in the same order
ufos = sorted(ufos, key=lambda ufo: ufo.info.openTypeOS2WeightClass)
font = to_glyphs(ufos)
font.save('InriaSansItalic.glyphs') Here is the resulting glyphs file
import glob
from fontTools.designspaceLib import DesignSpaceDocument
from glyphsLib import to_glyphs
doc = DesignSpaceDocument()
doc.read('spectral-build-roman.designspace')
font = to_glyphs(doc)
font.save('SpectralRoman.glyphs') Here is the resulting glyphs file
The same option exists for people who want to do Glyphs->UFOs->Glyphs: import glob
import os
from fontTools.designspaceLib import DesignSpaceDocument
from glyphsLib import to_glyphs, to_designspace, GSFont
doc = DesignSpaceDocument()
doc.read('spectral-build-roman.designspace')
font = to_glyphs(doc, minimize_ufo_diffs=True)
doc2 = to_designspace(font, propagate_anchors=False)
# UFOs are in memory only, attached to the doc via `sources`
# Writing doc2 over the original doc should generate very few git diffs (ideally none)
doc2.write(doc.path)
for source in doc2.sources:
path = os.path.join(os.path.dirname(doc.path), source.filename)
# You will want to use ufoNormalizer after
source.font.save(path)
font = GSFont('SpectralRoman.glyphs')
doc = to_designspace(font, minimize_glyphs_diffs=True, propagate_anchors=False)
font2 = to_glyphs(doc)
# Writing font2 over font should generate very few git diffs (ideally none):
font2.save(font.filepath) In practice there are always a few diffs on things that don't really make a difference, like optional things being added/removed or whitespace changes or things getting reordered... |
* Remove customName{1,2,3} which don't exist * Handle customName when going UFO->Glyphs only using the setter
Hello! This PR should be merged soon because it's become very big and I have started incorporating fixes that are out of the scope of "going from UFOs to glyphs". We have started using this branch internally at Dalton Maag to work on UFOs with Glyphs and it works good enough. I'm pretty sure that I have introduced some regressions in the Glyphs -> UFOs -> TTFs pipeline, but I would be happy to fix them once this is merged and people start using this code. @moyogo @anthrotype Are you OK with this? |
Hey Jany, thanks. Yeah I was discussing this with Denis at Robothon, and I also think we should merge before it diverges too much.
Why? what makes you think so? |
Just "statistically": there are so many changes that I have probably broken something. For example, I suspect that there might be problems with how custom parameters are stored in UFO lib and used again later in the toolchain by ufo2ft or fontmake. That's why I was asking about Noto non-regression testing a few days ago, to hunt for such regressions. |
not yet, but we'll be working on that. OK, let's merge to master and hold the release a few more days, while I try to rebuild all the Noto fonts compare them with notodiff/fontdiff. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @belluzj for working on this! 👍
I think I'm gonna need some help to draft the changelog for this ;)
I just bumped the minor version (2.3.0.dev0), as I presume there aren't no backward compatibility changes, only new features. To all glyphsLib users, please try this out and let us know if you spot any regressions! Thanks 🙌 I could tag a pre-release (beta, or release candidate) and publish wheels to make it easier for other users to test this out with Or maybe it's fine to just install from git
|
Lib/glyphsLib/builder/instances.py
Outdated
@@ -326,15 +327,17 @@ def set_width_class(ufo, designspace, instance): | |||
_set_class_from_instance(ufo, designspace, instance, WIDTH_AXIS_DEF) | |||
|
|||
|
|||
def apply_instance_data(designspace): | |||
# DEPRECATED: needs better API |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@belluzj what do you propose to use as an alternative?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there is a working replacement now so maybe I should not mark this as deprecated but only as "todo"? The idea behind the deprecated is:
I think the apply_instance_data
should work as independently from glyphsLib as possible. I marked the current function as "deprecated" because it takes as a parameter something that is produced by glyphsLib only, whereas I think it should take only a designspace and find everything it needs in it.
Actually, in the code here, the parameter "instance_data" is already only stuff from the designspace, but weirdly wrapped in something that looks like a dictionary to conform to expectations of existing users of the API. I think there should be a new API that more clearly exposes the intent, which is:
- inputs:
- a designspace file that specifies instances
- interpolated UFOs for those instances, at the locations pointed by each
instance.path
of the designspace
- outputs:
- each existing UFO is updated according to special Glyphs.app customParameters/filters/whatever, whose values/configuration were found in the
instance.lib
of the designspace (and only there, no need to read the glyphs file again).
- each existing UFO is updated according to special Glyphs.app customParameters/filters/whatever, whose values/configuration were found in the
Also from a broader perspective I guess this feature of applying post-processing to interpolated instance UFOs will be needed as well in workflows that are not based on Glyphs, so I guess this function could be part of something else than glyphsLib, and implement postprocessing in a similar way as ufo2ft filters (the code of Glyphs.app-specific filters could be provided by glyphsLib though). It could also be done by MutatorMath or the "ufoProcessor" from the designspaceDocument repository, I'm not sure which is suitable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation. Yeah, I don't like the interface of that function for the same reasons, but any changes will need to be coordinated with fontmake.
It'd be nice to make this feature generic and non-glyphsLib specific. Probably mutatorMath (or ufoProcessor if/when it's fully feature-compatible with the latter) is a better place to define this API than is ufo2ft. The latter doesn't know about designspace yet, it's only concerned with converting UFOs to OTFs.
This will have the code the for the UFO roundtrip as per #235