Annotation: always use FancyBboxPatch instead of bbox_artist #4178
Conversation
|
I'm not entirely happy with the second commit because I still don't understand how, when, and where the transforms are evolving. I determined that prior to this commit the return from |
|
I am regretting both my |
|
The Travis failure is because the pylab_examples/text_rotation.py is including a "pad" in a dictionary provided as the 4th arg to |
|
@tacaswell, worms probably would have started crawling out sooner or later; but my dislike for working in this part of the code, which I have mostly avoided, has been strongly reinforced. |
|
Adjusting the size of the FancyBboxPatch appears to be rather difficult and intrusive. I tried what seemed like a straightforward way to remove some of the extra space below the text--and I failed miserably. |
| @@ -3897,7 +3897,7 @@ def __init__(self, posA=None, posB=None, | |||
| """ | |||
| If *posA* and *posB* is given, a path connecting two point are | |||
| created according to the connectionstyle. The path will be | |||
| clipped with *patchA* and *patchB* and further shirnked by | |||
| clipped with *patchA* and *patchB* and further shrinked by | |||
| else: | ||
| self._bbox.update(dict(facecolor=color)) | ||
| self._bbox_patch.update(dict(facecolor=color)) |
There was a problem hiding this comment.
@tacaswell, your comment on this line, and my response, were lost when I rebased. You thought the line should be dedented, but I don't understand why. The update is needed only if a new _bbox_patch has not just been created.
There was a problem hiding this comment.
I don't remember exactly what I was thinking when I left the last message. I think what I was suggesting was to get rid of the else block and just always update the _bbox_patch (even if we just created it with the correct color). It is a no-op and I think makes the code path a little easier to follow (which is a style call).
Avoiding else statements and having if blocks which just ensure assumptions are true is is a newish (and possibly not good) style-tick I have developed.
I think my comment on this can be ignored.
|
I think this also closes #4012 |
|
Can you add an API changes note? We probably want to back-port this to color-overhaul as I broke this by merging what I thought were bug-fixes into 1.4.3. |
|
This is getting close to ready. @mdboom or @tacaswell, the remaining problem is that the arrow starts inside the patch. The expected clipping by that patch is not working yet. |
|
This is just too complicated. I think something is going wrong in the chain of events leading to the clip method of the Base class inside the ConnectionStyle class which is called by FancyArrowPatch. My guess is that some transform is being changed too early, or too late, so that the attempted clipping is working with a bbox_patch and an arrow_patch that are not being transformed the same way. I can't spend any more time on it. @mdboom, will you be able to take a look? Maybe at the same time you will be able to figure out a way to make all this less convoluted and hard to follow. The capabilities provided by all the Fancy things are wonderful, but figuring out how it all works--or, at present, doesn't quite work--is extraordinarily difficult. Maybe all it needs is some overview documentation or additional comments. |
|
The Travis failure is because of a subtle change in arrow patch location and size. We can either change the test images or change the patch location and size for these cases. I know how to do either; this is not a problem. In fact, I suspect the aesthetic optimum might be half way between the two cases; previously there was a little too much space below the text, because the full descent was being added. I took that out to get a better match to what the bbox_artist was doing. |
|
attn @leejjoon. I think you wrote the original version of most of this code, any guidance on where it is going wrong? |
|
I rebased this to master, but I'm still struggling with the problem that the YAArrow extends to the text bbox, which in general is inside the fancy patch. I tried setting a direction-reversed fancy patch path as a clip region, hoping this would mean the inside of the polygon would be the blanked-out region, but that didn't work. The solution might be to abandon the YAArrow and simulate it with a FancyArrowPatch. An advantage would be that this would mean that the arrow in Annotation would always be a FancyArrowPatch. The trick would be to make the API and behavior similar enough to the original YAArrow version to keep people happy. |
|
👍 in principle to dropping YAArrow and just using FanceArrowPatch for On Sat, Jun 20, 2015 at 10:29 PM Eric Firing notifications@github.com
|
|
I think this is getting close to being ready. It won't pass the tests, but we have to figure out where this means the code needs adjustment, and where we need new test images. There is also some more documentation and cleanup to be done. The YAArrow replacement does not exactly reproduce the old behavior and API, but I think it will be close enough, and noticeably improved. It removes the embarrassing aspect of annotation_demo.py in which the short arrow has an absurdly short head. |
Some comment and docstring typos are also fixed.
YAArrow is now partly simulated with the FancyArrowPatch, which remains as the only arrow class used by Annotation. I ignored the 'frac' key and added the 'headlength' key; the 'frac' was never a good API because it scaled head length with the arrow length, but left all other dimensions in units of points.
|
I think this is ready to go. |
| if self.arrow_patch.figure is None and self.figure is not None: | ||
| self.arrow_patch.figure = self.figure | ||
| self.arrow_patch.draw(renderer) | ||
|
|
||
| # Draw text, including FancyBboxPatch, after FancyArrowPatch. | ||
| # Otherwise, a wedge arrowstyle can land partly on top of the Bbox. | ||
| Text.draw(self, renderer) |
There was a problem hiding this comment.
use super here (you use it in update)?
There was a problem hiding this comment.
True, I did. That is new code, and right next to another method in which super is used. But the "Text.draw()" is not new, and it is one of several such lines in this module. I'm inclined to think that "super-ifying" the module is out of scope for this PR. I also note that not all parent class method calls in this module can be handled with super; Annotation.__init__ needs to call the __init__ from each of its immediate parents. The only way to avoid that would be to rewrite it so as to use composition rather than multiple inheritance. In the long run that could bring a big gain in readability and maintainability, but given the present complexity of this entire family of code, it would be quite a job.
MNT: always use FancyBboxPatch instead of bbox_artist
This is response to breaking changes (matplotlib/matplotlib#4178) in MPL 1.5.
This fixes #4139, as an alternative to #4145, by eliminating the use of the bbox_artist debugging function. With a second commit, it also fixes #4140.