8000 Change gasp merge logic to 'first' by zjusbo · Pull Request #862 · fonttools/fonttools · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Change gasp merge logic to 'first' #862

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 5 commits into from
Feb 27, 2017
Merged

Change gasp merge logic to 'first' #862

merged 5 commits into from
Feb 27, 2017

Conversation

zjusbo
Copy link
Contributor
@zjusbo zjusbo commented Feb 24, 2017

Reference #859
Still open to other ideas.

@zjusbo
Copy link
Contributor Author
zjusbo commented Feb 25, 2017

The test fails due to #857

@behdad
Copy link
Member
behdad commented Feb 25, 2017

I'd say onlyExisting(equal) is better.

@zjusbo
Copy link
< B695 div class="d-none d-sm-flex"> Contributor Author
zjusbo commented Feb 25, 2017

I guess you are suggesting:

ttLib.getTableClass('gasp').mergeMap = {
'tableTag': onlyExisting(equal),
'version': onlyExisting(max), # max(NotImplemented, 1) returns NotImplemented
'gaspRange': onlyExisting(first), # FIXME? Appears irreconcilable
}

The difference between these two options happens in this case

fontA['gasp'] = NotImplemented
fontB['gasp'] = a gasp table that is designed for glyphs in fontB

pyftmerge fontA, fontB

Your option returns the gasp in fontB while current option drops gasp and returns nothing.

I think the root problem is: does a gasp table designed for fontB probably hurt the legibility of fontA?

If so, probably dropping the gasp in fontB is a better choice? Actually, default rules to decide how to render the glyphs on grayscale devices are used if the gasp table is absent. Using gasp in fontB implicitly drops the default rules in fontA.

@behdad
Copy link
Member
behdad commented Feb 25, 2017

Actually, since we retain hinting from first don't only, this also has to be first. Lgtm

@anthrotype anthrotype merged commit b45fcf0 into fonttools:master Feb 27, 2017
@zjusbo zjusbo deleted the gasp1 branch February 28, 2017 21:33
zjusbo added a commit to zjusbo/fonttools that referenced this pull request Mar 8, 2017
@zjusbo
Change gasp merge logic to 'first' (fonttools#862)
a4cff48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
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
@zjusbo @behdad @anthrotype
0