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

Conversation

belluzj
Copy link
Collaborator

@belluzj belluzj commented Nov 17, 2023

Glyphs.app allows brace layers with the same coordinates to have different associatedMasterId. The same situation is not allowed in Designspace + UFO (because you can't have 2 sources with the same location), so the idea for a fix is: for each brace layer location, select one UFO = designspace source that will receive all the brace layers for that location.

Related issue: #925

NOTE: the code is very ugly, I was just exploring ideas of how to fix this. I marked as Draft for now.

Example that works without this patch:

  • dollar
    • Regular
      • {401, 100}
    • Bold
  • cent
    • Regular
      • {401, 100} -- good because {401, 100} is always associated to Regular
    • Bold

Example that needs this patch to work:

  • dollar
    • Regular
      • {401, 100}
    • Bold
  • cent
    • Regular
    • Bold
      • {401, 100} -- bad because the brace layers for /dollar and /cent have the same coordinates {401, 100} but are associated to different masters

Comment on lines +176 to +178
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
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?

Comment on lines +179 to +181
# ... as they may need to be filled up with the values of the
# associated master.
master = self._sources[master_id]
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?

@anthrotype
Copy link
Member

for each brace layer location, select one UFO = designspace source that will receive all the brace layers for that location.

the proposed solution sounds ok to me, but maybe the PR needs some clean up work.
I'm also tempted to say this can be fixed in the sources relatively easily and I don't see a strong need to support the use case of defining intermediate layers with same location but associated to different masters in different glyphs, I understand that Glyphs.app UI doesn't prevent that and the designer can easily create those unknowingly but I doubt that they want to deliberately do that.

@belluzj
Copy link
Collaborator Author

belluzj commented Nov 17, 2023

Discussing this with @schriftgestalt , he suggests that the default behaviour of glyphsLib could be cleaner on the UFO side (as there's not a problem anymore of "under which full UFO to put the sparse layer". However this is harder to round-trip because it loses the association to a specific master on the Glyphs.app side

@belluzj
Copy link
Collaborator Author

belluzj commented Nov 17, 2023

Georg adds that merging his Glyphs3 branch should probably be a reason to bump the major version number, and that could also be an opportunity to change the default output for brace layers from layers to standalone UFOs.

One question with standalone is how to choose the filename of the UFOs, and it could be some combo of axis tags + values joined by underscores.

@anthrotype
Copy link
Member

I disagree that using standalone UFOs instead of UFO layers referenced from DS source via layer attribute is a solution here.
Standalone UFOs are currently treated like full masters that have their own fontinfo and kerning dictionaries, unlike brace/intermediate layers which should only contribute to variate glyph outlines and metrics. We added DS source.layerName exactly to support glyphs brace layers!

@schriftgestalt
Copy link
Collaborator

But all projects (that I have seen recently) that are build as designspace/ufo (not in Glyphs) have the sparse masters in separate .ufos (with or without extra font info/kerning). So it would be good to being able to read/write those designspace files without massively changing the file structure. Extra ufos makes it easier to edit/audit them.

And the vertical metrics defined in the full/"parent" source are most likely not going to match the outlines in the brace layers, because those need to be interpolated from the surrounding sources.

@anthrotype
Copy link
Member

Extra ufos makes it easier to edit/audit them.

why?

And the vertical metrics defined in the full/"parent" source are most likely not going to match the outlines in the brace layers

the sparse UFO layers don't have define any global metrics of their own, their parent UFO does, so they don't contribute to the interpolation of vertical metrics, similarly to the Glyphs.app's brace/intermediate layers themselves, which only contribute glyph outlines and glyph advances.

@schriftgestalt
Copy link
Collaborator

Extra ufos makes it easier to edit/audit them.

why?

because you can open them in any ufo editor and just edit the included glyphs. Otherwise you need to hunt down the layers of the other glyphs?

@anthrotype
Copy link
Member

UFO editors (Robofont) should allow to select the layer you want to edit

@schriftgestalt
Copy link
Collaborator

But does it show what glyphs have multiple layers or lets filter for glyphs that have?

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.

3 participants