I changed the offset code for draw_text for RendererAgg fixing issue #13044 #20057

safdarzareef · 2021-04-23T19:31:20Z

PR Summary

I tried to fix issue #13044. This a script where the issue can be reproduced (from @dzh527's example in the PR)

import matplotlib.pyplot as plt

fig, axes = plt.subplots(2, 2)
for idx, ax in enumerate(axes.flatten()):
      ax.axvline(.5, linewidth=.5, color='.5')
      ax.axhline(.5, linewidth=.5, color='.5')
      ax.text(
          .5, .5, 'pP', size=50,
          bbox=dict(facecolor='None', edgecolor='red', pad=0),
          rotation=idx*90,
          va='center_baseline',
          rotation_mode='anchor'
      )

This is the original output:

This is the output after our change:

So the difference is caused by the offset in the code, with xo and yo not accounting for the change in the rotation angle, so every time we rotate the x and y components of the offset are not adjusted accordingly with the angle. That is what we tried to implement.

I think this broke a lot of tests as we changed the location of any text added to a plot - because even if the angle is 0, the offset components would be different. We haven't modified anything except for the draw_text function in the backend_agg.py. I think the new behavior does change the plots for all types of text added to a plot, but even though it is offset differently, it is equivalent in theory. Also instead of adding yo to y, we subtracted it because that gave the best result and draw_mathtext was implemented like that.

PR Checklist

Has pytest style unit tests (and pytest passes).
Is Flake 8 compliant (run flake8 on changed files to check).
New features are documented, with examples if plot-related.
Documentation is sphinx and numpydoc compliant (the docs should build without error).
Conforms to Matplotlib style conventions (install flake8-docstrings and run flake8 --docstring-convention=all).
New features have an entry in doc/users/next_whats_new/ (follow instructions in READM 8000 E.rst there).
API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

github-actions

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

jklymak · 2021-04-23T20:19:56Z

I don't understand why these are leading to broken tests? I think its obviously OK to break the tests for non-zero rotation, but breaking them for zero rotation doesn't seem right.

safdarzareef · 2021-04-23T20:32:23Z

I don't understand why these are leading to broken tests? I think its obviously OK to break the tests for non-zero rotation, but breaking them for zero rotation doesn't seem right.

I think it is because when taking the components of the offset according to the non-rotation angle which is 0 degrees, we are setting y0 to be sin(0)*old y0 = 0. But I don’t think there should be a y offset due to y0, there is a y offset regardless due to yd. So that is why it might break tests with non-rotated text.

jklymak · 2021-04-23T21:40:53Z

I think you'll need to give us a little more detail. We can't break everyone who depends on our code without a really good reason. I think this would be easier with a diagram and some numbers. I appreciate that none of this is helped by the fact that draw_text_image is not documented, so its hard to know what the right thing to do is. But we should be certain we get this right if we are changing it.

WeatherGod

I am thinking that you are on the right track here. I haven't looked at how much the images would change, but I am inclined to say that it might be worth fixing if the results have been seriously wrong all this time. Maybe we can do some sort of special back-compat mode for unrotated text in order to limit the extent of the impact?

Can you comment on the difference between this fix and #19114?

WeatherGod · 2021-04-24T00:58:13Z

lib/matplotlib/backends/backend_agg.py

@@ -208,10 +208,13 @@ def draw_text(self, gc, x, y, s, prop, angle, ismath=False, mtext=None):
        xo, yo = font.get_bitmap_offset()
        xo /= 64.0
        yo /= 64.0
+        h = sqrt(xo**2 + yo**2)


Personally, I prefer using hypot() instead, but I don't know if it really matters in the context of bounding box calculations.

I don't know if it will matter, but we could still change it.

WeatherGod · 2021-04-24T00:59:03Z

lib/matplotlib/backends/backend_agg.py

        xd = d * sin(radians(angle))
        yd = d * cos(radians(angle))
+        xo = h * cos(radians(angle))
+        yo = h * sin(radians(angle))


If we are going to calculate radians(angle) four times, might as well just do it once and store it in a variable.

Also, these four lines here would greatly benefit from a comment explaining the trigonometry. Usually, cos() goes with x, and y goes with sin(). Seeing it two different ways here for the offset and the descent is confusing.

I can explain the offset trigonometry, but I just left the code for the descent as it was before. The descent seems wrong - but I am unaware of the reason why it was like that before so I left it there. Basic trigonometry suggests that x components should be calculated by multiplying a number by the cosine of its angle and y components should be calculated by multiplying a number by the sine of its angle - so that is why I used that for the offset but I do think the descent should be in the same form too.

WeatherGod · 2021-04-24T01:02:41Z

lib/matplotlib/backends/backend_agg.py

        x = round(x + xo + xd)
-        y = round(y + yo + yd)
+        y = round(y + yo - yd)


much like the trig above, I think this will also need a quick proof in the comments.

I don't think I have a proof for this at the moment except that in the draw_mathtext function the y is calculated in a similar manner and by subtracting the y offset we get a satisfactory result.

safdarzareef · 2021-04-24T06:28:28Z

This fix and #19114 is different because #19114 tried to fix the bounding box and we are trying to change the offset for the text. But the problem is not with the bounding box but rather the text itself - here's an example to prove it:

import matplotlib.pyplot as plt

fig, axes = plt.subplots(2, 2)
for idx, ax in enumerate(axes.flatten()):
      ax.axvline(.5, linewidth=.5, color='.5')
      ax.axhline(.5, linewidth=.5, color='.5')
      ax.text(
          .5, .5, ' ', size=50,
          bbox=dict(facecolor='None', edgecolor='red', pad=0),
          rotation=idx*90,
          va='center_baseline',
          rotation_mode='anchor'
      )

plt.show()

Here's an example of text without the bounding box:

import matplotlib.pyplot as plt

fig, axes = plt.subplots(2, 2)
for idx, ax in enumerate(axes.flatten()):
      ax.axvline(.5, linewidth=.5, color='.5')
      ax.axhline(.5, linewidth=.5, color='.5')
      ax.text(
          .5, .5, 'pP', size=50,
          #bbox=dict(facecolor='None', edgecolor='red', pad=0),
          rotation=idx*90,
          va='center_baseline',
          rotation_mode='anchor'
      )

plt.show()

And you can see there is a problem with the offset of the text even when a bounding box isn't drawn.

safdarzareef · 2021-04-24T06:37:50Z

I think you'll need to give us a little more detail. We can't break everyone who depends on our code without a really good reason. I think this would be easier with a diagram and some numbers. I appreciate that none of this is helped by the fact that draw_text_image is not documented, so its hard to know what the right thing to do is. But we should be certain we get this right if we are changing it.

So I never could find the draw_text_image function in the source code actually. I was trying to look for it to check whether it solves rotation correctly or not.

jklymak · 2021-05-15T15:33:36Z

Yes, draw_text_image is implemented in the agg c-code. (_backend_agg_wrapper.cpp).

oscargus · 2022-05-07T09:46:28Z

Closing and reopening to get fresh test results.

melissawm · 2023-03-29T14:46:51Z

👋🏻 Hello, @safdarzareef ! Are you interested in picking this back up? Can we help you with this? Please let us know 😄

github-actions · 2023-12-15T01:49:37Z

Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it.

changed the offset code for draw_text for RendererAgg

1041b32

github-actions bot reviewed Apr 23, 2021

View reviewed changes

WeatherGod requested changes Apr 24, 2021

View reviewed changes

jklymak marked this pull request as draft May 15, 2021 15:33

oscargus closed this May 7, 2022

oscargus reopened this May 7, 2022

github-actions bot added the status: needs rebase label Oct 18, 2022

github-actions bot removed the status: needs rebase label Jul 15, 2023

github-actions bot added the status: inactive Marked by the “Stale” Github Action label Dec 15, 2023

safdarzareef closed this by deleting the head repository Jul 10, 2024

WPurre mentioned this pull request Nov 27, 2024

Fix center of rotation with rotation_mode='anchor' #29199

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

I changed the offset code for draw_text for RendererAgg fixing issue #13044 #20057

I changed the offset code for draw_text for RendererAgg fixing issue #13044 #20057

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

I changed the offset code for draw_text for RendererAgg fixing issue #13044 #20057

I changed the offset code for draw_text for RendererAgg fixing issue #13044 #20057

Uh oh!

Conversation

Uh oh!

PR Summary

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!