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

Further improve vertical typesetting #639

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

Further improve vertical typesetting #639

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2c62ade
Add additional check for presence of vertical-specific OT features
aaronbell Nov 19, 2020
7821ad0
Update glyph.py
aaronbell Nov 19, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Lib/glyphsLib/builder/builders.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,9 @@ def __init__(
self._do_filter_instances_by_family = True

def _is_vertical(self):
for feature in self.font.features:
if feature.name in ["vert", "vrt2", "vkna"]:
return True
master_ids = {m.id for m in self.font.masters}
for glyph in self.font.glyphs:
for layer in glyph.layers:
Expand Down
6 changes: 4 additions & 2 deletions Lib/glyphsLib/builder/glyph.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ def to_ufo_glyph(self, ufo_glyph, layer, glyph): # noqa: C901
self.to_ufo_components(ufo_glyph, layer)
self.to_ufo_glyph_anchors(ufo_glyph, layer.anchors)
if self.is_vertical:
self.to_ufo_glyph_height_and_vertical_origin(ufo_glyph, layer)
self.to_ufo_glyph_height_and_vertical_origin(ufo_font, ufo_glyph, layer)


def to_glyphs_glyph(self, ufo_glyph, ufo_layer, master): # noqa: C901
Expand Down Expand Up @@ -261,7 +261,7 @@ def to_glyphs_glyph(self, ufo_glyph, ufo_layer, master): # noqa: C901
self.to_glyphs_glyph_height_and_vertical_origin(ufo_glyph, master, layer)


def to_ufo_glyph_height_and_vertical_origin(self, ufo_glyph, layer):
def to_ufo_glyph_height_and_vertical_origin(self, ufo_font, ufo_glyph, layer):
# implentation based on:
# https://github.com/googlefonts/glyphsLib/issues/557#issuecomment-667074856
assert self.is_vertical
Expand All @@ -270,6 +270,8 @@ def to_ufo_glyph_height_and_vertical_origin(self, ufo_glyph, layer):

if layer.vertWidth:
ufo_glyph.height = layer.vertWidth
elif "rotat" in ufo_glyph.name:
Copy link
Member

Choose a reason for hiding this comment

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

I've never heard of this "rotat" glyph name suffix. Is that your particular glyph naming scheme, or some new magic feature in Glyphs.app? I sincerely hope that's not the latter.

Copy link
Author

Choose a reason for hiding this comment

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

From what I can tell, it is a common suffix for Japanese development using the AFDKO (Fontworks has it in its AFDKO sources). Yes, it does have 'magic feature' in Glyphs, but I do not believe it is exclusive to Glyphs.

Copy link
Author

Choose a reason for hiding this comment

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

Furthermore, given that I've been reviewing fonts from 17 different foundries, and they have all used it when indicating rotated forms gives me confidence that it can be explicitly used :).

Copy link
Member

Choose a reason for hiding this comment

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

can't the font developer simply set the vertWidth explicitly for these glyphs instead of relying on a magic suffix?

Copy link
Author

@aaronbell aaronbell Nov 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I've provided you an example of a font that doesn't have vertWidth set, that clearly wants the font to be used for vertical typesetting, and has followed (what appears to be) a common convention for glyph naming in the Japanese type design industry.

All this code does is provide a safety net in case a designer does not set that field. It is really no different than setting it to ascender - descender. Do you deliberately want them to produce bad fonts?

Copy link
Member

Choose a reason for hiding this comment

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

is this documented anywhere? How can we glyphsLib devs know that? The job of the tool is to try mimic what Glyphs.app does. If we can get confirmation from some authoritative source that that's indeed the case, we will certainly do what needs to be done (even though I do confess I personally dislike this kind of automatic features).

Copy link
Author

Choose a reason for hiding this comment

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

TBH, I see this more as a fallback safety net than an automatic feature that someone would want to use—it'll always be better to set these values manually.

As for the documentation, I haven't seen any but what I have found in my own testing. One thing I did locate was this comment which seems to indicate the behavior I suggest:

If the glyph has a suffix “.rotat”, “.vert” or “.vrt2” and contains a component that is rotated by 90° the automatic alignment will position the component and set the vertWidth correctly.

https://forum.glyphsapp.com/t/how-to-make-rotat-glyphs-in-japanese-fonts/13014/4

But probably best to wait for Georg to confirm.

ufo_glyph.height = ufo_font[ufo_glyph.name.replace(".rotat", "")].width
else:
ufo_glyph.height = ascender - descender

Expand Down