8000 Merger improvements by behdad · Pull Request #2473 · fonttools/fonttools · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Merger improvements #2473

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 24 commits into from
Dec 16, 2021
Merged

Merger improvements #2473

merged 24 commits into from
Dec 16, 2021

Conversation

behdad
Copy link
Member
@behdad behdad commented Dec 14, 2021

With this branch I can try merging all of NotoSans Regular fonts:

behdad:ttf 1 (main)$ ~/fonttools/fonttools merge --verbose NotoSans*/NotoSans*-Regular.ttf --drop-tables+=vhea,vmtx

and hit the 64k glyph limit:

WARNING: Dropped mapping from codepoint 0X0374 to glyphId 'uni0374'
WARNING: Dropped mapping from codepoint 0X0374 to glyphId 'uni0374#1'
WARNING: Dropped mapping from codepoint 0X0374 to glyphId 'uni0374#2'
Merged 'cmap'.
Merging 'GSUB'.
WARNING: Have duplicates to resolve for font 37 but no GSUB: {'tildecomb': 'tildecomb#1', 'overlinecomb': 'uni0305#1', 'uni0484': 'uni0484#1', 'uni0487': 'uni0487#1', 'uniA66F': 'uniA66F#1'}
WARNING: Have duplicates to resolve for font 69 but no GSUB: {'hyphen': 'hyphen#17', 'uni02BC': 'uni02BC#5', 'uni02CD': 'uni02CD#1'}
WARNING: Have duplicates to resolve for font 90 but no GSUB: {'zeroguru': 'zeroguru#2', 'oneguru': 'oneguru#2', 'twoguru': 'twoguru#2', 'threeguru': 'threeguru#2', 'fourguru': 'fourguru#2', 'fiveguru': 'fiveguru#2', 'sixguru': 'sixguru#2', 'sevenguru': 'sevenguru#2', 'eightguru': 'eightguru#2', 'nineguru': 'nineguru#2'}
WARNING: Have duplicates to resolve for font 101 but no GSUB: {'uni20B9.beng': 'uni20B9#3'}
WARNING: Have duplicates to resolve for font 105 but no GSUB: {'gravecomb': 'uni0300', 'uni0306': 'uni0306#3', 'uni0307': 'uni0307#6', 'uni0308': 'uni0308#7', 'uni0313': 'uni0313#3', 'uni0483': 'uni0483#3', 'uni20DB': 'uni20DB#1'}
WARNING: Have duplicates to resolve for font 131 but no GSUB: {'hyphen': 'hyphen#24'}
WARNING: Have duplicates to resolve for font 139 but no GSUB: {'uni1736': 'uni1736#3'}
WARNING: Have duplicates to resolve for font 142 but no GSUB: {'uniA78B': 'uniA78B#3', 'uniA78C': 'uniA78C#3'}
WARNING: Have duplicates to resolve for font 149 but no GSUB: {'exclam': 'exclam#16', 'parenleft': 'parenleft#13', 'parenright': 'parenright#13', 'comma': 'comma#15', 'period': 'period#15', 'colon': 'colon#16', 'semicolon': 'semicolon#12', 'uni060C': 'uni060C#4', 'uni061B': 'uni061B#4', 'uni061F': 'uni061F#6', 'uni0660': 'uni0660#3', 'uni0661': 'uni0661#3', 'uni0662': 'uni0662#3', 'uni0663': 'uni0663#3', 'uni0664': 'uni0664#3', 'uni0665': 'uni0665#3', 'uni0666': 'uni0666#3', 'uni0667': 'uni0667#3', 'uni0668': 'uni0668#3', 'uni0669': 'uni0669#3', 'uni066A': 'uni066A#4', 'uni066B': 'uni066B#3', 'uni066C': 'uni066C#3', 'quoteleft': 'quoteleft#11', 'quoteright': 'quoteright#11', 'quotedblleft': 'quotedblleft#12', 'quotedblright': 'quotedblright#12', 'uniFDF2': 'uniFDF2#2', 'uniFDFD': 'uniFDFD#2'}
WARNING: Have duplicates to resolve for font 178 but no GSUB: {'exclam': 'exclam#17', 'quotedbl': 'quotedbl#12', 'quotesingle': 'quotesingle#11', 'parenleft': 'parenleft#14', 'parenright': 'parenright#14', 'asterisk': 'asterisk#10', 'plus': 'plus#9', 'comma': 'comma#16', 'hyphen': 'hyphen#29', 'period': 'period#16', 'slash': 'slash#12', 'nine': 'nine#10', 'colon': 'colon#17', 'semicolon': 'semicolon#13', 'less': 'less#8', 'equal': 'equal#9', 'greater': 'greater#8', 'question': 'question#15', 'asciicircum': 'asciicircum#8'}
WARNING: Have duplicates to resolve for font 181 but no GSUB: {'u3001': 'uni3001#2', 'u3002': 'uni3002#2', 'uni3008': 'uni3008#2', 'uni3009': 'uni3009#2', 'uni300A': 'uni300A#3', 'uni300B': 'uni300B#3', 'u300C': 'uni300C#1', 'u300D': 'uni300D#1', 'u300E': 'uni300E#1', 'u300F': 'uni300F#1', 'uni3010': 'uni3010#1', 'uni3011': 'uni3011#1', 'uni3014': 'uni3014#1', 'uni3015': 'uni3015#1', 'uni3016': 'uni3016#1', 'uni3017': 'uni3017#1', 'uni3018': 'uni3018#1', 'uni3019': 'uni3019#1', 'uni301A': 'uni301A#1', 'uni301B': 'uni301B#1'}
Merged 'GSUB'.

...
  File "/home/behdad/fonttools/Lib/fontTools/ttLib/tables/_g_l_y_f.py", line 898, in compileComponents
    data = data + compo.compile(more, haveInstructions, glyfTable)
  File "/home/behdad/fonttools/Lib/fontTools/ttLib/tables/_g_l_y_f.py", line 1464, in compile
    return struct.pack(">HH", flags, glyphID) + data
struct.error: 'H' format requires 0 <= number <= 65535

@behdad behdad requested a review from anthrotype December 14, 2021 23:33
@behdad
Copy link
Member Author
behdad commented Dec 14, 2021

Of course there's also #1260 that should be fixed.

rsheeter
rsheeter reviewed Dec 14, 2021
View reviewed changes
rsheeter
rsheeter reviewed Dec 15, 2021
View reviewed changes
rsheeter
rsheeter reviewed Dec 15, 2021
View reviewed changes
rsheeter
rsheeter reviewed Dec 15, 2021
View reviewed changes
This was referenced Dec 15, 2021
Closed
Closed
anthrotype
anthrotype reviewed Dec 15, 2021
View reviewed changes
@dscorbett dscorbett mentioned this pull request Dec 15, 2021
Closed
@behdad
Copy link
Member Author
behdad commented Dec 15, 2021

See #2476
I need to do more work on the glyphsAreSame to re-enable it.

@behdad
Copy link
Member Author
behdad commented Dec 16, 2021

I addressed comments. Also split the merger code into multiple smaller modules. I like to merge this now, then go onto unifying identical glyphs in a followup PR.

anthrotype
anthrotype approved these changes Dec 16, 2021
View reviewed changes
Copy link
Member
@anthrotype anthrotype left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Behdad!

I like that you split the module to make it easier to navigate the code. I am not super excited about the blanket star-imports at the top, as it obscures where the symbols come from (they will appear undefined in linters). Would it be too much trouble if you replace them with explicit named imports?

@behdad
Copy link
Member Author
behdad commented Dec 16, 2021

Thanks for the review. Great ideas.

@behdad
Copy link
Member Author
behdad commented Dec 16, 2021

PTAL. Addressed all except for running black.

@behdad
Copy link
Member Author
behdad commented Dec 16, 2021

Okay. Merging this and continuing.

@behdad behdad merged commit a80890b into main Dec 16, 2021
@khaledhosny khaledhosny deleted the merge-noto branch December 18, 2021 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rsheeter rsheeter rsheeter left review comments

@anthrotype anthrotype anthrotype approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@behdad @rsheeter @anthrotype
0