-
Notifications
You must be signed in to change notification settings - Fork 168
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
Enhance otf2ttf for ttc, parallel, and file wildcards #1000
Conversation
However, I do not know how to make a test ttc font for unit test. So, unit test for this PR is missing. |
The test error is expected, because multiprocessing module will mess up the logging module, particularly in Windows.
|
Looking at the failure makes me think that removing the Thanks for the PR, by the way. I have a few more comments to make but I need to run some validations first. |
@josh-hadley perhaps something to look into: CircleCI didn't run. I wonder if it didn't happen because the branch to be merged doesn't belong to the afdko repo. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
My only request is to handle non-OTF inputs and TTCs containing TTFs more gracefully. Right now the whole process stops at the assert ttFont.sfntVersion == "OTTO"
. It'd be better to skip the fonts that cannot be converted and log a warning.
Attached are fonts you can use for new tests.
pr1000.zip
@miguelsousa
|
@msoxzw if you look in https://github.com/adobe-type-tools/afdko/tree/develop/tests/otf2ttf_data/input there's only OTFs in there. The TTF and TTC files I provided in the ZIP are for adding new tests. Here's how I expect otf2ttf to work with the new test input files:
|
@miguelsousa |
Yes, keep the original TTFs inside the TTCs. |
@miguelsousa I assume that the the results of otf_otf.ttc, otf_ttf.ttc, and ttf_otf.ttc are identical to ttf_ttf.ttc excluding checkSumAdjustment, created, and modified. However, the test error seems to detect the bugs in my codes. After a while, I think that I spot the cause of error: the 'hmtx' tables of test fonts are likely to not strictly conform to the OpenType spec, which makes some minor differences between ttf_ttf.tcc and others. |
Yes, I also expected that the TTX dump of ttf_ttf.ttc would be the same as the dump of the other TTCs after their conversion. |
tests/test_utils.py
Outdated
font = TTCollection(font_path) | ||
|
||
temp_path = get_temp_file_path() | ||
font.saveXML(temp_path, tables=tables) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add font.close()
after font.saveXML()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have investigated this issue before I decided to remove "with" statement. "TTCollection" class does not provide the "close" method. Therefore, adding font.close()
is impracticable. However, due to garbage collector of Python, the font object will be automatically closed and deleted, but heavily relying on garbage collector is a bad practice. Of course, there are at least two workarounds:
- Re-implement the close method:
if font.reader is not None:
font.reader.close()
- Open font file by built-in open:
with open(font_path, 'rb') as f:
try:
font = TTFont(f)
except TTLibError:
font = TTCollection(f)
temp_path = get_temp_file_path()
font.saveXML(temp_path, tables=tables)
return temp_path
Nevertheless, both methods, I think, will add maintenance burden if the "TTCollection" class implement close method in some day.
So, how do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the best would be to implement a close()
method in the TTCollection
class.
Would you mind submitting such change to https://github.com/fonttools/fonttools ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moreover, if I submit such change and it is merged in "fonttools", this whole project will require the latest "fonttools". Hence, is it worthy to do so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. The afdko always updates to the latest version of FontTools.
A lot of other projects rely on FontTools so every opportunity to make the changes there benefits everyone.
@msoxzw code coverage on otf2ttf dropped quite a lot, most likely because of the parallelisation. Do you know how to fix the reporting? |
@miguelsousa |
@msoxzw thanks for the FontTools contribution. We'll integrate the upcoming version shortly after it's released. |
@msoxzw FontTools v4.1.0 has been integrated |
@miguelsousa I could not figure out why PEP 448 and PEP 498 are identified as invalid syntax in Codacy/PR Quality Review. Probably, the Python version is too old in the PR Quality Review. |
@msoxzw no worries, we're actually thinking of disconnecting Codacy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the good work.
I mainly pick these from my repo
From my view, converting ttc font to the first font in ttc is very confusing. Hence, processing ttc font as a whole is a better way compared with splitting font, then merging it.
Moreover, calculating TrueType outline from PostScript outline is time-consuming. So, I add data parallel and file wildcards input for otf2ttf.