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

Add a convenience method to rotate a patch #1156

Closed
wants to merge 2 commits into from

Conversation

dmcdougall
Copy link
Member

The default behaviour is to rotate the patch about it's centre of mass, otherwise rotate about a specified pivot.

This is to address #987.

I think it'd be better to have this is a convenience method in mlab rather than a new kwarg in the Patch constructor because the patch's transform can't be decided until an Axes object is created.

The default behaviour is to rotate the patch about it's centre of mass,
otherwise rotate about a specified pivot.
@@ -3195,6 +3195,41 @@ def quad2cubic(q0x, q0y, q1x, q1y, q2x, q2y):
# c3x, c3y = q2x, q2y
return q0x, q0y, c1x, c1y, c2x, c2y, q2x, q2y

def rotate_patch(p, ax, theta, pivot=None):
"""
Rotates a simple closed polygon *p* of type :class:`Patch` *rad* radians
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your parameter is named theta instead of rad.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Last minute change I forgot about!

@pelson
Copy link
Member

pelson commented Aug 29, 2012

Question to other devs: would you prefer to see this in patches.py?

@WeatherGod
Copy link
Member

As a method of patch, it would make sense to have rotation as a property that can be changed at will. Right now, the convenience function is a one-shot deal, which is fine in-of-itself. The problem is that no one is going to be looking in mlab for such a function. Also, what about PatchCollections? Could this be extended to them?

@dmcdougall
Copy link
Member Author

@WeatherGod Good point about mlab. My feeling is that I'm happy to put it in either. If putting in patches.py, would it be a standalone method? So:

import matplotlib.patches as mpatches

...

mpatches.rotate_patch(p, ax, theta)
ax.add_patch(p)

Regarding PatchCollections: can they be disjoint? If so, what should be the default pivot?

@WeatherGod
Copy link
Member

If it is going to be a stand-alone function, I would put keep it in mlab for now. I would only want it to be a method of the Patch class itself if one could get/set a rotation value as many times as they like without it adding more rotations to the transform (although I could imagine one wanting a way to apply multiple rotations in series, which your stand-alone function does).

As for the collection, I would think that the desired behavior would be the pivot point is for each patch, rather than the collection as a whole, but I am sure some may think the opposite.

@efiring
Copy link
Member

efiring commented Aug 29, 2012

This is half-baked; it seems clear that it doesn't belong in mlab, so I don't think that putting it there "for now" is a good idea. Having it as a stand-alone function in patches seems better, and maybe good enough. But overall, it seems like the best course of action would be to back off, define the problem carefully, and then solve it. The present function might be a good part of the solution--maybe even all of it--but that needs to be clarified.

@wkerzendorf
Copy link

Hi guys, I was the original author of the feature request. It would be great if patches.Rectangle had a keyword angle as patches.Ellipse has. This would make many things a lot easier.

@dmcdougall
Copy link
Member Author

Closing this in favour of a new attempt at #1405, implementing @wkerzendorf's suggestion.

@dmcdougall dmcdougall closed this Oct 15, 2012
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

6 participants