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

[Review] comparing ttf output from chain against RobotoTTF #4

Closed
m4rc1e opened this issue Nov 23, 2017 · 41 comments
Closed

[Review] comparing ttf output from chain against RobotoTTF #4

m4rc1e opened this issue Nov 23, 2017 · 41 comments
Assignees

Comments

@m4rc1e
Copy link
Collaborator

m4rc1e commented Nov 23, 2017

First things first, the quality of the chain and font work is fantastic. Building Roboto using the official buildchain takes a long time. I was super happy to see your chain produce the unhinted fonts in a tenth of the time.

For this review, I'm testing ttfs which have been built by running fontmake on your design space files with the following command:

fontmake -m ./Roboto.designspace -o ttf

Once the fonts were built, I've compared them against the official Roboto v2.138 unhinted fonts

So far I've only focused on the Black weight.

Has the kerning been rebuilt? I don't mind if it has. As long as line lengths remain consistent, I'm happy. I'll figure out a way to test this.

I've used a new tool we're currently developing in order to catch visual regressions. I'm attaching a zip of the sample images it has generated as well. If I produce a plot of all the characters and view them across 4 browsers, I can only find 2 issues.

Acutes have shifted
before
screen shot 2017-11-23 at 16 50 59

after
screen shot 2017-11-23 at 16 49 32

Misplaced mark positions
before
screen shot 2017-11-23 at 16 54 15

after
screen shot 2017-11-23 at 16 54 06

Keep this thread open, I need to do further checks.

TN_Roboto_img.zip

@m4rc1e
Copy link
Collaborator Author

m4rc1e commented Dec 8, 2017

I've just had a look at the Regular and it has the same issues.

roboto_marks

roboto_reg

When I diff the table's of the Reg using https://github.com/googlefonts/tools/blob/master/bin/gftools-compare-font.py. We get the following.

  Table Changes (delta bytes, from=>to, % change)
    BASE, +0, 0=>0, 0.0%
    CFF , +0, 0=>0, 0.0%
    DSIG, +0, 0=>0, 0.0%
    FFTM, +0, 0=>0, 0.0%
    GDEF, +656, 934=>1590, 0.2%
    GPOS, +65478, 5462=>70940, 21.3%
    GSUB, -74, 11536=>11462, -0.0%
    LTSH, +0, 0=>0, 0.0%
    OS/2, +0, 96=>96, 0.0%
    VORG, +0, 0=>0, 0.0%
    cmap, +20, 6408=>6428, 0.0%
    cvt , +0, 0=>0, 0.0%
    fpgm, +0, 0=>0, 0.0%
    gasp, +0, 0=>0, 0.0%
    glyf, -8517, 205393=>196876, -2.8%
    hdmx, +0, 0=>0, 0.0%
    head, +0, 54=>54, 0.0%
    hhea, +0, 36=>36, 0.0%
    hmtx, +34, 13546=>13580, 0.0%
    kern, +0, 0=>0, 0.0%
    loca, +32, 13552=>13584, 0.0%
    maxp, +0, 32=>32, 0.0%
    name, +0, 760=>760, 0.0%
    post, -12914, 49826=>36912, -4.2%
    prep, +0, 0=>0, 0.0%
    vhea, +0, 0=>0, 0.0%
    vmtx, +0, 0=>0, 0.0%
    TOTAL, +44715, 307635=>352350, 14.5%

As I mentioned in the first post, I believe the kerning has been rebuilt which is fine.

I think a lot of these reported changes above are false positives. I suspect this is being caused by fontmake converting glyph names to unixxxx' e.g Aogonek.NAV has become uni0104.NAV which is fine.

It seems your fonts are missing mark, mkmk features. This could be what is causing the issues I have already highlighted.

@davelab6
Copy link
Member

davelab6 commented Dec 8, 2017

Thanks Marc!

@davelab6
Copy link
Member

davelab6 commented Mar 13, 2018

@m4rc1e I spoke with @sberlow about this today and @asaumierdemers will take a look at it soon, and then it would be great if you could arrange a video call with them to go over how your review and checking workflow is going these days :)

@sberlow
Copy link

sberlow commented Mar 27, 2018

I added @cjdunn and @djrrb to this.
They will be assisting in getting this done.

@davelab6
Copy link
Member

I sync'd with @sberlow just now and he says to expect a full report of the deltas next week likely from @justvanrossum

@sberlow
Copy link

sberlow commented Mar 30, 2018

Order of events

  1. @cjdunn will post fix to this specific issue
    (as not to make to many changes at once)

  2. Mark reviews to make sure the accents are correct in all styles, mkmk features

  3. @cjdunn will add the instances,

  4. @cjdunn we will run Just's scripts to compare var instances to static....
    Report on diff

@cjdunn
Copy link
Contributor

cjdunn commented Mar 30, 2018

@davelab6 Here's my PR:
#5

Please let us know if this works for you

@m4rc1e
Copy link
Collaborator Author

m4rc1e commented Apr 3, 2018

Looking into this now.

@m4rc1e m4rc1e mentioned this issue Apr 5, 2018
@m4rc1e
Copy link
Collaborator Author

m4rc1e commented Apr 6, 2018

Class Kerns are missing. Only Format 1s exist.

class_kerning2
class_kerning

@cjdunn
Copy link
Contributor

cjdunn commented Apr 12, 2018

@m4rc1e can you point me to Roboto sources which do have class kerning? The UFOs in the main Roboto repo here:
https://github.com/google/roboto/tree/master/src/v2
do not have class kerns or groups of any kind, only flat kerns. Thanks.

@davelab6
Copy link
Member

davelab6 commented Apr 13, 2018 via email

@cjdunn
Copy link
Contributor

cjdunn commented Apr 13, 2018

@davelab6 good question. I wasn't involved early on so I'm not sure what the original source was, but currently there is no class kerning in these UFOs and I'm trying to track it down.

Are these the shipping TTFs you're talking about:
https://github.com/google/roboto/releases/download/v2.138/roboto-unhinted.zip ? Do you know the source of the class kerning in here? If not, perhaps we can try extracting the class kerning from these TTFs. Please let me know if you have any further information about the kerning sources, thanks.

@dberlow
Copy link
Contributor

dberlow commented Apr 14, 2018 via email

@asaumierdemers
Copy link
Contributor

@davelab6 @dberlow @cjdunn
UFO were the sources.

Read the README.md for more details

@anthrotype
Copy link
Member

@anthrotype
Copy link
Member

I'd recommend you extract these kerning classes that are currently defined in the Roboto UFOs features.fea files, and store them instead in groups.plist, and also use them in kerning.plist in place of the "key" glyphs that are currently being used.

This is how the ufo2ft kernFeatureWriter does it:
https://github.com/googlei18n/ufo2ft/blob/5c13d7fefa01090556e1ffbc3b673da06fec9485/Lib/ufo2ft/featureWriters/kernFeatureWriter.py#L159-L209

Note that in the next version of ufo2ft we are planning to drop support for this -- as far as I'm aware, this trick is only used by Roboto, and it is not the way UFO is supposed to store class kerning.

We will only keep supporting the following:

  • UFO format 2: kerning classes in groups.plist/kerning.plist prefixed with @MMK_L_ and @MMK_R_;
  • UFO format >= 3: kerning classes with public.kern1. and public.kern2. prefixes

@m4rc1e
Copy link
Collaborator Author

m4rc1e commented Apr 16, 2018

@anthrotype thank you for this. I was in the middle of writing something similar.

@davelab6
Copy link
Member

UFO were the sources

But those UFOs were derived from the release TTFs, right?

I made a PR to clarify the README :)

@m4rc1e
Copy link
Collaborator Author

m4rc1e commented Apr 16, 2018

@davelab6 during the process of running the original Roboto build chain, it will generate ufos for all weights and then make the ttfs. I'm pretty sure these ufos have been used https://github.com/TypeNetwork/Roboto/tree/master/master_ufo

@asaumierdemers
Copy link
Contributor

@davelab6 @m4rc1e exactly!

@davelab6
Copy link
Member

Okay :) In that case I will update the README PR.

Still, the brief is to make the fonts a 1:1 match for the TTFs.

@cjdunn
Copy link
Contributor

cjdunn commented Apr 27, 2018

@m4rc1e @davelab6 I tried copying the groups and class kerns from the post-processed TTFs. Here's an updated var font:

Roboto-VF.ttf.zip

Please let me know if this works for you.
cc @sberlow

@m4rc1e

This comment has been minimized.

@davelab6
Copy link
Member

davelab6 commented May 8, 2018

Yes - please prioritize this project above everything except Markazi

@cjdunn
Copy link
Contributor

cjdunn commented May 8, 2018

@m4rc1e If you look at the data in the font files you'll see that the kern values for these pairs in the Robot var font and the Robot non-var font are the same. They are as follows:
/A /Ocircumflex -5
/A /Gcommaaccent -5
/A /Gdotaccent -5
/A /ydieresis -24
/A /O -5

The very small differences you're seeing may be due to rounding errors or grid fitting issues related to rasterization for var fonts vs non-var fonts or something else, but it is not the kerning.

@m4rc1e
Copy link
Collaborator Author

m4rc1e commented May 9, 2018

@cjdunn

I tried copying the groups and class kerns from the post-processed TTFs. Here's an updated var font

Your results are much better than mine. I mistakenly didn't diff the VF you attached above. Ignore my previous two comments.

When you say you copied the groups and classes, did you mean you copied/manipulated them from the GPOS table in the ttf? are they now stored in the sources? will running fontmake produce this result?

If the answer is yes to the two previous questions, I'll be buying you a brewery, a beer isn't enough.

@davelab6
Copy link
Member

davelab6 commented May 9, 2018

I mistakenly didn't diff the VF you attached above

But you have now done so and the diff check is showing 0 differences?

When you say you copied the groups and classes, did you mean you copied/manipulated them from the GPOS table in the ttf?

That was what I understood - since the rest of the project took a starting point from quadratic UFOs, not TTFs, but the GPOS table data apparently wasn't in those UFOs.

are they now stored in the sources? will running fontmake produce this result?

@anthrotype does fontmake allow TTX sources to be included and stitched in, as a standard feature?

@cjdunn
Copy link
Contributor

cjdunn commented May 9, 2018

@m4rc1e @davelab6

When you say you copied the groups and classes, did you mean you copied/manipulated them from the GPOS table in the ttf?
Yes, after researching this problem a bit I figured out I could use this script:
https://github.com/adobe-type-tools/kern-dump/blob/master/convertKernedOTFtoKernedUFO.py
to get compiled kerning and groups from TTFs to non-var UFOs. I then imported the groups and kerning into the var UFO masters. And then I ran a script to remove redundant exceptions, which I think were in the originals but perhaps overlooked because of this roundabout way of getting class kerning.

are they now stored in the sources?
Yes.

will running fontmake produce this result?
Yes, this var font was made via fontmake with no post-processing.

Here's the PR with my updated UFOs: #8

@anthrotype
Copy link
Member

does fontmake allow TTX sources to be included and stitched in, as a standard feature?

Yes, just place the ttx files inside a UFO’s data subfolder, and they will be merged in at compile time.
But I would not recommend to do so for the GPOS kerning. Better to try reconstruct a UFO3-compliant class kerning source from the data available in the UFO2 sources, as I explained above. Not from the binary TTFs generated by fontmake.

@m4rc1e
Copy link
Collaborator Author

m4rc1e commented May 10, 2018

@cjdunn Fixing the missing marks is relatively straight forward. If you inspect the missing marks from the report in #8, you'll see the mark anchor is named differently to the base anchor.

screen shot 2018-05-10 at 12 51 36

screen shot 2018-05-10 at 12 51 53

If you remove "mark" from all mark anchors, the amount missing goes from 5,000+ to 900. The remaining 900 all involve uni035D, which I'm looking into now.

@cjdunn
Copy link
Contributor

cjdunn commented May 11, 2018

@m4rc1e OK got it, I have removed "mark" from all mark anchors. I'm still not sure why these names don't match since we started with your sources. But if you can let me know what other anchors need to be renamed to resolve those remaining 900 missing, I can renamed those as well.

Here's the updated var font:
Roboto-VF.ttf.zip

Please let me know if this works better now, thanks.

@m4rc1e
Copy link
Collaborator Author

m4rc1e commented May 15, 2018

@cjdunn I've just discovered that fontmake allows us to add our own custom --kern-writer-module and --mark-writer-module. I've just written one quickly based on the MarkFeatureWriter in the original Roboto. The results are pretty much spot on.

Roboto-Regular.ttf vs Roboto-Regular.ttf

attribs 1 modified

table attrib value_a value_b
head modified 2016/12/15 22:52:08 2018/05/15 09:38:12

mkmks 7656 new

mark1_glyph mark2_glyph mark1_x mark1_y mark2_x mark2_y
acutecomb macroncomb_uni1ABB -566 1290 -578 1540
acutecomb dieresisnosp_uni1ABB -566 1290 -671 1540
acutecomb tildecomb_uni1ABB -566 1290 -537 1562
acutecomb uni034A -566 1290 -583 1644
acutecomb acuterightnosp -566 1290 -103 1614

names 2 modified

id string_a string_b
(3, 3, 1, 1033) Google:Roboto Regular:2016 Google:Roboto Regular:2017
(4, 3, 1, 1033) Roboto Roboto Regular

The new mkmks may be false. I need to double check these and update the diffenator if so.

@anthrotype I need a second opinion. I think it may be better to provide a custom MarkFeatureWriter and KernFeatureWriter, based off the original chain. Atm, we're changing the sources so the fonts will have the same output when genned with default fontmake settings. I think reviewers will appreciate seeing sources which are as close as possible to the previous.

@anthrotype
Copy link
Member

I was planning to drop those two fontmake command-line options actually... The plan is to have these custom writers specified in the UFO lib (as custom key/value pairs), and ufo2ft will load and use these instead of the default. There's a feature-writer-wip branch in ufo2ft repo where I'm currently working on that. The API of the feature writer class will change also: instead of manipulating strings, they will work directly on the feaLib AST that results from parsing the existing feature file, and extend that in place before compilation. I would still recommend you change your sources so that they won't require any custom feature writer. There's nothing special about Roboto's kern or mark feature, it should be possible to upgrade them to UFO3 standard without need of a custom writer.

@cjdunn
Copy link
Contributor

cjdunn commented May 31, 2018

@m4rc1e According to what @anthrotype wrote, it sounds like we should still change the sources.

I already renamed all of the pairs mentioned in the link you posted above, which are:

        self.anchorPairs = [
            ["top", "_marktop"],
            ["bottom", "_markbottom"],
            ["top_dd", "_marktop_dd"],
            ["bottom_dd", "_markbottom_dd"],
            ["rhotichook", "_markrhotichook"],
            ["top0315", "_marktop0315"],
            ["parent_top", "_markparent_top"],
            ["parenthesses.w1", "_markparenthesses.w1"],
            ["parenthesses.w2", "_markparenthesses.w2"],
            ["parenthesses.w3", "_markparenthesses.w3"]]

        self.mkmkAnchorPairs = [
            ["mkmktop", "_marktop"],
["mkmkbottom_acc", "_markbottom"],
            ["", "_bottom"],
]

If you can let me know what other anchors need to be renamed to resolve those remaining 900 missing, I can renamed those as well, thanks.

@m4rc1e
Copy link
Collaborator Author

m4rc1e commented Jun 1, 2018

Hey @cjdunn. Sorry but I've had to work on other projects for the time being.

Could we have a hangout sometime next week? I'd like to run you through how I'm QAing so you can do it yourself. A large part of this project is understanding Roboto's buildchain (pre fontmake days)

@cjdunn
Copy link
Contributor

cjdunn commented Jun 1, 2018

@m4rc1e Sure, some time towards the end of next week would be good for me, thanks.

@sberlow
Copy link

sberlow commented Jun 4, 2018

@cjdunn @m4rc1e Thursday noon?

@cjdunn
Copy link
Contributor

cjdunn commented Jun 4, 2018

@sberlow @m4rc1e that time works for me

@davelab6
Copy link
Member

Here are the notes Marc prepared for the review on 2018-06-07:


We need to merge #8 - the kerning issue is now solved.

The only issue remaining is the mark positioning. Cosimo says this should be fixed at source, #4 (comment)

Test Setup

To test the fonts, we’re comparing the unhinted v2.136 fonts against your chain, by running fontmake to generate the fonts from your build sources like this:

fontmake --no-production-names -m Roboto.designspace -o ttf

This generates static ttfs. We want to compare the v2.136 ttfs against these instead of a variable font to reduce any noise which may be caused by fontmake producing variable fonts. This noise is not an issue you should be dealing with.

To make the comparisons, we’re using my fontdiffenator like this:

diffenator Roboto-Regular.ttf (v2.136) Roboto-Regular.ttf (from our chain) -ol 5

We should get the following report once the pr is merged:

Roboto-Regular.ttf vs Roboto-Regular.ttf

attribs 1 modified

table               attrib              value_a             value_b
head                modified            2016/12/15 22:52:08 2018/06/07 14:47:00

mkmks 615 new

mark1_glyph         mark2_glyph         mark1_x             mark1_y             mark2_x             mark2_y
acute               uni0237             225                 1290                266                 1249
acutecomb           uni0237             -566                1290                266                 1249
breve               uni0237             429                 1290                266                 1249
breveinvnosp        uni0237             -585                1196                266                 1249
candrabindunosp     uni0237             -585                1196                266                 1249

mkmks 3000 missing

mark1_glyph         mark2_glyph         mark1_x             mark1_y             mark2_x             mark2_y
acutesubnosp        dotdblsubnosp_uni1ABD-607                0                   -671                -352
acutesubnosp        breveinvsubnosp_uni1ABD-607                0                   -582                -372
acutesubnosp        tildesubnosp_uni1ABD-607                0                   -585                -426
acutesubnosp        bridgesubnosp       -607                0                   -585                -464
acutesubnosp        commaaccent         -607                0                   256                 -162

names 1 modified

id                  string_a            string_b
(3, 3, 1, 1033)     Google:Roboto Regular:2016Google:Roboto Regular:2017

marks 59390 new

mark1_glyph         mark2_glyph         mark1_x             mark1_y             mark2_x             mark2_y
A                   uni1AB9             686                 0                   -235                0
A                   uni1ABA             686                 0                   -236                0
A                   uni02BE             672                 1601                70                  1160
A                   uni02F3             686                 0                   302                 -1
A                   comma               1306                0                   128                 68

marks 1777 modified

mark1_glyph         mark2_glyph         diff_x              diff_y
l                   uniFE26             314.0               -157.0
l                   uniFE24             314.0               -157.0
l                   uniFE25             314.0               -157.0
l                   uniFE23             314.0               -157.0
l                   uniFE20             314.0               -157.0

marks 305569 missing

mark1_glyph         mark2_glyph         mark1_x             mark1_y             mark2_x             mark2_y
A                   uni2DE8             672                 1601                -588                1170
A                   ringsubnosp_uni1ABD 686                 0                   -588                0
A                   uniFE21             672                 1601                360                 1170
A                   uniFE24             672                 1601                -312                1170
A                   uniFE20             672                 1601                -360                1170

glyphs 23 modified

glyph               diff
uni1AB5             5577.0
uni047A             1869.08333333
uniFFFC             1280.0
threeeighths        1112.58333333
uni20BA             1056.79166667

We don’t care about these 23 glyphs that are modified, because the diff amounts are negligible - and being introduced by fontmake.

The only issues we care about are the missing and modified marks and mkmks.

When I added my own Markwriters to fontmake, I got the desired result.

Once the marks are fixed, we should end up with this report:

Roboto-Regular.ttf vs Roboto-Regular.ttf

attribs 1 modified

table               attrib              value_a             value_b
head                modified            2016/12/15 22:52:08 2018/06/07 14:47:00

glyphs 23 modified

glyph               diff
uni1AB5             5577.0
uni047A             1869.08333333
uniFFFC             1280.0
threeeighths        1112.58333333
uni20BA             1056.79166667

New marks and mkmks are fine.

Notes from meeting

Here’s the test results of CJ’s anchor renaming:

marks 59390 new

mark1_glyph         mark2_glyph         mark1_x             mark1_y             mark2_x             mark2_y
A                   uni1AB9             686                 0                   -235                0
A                   uni02BE             672                 1601                70                  1160
A                   uni02F3             686                 0                   302                 -1
A                   cedilla             686                 0                   213                 0
A                   uni1FBF             672                 1601                181                 1182

marks 2437 modified

mark1_glyph         mark2_glyph         diff_x              diff_y
uni1E79             uni1ABC             -1.0                664.0
uni1E4D             uni1ABC             -1.0                664.0
uni1E4D             uni1ABB             -1.0                664.0
uni1E79             uni1ABB             -1.0                664.0
uni1E53             uni1ABB             0.0                 646.0

marks 3871 missing

mark1_glyph         mark2_glyph         mark1_x             mark1_y             mark2_x             mark2_y
A                   uni035D             1336                1600                0                   1162
A                   uni0361             1336                1600                0                   1193
A                   uni035E             1336                1600                0                   1243
A                   uni0360             1336                1600                0                   1274
A.smcp              uni035E             1158                1600                0                   1243

@davelab6
Copy link
Member

I believe this review is now complete; @m4rc1e please close if so

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

No branches or pull requests

8 participants