fontforge / fontforge

Free (libre) font editor for Windows, Mac OS X and GNU+Linux
http://fontforge.github.io/
Other
6.57k stars 707 forks source link

ufo.c producing UFOs that RoboFab can't read #1444

Closed davelab6 closed 10 years ago

davelab6 commented 10 years ago
$ cd src
$ git clone https://github.com/frank-trampe/lohit2/; 
$ cd lohit2;
$ sudo pip install robofab
Password:
Downloading/unpacking robofab
  Downloading robofab-1.2.1.tar.gz (148kB): 148kB downloaded
  Running setup.py (path:/private/tmp/pip_build_root/robofab/setup.py) egg_info for package robofab

Installing collected packages: robofab
  Running setup.py install for robofab

Successfully installed robofab
Cleaning up...
$ python
>>> from robofab.world import OpenFont
>>> path = '/Users/me/src/lohit2/Lohit-Devanagari.ufo'
>>> f = OpenFont(path)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python2.7/site-packages/robofab/objects/objectsRF.py", line 38, in OpenFont
    return RFont(path)
  File "/usr/local/lib/python2.7/site-packages/robofab/objects/objectsRF.py", line 138, in __init__
    self._loadData(path)
  File "/usr/local/lib/python2.7/site-packages/robofab/objects/objectsRF.py", line 204, in _loadData
    self._glyphSet = reader.getGlyphSet()
  File "/usr/local/lib/python2.7/site-packages/robofab/ufoLib.py", line 324, in getGlyphSet
    return GlyphSet(glyphsPath)
  File "/usr/local/lib/python2.7/site-packages/robofab/glifLib.py", line 126, in __init__
    self.contents = self._findContents()
  File "/usr/local/lib/python2.7/site-packages/robofab/glifLib.py", line 290, in _findContents
    contents = readPlist(contentsPath)
  File "/usr/local/lib/python2.7/site-packages/robofab/plistlib.py", line 84, in readPlist
    rootObject = p.parse(pathOrFile)
  File "/usr/local/lib/python2.7/site-packages/robofab/plistlib.py", line 409, in parse
    parser.ParseFile(fileobj)
xml.parsers.expat.ExpatError: not well-formed (invalid token): line 46, column 15
>>> ^D
frank-trampe commented 10 years ago

Fixed in 9b14aba3e0ac3fee35c0e3980cb924d858c86396.

davelab6 commented 10 years ago

https://groups.google.com/forum/#!topic/googlefontdirectory-discuss/VAhs5MnjZ30 reports another error:

The document “Lohit-Devanagari-FrankFail.ufo” could not be opened. The file isn’t in the correct format.
Traceback (most recent call last):
  File "lib/doodleDocument.pyc", line 280, in _loadFont
  File "/Applications/RoboFont.app/Contents/Resources/lib/python2.7/defcon/objects/font.py", line 277, in __iter__
  File "lib/fontObjects/doodleFont.pyc", line 215, in __getitem__
  File "lib/fontObjects/doodleFont.pyc", line 67, in _loadGlyph
  File "/Applications/RoboFont.app/Contents/Resources/lib/python2.7/defcon/objects/font.py", line 215, in _loadGlyph
AssertionError
frank-trampe commented 10 years ago

That's RoboFont.

davelab6 commented 10 years ago

It looks like the error is in defcon tho

frank-trampe commented 10 years ago

At line 489 of glifLib.py, Robofab demands that a lib entry in a glif have one child. As it turns out, the specification mandates that the lib entry have a dict child even if the dict is empty, which we were failing to do. Pull request #1453 addresses that problem and also strips a leading newline from the feature file.

frank-trampe commented 10 years ago

The remaining problems result from parsing of feature files. Robofont wants only spaces as delimiters in lookupflag lists, but FontForge uses commas. The specification says that one is to use commas, but the example uses only spaces. I have asked @readroberts for clarification.

davelab6 commented 10 years ago

Just switch to using spaces. There isn't enough implementations to argue about spec conformance, I think - and I would advise Adobe to update the spec to reflect actual practice ;) On 24 Jun 2014 15:28, "Frank Trampe" notifications@github.com wrote:

The remaining problems result from parsing of feature files. Robofont wants only spaces as delimiters in lookupflag lists, but FontForge uses commas. The specification says that one is to use commas, but the example uses only spaces. I have asked @readroberts https://github.com/readroberts for clarification.

— Reply to this email directly or view it on GitHub https://github.com/fontforge/fontforge/issues/1444#issuecomment-47019041 .

frank-trampe commented 10 years ago

@miguelsousa advises that spaces are correct.

davelab6 commented 10 years ago

@miguelsousa, the spec at http://www.adobe.com/devnet/opentype/afdko/topic_feature_file_syntax.html says, bold emphasis mine,

Here, the individual lookup flag values to be set are expressed in a comma-separated list of one or more <named lookupflag value>s, in no particular order. A <named lookupflag value> is one of the following:

What's the best way to suggest this be updated to 'expressed in a space-separated list' in the next revision? :)

I also suggest adding a 'how to give feedback' section to the document - or even putting a version on github under a license that permits modification so people can send edits as pull requests :dancers:

miguelsousa commented 10 years ago

What's the best way to suggest this be updated to 'expressed in a space-separated list' in the next revision? :)

@readroberts Knows about that issue. He's also the maintainer of the documentation, so send the suggestions to him.

davelab6 commented 10 years ago

Done, thanks!

frank-trampe commented 10 years ago

The only known subissue that remains involves handling of subtables. makeotf only allows subtable breaks for pair positioning, which makes it difficult to losslessly store lookup subtables from FontForge. @miguelsousa has deferred to @readroberts about whether makeotf might gain additional subtable support in the future or what the most elegant way to map the subtables might be otherwise.

davelab6 commented 10 years ago

@pravins The context here is that, since most designers don't use only FontForge, I want to use UFO as an interchange format for Lohit2 sources so that designers can use various font editors in their work when using Lohit2 as a base. As part of this effort, Frank is working on the UFO import/export to make it very robust, and now his code produces UFOs that work well with all other editors :)

However, the FEA file in Lohit2 can not be processed by the Adobe Font Development for OpenType (FDK) compiler (makeotf) - despite that it is correct for the specification and works in FontForge - as makeotf only allows subtable breaks for pair positioning, but the Lohit2 FEA file has a subtable break in PresSub_Chain_VowelSign - https://github.com/pravins/lohit2/blob/master/devanagari/Lohit-Devanagari.fea#L288-L297

So @frank-trampe is considering adding a feature to FontForge's FEA export method to export FDK-valid tables, but @readroberts, the engineer in the Adobe Type team in California who has written most of the FDK, is away on vacation for a few weeks right now, and so we have to wait for him to get back before knowing what to do in FontForge.

However, perhaps we can change the PresSub_Chain_VowelSign to not use a subtable? Looking forward to your thoughts :)

pravins commented 10 years ago

I think UFO is missing things in Lohit2. Will be very helpful if we get coverter for same. I am available for help in this regards.

frank-trampe commented 10 years ago

Hi, @pravins. If you see anything in particular, this would be an excellent time to let me know since I'm working on the U. F. O. driver.

davelab6 commented 10 years ago

@pravins can you change the PresSub_Chain_VowelSign lookup in Lohit2 FEA to not use a subtable?

adrientetar commented 10 years ago

@miguelsousa: It's been 21 days, do you know if Read Roberts is still on vacation?

miguelsousa commented 10 years ago

@adrientetar: @readroberts is back. I'll remind him of this discussion.

adrientetar commented 10 years ago

Thanks, Miguel – proper support for feature files is crucial for us.

miguelsousa commented 10 years ago

@adrientetar who is "us"?

readroberts commented 10 years ago

I am recently back, and still working through issues from when I was away. It is correct that makeotf only supports subtable breaks in pair positioning, and the spec does not say this limit is necessary. However, I do not understand how a subtable break is useful in any other lookup type. In PairPos 2 tables, use of the subtable break can significantly decrease the size of the lookup. It is not obvious to me that this is useful in any other lookup type. Without understanding this, it makes more sense to me to change the spec to limit subtable breaks to PairPos2 tables. I could probably add support for subtable breaks in an additional lookup type in the next round of work, over the next few weeks. However, I would first like to understand why this is needed. I see that there is a subtable break in the 'PresSub_Chain_VowelSign' lookup referenced above, but I do not understand what purpose it has in this context.

readroberts commented 10 years ago

PS: makeotf currently has a somewhat inefficient implementation of chaining lookups. If my memory is correct, each chaining pos and sub statement is already stored as a subtable, so adding support for the subtable keyword would have no effect in the case of the 'PresSub_Chain_VowelSign' lookup.

davelab6 commented 10 years ago

@pravins please explain :)

adrientetar commented 10 years ago

@miguelsousa Me, and – I guess, I am not their representative – the people working on Devanagari typefaces out there using FontForge. Probably also others FontForge developers, who like to fix issues. :)

davelab6 commented 10 years ago

I guess we fontforge developers should have a "who's who" page on the website that describes who we are, what are background is, what we hope to do with FF, etc....

readroberts commented 10 years ago

Looking more at the feature file, I see that there is a 'subtable' keyword after each chaining substitution rule, in the few lookups that contain more than one chaining substitution rule. I suspect that this reflects the way makeotf builds chaining substitution lookups, which is that each rule is containing in a separate subtable. This is an implementation issue - I did this in makeotf because I implemented only class-based chaining rules, and this made implementation easier as I then needed to define classes for the lookahead and backtrack sequences only for the current rule. However, there are a number of functionally equivalent ways to build chaining lookups from the same feature file chaining statement. The feature file rule says nothing, and shouldn't say anything, about which format to use or whether or not to put more than one rule into a subtable. I suggest that the use of the subtable keyword here does not serve any useful purpose for this lookup type, and suspect that it is present because of an artifact of decompiling fonts built by makeotf. The chaining rules in this feature file have the same lookahead or backtrack sequence (this is, none), and could be stored more efficiently by having them all in the same table. This improvement is on my list for makeotf, but is very low priority on a long list.

frank-trampe commented 10 years ago

Hi, @readroberts.

The reason this came up on our end is that FontForge supports subtables for lookups as a human-accessible feature (not necessarily as a performance fix). I don't know the original reason for that feature, but having no subtable break support in the feature file makes it more difficult for me to make the lookup data round-trip between U. F. O. (which depends upon the feature file for lookup storage) and S. F. D.. I still haven't heard back from anybody on the original reason for this functionality. It may be best (if there is no compelling reason for that functionality) for FontForge to discard those subtable breaks when generating feature files, particularly since the main point of the feature file is to provide data to an OpenType compiler, not to interchange in-progress designs.

davelab6 commented 10 years ago

It seems kinda funny to me that makeotf can't process its own output, but making the FEA export from fontforge work in a way that it's output doesn't break makeotf sounds like a good fix (and editing or reexporting Lohit FEA files) On 17 Jul 2014 15:02, "Frank Trampe" notifications@github.com wrote:

Hi, @readroberts https://github.com/readroberts.

The reason this came up on our end is that FontForge supports subtables for lookups as a human-accessible feature (not necessarily as a performance fix). I don't know the original reason for that feature, but having no subtable break support in the feature file makes it more difficult for me to make the lookup data round-trip between U. F. O. (which depends upon the feature file for lookup storage) and S. F. D.. I still haven't heard back from anybody on the original reason for this functionality. It may be best (if there is no compelling reason for that functionality) for FontForge to discard those subtable breaks when generating feature files, particularly since the main point of the feature file is to provide data to an OpenType compiler, not to interchange in-progress designs.

— Reply to this email directly or view it on GitHub https://github.com/fontforge/fontforge/issues/1444#issuecomment-49349365 .

davelab6 commented 10 years ago

Well, interchanging in-progress designs is what UFO is for, and UFO delegates FEA to Adobe spec, and every other UFO capable font editor uses makeotf.

On 17 Jul 2014 15:41, dave@lab6.com wrote:

It seems kinda funny to me that makeotf can't process its own output, but making the FEA export from fontforge work in a way that it's output doesn't break makeotf sounds like a good fix (and editing or reexporting Lohit FEA files)

On 17 Jul 2014 15:02, "Frank Trampe" notifications@github.com wrote:

Hi, @readroberts.

The reason this came up on our end is that FontForge supports subtables for lookups as a human-accessible feature (not necessarily as a performance fix). I don't know the original reason for that feature, but having no subtable break support in the feature file makes it more difficult for me to make the lookup data round-trip between U. F. O. (which depends upon the feature file for lookup storage) and S. F. D.. I still haven't heard back from anybody on the original reason for this functionality. It may be best (if there is no compelling reason for that functionality) for FontForge to discard those subtable breaks when generating feature files, particularly since the main point of the feature file is to provide data to an OpenType compiler, not to interchange in-progress designs.

— Reply to this email directly or view it on GitHub.

readroberts commented 10 years ago

Actually, makeotf doesn't produce feature files. 'spot' does have a feature file-ish output, but this requires a lot of massaging before it can be used as a feature file.

I do see that the spec is mis-leading about the use of the subtable breaks,. The original idea of the feature file syntax is that it should require specifying only data that affects layout. Internal structure that does not affect layout - like which table format to use, subtable breaks, which tables to share, and and order of tables - should be left up to the compiler. The subtable keyword is an unfortunate exception. I added it because I couldn't figure out how to automatically place subtable breaks in pair pos2 lookups so as to minimize lookup size - and important issue, as pair pos tables basically look like excel spreadsheets, and get bloated with a lot of 0 value class pairs entries. Unless someone can come up with another case for manually placed subtable breaks, I will fix this by changing the spec to make it clear that subtable breaks are a hint to the compiler that are used only for Pair Pos 2 lookups.

I will also fix makeotf so that it emits a warning, rather than a fatal error, when a subtable break is used in other than a pair pos 2 statement. This is a trivial fix, and will allow makeotf to work with the FontForge output. This will be out in a few week.s

davelab6 commented 10 years ago

Okay great, nothing to do on our end then :) I suggest we leave his open until the fix ships On 17 Jul 2014 16:13, "Read Roberts" notifications@github.com wrote:

Actually, makeotf doesn't produce feature files. 'spot' does have a feature file-ish output, but this requires a lot of massaging before it can be used as a feature file.

I do see that the spec is mis-leading about the use of the subtable breaks,. The original idea of the feature file syntax is that it should require specifying only data that affects layout. Internal structure that does not affect layout - like which table format to use, subtable breaks, which tables to share, and and order of tables - should be left up to the compiler. The subtable keyword is an unfortunate exception. I added it because I couldn't figure out how to automatically place subtable breaks in pair pos2 lookups so as to minimize lookup size - and important issue, as pair pos tables basically look like excel spreadsheets, and get bloated with a lot of 0 value class pairs entries. Unless someone can come up with another case for manually placed subtable breaks, I will fix this by changing the spec to make it clear that subtable breaks are a hint to the compiler that are used only for Pair Pos 2 lookups.

I will also fix makeotf so that it emits a warning, rather than a fatal error, when a subtable break is used in other than a pair pos 2 statement. This is a trivial fix, and will allow makeotf to work with the FontForge output. This will be out in a few week.s

— Reply to this email directly or view it on GitHub https://github.com/fontforge/fontforge/issues/1444#issuecomment-49359201 .

readroberts commented 10 years ago

The current version of makeotf, from teh AFDKO web page, now only warns about subtable breaks in unsupported tables instead of reporting a fatal error.

frank-trampe commented 10 years ago

Excellent!

pravins commented 10 years ago

Do we still need any fix in Lohit after https://github.com/pravins/lohit2/issues/13 ?