8000 [varLib.mutator] Add support for macOS overlap rendering flag in instantiateVariableFont by chrissimpkins · Pull Request #1518 · fonttools/fonttools · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[varLib.mutator] Add support for macOS overlap rendering flag in instantiateVariableFont #1518

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 12 commits into from
Feb 28, 2019
Merged

[varLib.mutator] Add support for macOS overlap rendering flag in instantiateVariableFont #1518

merged 12 commits into from
Feb 28, 2019

Conversation

chrissimpkins
Copy link
Member
@chrissimpkins chrissimpkins commented Feb 24, 2019 •
edited
Loading

Associated IR: #1281 #730

Associated PR: #1316

Edd Harrington came across the macOS overlapping contour rendering issue that is described in https://github.com/twardoch/test-fonts/tree/master/varia/160413-EvenOddTT with instances that were built from variable fonts using a fontTools.varLib.mutator.instantiateVariableFont based Python 3 script. Instance font glyphs with overlapping contours render with inverted "holes" at the sites of overlap when viewed in Chrome 73.0.3683.27 on macOS 10.14.3.

From Microsoft glyf table documentation:

A non-zero-fill algorithm is needed to avoid dropouts when contours overlap. The OVERLAP_SIMPLE flag is used by some rasterizer implementations to ensure that a non-zero-fill algorithm is used rather than an even-odd-fill algorithm. Implementations that always use a non-zero-fill algorithm will ignore this flag. Note that some implementations might check this flag specifically in non-variable fonts, but always use a non-zero-fill algorithm for variable fonts. This flag can be used in order to provide broad interoperability of fonts — particularly non-variable fonts — when glyphs have overlapping contours.

Note that variable fonts often make use of overlapping contours. This has implications for tools that generate static-font data for a specific instance of a variable font, if broad interoperability of the derived font is desired: if a glyph has overlapping contours in the given instance, then the tool should either set this flag in the derived glyph data, or else should merge contours to remove overlap of separate contours.

Cosimo provided the source for the visually verified fix in our VF-> static build script and this PR adds this source to the fontTools library using a new parameter in the instantiateVariableFont function. These changes set the OVERLAP_SIMPLE bit to 1 by default during execution of this function and on execution of the fonttools varLib.mutator command line executable.

It also includes:

- a new --setbit6 switch to activate this new, non-default setting with fonttools varLib.mutator on the command line
- new test + *.ttx test file

  • revision of the expected *.ttx files for mutator unit tests according to new default behavior added here

EDIT: After discussions in this thread, this PR also modifies instantiateVariableFont function to set the OVERLAP_COMPOUND bit to 1 in composite glyphs by default too.

@chrissimpkins
[varLib.mutator] add support for instantiateVariableFont to set bit6 …
c3d258a 
…of the glyf table, glyph first Outline Flag byte to 1

addresses macOS-specific inverted rendering issue with overlapping contours
@chrissimpkins
[varLib.mutator] add test of instantiateVariableFont function support…
bfcabc0 
… of macOS overlap rendering flag setting
@chrissimpkins
[varLib.mutator] refactor name of macOS overlap rendering bit parameter
be978cb
@chrissimpkins chrissimpkins mentioned this pull request Feb 24, 2019
Closed
@anthrotype
Copy link
Member

Thanks Chris. I’m thinking that maybe we should always set that bit 6 by default when instancing a varfont with mutator.
Are there any unwanted side effects when setting that bit for implementations different than macOS (I think they simply ignore it)?
Since varfonts almost always have overlapping contours, I think this should be the default behaviour of mutator.

anthrotype
anthrotype reviewed Feb 24, 2019
View reviewed changes
@chrissimpkins
Copy link
Member Author
chrissimpkins commented Feb 24, 2019 •
edited
Loading

Are there any unwanted side effects...

Not sure. According to docs that I've come across the answer seems to be that they should not but I've done no testing to confirm this.

I think this should be the default behavior of mutator

OK. No problem. I will change it. Do you support allowing it to be turned off with the new parameter in instantiateVariableFont? If so, I will change the switch to one that deactivates this change in the instance builds (Edit: was reading this on phone and didn't see your source review above which indicates changes to name of parameter). Will need to update the existing mutator test files if this becomes the default but I think that this just involves adding a single line to each glyph with contours in the expected ttx files. Will check and make those changes too.

Thanks for taking a look at this Cosimo.

@chrissimpkins
Copy link
Member Author

By the way, I also came across this information in the Microsoft docs re: overlapping contours between the components of compound glyphs:

OVERLAP_COMPOUND Bit 10: If set, the components of the compound glyph overlap. Use of this flag is not required in OpenType — that is, it is valid to have components overlap without having this flag set. It may affect behaviors in some platforms, however. (See Apple’s specification for details regarding behavior in Apple platforms.) When used, it must be set on the flag word for the first component. See additional remarks, above, for the similar OVERLAP_SIMPLE flag used in simple-glyph descriptions.

Something that we should be addressing in the same fashion as the OVERLAP_SIMPLE issue here?

@anthrotype
Copy link
Member

Composite glyphs with overlapping components are less common. I’m leaning towards tackling that separately, and only when/if need arises.

@miguelsousa
Copy link
Collaborator

Composite glyphs with overlapping components are less common.

Letters with cedilla (ç Ç ş Ş) and with ogonek (Ą ą Ę ę) come to mind as possible common cases.

@khaledhosny
Copy link
Collaborator

I suggest doing The Right Thing™ which is setting this bit by default. If someone later asks for making it configurable and shows a valid need for it, then it can be made so.

@chrissimpkins
Copy link
Member Author

@khaledhosny I am re-working this to make setting the bit the default behavior. Are you suggesting that we eliminate the flag to turn this behavior off in the command line executable?

@chrissimpkins
Copy link
Member Author

Or better stated, that we not replace the current switch that turns the behavior on when it is non-default with a switch to turn it off when it is default...

@khaledhosny
Copy link
Collaborator

@khaledhosny I am re-working this to make setting the bit the default behavior. Are you suggesting that we eliminate the flag to turn this behavior off in the command line executable?

Yes. If we don’t know of a valid reason why the flag should not be set, then it is just over-engineering to make an option for it. This discussion is deep into bikeshedding already, so please do whatever you feel is sensible.

@chrissimpkins
Copy link
Member Author

Composite glyphs with overlapping components are less common.

Letters with cedilla (ç Ç ş Ş) and with ogonek (Ą ą Ę ę) come to mind as possible common cases.

@miguelsousa Shall we open a new issue report to investigate this? I haven't looked into the compound glyph / bit10 setting issue and can't verify that the same rendering problem that we are addressing here exists.

@chrissimpkins
Copy link
Member Author

If we don’t know of a valid reason why the flag should not be set, then it is just over-engineering to make an option for it

Sounds good. Thanks.

@chrissimpkins
[varLib.mutator] refactor set bit 6 to value of 1 to use _g_l_y_f lib…
1da324a 
…rary flagOverlapSimple constant
@chrissimpkins
[varLib.mutator] convert to set OVERLAP_SIMPLE bit to 1 as default in…
bf5d5eb 
… instantiateVariableFont function

Also removes command line switch from varLib.mutator executable and eliminates unnecessary unit test. This new default behavior breaks the mutator unit tests
@chrissimpkins
[Tests] revise expected ttx files for varLib.mutator tests
e4c2e2e 
This revision was made as a result of the new default instantiateVariableFont behavior that sets OVERLAP_SIMPLE flag to 1.  This modifies the expected ttx XML to include overlap="1" setting on first point of contour definitions
@chrissimpkins
Copy link
Member Author

bf5d5eb : revision to set OVERLAP_SIMPLE bit to 1 by default in instantiateVariableFont function, removes varLib.mutator executable switch for bit 6 flag setting (these changes break existing mutator tests b/c expected ttx XML does not include the overlap="1" setting on the first point of glyph definitions)

e4c2e2e : fixes unit test expected ttx files with overlap="1" setting on first point of glyph contour in the XML according to new default behavior of instantiateVariableFont function

GTG on my end based upon requests for changes so far. Let me know if there is anything else that you need.

@anthrotype
Copy link
Member

Yeah, I think we should enable both OVERLAP_SIMPLE and OVERLAP_COMPOUND flags by default. They are supposed to force the rasterizer to use nonzero fill rule, which is the only one supported for variable fonts, hence should also be for the static instances derived from the latter.
I think the reason we didn’t do that earlier was that OpenType Sanitizer used to reject fonts with those bits set. But that’s no longer the case, so it’s safe to turn them on all the time, I believe.

@chrissimpkins
Copy link
Member Author
chrissimpkins commented Feb 25, 2019 •
edited
Loading

From Microsoft OT docs for Composite Glyph Flag OVERLAP_COMPOUND:

When used, it must be set on the flag word for the first component.

Something like this do that?

flagOverlapSimple = 0x40  # defined in ttLib.tables._g_l_y_f
OVERLAP_COMPOUND = 0x0400  # defined in ttLib.tables._g_l_y_f

for glyph_name in glyf.keys():
    glyph = glyf[glyph_name]
    
    if glyph.isComposite():
        glyph.components[0].flags |= OVERLAP_COMPOUND
    elif glyph.numberOfContours > 0:
        glyph.flags[0] |= flagOverlapSimple

@anthrotype
Copy link
Member

@chrissimpkins yes, that patch should work. I'm slightly annoyed by the stylistic difference (camelCase vs SNAKE_CASE) in the two constant names, but I guess it's too late to change that.

@chrissimpkins
update Mutator-Getvar-instance.ttx and Mutator_IUP-instance.ttx test …
135632f 
…files

this adds expected flags for new OVERLAP_COMPOUND (bit 10) set to 1 by default in the instantiateVariableFont function
@chrissimpkins
[varLib.mutator] add default support for setting OVERLAP_COMPOUND bit…
68949a4 
… to 1

adds this to instantiateVariableFont function
@chrissimpkins
Copy link
Member Author

Thanks Cosimo!

135632f modifies the expected flags in mutated ttx test files with composite glyphs (breaks tests)

68949a4 adds the source support to set OVERLAP_COMPOUND bit to 1. Tests pass.

Should be gtg. Let me know if there is anything else that you need here.

anthrotype
anthrotype approved these changes Feb 28, 2019
View reviewed changes
@chrissimpkins
Copy link
Member Author

@miguelsousa this PR now sets the OVERLAP_COMPOUND bit by default in the instantiateVariableFont function. Thanks for your feedback!

@chrissimpkins
Copy link
Member Author
chrissimpkins commented Feb 28, 2019 •
edited
Loading

@khaledhosny We now set OVERLAP_SIMPLE and OVERLAP_COMPOUND by default in instantiateVariableFont function as you recommended. The command line option was removed. For those who need to configure this for any reason, it is still possible through the new overlap parameter in the function that was introduced here. Thanks for weighing in.

@anthrotype anthrotype merged commit 16bb3fd into fonttools:master Feb 28, 2019
@anthrotype
Copy link
Member

thanks Chris!

@anthrotype
Copy link
Member

I added back a --no-overlap command line option for fonttools varLib.mutator script, it can be useful for debugging or testing different renderings with/without those flags
67d9830

@chrissimpkins chrissimpkins deleted the mutator-macos-overlap-flag branch February 28, 2019 17:51
@Lorp Lorp mentioned this pull request Jun 10, 2019
Open
@Lorp
Copy link
Lorp commented Jun 10, 2019

Note that woff2_compress currently sets this bit to zero. This is undesirable! See woff2 issue #20.

@moyogo moyogo mentioned this pull request Jul 8, 2020
Open
@jzmexec jzmexec mentioned this pull request Jan 19, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@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.
5 participants
@chrissimpkins @anthrotype @miguelsousa @khaledhosny @Lorp
0