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

Add a phase that eliminates mixed contour+component glyphs; legal in source but not binary #123

Merged
merged 8 commits into from
Mar 2, 2023

Conversation

rsheeter
Copy link
Contributor

@rsheeter rsheeter commented Feb 15, 2023

Split IR glyphs with contours and components - observed in Lexend - apart such that a glyph with both migrates its contours into a new glyph then references that glyph as a component.

Update the ACL rigging to accomodate; we need the ability to express the ability to write to any glyph including those we've never seen before because we're creating them.

Fixes #125 by converting glyphs with different component definitions at different positions in designspace to simple glyphs.

Split out from #120 to make review more tractable.

The most interesting changes are in fontir/src/glyph.rs.

This can generate more glyphs and update static metadata.
@rsheeter rsheeter marked this pull request as ready for review February 15, 2023 05:55
@anthrotype
Copy link
Member

Split IR glyphs with contours and components

Currently, the expectation from font developers for such mixed contour-component glyphs (based on established font editors' behavior, and followed by fontmake itself) is that they should get decomposed to simple glyphs, but it looks like you went the other way around and decided to upgrade them to full composite glyphs (by pushing the contours to separate glyphs and compositing from those).
This does not mean we have to continue to do so. Besides, it seems you have put a great deal of work on this already. Was your thinking that by doing so it increases the chance of overall size savings? Could/should this sort of optimization be left to a separate (optional) step that applies to all glyphs maybe (e.g. #102)?

@rsheeter
Copy link
Contributor Author

Tbh I was not aware that the font developer would expect that; we don't have to do it this way if it's important. I was indeed imagining this might contribute to size savings.

@anthrotype
Copy link
Member

well yes, I'm quite sure that's what all the font editors do in this situation (decompose mixed glyphs)

@rsheeter
Copy link
Contributor Author

@anthrotype pointed out this approach consumes additional gids whereas reducing a path + components to a single simple glyph does not.

@rsheeter
Copy link
Contributor Author

Per IM will update to match behavior of existing compilers. Conveniently that's actually simpler.

@rsheeter
Copy link
Contributor Author

rsheeter commented Feb 15, 2023

@anthrotype notes in ufo2ft the recursive function that accomplishes that task is called deepCopyContours

@rsheeter
Copy link
Contributor Author

Another option might be to keep the behavior but off by default

@rsheeter
Copy link
Contributor Author

@khaledhosny mentioned FontForge defaults to converting to simple glyphs but also has a script command to do what my initial implementation here does.

@rsheeter rsheeter changed the title Add a phase that updates static metadata based on IR glyphs Add a phase that eliminates mixed contour+component glyphs; legal in source but not binary Feb 28, 2023
@rsheeter
Copy link
Contributor Author

rsheeter commented Mar 1, 2023

Currently, the expectation from font developers for such mixed contour-component glyphs (based on established font editors' behavior, and followed by fontmake itself) is that they should get decomposed to simple glyphs

This is now the default behavior but can be flagged off.

…tmake-rs using 0.0.6 is deemed incompatible because the leftmost non-zero of the version changed
Copy link
Member

@dfrg dfrg left a comment

Choose a reason for hiding this comment

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

Looks good! Functionality seems solid. Left some suggestions inline for style, readability and comments.

fontbe/src/orchestration.rs Outdated Show resolved Hide resolved
fontbe/src/orchestration.rs Outdated Show resolved Hide resolved
fontc/src/main.rs Outdated Show resolved Hide resolved
fontc/src/main.rs Outdated Show resolved Hide resolved
fontc/src/main.rs Show resolved Hide resolved
fontdrasil/src/orchestration.rs Outdated Show resolved Hide resolved
fontdrasil/src/orchestration.rs Outdated Show resolved Hide resolved
fontir/src/glyph.rs Outdated Show resolved Hide resolved
fontir/src/glyph.rs Show resolved Hide resolved
fontir/src/orchestration.rs Outdated Show resolved Hide resolved
@rsheeter rsheeter merged commit 9c961da into main Mar 2, 2023
@rsheeter rsheeter deleted the static_meta branch March 2, 2023 23:08
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.

Support components whose transforms vary across designspace
3 participants