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

Don't skip over spacing marks when kerning #720

Merged
merged 3 commits into from
Mar 3, 2023

Conversation

simoncozens
Copy link
Contributor

Consider the following font:

Screenshot 2023-03-03 at 09 24 18

We want to kern between ka-deva and ra-deva:

Screenshot 2023-03-03 at 09 24 55

But (to do certain bits of OpenType magic) we make highspacingdot-deva a "spacing combining" mark (spacing mark). This is so we can filter it out when doing conjuncts or something like that. (This is slightly contrived, but an analogous situation does exist for many Indic scripts.)

We also want kerning between ka-deva and the mark, so we make a base-mark kern:

Screenshot 2023-03-03 at 09 27 25

But because it's a mark class in GDEF, when we apply base-base kerning, this gets skipped. So with a sequence like ka-deva|highspacingdot-deva|ra-deva, the result, with current ufo2ft, is:

  • kern_Deva will IgnoreMarks and kern ka-deva|ra-deva -250
  • kern_Deva_marks will kern ka-deva|highspacingdot-deva -150.

shape

Instead what we need to do is, rather than filtering out all marks, only filter out non-spacing marks, allowing spacing marks to break the base-base kern, so that only base-mark kerning is applied:

shape

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.

LGTM, thanks!

@simoncozens simoncozens merged commit 87c6881 into main Mar 3, 2023
spacing = []
if marks:
spacing = [mark for mark in marks if self.context.font[mark].width != 0]
if not spacing:
Copy link
Collaborator

Choose a reason for hiding this comment

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

How does this work? Many font sources have none spacing marks with +ve advances and rely on the compiler to zero them, would they be considered spacing marks in this context? Or the zeroing would have happened already at this point (but this cide would be noop then)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If this depends on Glyphs’s “Spacing Combining” sub category, then it probably should have checked for this, checking for glyph width feels like the kind of indirection that comes back to bite you in the rear.

Copy link
Member

Choose a reason for hiding this comment

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

Only non-spacing combining marks are zeroed by the compiler, spacing ones are not and as such have a non zero advance width.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But spacing combining marks do not exist in OpenType nor AFDKO feature files, only in Glyphs, so what happens to sources not coming from Glyphs that happen to have marks with advance widths (which is harmless as the layout engine will be zerowing them for most scripts)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If your sources say that a glyph has advance width, you should not be surprised when ufo2ft treats it as a glyph with advance width.

(And remember, the change here only affects the very specific case of ignoring marks in kerning between two bases. It doesn't change anything that's already happening or not happening with regard to mark width zeroing.)

Copy link
Member

Choose a reason for hiding this comment

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

spacing combining marks do not exist in OpenType nor AFDKO feature files, only in Glyphs

well, "spacing combining marks" do exist in unicode, general category "Mc", don't they?

Copy link
Collaborator

@khaledhosny khaledhosny Mar 3, 2023

Choose a reason for hiding this comment

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

If your sources say that a glyph has advance width, you should not be surprised when ufo2ft treats it as a glyph with advance width.

They have the right glyph class, the fact that they have an advance width is irrelevant as the layout engine will zero them when shaping.

Copy link
Collaborator

Choose a reason for hiding this comment

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

spacing combining marks do not exist in OpenType nor AFDKO feature files, only in Glyphs

well, "spacing combining marks" do exist in unicode, general category "Mc", don't they?

But we are not using that here, and it is also irrelevant as OpenType knows only about only one class of mark glyphs and the font sets the glyph class, but we are second guessing its intentions based on a tangentially relevant heuristic. The whole concept of feature writers is broken beyond repair, but adding more surprising exceptions and heuristics tuned for individual fonts is not helping.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Here is a more real example. Some monospace fonts, e.g. DejaVu Sans Mono, give combining marks an advance width (so that legacy systems that require all glyphs in monospace fonts to have the same adcance width would still recognize them as monospace) then zeros the width using GPOS single positioning lookups. Monospace fonts obviously don't need kerning, but this shows a combining mark can have an advance width while not being a spacing combining one.

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

Successfully merging this pull request may close these issues.

3 participants