[varlib] Cannot build VF font for SourceSerifPro with fontmake #1296
Comments
|
@readroberts those instructions didn't work for me. I tried them with a clean installation of fontmake version 1.6.0, and the One was in fontmake and I logged it at googlefonts/fontmake#451 . My solution was to comment line 585. The other one was in the feature files of Source Serif, and relates to this change on how the |
|
@behdad this is the traceback. Any idea of what's wrong? Traceback (most recent call last):
File "bin/fontmake", line 11, in <module>
sys.exit(main())
File "lib/python2.7/site-packages/fontmake/__main__.py", line 256, in main
project.run_from_designspace(designspace_path, **args)
File "lib/python2.7/site-packages/fontmake/font_project.py", line 623, in run_from_designspace
**kwargs)
File "lib/python2.7/site-packages/fontmake/font_project.py", line 689, in run_from_ufos
master_bin_dir=master_bin_dir)
File "lib/python2.7/site-packages/fontmake/font_project.py", line 261, in build_variable_font
font.save(output_path)
File "lib/python2.7/site-packages/fontTools/ttLib/ttFont.py", line 172, in save
writer_reordersTables = self._save(tmp)
File "lib/python2.7/site-packages/fontTools/ttLib/ttFont.py", line 211, in _save
self._writeTable(tag, writer, done, tableCache)
File "lib/python2.7/site-packages/fontTools/ttLib/ttFont.py", line 632, in _writeTable
tabledata = self.getTableData(tag)
File "lib/python2.7/site-packages/fontTools/ttLib/ttFont.py", line 650, in getTableData
return self.tables[tag].compile(self)
File "lib/python2.7/site-packages/fontTools/ttLib/tables/otBase.py", line 71, in compile
return writer.getAllData()
File "lib/python2.7/site-packages/fontTools/ttLib/tables/otBase.py", line 397, in getAllData
tableData = table.getData()
File "lib/python2.7/site-packages/fontTools/ttLib/tables/otBase.py", line 271, in getData
raise OTLOffsetOverflowError(overflowErrorRecord)
fontTools.ttLib.tables.otBase.OTLOffsetOverflowError: ('GPOS', 'LookupIndex:', 0, 'SubTableIndex:', 0, 'ItemName:', 'BaseArray.BaseAnchor.YDeviceTable', 'ItemIndex:', None) |
|
And this is the traceback from a different project, built using buildCFF2VF instead of fontmake. Traceback (most recent call last):
File "bin/buildCFF2VF", line 11, in <module>
sys.exit(run())
File "lib/python2.7/site-packages/afdko/Tools/SharedData/FDKScripts/buildCFF2VF.py", line 986, in run
post_format_3)
File "lib/python2.7/site-packages/afdko/Tools/SharedData/FDKScripts/buildCFF2VF.py", line 945, in buildCFF2Font
varFont.save(varFontPath)
File "lib/python2.7/site-packages/fontTools/ttLib/ttFont.py", line 172, in save
writer_reordersTables = self._save(tmp)
File "lib/python2.7/site-packages/fontTools/ttLib/ttFont.py", line 211, in _save
self._writeTable(tag, writer, done, tableCache)
File "lib/python2.7/site-packages/fontTools/ttLib/ttFont.py", line 632, in _writeTable
tabledata = self.getTableData(tag)
File "lib/python2.7/site-packages/fontTools/ttLib/ttFont.py", line 650, in getTableData
return self.tables[tag].compile(self)
File "lib/python2.7/site-packages/fontTools/ttLib/tables/otBase.py", line 71, in compile
return writer.getAllData()
File "lib/python2.7/site-packages/fontTools/ttLib/tables/otBase.py", line 397, in getAllData
tableData = table.getData()
File "lib/python2.7/site-packages/fontTools/ttLib/tables/otBase.py", line 271, in getData
raise OTLOffsetOverflowError(overflowErrorRecord)
fontTools.ttLib.tables.otBase.OTLOffsetOverflowError: ('GPOS', 'LookupIndex:', 0, 'SubTableIndex:', 3, 'ItemName:', 'PairSet.<none>', 'ItemIndex:', None) |
|
I have a fix for the first overflow error that @miguelsousa mentioned, i.e. the one occurring in The second overflow error mentioned above seems to be occuring in PairPos subtable format 1, which supposedly fonttools is able to fix. Not sure what's going on, I'll have to check. Maybe @behdad knows. |
|
Here's how I managed to reproduce, in case @behdad wants to also have a look.
This is the error: Inspecting the overflowing subtable with python debugger, I see it has ClassCount of 1, so our splitMarkBasePos (#1297) doesn't make any difference for this. I'm not sure how the commits @readroberts referenced in his original post may affect this issue. I'll try to revert those and rebuild to see if they change anything. |
|
it appears the offset that is overflowing is the one from the beginning of BaseAnchor (format 3) subtable to the VariationIndex table for the Y coordinate. |
|
if I completely disable the GDEF varstore optimization and the remapping of GPOS VariationIndexes, I can finish compiling SourceSerifPro-VF. This reminds me of another similar issue... lemme remember |
|
found this #1206 (comment) |
|
@behdad here is the interpolatable master TTFs and the accompanying designspace file so you can try reproducing the bug |
|
I think this has to do with the reusing of the same objects in combination with commit 03b9c50, because the problem seems to disappear if one first compiles without optimization and then applies the varStore optimizer in a separate run, after reading the font from disk (in which I presume this reusing of the same objects doesn't occur). |
|
besides commenting out the varstore optimization in diff --git a/Lib/fontTools/varLib/__init__.py b/Lib/fontTools/varLib/__init__.py
index 5f0f9bd2..4156ece4 100644
--- a/Lib/fontTools/varLib/__init__.py
+++ b/Lib/fontTools/varLib/__init__.py
@@ -560,10 +560,10 @@ def _merge_OTL(font, model, master_fonts, axisTags):
GDEF.VarStore = store
# Optimize
- varidx_map = store.optimize()
- GDEF.remap_device_varidxes(varidx_map)
- if 'GPOS' in font:
- font['GPOS'].table.remap_device_varidxes(varidx_map)
+ # varidx_map = store.optimize()
+ # GDEF.remap_device_varidxes(varidx_map)
+ # if 'GPOS' in font:
+ # font['GPOS'].table.remap_device_varidxes(varidx_map)
diff --git a/Lib/fontTools/varLib/varStore.py b/Lib/fontTools/varLib/varStore.py
index a1ca7df6..baf860a8 100644
--- a/Lib/fontTools/varLib/varStore.py
+++ b/Lib/fontTools/varLib/varStore.py
@@ -24,11 +24,9 @@ class OnlineVarStoreBuilder(object):
self._store = buildVarStore(self._regionList, [])
self._data = None
self._model = None
- self._cache = {}
def setModel(self, model):
self._model = model
- self._cache = {} # Empty cached items
def finish(self, optimize=True):
self._regionList.RegionCount = len(self._regionList.Region)
@@ -58,24 +56,17 @@ class OnlineVarStoreBuilder(object):
self._store.VarData.append(data)
def storeMasters(self, master_values):
- deltas = [otRound(d) for d in self._model.getDeltas(master_values)]
- base = deltas.pop(0)
- deltas = tuple(deltas)
- varIdx = self._cache.get(deltas)
- if varIdx is not None:
- return base, varIdx
-
if not self._data:
self._add_VarData()
+ deltas = [otRound(d) for d in self._model.getDeltas(master_values)]
+ base = deltas.pop(0)
inner = len(self._data.Item)
if inner == 0xFFFF:
# Full array. Start new one.
self._add_VarData()
return self.storeMasters(master_values)
self._data.Item.append(deltas)
-
varIdx = (self._outer << 16) + inner
- self._cache[deltas] = varIdx
return base, varIdx
then I can build the non-varstore-optimized SourceSerifVariable font, and I can apply the varstore optimization in a separate I'm going to leave this like that for now, for when Behdad or Read find the time to take a look. I hope I didn't confuse things even more ;) |
|
I'll look into this today. |
Trying to build this with master fontmake stack, I get include error: Is it because of how we changed what is the path relative to? |
|
I had to modify all .fea files to remove one set of '../' from all includes. |
That's actually understandable. When you don't optimize, DeviceTable sharing doesn't happen. Font compiles fine, though with multiple lookups being upgraded to Extension. When you rerun optimizer, it still compiles back fine, because lookups in Extension don't share objects together, and the Extension decisions from before are still in effect... |
|
This is very hard to fix within the current table-packing algorithm. Maybe I should just finish the 99proof... Working on it. |
|
Ok, pushed a bandaid. Please test. |
|
Thanks @behdad! |
|
@behdad Yes, this fixes the problems with Minion3 and with SourceSerifPro Thanks! I was really looking in the wrong place. |
|
thank you @behdad. @anthrotype can you issue a new fonttools version by Monday? There's this and other fixes that we'd like to pick up. Thanks |
Any comments on this one? |
|
I wasn't in favor of that change (discussed at googlefonts/fontmake#157), but the spec got updated (unified-font-object/ufo-spec#66) so we'll just have to adjust the tooling accordingly, I guess. |
|
The part that bothers me most is that in included feature files, paths are still relative to the ufo... I still think we should fix this. A proposal, eg. have paths starting with "./" be relative to current file. Or "dir/" or something... It's fine to keep whatever current behavior and add new syntax for relative-to-ufo, relative-to-inside-the-ufo, and relative-to-current-file... |
|
Another possibility, to borrow from C: Includes with double quotes (include "...") be relative to current file and includes with pointed brackets (include <...>) be relative to "search path", which will be inside the UFO directory in case of UFOs, or whereever else one supplies to the compiler (-I...). Includes with parantheses can stay with whatever meaning they have right now. |
|
I agree that the whole |
|
@miguelsousa @behdad Will do. Was just reading up on this this afternoon. The feature file spec is clear that relative include paths are relative to the directory of the including file. The ufo spec is clear that included files from the main feature file are relative to the parent directory of the ufo font, not so clear about children. What makeotfexe does is to first try to find the file relative to the including file parent directory, and them, for legacy projects, try to find the file relative to the current directory. |
|
this issue with include statements has been discussed several times, e.g. fontmake now follows what Robofont does, and the UFO spec makes it clear how features.fea inside the UFO must treat includes as relative to the directory of the UFO, not to the UFO itself seen as a directory (which is an implementation detail, and a future implementation of the format may allow a single file instead of a folder/package). I like the idea of adding an extra notation for allowing C-style includes, with optional search paths and stuff. But I'm not super enthusiastic about being the one to implement it :) There is also this somewhat related issue: maybe the discussion can continue there. |
varlib used to be able to merge the master designs of SourceSerifPro. This broke after the application of varIDXmap optimization to ValueRecords in in varStore.py:: _visit() of (added 2/27 with commit [54fd71f]), and the use of caching in varStore.py:: OnlineVarStoreBuilder.storeMasters() (added 2/21 with commit [e299650]). The immediate problem is that an offset overflow error is not being handled, but this didn't happen before the changes. You can reproduce this by downloading the sources for Source Serif Pro from:
https://github.com/adobe-fonts/source-serif-pro
cd to the Roman/Master directory, and say:
fontmake -m SourceSerifVariable-Roman.designspace -o variable
This will fail the first time, as the interpolatable ttf file for each master design will be built in the directory Masters/master_ttf_interpolatable/, but the varlib merge codes expects them to be in the same directories as the source ufo fonts, Masters/master_[0,1,2]. You can work around this by copying them to the ufo directories, and running fontmake again.
I hope to look at this sometime fairly soon, but would welcome any suggestions, if anyone else has seen similar problems.
The text was updated successfully, but these errors were encountered: