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

Adding vertical origin to <advance> element #95

Open
justvanrossum opened this issue May 25, 2019 · 24 comments
Open

Adding vertical origin to <advance> element #95

justvanrossum opened this issue May 25, 2019 · 24 comments
Labels
considering Specification change under consideration. proposal Proposed specification change. ufo4 UFO 4 issues.

Comments

@justvanrossum
Copy link
Contributor

I think we need to upgrade the public.verticalOrigin lib key to a proper item in the next version of UFO.

Perhaps like this:

<advance height="1000" width="450" verticalOriginY="850"/>

or just

<advance height="1000" width="450" verticalOrigin="850"/>

@anthrotype
Copy link
Member

I support this. How can we make that happen?

@anthrotype
Copy link
Member

question was partly rethorical. Of course we'd need a PR to update the spec.
I think this would probably deserve a GLIF format minor version bump, since we are adding a new attribute to one of the public elements?
Probably also the UFO format spec would need a minor version bump?

I wonder how that would effect the current UFO readers, especially fontTools.ufoLib. I imagine it would blow up with some NotImplementedError or similar.

@anthrotype
Copy link
Member

oh.. formatVersion is an int #78

@benkiel
Copy link
Contributor

benkiel commented Nov 1, 2019

It seems like I'm missing something here, but wouldn't ufoLib get updated to understand the presence of this? PR the wording and we can get it done!

@anthrotype
Copy link
Member

anthrotype commented Nov 1, 2019

@benkiel yeah, I know. I am only concerned how we can introduce changes such as this without disrupting existing implementations of the spec. Even changing the type of formatVersion from int to float, to distinguish a major.minor version, is a breaking change.

@anthrotype
Copy link
Member

as well as upgrading vericalOrigin from lib key to a proper glif <advance> attribute, I would also like to propose that we change the default value of the "height" attribute from the current value of 0 to:

the distance between the values of the of OS/2.TypoAscender and the OS/2.TypoDescender.

That is what the Adobe Feature File specification expects
https://github.com/adobe-type-tools/afdko/blob/develop/docs/OpenTypeFeatureFileSpecification.md#9h-vmtx-table

And it is also what Glyphs.app uses as the default glyph advance height.
I would like to add support in glyphsLib for exporting vertical layout fonts from Glyphs <=> UFOs and I am having troubles translating between the two.
Given that Glyphs.app and UFO spec expect different default values for the advance height, to be able to encode this field without losing data I would have to write the height attribute all the time when going from Glyphs => UFO, even when (which is 90 % of the case?) a font is not meant to be set vertically.

If we align the expected default height value between FDK and Glyphs.app on the one hand, and UFO on the other hand, it makes it easier to translate between them. One would only need to set a the glyph.height attribute when it is different from the default value (ie. OS2.TypoAscender - OS2TypoDescender).

@LettError
Copy link
Contributor

What happens when OS/2.TypoAscender or OS/2.TypoDescender aren't set?

@justvanrossum
Copy link
Contributor Author

I would also like to propose that we change the default value of the "height" attribute from the current value of 0 to:

the distance between the values of the of OS/2.TypoAscender and the OS/2.TypoDescender.

+1. Although I would like to see what consequences this has for, say, defcon and fontParts.

@anthrotype
Copy link
Member

for ref, here's the linked glyphsLib issue:
googlefonts/glyphsLib#557 (comment)

When a glyph doesn't have height explicitly set, the Defcon's glyph.height property is still returning a default value of 0 (following the UFO spec). But this creates a problem when deciding what to do with the value. ufo2ft is currently simply taking that at face value. But if the default glyph height is always 0, how can one distinguish between a true 0 height from one which was not explicitly set and thus should default to typoAscender - typoDescender?

@anthrotype
Copy link
Member

What happens when OS/2.TypoAscender or OS/2.TypoDescender aren't set?

We fall back to info.ascender and info.descender, and if those aren't set either, then it is undefined.

@justvanrossum
Copy link
Contributor Author

and if those aren't set either, then it is undefined

Would glyph.height then return None or 0 as it currently does?

@anthrotype
Copy link
Member

I guess None, and then client needs to check font.info for the other values. I would not like to depend on implementation details like Glyph having a pointer to its parent Font object (in ufoLib2 we don't have such)

@justvanrossum
Copy link
Contributor Author

I would not like to depend on implementation details like Glyph having a pointer to its parent Font object (in ufoLib2 we don't have such)

So how would ufoLib2 then get at the right value? Will all glyphs be initialized with a value for height?

@anthrotype
Copy link
Member

ufoLib2 Glyph.height property would simply return on whatever is stored in the GLIF. If the height attribute is set in the advance element, then return that. Otherwise return None.
The meaning of None is "check font.info ascender - descender or give up".

@justvanrossum
Copy link
Contributor Author

I think that's good, but is not the same as saying to change the default value to some calculated value :)

@anthrotype
Copy link
Member

and for UFOReader and UFOWriter classes, they would similarly just read or write whatever is not None in the current Glyph object.

@anthrotype
Copy link
Member

is not the same as saying to change the default value to some calculated value :)

right.

@justvanrossum
Copy link
Contributor Author

So, the spec should say something like: if this value is not in the glif data, return None, which signals client code to do the appropriate ascender - descender calculation.

@anthrotype
Copy link
Member

btw, i'm working now on the formatVersionMinor (#78) PR in fonttools which should unblock this.

@anthrotype
Copy link
Member

anthrotype commented Jul 31, 2020

About changing the default value for glyph.height, I think I don't need that any more. The only reason was to make it match Glyphs.app default value to avoid having to write out explicit glyph.height for all UFO glyphs, even when a font was not explicitly designed for vertical layout. But I can use some other heuristic to guess if a given .glyphs source is intended for vertical typesetting, and if so I will export all verticalOrigin and height attributes, and ufo2ft will do its job.
googlefonts/glyphsLib#557 (comment)

The addition of verticalOrigin as first-class citizen in GLIF would still be nice, even though not a blocker.
Then we need to update fontMath to interpolate that robotools/fontMath#161

@typesupply typesupply added considering Specification change under consideration. proposal Proposed specification change. ufo4 UFO 4 issues. labels Aug 12, 2020
@schriftgestalt
Copy link

I just run into the same issue again. Having it default to zero is just plain wrong. That should be fixed ASAP. It should default to None if not defined in the file. That way one that needs to write a vmtx table can distinguish it from a actually zero value. And as it is not possible to write a proper vmtx table with the current spec, fixing it can’t be any worse.

@anthrotype
Copy link
Member

The default value is different but it is definitely possible to build vmtx table and ufo2ft supports it. I agree with you the default should change.

@schriftgestalt
Copy link

You can build a vmtx table but it will not be correct. At least I don’t know how without manually setting the height in each glyph.

@anthrotype
Copy link
Member

Yes that is what I meant, you need to explicitly set the height in all glyphs whose height is != 0, because that's the current default value in the UFO spec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
considering Specification change under consideration. proposal Proposed specification change. ufo4 UFO 4 issues.
Projects
None yet
Development

No branches or pull requests

6 participants