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
Clipping for OffsetBoxes #4466
Clipping for OffsetBoxes #4466
Conversation
e31da20
to
e477723
Compare
An example
used an API term ( Would this situation better be suited for a deprecation warning? |
ff44733
to
2c54249
Compare
# Clipping has not been implemented in the OffesetBox family, so | ||
# disable the clip flag for consistency. It can always be turned back | ||
# on to zero effect. | ||
self.set_clip_on(False) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably should leave this.
I think that a cilp on the offset box does make sense, the path of the DrawArea would first be clipped to it's clip path and then the artist would be clipped to that path. The use case I am thinking of is an OffsetBox which is half outside of an This looks reasonable to me, but I would prefer to keep the |
was done with the `clip` argument. The object did not do any clipping | ||
regardless of that parameter. Now the object can and does clip the child `Artists` if they are set to be clipped. | ||
|
||
You can turn off the clipping using :method:`Artist.set_clip_on(False)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you be more explicit about which artists need to be changed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No Artist needs to be changed. Maybe I will use a more explicit example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By changed, I mean what artist do you call set_clip_on(False)
on. It is not clear from this text that it should be on the children, not on the DrawingArea.
On the clipping expectation, I had gone for consistency with children added to the |
2c54249
to
a9d9084
Compare
@@ -0,0 +1,8 @@ | |||
'OffsetBox.DrawingArea' no longer accepts the 'clip' keyword argument |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The title is no longer accurate. Should be 'OffsetBox.DrawingArea now respects clip
' or something to that effect.
That is a fair point and if we were adding these classes from scratch I think I would agree with your original implementation, but this code has been in the wild for a long time and maintaining current behavior is a very high priority. Other than a few comments on the documentation, this looks good! |
afef2b2
to
6ade0dd
Compare
- Child `Artists` of `DrawingArea` can now get clipped to the bounds of the parent
6ade0dd
to
ea082aa
Compare
It is fixed up. Plus, are there any objections to |
I have no protests to including |
@@ -569,14 +570,16 @@ class DrawingArea(OffsetBox): | |||
""" | |||
The DrawingArea can contain any Artist as a child. The DrawingArea | |||
has a fixed width and height. The position of children relative to | |||
the parent is fixed. | |||
the parent is fixed. The children can be clipped at the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super picky, extra space between 'be' and 'clipped'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will take care of this. I want to extend this PR a bit and will just do it my self rather than explain what I want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I rebased and pushed already.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, it looks like I won the race condition on that and merged the previous version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And if you have a change set, please open a PR.
On Mon, May 25, 2015 at 11:25 PM Hassan Kibirige notifications@github.com
wrote:
In lib/matplotlib/offsetbox.py
#4466 (comment):@@ -569,14 +570,16 @@ class DrawingArea(OffsetBox):
"""
The DrawingArea can contain any Artist as a child. The DrawingArea
has a fixed width and height. The position of children relative to
- the parent is fixed.
- the parent is fixed. The children can be clipped at the
Well, I rebased and pushed already.
—
Reply to this email directly or view it on GitHub
https://github.com/matplotlib/matplotlib/pull/4466/files#r31002511.
ENH/API: Clipping for OffsetBoxes
Thanks! |
I already have a changeset for the |
The discussion is a bit scattered, most of it is at #1109 |
Artists
ofDrawingArea
now get clipped to the bounds of the parentFor reviewers
Offsetbox
is another commit (or PR whichever is best) that will follow. This will includeOffsetboxes
in thetight_layout
. In the tests it would make sense to cover invisible offsetboxes anchored to (in)visible axes. These tests would probably be sufficient for PR tight_layout ignores invisible axes #2008. Is O.K. to cherry-pick that PR?