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

bug: path effects in text() change text properties #2889

Closed
megies opened this Issue Mar 10, 2014 · 12 comments

Comments

Projects
None yet
5 participants
@megies
Contributor

megies commented Mar 10, 2014

After encountering strange behavior across matplotlib versions 1.3/1.4 in our image tests in @obspy and a couple of hours of debugging and git bisecting roughly a dozen steps through matplotlib git repo I have pinpointed a problem with path effects..

When using path effects in plt.text() and using plt.savefig("...png") the text properties get changed in a strange way. What I experienced is that font size gets decreased by roughly 25% when I used a path effect on text.

The commit that introduces the bug is 3d31865. The parent commit d5f9876 shows the expected behavior.

The following shows a minimal test case to reproduce:

import matplotlib
matplotlib.use("TKAGG")
import matplotlib.pyplot as plt
import matplotlib.patheffects as PathEffects

plt.text(0, 0, "BAD", size=100,
         #path_effects=[PathEffects.withStroke(linewidth=3,
         #                                     foreground="red")])
         # EDIT: even `Normal()` exposes the bug
         path_effects=[PathEffects.Normal()])
plt.text(0, 0, "        OK", size=100)
plt.savefig("/tmp/patheffects_bug.png")
plt.show()

Note that the plot that shows after savefig has the text with correct size.

Here's the buggy output png image:
patheffects_bug

@tacaswell tacaswell added this to the v1.4.0 milestone Mar 10, 2014

@tacaswell

This comment has been minimized.

Show comment
Hide comment
@tacaswell

tacaswell Mar 10, 2014

Member

@pelson Can you take a look at this one too?

Member

tacaswell commented Mar 10, 2014

@pelson Can you take a look at this one too?

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Mar 10, 2014

Contributor

Here's my build output.. just in case. https://gist.github.com/megies/9468079

Contributor

megies commented Mar 10, 2014

Here's my build output.. just in case. https://gist.github.com/megies/9468079

@pelson

This comment has been minimized.

Show comment
Hide comment
@pelson

pelson Mar 20, 2014

Member

@pelson Can you take a look at this one too?

Yep, just looking now. I'm very surprised about this one - the fact that it shows correctly is weird and I can't see any obvious bugs in my code. Right now I'm suspicious that there is an underlying bug in the renderer somewhere and that my change has exposed us to that - if that is the case, this could be particularly difficult to track down. I'll update on any progress.

Member

pelson commented Mar 20, 2014

@pelson Can you take a look at this one too?

Yep, just looking now. I'm very surprised about this one - the fact that it shows correctly is weird and I can't see any obvious bugs in my code. Right now I'm suspicious that there is an underlying bug in the renderer somewhere and that my change has exposed us to that - if that is the case, this could be particularly difficult to track down. I'll update on any progress.

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies Mar 20, 2014

Contributor

Btw, it doesn't take withStroke(...), even path_effects=[PathEffects.Normal()] produces the bug.

Contributor

megies commented Mar 20, 2014

Btw, it doesn't take withStroke(...), even path_effects=[PathEffects.Normal()] produces the bug.

@pelson

This comment has been minimized.

Show comment
Hide comment
@pelson

pelson Mar 20, 2014

Member

path_effects=[PathEffects.Normal()] produces the bug.

Thanks, that is worth knowing.

Member

pelson commented Mar 20, 2014

path_effects=[PathEffects.Normal()] produces the bug.

Thanks, that is worth knowing.

@pelson

This comment has been minimized.

Show comment
Hide comment
@pelson

pelson Mar 20, 2014

Member

P.S. Workaround: plt.savefig("/tmp/patheffects_bug.png", dpi=plt.gcf().dpi)

Member

pelson commented Mar 20, 2014

P.S. Workaround: plt.savefig("/tmp/patheffects_bug.png", dpi=plt.gcf().dpi)

@pelson

This comment has been minimized.

Show comment
Hide comment
@pelson

pelson Mar 20, 2014

Member

P.S. Workaround: plt.savefig("/tmp/patheffects_bug.png", dpi=plt.gcf().dpi)

I think that might be the clue to the underlying problem - savefig, for some reason, has a different dpi to show. I'm wildly speculating that the smaller text is the correct size given the savefig's dpi, and the larger text is a fallout from the fact that its height is computed before the savefig call - essentially I think the bug here is that the OK text is too big.

I'm speculating as I'm not sure what units "size" is in. The two possible definitions (taken from http://kyleschaeffer.com/development/css-font-size-em-vs-px-vs-pt-vs/):

Pixels (px): Pixels are fixed-size units that are used in screen media (i.e. to be read on the computer screen). One pixel is equal to one dot on the computer screen (the smallest division of your screen’s resolution). Many web designers use pixel units in web documents in order to produce a pixel-perfect representation of their site as it is rendered in the browser. One problem with the pixel unit is that it does not scale upward for visually-impaired readers or downward to fit mobile devices.

Points (pt): Points are traditionally used in print media (anything that is to be printed on paper, etc.). One point is equal to 1/72 of an inch. Points are much like pixels, in that they are fixed-size units and cannot scale in size.

With the resulting PNG from your code, OK has a height of 100px, and bad has 72px (plus a little extra for the actual path effect).

@mdboom / @leejjoon - do either of you know what behaviour we actually want?

Member

pelson commented Mar 20, 2014

P.S. Workaround: plt.savefig("/tmp/patheffects_bug.png", dpi=plt.gcf().dpi)

I think that might be the clue to the underlying problem - savefig, for some reason, has a different dpi to show. I'm wildly speculating that the smaller text is the correct size given the savefig's dpi, and the larger text is a fallout from the fact that its height is computed before the savefig call - essentially I think the bug here is that the OK text is too big.

I'm speculating as I'm not sure what units "size" is in. The two possible definitions (taken from http://kyleschaeffer.com/development/css-font-size-em-vs-px-vs-pt-vs/):

Pixels (px): Pixels are fixed-size units that are used in screen media (i.e. to be read on the computer screen). One pixel is equal to one dot on the computer screen (the smallest division of your screen’s resolution). Many web designers use pixel units in web documents in order to produce a pixel-perfect representation of their site as it is rendered in the browser. One problem with the pixel unit is that it does not scale upward for visually-impaired readers or downward to fit mobile devices.

Points (pt): Points are traditionally used in print media (anything that is to be printed on paper, etc.). One point is equal to 1/72 of an inch. Points are much like pixels, in that they are fixed-size units and cannot scale in size.

With the resulting PNG from your code, OK has a height of 100px, and bad has 72px (plus a little extra for the actual path effect).

@mdboom / @leejjoon - do either of you know what behaviour we actually want?

@mdboom

This comment has been minimized.

Show comment
Hide comment
@mdboom

mdboom Mar 20, 2014

Member

The font size given to the Text object is specified in points. The PDF, PS and SVG backends all have a "hardcoded" dpi of 72 -- the native "unit" is always 72 units per inch, and the "dpi" passed to savefig is only intended to affect how images are resampled. Maybe that helps -- if I have time this afternoon, I may look a little deeper myself.

Member

mdboom commented Mar 20, 2014

The font size given to the Text object is specified in points. The PDF, PS and SVG backends all have a "hardcoded" dpi of 72 -- the native "unit" is always 72 units per inch, and the "dpi" passed to savefig is only intended to affect how images are resampled. Maybe that helps -- if I have time this afternoon, I may look a little deeper myself.

@leejjoon

This comment has been minimized.

Show comment
Hide comment
@leejjoon

leejjoon Mar 20, 2014

Contributor

It think It will be fixed by implementing the points_to_pixel method for the PathEffectRenderer class.

@@ -152,6 +152,9 @@ class PathEffectRenderer(RendererBase):
             renderer.draw_path_collection(gc, master_transform, paths,
                                           *args, **kwargs)

+    def points_to_pixels(self, points):
+        return self._renderer.points_to_pixels(points)
+
     def _draw_text_as_path(self, gc, x, y, s, prop, angle, ismath):
         # Implements the naive text drawing as is found in RendererBase.
         path, transform = self._get_text_path_transform(x, y, s, prop,
Contributor

leejjoon commented Mar 20, 2014

It think It will be fixed by implementing the points_to_pixel method for the PathEffectRenderer class.

@@ -152,6 +152,9 @@ class PathEffectRenderer(RendererBase):
             renderer.draw_path_collection(gc, master_transform, paths,
                                           *args, **kwargs)

+    def points_to_pixels(self, points):
+        return self._renderer.points_to_pixels(points)
+
     def _draw_text_as_path(self, gc, x, y, s, prop, angle, ismath):
         # Implements the naive text drawing as is found in RendererBase.
         path, transform = self._get_text_path_transform(x, y, s, prop,

tacaswell added a commit to tacaswell/matplotlib that referenced this issue May 9, 2014

@tacaswell

This comment has been minimized.

Show comment
Hide comment
@tacaswell

tacaswell May 9, 2014

Member

@megies I put @leejjoon 's fix in a PR, can you see if it fixes your problem?

Member

tacaswell commented May 9, 2014

@megies I put @leejjoon 's fix in a PR, can you see if it fixes your problem?

@megies

This comment has been minimized.

Show comment
Hide comment
@megies

megies May 13, 2014

Contributor

Looks good to me. 👍

Although it's been a while since I reported this and I can't exactly remember where this first popped up. But I put some effort in a solid bug report in the first place and the above code snippet seems fixed by this:

figure_1

Contributor

megies commented May 13, 2014

Looks good to me. 👍

Although it's been a while since I reported this and I can't exactly remember where this first popped up. But I put some effort in a solid bug report in the first place and the above code snippet seems fixed by this:

figure_1

@pelson

This comment has been minimized.

Show comment
Hide comment
@pelson

pelson May 21, 2014

Member

#3081 resolves (not quite merged, but I will do it in the next couple of days unless somebody else beats me to it 😉 )

Member

pelson commented May 21, 2014

#3081 resolves (not quite merged, but I will do it in the next couple of days unless somebody else beats me to it 😉 )

@pelson pelson closed this May 21, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment