Skip to content
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

incorrect bbox of text #2070

Merged
merged 2 commits into from May 29, 2013
Merged

incorrect bbox of text #2070

merged 2 commits into from May 29, 2013

Conversation

mdboom
Copy link
Member

@mdboom mdboom commented May 28, 2013

With current master, frames around texts are incorrectly located.

Here is some test output.

In v1.2.x branch, the results are correct.

text_bbox_test_v1 2

The frames are shifted in the master branch. Note that they have too much room toward the top of the texts.

text_bbox_test_master

These outputs are created with usetex=False. With usetex=True, the results are still incorrect, but the shifted direction of frames are different.

I am labeling this as a v1.3.x blocker, but change as you wish.

@mdboom
Copy link
Member

mdboom commented May 28, 2013

@leejjoon: Can you have a look at this fix?

@leejjoon
Copy link
Contributor Author

I think it is better to return the list of decent values, instead just returning the last one.

I made a pull request to your repo.

On the other hand, there are a few changes that I do not understand in you commit 4aed809.

I will make some inline comments there.

@mdboom
Copy link
Member

mdboom commented May 29, 2013

I don't know if it makes sense to return the list of descender values. Only the last one (the bottom one) is necessary for proper text alignment in the agg backend. The vector backends don't even need that. So I'd prefer to leave it in this simpler state.

@mdboom
Copy link
Member

mdboom commented May 29, 2013

I think I've fixed the post-transformation problem. It was hidden because there was a bug just before final text display of usetex text in the Agg backend. I think it's correct now.

@leejjoon
Copy link
Contributor Author

Well, the agg backend calculates the descent of every line by itself, so I guess it does not matter what "_get_layout" returns.
Technically, _get_text function can return incorrect result as it assumes all the lines have same descent, although chance of this to happen will be miniscule.

So, I guess this is a minor issue and I am fine with merging this if my inline comment of patheffect is addressed.

@mdboom
Copy link
Member

mdboom commented May 29, 2013

I believe I have addressed all of your comments. There is a rudimentary fix in here to expand the spacing when things get taller than "normal". There are a lot of things that could be fixed up about text layout, but I think I'll save that for a MEP -- this is just enough to get by to make the cut for 1.3.0.

@mdboom
Copy link
Member

mdboom commented May 29, 2013

The _get_textbox doesn't assume all the lines have the same descent -- it uses the y positions of each line as returned by _get_layout and doesn't really need the descent from each line. It only needs to deal with the descent of the bottom line to determine the position of the box. So I think this is correct.

@leejjoon
Copy link
Contributor Author

While there are a few issues that I do not completely agree with you, I think we can go ahead and merge it for 1.3.0 release.
I will open a separate issue which may not need to be a release blocker.

@mdboom
Copy link
Member

mdboom commented May 29, 2013

@leejjoon: Thanks for pushing on getting the spacing right. I have added commit a4fdc89 to address the issue in #2078. This may not have been how you implemented, but it seems to have the correct net effect, without breaking too many of the regression tests. (It improves mathtext alignment, so a number of the mathtext tests needed to be updated.)

@pelson
Copy link
Member

pelson commented May 29, 2013

The tests look much better, and in general there is an equal amount of magic in text as there was before. I'm 👍 for the change for v1.3.x once all of the tests are passing (a4fdc89).

@mdboom
Copy link
Member

mdboom commented May 29, 2013

Ok -- I've updated all the tests (I think some were failing on Travis but not for me due to the additional differences caused by a different libfreetype). We'll see how that goes, then I'd like to squash the commits to reduce the amount of new test data added to the main repo before merging.

mdboom added a commit that referenced this pull request May 29, 2013
@mdboom mdboom merged commit 212a8f9 into matplotlib:v1.3.x May 29, 2013
@mdboom mdboom deleted the issue2070 branch August 7, 2014 13:52
@anntzer anntzer mentioned this pull request Sep 19, 2020
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants