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

Recursive glyph references choke glyphsLib #375

Open
madig opened this issue Jun 19, 2018 · 29 comments
Open

Recursive glyph references choke glyphsLib #375

madig opened this issue Jun 19, 2018 · 29 comments

Comments

@madig
Copy link
Collaborator

madig commented Jun 19, 2018

(Filing this here to take it out of my inbox)

Jany found a file with the following properties:

  • parenleft is a glyph with outlines, and in the background it has bracketleft as a component
  • bracketleft is a glyph with outlines, and in the background it has parenleft as a component

The situation is valid but glyphsLib chokes on it (RecursionError: maximum recursion depth exceeded while calling a python object.) because for some reason it creates a component loop in the same layer... to investigate further

@belluzj
Copy link
Collaborator

belluzj commented Jun 19, 2018

It's a fringe use-case but from what I understand the problem is this: in Glyphs, when you put a component in the background, the component points to the foreground of the other glyph; while in UFOs, if you have a component in a layer (including the background layer), it points to the contents of the other glyph from the same layer. That's why it makes a reference loop in UFOs but works fine in Glyphs.

@anthrotype
Copy link
Member

Aargh.. yes, that’s how UFO3 layers are designed to work. could we detect this cases and decompose to avoid the recursion?

@moyogo
Copy link
Collaborator

moyogo commented Jun 19, 2018

Or we push for an update of the UFO spec: Components could have a baseLayer attribute to allow components from other layers #49.

@belluzj
Copy link
Collaborator

belluzj commented Jun 19, 2018

decompose to avoid the recursion?

Yes we could, but it loses the point of having those in Glyphs, which is to always have a synced visual of the parenleft in the background of the bracketleft for comparison purposes. We would have to "compose back" when going back to Glyphs to achieve something useful.

@schriftgestalt
Copy link
Collaborator

One way to avoid the problem a bit is to not export the background when called by fontmake. The ufos are only used to produce binary fonts and those do not use the background. So an option for the builder might help?

@madig
Copy link
Collaborator Author

madig commented Oct 24, 2018

I suppose that could in some instances also save time 🤔 I imagine that this might interfere with e.g. color fonts that need the layers, but I don't know if they are handled at all.

@schriftgestalt
Copy link
Collaborator

Color fonts have multiple layers, that has nothing to do with the background.

@madig
Copy link
Collaborator Author

madig commented Jun 6, 2019

Just hit the case where a glyph had a component referencing the glyph. glyphsLib should detect this and error out early.

@arrowtype
Copy link

This was a (temporary) blocker on Merriweather, this week.

Glyphs like /p, /d, /q, & /b were referencing one another in background layers. I had to dive into defcon code and put in some print() statements to figure out what kept crashing the build.

I see three options for how this might be handled:

  1. Make a check for this condition, and give the user a clearer error message so they can deal with it how they wish to. Something like, ERROR: Glyphs file cannot be converted to UFO due to recursive component references in background layer in glyphs /p /q /b /d /3 /8.
  2. Delete backgrounds and provide "INFO" message to let users know. (As @schriftgestalt says, this could happen specifically when FontMake is cueing the process. However, I imagine this error will happen in the conversion to UFO, whether or not FontMake is involved.
  3. Decompose the background layer and give "INFO" message to users. If we want to make it possible to go back from UFO → Glyphs, glyphsLib could store some custom data in the UFO glyph's lib. This could then be checked for on the way back in.

I'm just trying to contribute my thoughts here, but to me, option 3 seems best.

@schriftgestalt
Copy link
Collaborator

In Glyphs, components in the background reference the foreground in the other glyph. So those components are perfectly fine in Glyphs.

@arrowtype
Copy link

Right, the issue only comes from moving things over to UFO. So, that's a good point...

  1. The UFO format could be upgraded to make it possible to reference components between layers. I know it's a feature I'd probably use. :)

@madig
Copy link
Collaborator Author

madig commented Apr 24, 2020

I can't reproduce this anymore now with modern glyphsLib and fontmake, which uses ufoLib2 instead of defcon: Recursion.zip

Maybe I can still insert a check somewhere for loops.

@madig
Copy link
Collaborator Author

madig commented Apr 27, 2020

@schriftgestalt do component references always point to the foreground, regardless of which layer they are in? Or is the background layer treated specially?

@schriftgestalt
Copy link
Collaborator

schriftgestalt commented Apr 27, 2020

They always point to the foreground. Master layers pointing to the corresponding master layer in the other glyphs. None master layer try to find a layer with the same name in the other glyph. If that fails, they fall back to the associated master layer.

@madig
Copy link
Collaborator Author

madig commented May 1, 2020

@schriftgestalt ok thanks. what do master and non-master background layers point at?

@madig
Copy link
Collaborator Author

madig commented May 1, 2020

Also, do you mean names as in "layer.name" or layer IDs or something else?

Also also, does Glyphs do anything useful with brace/bracket layers containing components? Are there use cases for that?

@schriftgestalt
Copy link
Collaborator

ok thanks. what do master and non-master background layers point at?

Master layer alway point to the other master layer. Other layers point to layers with the same name (say bracket rules or color layers). It falls back to the master.

@madig
Copy link
Collaborator Author

madig commented May 1, 2020

This includes the date-stamped layer copies you can make with the "+", right?

@schriftgestalt
Copy link
Collaborator

Yes.

@madig
Copy link
Collaborator Author

madig commented May 4, 2020

@schriftgestalt So, my idea of bridging the UFO and Glyphs data model would be to unconditionally decompose all components in non-master layers. Do you know of a use case that would be prevented by this? Component spacing changes in bracket layers maybe?

@schriftgestalt
Copy link
Collaborator

Components in brace layers should not be decomposed because is messes with the interpolation.
For Color layers it would still work but greatly reduce the usefulness of the file when round tripping.
And the simple layer copies, in 99.9% of the cases they point to the default master layer as the layer name is quite unique (you don’t copy layer all at once). But those layers are not used for anything.
Bracket layers should keep components until they are moved to its own glyph. And there they can keep the components.
So there are not so many cases left that "need" decomposition.

@madig
Copy link
Collaborator Author

madig commented May 4, 2020

Hm. Well, this sucks a bit. There is a fundamental difference in UFO and Glyphs data models in that UFO components always point to the same layer and Glyphs components mostly point to the default layer. It's hard to reconcile this.

Maybe I'll limit myself to background layers for now, as those are where most of the problems turned out to be... @anthrotype ?

@schriftgestalt
Copy link
Collaborator

What happens in ufo, when there is no glyph in that layer the component points at?

@madig
Copy link
Collaborator Author

madig commented May 4, 2020

You get a dangling reference and possibly a compiler crash.

Also, what happens if some master layers are named the same, e.g. you duplicate the Regular twice but don't change the name yet?

@schriftgestalt
Copy link
Collaborator

Then it picks the first it finds. That is not really a problem as those layers are not relevant when producing fonts.

@madig
Copy link
Collaborator Author

madig commented May 4, 2020

Ok. What if two brace layers are called "{100,200}" and "{100, 200}" (note the space)?

@schriftgestalt
Copy link
Collaborator

This is a simple text comparison. So it will not pick the component form the other brace layer. But that is not a problem as the outlines are generated from an interpolation of that glyph and this will produce the shapes from the brace layer.

@madig
Copy link
Collaborator Author

madig commented May 4, 2020

Ok. Thanks for clearing that up :)

@madig
Copy link
Collaborator Author

madig commented May 4, 2020

Blargh. glyphsLib collates layers with the same name into one layer, Glyphs treats them as separate.

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

No branches or pull requests

6 participants