-
Notifications
You must be signed in to change notification settings - Fork 25
Improve fonts generation: remove unused codes #48
Conversation
This comment has been minimized.
This comment has been minimized.
I haven't looked at this yet, but what do you mean by "skew generation isn't used"? Don't we need this for italic correction/spreading out (in |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@ylemkimon this is awesome! Thanks for starting to simplify font creation. |
@edemaine We have different method of calculating skew in metrics generation. |
@kevinbarabash Of course, tests fail because KaTeX/KaTeX#1267 was not merged. Why didn't I think of that before 😄 Tested with latest KaTeX: KaTeX/KaTeX#1494. |
@ylemkimon can you link to where this is being done? |
katex-fonts/src/metrics/extract_tfms.py Lines 96 to 100 in e641de5
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Another question: Should we support SVG fonts? This seems relevant to KaTeX one day supporting SVG output (KaTeX/KaTeX#375). Ah, never mind: SVG fonts seem to have poor support in general, e.g., support for them was removed from Chrome. SVG can use any CSS-accessible fonts, at least in the browser. |
This PR does not change contents of fonts. (Checked with |
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.
Wow, this is substantial cleanup. The code changes look good to me.
However, I just tried running ./buildFonts.sh
on this branch and (several minutes later) the font files seem to have changed (all of them). You seem to have had the opposite experience. I'm not sure how to do diffs for TTF/WOFF[2] files. Any thoughts on how to debug this discrepancy? (Or maybe it's because another PR merged since?)
[update] Or is your point that I should run ttx
to dump the fonts and then compare?
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.
OK, given that you checked this already, I'm happy to approve.
This PR is part of #12.
fonts.less
install
target: copying font files is handled bybuildFonts.sh
fonts
andpack
target insrc/Makefile