Skip to content

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

Merged
merged 3 commits into from
Feb 19, 2015
Merged

Font advance width #4031

merged 3 commits into from
Feb 19, 2015

Conversation

mdboom
Copy link
Member

@mdboom mdboom commented Jan 23, 2015

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:

  • Virtually every test with text in it now fails, but from the spot-checking I've done, they've improved for the better. I'd suggest updating every image with any text, even if the difference isn't enough for the test to fail, just to reset the baseline correctly.
  • The Abel font @sgmentzer used in Align text using advance width, not glyph width #253 is a good one to catch this kind of error because the difference between advance width and control point bounding box is so large and because the kerning is so extreme. It might make sense to make a test or two based on it -- but then we have some technical issues to solve about how to get it. (It's under SIL Open Font License, so we can just incorporate it, but that might be overkill).

@mdboom mdboom self-assigned this Jan 23, 2015
@mdboom mdboom added this to the v1.5.x milestone Jan 23, 2015
@WeatherGod
Copy link
Member

While I am thinking of this, I should remember to double-check the results
from mplot3d, especially with offset text and other things that do
non-traditional anchoring. Most (all?) mplot3d test have text removed.
Also, the tests may not capture what one might see when interacting with
the 3d axes.

On Fri, Jan 23, 2015 at 2:47 PM, Michael Droettboom <
notifications@github.com> wrote:

Fix #253 #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:

Virtually every test with text in it now fails, but from the
spot-checking I've done, they've improved for the better. I'd suggest
updating every image with any text, even if the difference isn't enough for
the test to fail, just to reset the baseline correctly.

The Abel font @sgmentzer https://github.com/sgmentzer used in #253
#253 is a good one to
catch this kind of error because the difference between advance width and
control point bounding box is so large and because the kerning is so
extreme. It might make sense to make a test or two based on it -- but then
we have some technical issues to solve about how to get it. (It's under SIL
Open Font License, so we can just incorporate it, but that might be
overkill).


You can view, comment on, or merge this pull request online at:

#4031
Commit Summary

File Changes

Patch Links:


Reply to this email directly or view it on GitHub
#4031.

@mdboom
Copy link
Member Author

mdboom commented Jan 25, 2015

Note: There are some problems discovered with this approach. Working on a better solution now.

@mdboom
Copy link
Member Author

mdboom commented Jan 26, 2015

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...

@jenshnielsen
Copy link
Member

@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.

@tacaswell
Copy link
Member

That won't really help us with historical problem. The repo is currently almost 300MB, another 8 doesn't really matter.

tcaswell@eowyn:tmp$ git clone matplotlib
Cloning into 'matplotlib'...
remote: Counting objects: 101713, done.
remote: Compressing objects: 100% (29291/29291), done.
remote: Total 101713 (delta 72112), reused 101687 (delta 72095)
Receiving objects: 100% (101713/101713), 294.75 MiB | 9.67 MiB/s, done.
Resolving deltas: 100% (72112/72112), done.
Checking connectivity... done.

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.

@jenshnielsen
Copy link
Member

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.

@tacaswell
Copy link
Member

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.

@jenshnielsen
Copy link
Member

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|✔]

@jenshnielsen
Copy link
Member

For some reason I only had to download ~195 MB Strange

✘-1 /tmp/test git clone https://github.com/matplotlib/matplotlib.git
Cloning into 'matplotlib'...
remote: Counting objects: 96987, done.
remote: Compressing objects: 100% (115/115), done.
remote: Total 96987 (delta 79), reused 32 (delta 15)
Receiving objects: 100% (96987/96987), 195.26 MiB | 1.75 MiB/s, done.
Resolving deltas: 100% (70659/70659), done.
Checking connectivity... done.

@tacaswell
Copy link
Member

Oh, I know what the problem is, the mpl fork on my gh has a gh-pages branch, your number is more accurate.

@mdboom
Copy link
Member Author

mdboom commented Jan 27, 2015

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
Copy link
Member

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...

Copy link
Member Author

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)

Copy link
Member

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

@tacaswell
Copy link
Member

For reference to see just the code changes: mdboom/matplotlib@matplotlib:master...b52589b

@tacaswell
Copy link
Member

@mdboom Can you also document which FreeType and font version you used for this so that we can (eventually) pin that on travis?

@mdboom mdboom force-pushed the font-advance-width branch from 14a8da6 to d5284f5 Compare January 27, 2015 19:17
@mdboom
Copy link
Member Author

mdboom commented Jan 27, 2015

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.

@mdboom mdboom force-pushed the font-advance-width branch from d5284f5 to 3afdaf0 Compare January 27, 2015 19:57
@mdboom
Copy link
Member Author

mdboom commented Jan 27, 2015

These images were generated with freetype 2.5.3-21 on Fedora 21. The fonts used are all the ones that ship with matplotlib.

@WeatherGod
Copy link
Member

I have manually checked for any negative effects in mplot3d. I didn't see any regressions.

@tacaswell tacaswell modified the milestones: proposed next point release, next point release Feb 19, 2015
tacaswell added a commit that referenced this pull request Feb 19, 2015
API : Font advance width

This resets _all_ of the images with text.
@tacaswell tacaswell merged commit 005cfde into matplotlib:master Feb 19, 2015
@tacaswell
Copy link
Member

Merged this before we accumulate too many more test images which will need to be reset (I think we already have one or two).

@mdboom
Copy link
Member Author

mdboom commented Feb 19, 2015

I see master is passing on Travis. I assume that means all of our images are currently good, then?

@tacaswell
Copy link
Member

I fixed one last night after I merged this

On Thu, Feb 19, 2015, 08:19 Michael Droettboom notifications@github.com
wrote:

I see master is passing on Travis. I assume that means all of our images
are currently good, then?


Reply to this email directly or view it on GitHub
#4031 (comment)
.

@mdboom mdboom deleted the font-advance-width branch March 3, 2015 18:43
tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Apr 16, 2015
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
tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Apr 18, 2015
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Align text using advance width, not glyph width
4 participants