-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Font advance width #4031
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
Font advance width #4031
Conversation
While I am thinking of this, I should remember to double-check the results On Fri, Jan 23, 2015 at 2:47 PM, Michael Droettboom <
|
Note: There are some problems discovered with this approach. Working on a better solution now. |
All of the baseline images with text (all 7.7MB of them) have been updated so this passes. I'm not crazy about the increase in size in the repo, but I don't have any better ideas... |
@mdboom The only thing that I can think of is to put the test images in a git submodule similar to what IPython does with it's javascript dependencies. Not great since it will involve another setup step. |
That won't really help us with historical problem. The repo is currently almost 300MB, another 8 doesn't really matter.
What we need is a way to hijack git and only pull the large objects when needed or get people to start doing shallow clones. |
Yes moving the images to a submodule will of cause only stop the growth of the repository. There are ways of removing files entirely from the history https://help.github.com/articles/remove-sensitive-data/ but I don't really thing that it is a good idea and I would be very scared about doing such a thing to the matplotlib repository. |
That would change sha1s of every commit back to the first image we added which is a complete non-starter. This is a feature of git, you can't tamper with the development history with out destroying all of it. |
Yes I agree which was what I tried to say above. I don't think there is going to be a good way of only hijacking git to not pull large objects for the same reason. I think the only thing that can be done is to advice on using shallow clones and try to figure out a solution to stop making it worse. BTW I tried doing something similar to what Fernando did to the IPython repository here on a local copy but that only gives me a total save from 279 to 271 Mb ✔ /tmp/matplotlib [master|✔] du -d 1 -h
198M ./.git
4.0K ./.travis
6.9M ./doc
2.1M ./examples
3.9M ./extern
67M ./lib
36K ./LICENSE
8.0K ./release
712K ./src
24K ./tools
168K ./unit
279M .
✔ /tmp/matplotlib [master|✔] du -d 1 -h
190M ./.git
4.0K ./.travis
6.9M ./doc
2.1M ./examples
3.9M ./extern
67M ./lib
36K ./LICENSE
8.0K ./release
712K ./src
24K ./tools
168K ./unit
271M .
✔ /tmp/matplotlib [master|✔] |
For some reason I only had to download ~195 MB Strange
|
Oh, I know what the problem is, the mpl fork on my gh has a gh-pages branch, your number is |
Ok -- well let's not worry about repo size for this PR, then, but address it in some way in the future. I think this PR is ready to merge, then. |
@@ -802,7 +802,7 @@ def draw_text(self, gc, x, y, s, prop, angle, ismath=False, mtext=None): | |||
else: | |||
kern = 0 | |||
lastgind = gind | |||
thisx += kern/64.0 | |||
thisx -= kern/64.0 |
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.
why did the sign of the kern adjustment change? This no longer matches the sign in the pdf backend...
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'm looking into this further... I may have been mistaken about this change... (But in any event the sign does match because the analogous change was made in backend_pdf.py)
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.
This also may be an issue of me reading badly as this looked fine before the pep8 fixes
For reference to see just the code changes: mdboom/matplotlib@matplotlib:master...b52589b |
@mdboom Can you also document which FreeType and font version you used for this so that we can (eventually) pin that on travis? |
14a8da6
to
d5284f5
Compare
Thanks to @tacaswell's urging, it turns out the kerning wasn't as broken as I thought. I have a new approach that I hope is now correct, and I've rewritten the history so the updated test images are only in here once. I'll leave a few inline comments to @tacaswell's earlier comments. |
d5284f5
to
3afdaf0
Compare
These images were generated with freetype 2.5.3-21 on Fedora 21. The fonts used are all the ones that ship with matplotlib. |
I have manually checked for any negative effects in mplot3d. I didn't see any regressions. |
API : Font advance width This resets _all_ of the images with text.
Merged this before we accumulate too many more test images which will need to be reset (I think we already have one or two). |
I see |
I fixed one last night after I merged this On Thu, Feb 19, 2015, 08:19 Michael Droettboom notifications@github.com
|
Document what version of freetype was used to generate baseline images in PR matplotlib#4031 / commit 005cfde and which version is (currently) being run on travis.ci
Document what version of freetype was used to generate baseline images in PR matplotlib#4031 / commit 005cfde and which version is (currently) being run on travis.ci
Fix #253.
This changes the text bounding box calculation to use advance width of the character, rather than the bounding box of the control points.
It also fixes a long-standing bug in kerning in that (a) the conversion from 26.6 fixed point to floating-point was incorrect, and (b) the sign was backwards.
This PR needs a couple of things to push it over the finish line: