skosch / Crimson

The Crimson Text typeface
SIL Open Font License 1.1
520 stars 55 forks source link

Generating OTF produces wrong metrics #20

Closed katef closed 8 years ago

katef commented 8 years ago

With the current revision, fontforge generates OTF which has the wrong metrics (or something); glyphs appear as if their height is about half is what it ought to be.

I'm using fontforge from git, but the same happens for other people using different versions of fontforge; I asked people to confirm for me.

I bisected the history, and this seems to have been introduced by 2d24623, and is still present in 4ab614e. Earlier commits do not have the problem.

skosch commented 8 years ago

@katef Good catch. I'm not sure what causes this. For context, once upon a time people had trouble with vertical metrics, and @rainerschuhsler, @adrientetar et al. worked their magic to make those problems disappear (e.g. in bb90099). Maybe they have an idea ...

skosch commented 8 years ago

(Also, sorry! I wish contributing to this project was a less frustrating experience. Thank you all the more for trying :)

katef commented 8 years ago

@skosch Everything about computers is frustrating. I'm a C programer; don't worry about it :)

##fonts on Freenode:

<ankh> nothing obvious to me in the metrics
<ankh> if i generate a pfa font it doesn’t overlap
<kate> hm. could that be due to fontforge's otf output, then?
<ankh> yes, but i don’t see why
skosch commented 8 years ago

Ha, okay then!

If it's a FontForge bug, how about the other editors out there, like TruFont or the various online ones?

katef commented 8 years ago

Possibly related to fontforge/fontforge#2559, but that's all I could find for existing issues. I wonder if I could produce a minimal test case.

katef commented 8 years ago

I made a minimal-ish test case (containing just "A") from the version just before the bug, and an equivalent test case for the version with the bug. Looking at a diff between those, I've been reverting single lines to try and find a culprit.

I found that reverting this appears to give almost the right height glyph...

-LineGap: 256
+LineGap: 0
katef commented 8 years ago

The way I'm testing this is:

; ./x.pe Crimson-Roman.sfd;  echo -e 'Agj\nNfM' \
    | hb-view Crimson-Roman.otf --text-file=/dev/stdin --output-file=/tmp/x.png \
    && feh -. /tmp/x.png

Where x.pe is:

#!/home/kate/bin/fontforge
Open($1);
Generate($1:r+".otf");
Quit(0);

According to the SFD documentation, LineGap is the distance between lines without leading.

So I would expect that, with 0 leading, the Em square of one line ought to touch the next. In practice that means the lowest descender ought to be as close to the highest ascender of the next line, with whatever gap they have to their Em boxes.

It looks like the tail of jfor Crimson-Roman actually overhangs its Em square ever so slightly. Strictly I think it ought to be inside the Em square.

katef commented 8 years ago

Setting a LineGap of Ascent - Descent looks to be what I'd expect (taking into account that j overhangs its Em square by a small amount). That's 730-270 = 460:

--- a/Source Files/Crimson-Roman.sfd
+++ b/Source Files/Crimson-Roman.sfd
@@ -24,7 +24,7 @@ ModificationTime: 1428842982
 PfmFamily: 17
 TTFWeight: 400
 TTFWidth: 5
-LineGap: 0
+LineGap: 460
 VLineGap: 0
 Panose: 2 0 5 3 0 0 0 0 0 0
 OS2TypoAscent: 730

Which gives: x

This is what I'd expect for the current metrics, based on my understanding above. However this is quite different from the earlier releases of Crimson, where the line is about twice the height.

katef commented 8 years ago

FontForge FAQ: How do I set the line spacing on a font? - This also explains the difference for .pfa; PostScript fonts take the line spacing distance from the bounding box.

katef commented 8 years ago

I think setting LineGap to 460 must surely be incorrect, soley because none of the examples and test fonts for FontForge have that. They have either 0 or 90. So I set LineGap back to 0.

Since this only seems to affect TTF output, I from the FAQ above I gather that this must be a problem with Crimson's OS2 values. So through some experimentation, I found this:

--- a/Source Files/Crimson-Roman.sfd
+++ b/Source Files/Crimson-Roman.sfd
@@ -38,7 +38,7 @@ OS2WinDescent: 370
 OS2WinDOffset: 0
 HheadAscent: 830
 HheadAOffset: 0
-HheadDescent: 370
+HheadDescent: -370
 HheadDOffset: 0
 OS2SubXSize: 665
 OS2SubYSize: 614

Which gives output that looks very similar to the spacing for the pre-bug line spacing: x

I do not know if this is correct. A negative value for HheadDescent seems strange to me, and I found it by guesswork.

skosch commented 8 years ago

Whoa, that's interesting stuff. Would it help to make the j's descender a wee bit shorter then so it's inside the square? I have no objections to that if it helps.

katef commented 8 years ago

@skosch I think the j descender ought to be addressed somehow — probably not by changing the glyph shape! Perhaps by increasing the Em square instead — but that's unrelated to the issue here. I imagine there are other glyphs outside the box, too. Fontforge also complains about a few validation failures, which also need fixing. Perhaps this is one of them.

Edit: I made #21 to discuss that, so we don't confuse it here.

katef commented 8 years ago

Some more support; the other Crimson .sfd files all have negative HheadDescent. Surely Crimson-Roman's positive value must be a mistake!

Crimson-Bold.sfd:HheadDescent: -427
Crimson-BoldItalic.sfd:HheadDescent: -427
Crimson-Italic.sfd:HheadDescent: -427
Crimson-Roman.sfd:HheadDescent: 370
Crimson-Semibold.sfd:HheadDescent: -427
Crimson-SemiboldItalic.sfd:HheadDescent: -427
katef commented 8 years ago

Here's a rundown of values which I think look suspicious:

Bold Roman Italic BoldItalic
LineGap 256 0 256 256
OS2TypoAscent 742 730 742 742
OS2TypoDescent -282 -270 -282 -282
OS2TypoLinegap 256 200 256 256
OS2WinAscent 1130 830 1130 1130
OS2WinDescent 427 370 427 427
HheadAscent 1130 830 1130 1130
HheadDescent -427 370 -427 -427
OS2SubXOff 0 0 -16 -16
OS2SupXOff 0 0 76 76
OS2StrikeYSize 51 50 51 51
OS2StrikeYPos 265 255 261 269
OS2CapHeight 0 658 658 0
OS2XHeight 0 429 429 0
OS2Vendor 'PfEd' 'SbKo' 'PfEd' 'PfEd'

Perhaps some of these ought to differ between the faces, but I propose that in general these ought to mostly be the same for all faces.

skosch commented 8 years ago

Amazing! Something is clearly wrong with the Roman. What is 'SbKo' – is that supposed to be my name? LOL. I swear I had no part in any of this!

Does equalizing the values solve your production problems, then?

katef commented 8 years ago

Detective kate is finding out. ~pink panther music~

katef commented 8 years ago

Setting Roman to have the same values for most of these does seem sensible, but unfortunately the glyphs end up in the wrong place relative to the other faces. They're vertically offset by some amount. Which makes me think that all the glyphs are drawn in the wrong place, and ought to be moved to match the other faces.

--- a/Source Files/Crimson-Roman.sfd
+++ b/Source Files/Crimson-Roman.sfd
@@ -20,25 +20,25 @@ OS2Version: 0
 OS2_WeightWidthSlopeOnly: 0
 OS2_UseTypoMetrics: 1
 CreationTime: 1270926789
-ModificationTime: 1428842982
+ModificationTime: 1450233805
 PfmFamily: 17
 TTFWeight: 400
 TTFWidth: 5
-LineGap: 0
+LineGap: 256
 VLineGap: 0
 Panose: 2 0 5 3 0 0 0 0 0 0
-OS2TypoAscent: 730
+OS2TypoAscent: 742
 OS2TypoAOffset: 0
-OS2TypoDescent: -270
+OS2TypoDescent: -282
 OS2TypoDOffset: 0
-OS2TypoLinegap: 200
-OS2WinAscent: 830
+OS2TypoLinegap: 256
+OS2WinAscent: 1130
 OS2WinAOffset: 0
-OS2WinDescent: 370
+OS2WinDescent: 427
 OS2WinDOffset: 0
-HheadAscent: 830
+HheadAscent: 1130
 HheadAOffset: 0
-HheadDescent: 370
+HheadDescent: -427
 HheadDOffset: 0
 OS2SubXSize: 665
 OS2SubYSize: 614
@@ -48,12 +48,12 @@ OS2SupXSize: 665
 OS2SupYSize: 614
 OS2SupXOff: 0
 OS2SupYOff: 358
-OS2StrikeYSize: 50
+OS2StrikeYSize: 51
 OS2StrikeYPos: 255
 OS2CapHeight: 658
 OS2XHeight: 429
 OS2FamilyClass: 258
-OS2Vendor: 'SbKo'
+OS2Vendor: 'PfEd'
skosch commented 8 years ago

Weird. They all have the same size em square, no?

katef commented 8 years ago

Yes, but I believe the glyphs for Roman are drawn in different places within the square, than the glyphs for the other faces are. And this differences seems to be catered for by adjusting these metrics instead of positioning the glyphs in the same spot.

That's my hypothesis for the moment, but I haven't checked it in detail yet.

katef commented 8 years ago

If it turns out to be that the Roman glyphs are in the wrong place, then I was thinking perhaps I could make a Fontforge script to reposition them all.

But, it's possible that the glyphs will have been repositioned from original locations (consistent with the other faces) by 2d24623, which I noticed altered the outlines (i.e. I imagine to reposition them, rather than to change shapes). So I'll try to check that first. I hope that is what happened, because it'll make cleaning this up much easier - we can revert the diff, and then correct the metrics.

katef commented 8 years ago

I think I'm wrong about glyphs being in the wrong place - or at least certainly I don't understand what's going on. However I committed the above values as katef/Crimson@c93ec58 [edit: repository now deleted; same diff as commented below], without any other changes. That gives: c93ec58

I think that's correct per the design intention. Notice the Roman is ever so slightly taller than bold. I'd like somebody who isn't me to help test that the vertical metrics are correct, on various platforms.

I'm concerned because gnome-font-viewer, which I do not trust, shows glyphs at different vertical offsets. I do not know why. But they seem to be in the right place when used in an application, as in my screenshot above.

If that is correct, I'd be happy to call this fixed.

katef commented 8 years ago

(Side effect: j is now within its Em)

skosch commented 8 years ago

Cool. It really is strange, since I don't understand how the glyphs could have ended up vertically shifted. There's really only the one place to draw them, and that's on the baseline.

If you want these changes in the official repo you'll have to either create a merge/pull request or I can give you write access – maybe the latter is easier if you're planning on making more commits? In any case, thanks for putting so much time (and top-notch detective work) into this!

katef commented 8 years ago

I'd really like somebody else to confirm that this is correct though. And since you didn't complain about it, I presume the mismatched heights in my screenshot above are intentional for optical correction.

Commit access would be great! Thanks.

skosch commented 8 years ago

Alright, you should be able to push now. And yeup, the height differences are indeed intentional for optical correction.

katef commented 8 years ago

Thanks! Committed in 71a4e25. I'm going to close this ticket on the assumption that what I've done is correct; I have a hunch that if I wait for verification from other people, that it might be open for a very long time.

Anyone: Please re-open if this isn't the right approach :)