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 translate anchors filter #514

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

simoncozens
Copy link
Contributor

This is another experimental filter.

Mark glyphs with anchor attachment points can kind of be positioned anywhere; they're going to be repositioned when the mark attachment is done. (There's an assumption here that you probably need to test - what if a mark glyph "gets through" without being attached to anything?)

Because of this, we can translate the whole glyph such that the position of their first attaching anchor (_whatever) is (0,0). If a lot of anchors end up being zero, they all end up sharing a very small Anchor subtable. This naturally leads to big space savings.

class OptimizeAnchorsFilter(TransformationsFilter):
def set_context(self, font, glyphSet):
# Skip over transformations filter to base filter
return super(TransformationsFilter, self).set_context(font, glyphSet)
Copy link
Member

Choose a reason for hiding this comment

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

better to explicitly call the BaseFilter.set_context(self, font, glyphSet) since that is what you are after.

@simoncozens
Copy link
Contributor Author

Note to self: the warn should be a debug

@behdad
Copy link
Collaborator

behdad commented Jul 15, 2021

There's an assumption here that you probably need to test - what if a mark glyph "gets through" without being attached to anything?

We definitely should not assume that by default. It's legit to leave marks to have correction placement for a bunch of base glyphs and hence not require attachment.

@behdad
Copy link
Collaborator

behdad commented Jul 15, 2021

In general, moving glyphs around like that is not something a build tool should do by default; and this particular one feels rather fragile to me.

@simoncozens
Copy link
Contributor Author

This does nothing by default. It’s a filter that’s activated via a UFO’s lib, which can also include/exclude specific glyphs.

@behdad
Copy link
Collaborator

behdad commented Jul 15, 2021

This does nothing by default. It’s a filter that’s activated via a UFO’s lib, which can also include/exclude specific glyphs.

I understand. Thanks.

return False
# Also skip over marks which are deliberately positioned over the
# previous glyphs
if glyph.getBounds().xMax < 0:
Copy link
Member

Choose a reason for hiding this comment

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

as a heuristic as to guess whether a mark was "deliberately positioned over the previous glyph", I think this is a bit too fragile. It's conceivable that the xMax still be >= 0 while the mark being intentionally designed to fall on top of another (e.g. narrow) base glyph

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.

given this is turned off by default and opt-in only, and given one can filter the glyphs to include/exclude if one wishes, I think it's ok to merge this in its current state.

@moyogo
Copy link
Collaborator

moyogo commented Jul 16, 2021

We should probably update TransformationsFilter at some point so it updates composite glyphs using transformed glyphs as components. Possibly with a new flag that could be enabled in OptimizeAnchorsFilter.

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.

4 participants