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.

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

ccmp, locl, mark, mkmk, and gdef features #441

Merged
merged 21 commits into from
Mar 12, 2021
Merged

ccmp, locl, mark, mkmk, and gdef features #441

merged 21 commits into from
Mar 12, 2021

Conversation

benkiel
Copy link
Collaborator

@benkiel benkiel commented Feb 26, 2021 •
edited
Loading

ccmp and locl feature for all fonts.

Builds a mark, mkmk, and gdef feature for static fonts (currently those features are built by fontmake for the variable font (see #440). This may mean that marks behave differently between the static and variable font.

Some code fixups and style fixups also happened in this branch. Beta fonts for testing in /fonts

This closes #217, closes #367, closes #368, and closes #440.

benkiel and others added 20 commits February 18, 2021 21:12
@benkiel
WIP ccmp and locl feature
b5cd9c7
@arrowtype
Add WIP IJ implmentation for Dutch to locl & ccmp feature
7cb331e
@benkiel
Merge commit '59b61c05ccb03f0b1ad9c7ebf0dd2d2a87f60446' into ccmp_loc…
811d51c 
…l_mark_feature

* commit '59b61c05ccb03f0b1ad9c7ebf0dd2d2a87f60446':
  add code fonts with fixes to #418
@benkiel
Merge commit '7cb331eea6273b3effb323e4db59010d7ae5f266' into ccmp_loc…
e47e612 
…l_mark_feature

* commit '7cb331eea6273b3effb323e4db59010d7ae5f266':
  Add WIP IJ implmentation for Dutch to locl & ccmp feature

# Conflicts:
#	src/features/features/ccmp.fea
#	src/features/features/locl.fea
@benkiel
locl feature done. update ufo for correct feature order.
e86d287
@benkiel
ccmp feature, catch getting features correct in Mono masters
918fc39
@benkiel
Tweak locl feature, shouldn't be swapping those glyphs for vietnamese
6eaabb0
@benkiel
Revert "Tweak locl feature, shouldn't be swapping those glyphs for vi…
e0d197c 
…etnamese"

This reverts commit 6eaabb0.
@benkiel
Function to write out mark, mkmk, and gdef
5180724
@benkiel
Wire in the new mark/mkmk/gdef feature writer to the static fonts. St…
7c072bb 
…yle cleanup.
@benkiel
Style cleanup.
dd46ab0
@benkiel
Wire in adding the mark/mkmk/gdef to the variable font sources
6de3f96
@benkiel
Fix syntax
a0dc95b
@benkiel
Make the indentation of the GDEF table look prettier
248437c
@benkiel
Fixup for changes in the ufo2ft writeKerning method
39b13ba
@benkiel
Missing by's in ccmp
67cff52
@benkiel
Fix for locl feature
975e9fe
@benkiel
Remove mark/mkmk/gdef feature writing from the Var font build. fontma…
cbe2d08 
…ke will build these.
@benkiel
Cleanup
04bd610
@benkiel
Built fonts
a64a737
@benkiel benkiel marked this pull request as draft March 2, 2021 16:34
@benkiel benkiel marked this pull request as ready for review March 3, 2021 02:20
@arrowtype
Copy link
Owner

Thank you, Ben! This is an awesome upgrade.

In my initial testing, I’ve verified a few things are working better than before:

  • Combining accents are working mostly very well!
  • I’m especially excited that the .case versions of combining accents are being attached to uppercase glyphs, while normal accents are attaching to lowercase glyphs.
  • Localization features like a Dutch ij seem to be working well.
  • Mark-to-mark attachment seems to be working well, e.g. n + macroncomb + caroncomb stacks.

Things I’ve noticed that (I think) I can easily fix on my own, after merging:

  • The R̊ isn’t yet working, but I think this is a simple fix of add a _top anchor to the rincomb and ringcomb.case in the sources.
  • The K̇ is a bit off in the slanted styles, but this is definitely just a simple fix of moving anchors in the slanted sources.

One open question

In the static fonts, combining accents don’t yet seem to be working for feature-activated glyphs such as a.italic. (These are working well in the variable font.) For example, ǎ n̄ are failing in Sans & Mono statics, while r̊ is failing in Mono statics. This is the case whether these are set up via rvrn or applied via features like ss01. I am pretty sure I’ll merge this and make a new release, because the variable font seems to be working perfectly, and the statics are like 80% better. Also, this might be an upstream issue with the ufo2ft feature generator. But, if we were to fix the statics, would you be willing to speculate how we might do this?

Screenshot of testing

I tested this with FontGoggles.

The top five rows are statics of Version 1.075 (from this PR), the sixth row is the variable 1.075 (also from this PR), and the bottom two are version 1.074 (current/previous).

image

@arrowtype
Copy link
Owner

arrowtype commented Mar 4, 2021 •
edited
Loading

Additional follow-up question

There are still a few gaps before this can fully close #367. Characters with one mark attached, like R̥ r̥, are working, but characters with two attached marks, like Ḹ ḹ are not – even if those combining marks are attaching as expected, individually, (like L̄ Ḷ).

I’m slightly unsure whether this would use the mark feature or the mkmk feature. I’m guessing it might require some custom Python to fully solve this issue, rather than using the ufo2ft feature writer?

image

@arrowtype
Copy link
Owner

Ultimately, I think we should merge this. I think it nails issues #217, #368, and #440, but doesn’t yet close #367.

It would be great to chat (either here or elsewhere) about what it might take to completely close #367.

I’ll merge this and close those issues later today, unless @benkiel prefers something else.

@benkiel
Copy link
Collaborator Author

benkiel commented Mar 5, 2021 •
edited
Loading

but characters with two attached marks, like Ḹ ḹ are not – even if those combining marks are attaching as expected, individually

I wonder if this is because those glyphs are getting transformed to their Ldotbelow forms, which don't have anchors...

I'm digging into this now. Something is amiss in the static italics for sure.

@benkiel
Copy link
Collaborator Author

benkiel commented Mar 5, 2021

The R̊ isn’t yet working, but I think this is a simple fix of add a _top anchor to the rincomb and ringcomb.case in the sources.

ringcomb.case is missing the _top anchor, it is there in the ringcomb, fyi. That's why it works in lc but not UC

@arrowtype
Copy link
Owner

Thanks for specifying the exact cause of the R-ring issue! I’ve added the the _top anchor to ringcomb.case in the main branch.

Because this PR only has improvements and no regressions that I know of, I’ll merge it, and close the issues it closes, and we can separate plan out how to close any remaining issues.

Thanks for all the really good work here!

@arrowtype arrowtype merged commit ae81a66 into main Mar 12, 2021
@arrowtype arrowtype mentioned this pull request Mar 12, 2021
Merged
arrowtype added a commit that referenced this pull request Mar 12, 2021
@arrowtype
add "_top" anchor to ringcomb.case (see PR #441); bork UFO normalization
9e05a08
@arrowtype
Copy link
Owner

  • To fix the Ṝ ṝ Ḹ ḹ issue, I will add top anchors to R dot below etc
    • When I do, I’ll add the R dot below and L dot below to the base_uc class in src/features/features/ccmp.fea
    • We will also need to update mastering/utils.py

@arrowtype arrowtype mentioned this pull request Mar 12, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@arrowtype arrowtype Awaiting requested review from arrowtype

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@benkiel @arrowtype