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

Allow brace layers with the same coordinates to have different associatedMasterId #956

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
42 changes: 41 additions & 1 deletion Lib/glyphsLib/builder/builders.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@


from collections import OrderedDict, defaultdict
import collections
import os
from textwrap import dedent
from typing import Dict
Expand Down Expand Up @@ -113,6 +114,43 @@ def __init__(
# indexed by master ID, the same order as masters in the source GSFont.
self._sources = OrderedDict()

# Where to actually put brace layers
# (key, associatedMasterId) -> masterId
self._where_to_put_brace_layers = {}
# To construct a source layer, we need
# 1. The Designspace source filename and font object which holds the layer.
# 2. The (brace) layer name itself.
# 3. The location of the intermediate master in the design space.
# (For logging purposes, it's nice to know which glyphs contain the layer.)
#
# Note that a brace layer can be associated with different master layers (e.g. the
# 'a' can have a '{400}' brace layer associated with 'Thin', and 'b''s can be
# associte with 'Black').
# Also note that if a brace layer name has less values than there are axes, they
# are supposed to take on the values from the associated master as the missing
# values.
# First, collect all brace layers in the font and which glyphs and which masters
# they belong to.
layer_to_master_ids = collections.defaultdict(set)
for glyph in self.font.glyphs:
for layer in glyph.layers:
if layer._is_brace_layer():
key = (layer._brace_layer_name(), tuple(layer._brace_coordinates()))
layer_to_master_ids[key].add(layer.associatedMasterId)

# Next, insert the brace layers in a defined location in the existing designspace.
for key, master_ids in layer_to_master_ids.items():
# In a designspace, we can't create the brace layers with the same
# location under different UFOs because that would lead to several
# <source> elements with the same location. Instead we'll pick the first
# (after sorting) master and stick all the brace layers with the same
# key = location there.
# Redirect other masters who have brace layers at the same location to
# the "chosen" one.
master_id = sorted(master_ids)[0]
for other_master_id in master_ids:
self._where_to_put_brace_layers[key, other_master_id] = master_id

# Map Glyphs layer IDs to UFO layer names.
self._layer_map = {}

Expand Down Expand Up @@ -318,7 +356,9 @@ def to_ufo_layers(self):
):
continue
else:
ufo_layer = self.to_ufo_layer(glyph, layer) # .layers
# Brace layer
key = (layer._brace_layer_name(), tuple(layer._brace_coordinates()))
ufo_layer = self.to_ufo_layer(glyph, layer, masterId=self._where_to_put_brace_layers[key, layer.associatedMasterId]) # .layers
ufo_glyph = ufo_layer.newGlyph(glyph.name)
self.to_ufo_glyph(ufo_glyph, layer, layer.parent) # .glyph

Expand Down
4 changes: 2 additions & 2 deletions Lib/glyphsLib/builder/layers.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ def to_ufo_color_layer_names(self, master, ufo):
]


def to_ufo_layer(self, glyph, layer):
ufo_font = self._sources[layer.associatedMasterId or layer.layerId].font
def to_ufo_layer(self, glyph, layer, masterId=None):
ufo_font = self._sources[masterId or layer.associatedMasterId or layer.layerId].font

layer_name = layer.name
# Give color layers better names
Expand Down
73 changes: 41 additions & 32 deletions Lib/glyphsLib/builder/sources.py
Original file line number Diff line number Diff line change
Expand Up @@ -166,40 +166,49 @@ def _to_designspace_source_layer(self):
for key, master_ids in layer_to_master_ids.items():
brace_coordinates = list(key[1])
layer_name = key[0]
for master_id in master_ids:
# ... as they may need to be filled up with the values of the associated
# master.
master = self._sources[master_id]
master_coordinates = brace_coordinates
if len(master_coordinates) < len(designspace.axes):
master_locations = [master.location[a.name] for a in designspace.axes]
master_coordinates = (
brace_coordinates + master_locations[len(brace_coordinates) :]
)
elif len(master_coordinates) > len(designspace.axes):
logger.warning(
"Glyph(s) %s, brace layer '%s' defines more locations than "
"there are design axes.",
layer_to_glyph_names[key],
layer_name,
)

# If we have more locations than axes, ignore the extra locations.
layer_coordinates_mapping = collections.OrderedDict(
(axis.name, location)
for axis, location in zip(designspace.axes, master_coordinates)
# In a designspace, we can't create the brace layers with the same
# location under different UFOs because that would lead to several
# <source> elements with the same location. Instead we'll pick the first
# (after sorting) master and stick all the brace layers with the same
# key = location there.
# Redirect other masters who have brace layers at the same location to
# the "chosen" one.
master_id = sorted(master_ids)[0]
for other_master_id in master_ids:
self._where_to_put_brace_layers[key, other_master_id] = master_id
Comment on lines +176 to +178
Copy link
Member

Choose a reason for hiding this comment

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

looks like you are doing this twice (also in builder/builders.py:150-152)? why?

# ... as they may need to be filled up with the values of the
# associated master.
master = self._sources[master_id]
Comment on lines +179 to +181
Copy link
Member

Choose a reason for hiding this comment

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

are you sure you're filling up the values from the actually associated master and not from the first one that you picked when multiple ones are available?

master_coordinates = brace_coordinates
if len(master_coordinates) < len(designspace.axes):
master_locations = [master.location[a.name] for a in designspace.axes]
master_coordinates = (
brace_coordinates + master_locations[len(brace_coordinates) :]
)
elif len(master_coordinates) > len(designspace.axes):
logger.warning(
"Glyph(s) %s, brace layer '%s' defines more locations than "
"there are design axes.",
layer_to_glyph_names[key],
layer_name,
)

s = fontTools.designspaceLib.SourceDescriptor()
s.filename = master.filename
s.font = master.font
s.layerName = layer_name
s.name = f"{master.name} {layer_name}"
s.location = layer_coordinates_mapping

# We collect all generated SourceDescriptors first, grouped by the masters
# they belong to, so we can insert them in a defined order in the next step.
layers_to_insert[master_id].append(s)
# If we have more locations than axes, ignore the extra locations.
layer_coordinates_mapping = collections.OrderedDict(
(axis.name, location)
for axis, location in zip(designspace.axes, master_coordinates)
)

s = fontTools.designspaceLib.SourceDescriptor()
s.filename = master.filename
s.font = master.font
s.layerName = layer_name
s.name = f"{master.name} {layer_name}"
s.location = layer_coordinates_mapping

# We collect all generated SourceDescriptors first, grouped by the masters
# they belong to, so we can insert them in a defined order in the next step.
layers_to_insert[master_id].append(s)

# Splice brace layers into the appropriate location after their master.
for master_id, brace_layers in layers_to_insert.items():
Expand Down
Loading