googlefonts / ufo2ft

A bridge from UFOs to FontTools objects (and therefore, OTFs and TTFs).
MIT License
151 stars 43 forks source link

post table underlinePosition #760

Closed benkiel closed 1 year ago

benkiel commented 1 year ago

The postscriptUnderlinePosition wasn't well specified in the UFO: it conflated the value for CFF/Type1 and post/CFF2, where those values are actually different: post/CFF2 value is the top of the underline, CFF/Type1 is the middle of the underline.

The UFO spec got clarified to be clear that postscriptUnerlinePosition is `CFF/Type1' (https://github.com/unified-font-object/ufo-spec/issues/217).

The behavior for calculation from above is what the AFDKO has been doing, just need to update ufo2ft to follow suit.

I will PR a change (and tests) soon.

skef commented 1 year ago

Whoops! Sorry, I was reading the code wrong. It looks like the calculation was cff_underline_position - cff_underline_thickness / 2 . I wrote "+" earlier in the other issue. I hope it's not too hard to go back and update the PR.

skef commented 1 year ago

Oh, dammit, wait never mind, I was right the first time! I just confused myself by looking at the inverse calculation. What you have is fine!

khaledhosny commented 1 year ago

This does not make sense IMO, the default interpretation would be matching OpenType spec not legacy Type1/CFF. Changing this now is going to break all fonts that were never produced would AFDKO.

If the UFO spec wants to fix its omission properly, it should have provisions for backward compatibility, not randomly selecting one implmentation as the correct one and breaking everything else.

ryanbugden commented 1 year ago

Just breaking this down into simple terms for myself (and highlighting the two pain-points), but let me know if/where I'm wrong:

Generating with AFDKO (ufo2fdk):

  1. Looks at UFO underline position. Interprets as cff, middle of the stroke (🟠).
  2. Writes that value into cff table.
  3. Calculates the post value (top of stroke) as cff_position + thickness / 2.
  4. Writes that value into the post table.

Generating with fontmake (ufo2ft):

  1. Looks at UFO underline position. Interprets as post, top of the stroke (🔴).
  2. Writes that value into cff table (meant to mean center-of-stroke).
  3. Also writes that value into the post table.

Conclusion: Either: 🟠 UFO updates to consider the postscriptUnderlinePosition to be the top of the stroke (post). But as @benkiel is saying, the UFO spec clarified that this value should correspond to cff. or 🔴 ufo2ft updates to not interpret the cff-specific value as top of the stroke (post logic).

If I'm right about items 2 and 3 in the second list, then ufo2ft is at least doing something wrong, because it's putting the same value in both tables.

With regards to:

Changing this now is going to break all fonts that were never produced would AFDKO.

I'd venture a guess that most type designers up until this point have thrown up a big ¯\_(ツ)_/¯ with how these values render across platforms. I think the syncing-up of logic across compilers would be a step in the right direction.

khaledhosny commented 1 year ago

I don't care about CFF table, and I doubt there are many (any?) modern applications read underline position and other duplicated data from CFF table.

It is post table that matters for me, and I carefully set up my underline position and thickness and tested it in various environments and I’m happy with the result. But now if this change made it to ufo2ft, updating my dependencies will break my carefully crafted fonts.

The spec being unilaterally updated to say something contradicting what it previously said and without even changing the version number, makes one seriously reconsider the value of said spec, specially when said change affects existing and widely used implementations.

ryanbugden commented 1 year ago

contradicting what it previously said

I'm not sure the UFO spec ever claimed that the underlinePosition font info key corresponded to the post table. See here.

khaledhosny commented 1 year ago

Before this change it said:

Underline position value. Corresponds to the Type 1/CFF/post table UnderlinePosition field.

It incorrectly implied the three font formats treat the value the same, but that its own fault not implementors who trusted it.

benkiel commented 1 year ago

@khaledhosny I'm sorry that this is making you reconsider the value of the spec — this isn’t my intent.

You are right, UFO spec incorrectly implied that the value for underline in post, CFF, and Type1 is treated the same; I was seeking to clarify this as it has lead to confusion (see the ADFDK and ufo2ft treat the value differently).

Because postscriptUnderlineThickness is in the section of the UFO spec that specifies Postscript specific data, I choose to favor that interpretation of the value over the post interpretation. This seemed to be the safest thing, as making it the post definition in the Postscript specific data section with the name postscriptUnderlineThickness the post definition seemed inconsistent and further confusing.

There was discussion about also adding a public key for the post table value — your rightful objection makes it clear that we should do that. Then you can copy the postscriptUnderlineThickness to that key in your UFOs and loose nothing.

I know that there isn't a perfect solution here, but does that seem an agreeable solution?

UFO4 should deprecate postscriptUnderlineThickness and just have underlineThickness, compliers can then do the math.

khaledhosny commented 1 year ago

Adding a new font info key is not going to fix the backward incompatibility issue as it requires modifying any font that sets postscriptUnderlinePosition and uses ufo2ft/fontmake.

The way to fix this IMO is either:

  1. Accept the reality of currently incompatible implementations and fix the issue in UFO 4, or
  2. Make a UFO 3.1 or something with the new definition (if UFO has minor versions and incompatible changes are acceptable in minor version), or
  3. Add a lib key that specifies the new definition (public.underLinePositionFollowsCFFDefinition or some such), and in the absence of such key the handling of postscriptUnderlinePosition is implementation-dependent.
anthrotype commented 1 year ago

I agree with Khaled, changing the handling of postscriptUnderlinePosition in ufo2ft would count a breaking change. It is unfortunate that the fontmake and makeotf implementations differ on this (I honestly wasn't aware of it until you filed this issue, my bad), but changing either one to match the other would respectively count as a breaking change. I think Khaled's option 3 is the safest one:

  1. Add a lib key that specifies the new definition (public.underLinePositionFollowsCFFDefinition or some such), and in the absence of such key the handling of postscriptUnderlinePosition is implementation-dependent.
benkiel commented 1 year ago

@anthrotype, to be fair, ufo2ft is doing the same thing that ufo2fdk did here, which didn't match makeotf (iirc, for a while makeotf ignored this value)

anthrotype commented 1 year ago

yeah, because it started as a fork of ufo2fdk, and nobody noticed/reported it that until now.

benkiel commented 1 year ago

@khaledhosny We have returned the UFO spec to how it was

khaledhosny commented 1 year ago

@benkiel Looks good now, thanks.