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

Go halfway back to the old kern writer #858

Closed
wants to merge 5 commits into from
Closed

Conversation

madig
Copy link
Collaborator

@madig madig commented Jul 11, 2024

Take the new kern writer and rework it to split by direction again, like the old writer. Replace the old writer.

Internal testing showed that the new writer likes to stick together a lot of scripts in fonts that employ cross-script kerning for ease of design, negating the advantages of the script splitter. We had the idea of reworking the splitter to put things into the same lookup instead and insert subtable breaks by script, but then we found that repacking the kerning generated by the old writer with Harfbuzz and then using the GPOS compression in fonttools actually gets us back to the small sizes by the original splitter at reasonable compile times. Maybe there is no need to try and be smarter.

We decided to propose going back to the old writer that splits by direction (plus the detail enhancements the new one got) and use GPOS compression instead for releasing fonts. The resulting kerning will work in InDesign with the default composer and in other apps that allow cross-script kerning. The difference is that it will always work, not just when the font has cross-script kerning between the desired scripts.

I took the opportunity to refactor the code a bit, by copy-pasting and adapting the new writer piece by piece and getting the important business logic in a straight line. The writer has been validated with this branch of kerning-validator, which was modified to generate all glyph pairs that are allowed by (my understanding of the) BiDi logic to be in the same run.

I rewrote the tests to use syrupy for snapshot testing, because it made it easier to update tests after changing the writer. I also ported all the tests to use plain pytest conventions instead of unittest.

TODO:

  • How important is it to have camelCase instead of snake_case? Not important for internal code.
  • Potentially some TODO or XXX cleanups I need to do another pass over.

@madig
Copy link
Collaborator Author

madig commented Aug 6, 2024

Cosimo suggests replacing the old writer with the new old writer but leave the default writer alone.

@madig madig marked this pull request as ready for review August 22, 2024 14:37
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.

a lot of these new private functions like extract_kerning_data, get_kerning_groups etc. could be shared by the two kernFeatureWriters, I wish we didn't have all this code duplication (the duplication was there already I know).

Regarding the use of syrupy, I don't know if I like the fact that now the expected FEA snippets are no longer embedded in the test itself.. but if it helps with updating them when code changes, I may live with that. Ideally both writers would use a similar testing infrastructure (though probably out of scope for this PR).

Lib/ufo2ft/featureWriters/kernFeatureWriter2.py Outdated Show resolved Hide resolved
@anthrotype
Copy link
Member

Take the new kern writer and rework it to split by direction again

could you summarize what actually changed in the resulting generated kern feature compared to the old kernFeatureWriter2.py?

@madig
Copy link
Collaborator Author

madig commented Sep 3, 2024

could you summarize what actually changed in the resulting generated kern feature compared to the old kernFeatureWriter2.py?

I think the primary difference is that the kern feature is always structured like

script
languages
lookups

whereas the old writer would try to leave out script statements if it could. The lookups should be about the same, with some detail improvements that landed in the newer one like #720.

@anthrotype
Copy link
Member

the fact that the tests were overhauled doesn't make that super clear, I wish you had split the two changes

@madig
Copy link
Collaborator Author

madig commented Sep 3, 2024

You mean keep the old test structure but insert new results, and then overhaul it? I could spend some time on that, but not sure if this week.

@anthrotype
Copy link
Member

You mean keep the old test structure but insert new results, and then overhaul it?

maybe the reverse (change the tests infra first, then change the code, let the tests fail so we can actually see what changed, then update the test expectations), or whichever is easier. As long as the two sets of changes (to the tests and the lib itself) are kept separate

madig added a commit that referenced this pull request Sep 5, 2024
@madig
Copy link
Collaborator Author

madig commented Sep 5, 2024

Continued in #870.

@madig madig closed this Sep 5, 2024
@madig madig deleted the rewrite-kern-writer branch September 5, 2024 08:42
madig added a commit that referenced this pull request Sep 5, 2024
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.

2 participants