8000 [varLib] Implement passing the layer name to MasterFinder by madig · Pull Request #1387 · fonttools/fonttools · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[varLib] Implement passing the layer name to MasterFinder #1387

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[varLib] Implement passing the layer name to MasterFinder #1387

wants to merge 1 commit into from

Conversation

madig
Copy link
Collaborator
@madig madig commented Nov 23, 2018

This is preparatory work for googlefonts/ufo2ft#295.

The idea is that ufo2ft should be able to produce sparse intermediate master OT binaries from ("brace") layers that varLib can then merge into a variable font, thanks to Behdad's sparse master work. varLib needs a way to associate a sparse "source" with an OT binary, so this PR adds passing the layer name, if existant, to MasterFinder.

To test-drive this, get smastertest.zip, extract it somewhere and do in that folder:

$ python3 -m venv venv && . venv/bin/activate
$ pip install fonttools[ufo]  # to pull in ufoLib dependencies
$ pip install git+https://github.com/madig/fonttools.git@varLib-MasterFinder-add-layerName git+https://github.com/madig/ufo2ft.git@sparse-master-by-layerName
$ python build.py

Side-thought: in the spirit of avoiding assigning None to anything, maybe the layer_name argument should default to "public.default" instead?

@madig madig changed the title Implement passing the layer name to MasterFinder [varLib] Implement passing the layer name to MasterFinder Nov 23, 2018
@behdad
Copy link
Member
behdad commented Nov 23, 2018

Side-thought: in the spirit of avoiding assigning None to anything, maybe the layer_name argument should default to "public.default" instead?

Makes sense to me.

Also, peple use the varLib.build() API outside fonttools already. I think we should leave that as is and add new entry point for layers. Otherwise it's an API break.

@madig
Copy link
Collaborator Author
madig commented Nov 23, 2018 •
edited
Loading

Side-thought: in the spirit of avoiding assigning None to anything, maybe the layer_name argument should default to "public.default" instead?

Makes sense to me.

On a second thought, this is putting UFO semantics into varLib. Not sure this is a good idea 🤔

Edit: @anthrotype just contacted me and said that he'd like to ponder the mapping of source to master OT binary some more (maybe map things explicitly in the designspace document?). So I suppose this PR is on a hold until we come to a conclusion.

@behdad
Copy link
Member
behdad commented Nov 23, 2018

On a second thought, this is putting UFO semantics into varLib. Not sure this is a good idea thinking

That also makes sense to me :D.

Edit: @anthrotype just contacted me and said that he'd like to ponder the mapping of source to master OT binary some more (maybe map things explicitly in the designspace document?). So I suppose this PR is on a hold until 8000 we come to a conclusion.

Yeah, sounds a bit hacky here.

That said, here's another suggestion'ish. Use CSS-like filename#subtag to refer to layers / TTC indices.

@madig
Copy link
Collaborator Author
madig commented Nov 23, 2018

I.e. use a # instead of @? I think Cosimo is unhappy that this approach is adding finder magic when maybe the .designspace should contain an explicit mapping. Although f"{pathlib.Path(source.filename).stem}#{userNameToFileName(source.layerName)}" is.. ok I guess? Not sure what a better option would look like...

@madig
Copy link
Collaborator Author
madig commented Dec 5, 2018 •
edited
Loading

I noticed that with this approach, varLib.load_designspace() chooses the wrong base master when the axis defaults point to a sparse master/layer. In varLib.build(), the base font vf is then made the sparse master with its limited glyph set, making OT feature compilation fail with cryptic error messages. If we go this path, I'll insert a guarding assert to make sure the default lands on a full master.

@anthrotype
Copy link
Member
anthrotype commented Dec 18, 2018 •
edited
Loading

so I guess we shall do this differently.
In this PR on ufo2ft repo googlefonts/ufo2ft#300 we added a new compileInterpolatableTTFsFromDS function that takes a designspaceLib.DesignSpaceDocument object, where each SourceDescriptor has a font attribute with an already loaded defcon.Font (or compatible class). This function compiles the TTFonts and store them in the respective source.font attributes and returns a copy of the DesignSpaceDocument (or the same if inplace=True).

Given this, we could modify varLib.build to take a DesignSpaceDocument object (in addition of a path to a designspace file) and first look at the source.font attributes, if these are already loaded TTFont objects, we can use them as master fonts, otherwise do run finder on the filenames.

@anthrotype anthrotype closed this Dec 18, 2018
@madig madig deleted the varLib-MasterFinder-add-layerName branch December 18, 2018 16:47
@madig madig mentioned this pull request Dec 19, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@madig @behdad @anthrotype
0