Fix Annotation arrow not updating after setting patchA#31438
Fix Annotation arrow not updating after setting patchA#31438dikshajangra12918-oss wants to merge 7 commits intomatplotlib:mainfrom
Conversation
Setting patchA on Annotation.arrow_patch has no visible effect because the arrow position is not recomputed. This change ensures that the annotation is marked as stale when patchA changes, triggering proper recomputation during rendering.
QuLogic
left a comment
There was a problem hiding this comment.
I don't really understand how this is a fix. update_positions is called from draw, which means the artist is already stale and you're triggering a second draw now by marking it stale again.
Also, is this fixing some existing bug? Please add a test to show what you are actually fixing.
Add logic to prevent overriding user-set patchA in update_positions. Fixes issues matplotlib#28316.
Add test to verify that manually set patchA on annotation arrow is preserved after draw.
|
Thankyou for the review! |
1. Add 2 blank lines before function definition 2. Split long comment line to stay within 80 chars
Only update patchA internally if user has not manually set it.
| if self.arrow_patch.patchA is getattr( | ||
| self, '_internal_patchA', None): | ||
| self.arrow_patch.set_patchA(patchA) | ||
| self._internal_patchA = patchA | ||
|
|
There was a problem hiding this comment.
This does not make sense to me. Please explain what this does and why it is needed.
There was a problem hiding this comment.
Sir,
The issue is that update_positions() runs on every draw and always overwrites patchA with a recomputed value - so if a user manually calls set_patchA(), their value gets silently lost on the next draw.
The fix: I store the last patchA I set internally in _internal_patchA. Before overwriting, I check if arrow_patch.patchA is still the same object. If yes then user hasn't touched it, safe to update. If no then user set it manually, so I skip the override.
getattr(self, '_internal_patchA', None) handles the first draw gracefully- defaults to None so the initial set always works fine.
Added test_annotation_patchA_not_overridden to verify this- it manually sets a custom patchA, triggers draw_without_rendering(), and asserts the value is preserved after the draw.
There was a problem hiding this comment.
I see. I would question if manually setting patchA is a valid use case. Annotation is primarily a Text object with the ability to refer to a coordinate position xy. It can optionally draw an arrow from its xytext postion to xy. That arrow (starting at xytext) may be partially covered by the text and the text bounding box serves as patchA. I would say that the patchA is owned by the Annotation and I am not clear that a user-override is reasonable.
It may be that the use case should instead be solved via ConnectionPatch, see #26440. @story645 do you agree?
There was a problem hiding this comment.
Sir,
The original use case (from @Phlya) is in the adjustText library, where ax.annotate() is used instead of FancyArrowPatch directly — and the user needs to modify arrow_patch.patchA afterward to avoid arrows passing through text objects.
If ConnectionPatch is the recommended approach for this use case, I'm happy to close this PR. But if set_patchA() is documented as a valid method on arrow_patch, silently ignoring it seems like a bug worth fixing.
Happy to follow your guidance on the right direction!
There was a problem hiding this comment.
It is unclear why the original user wants to overwrite the patchA - #28316 (comment).
Let's first try to find out what the actual problem is that the user wants to solve.
PR summary
Why is this change necessary?
Setting
patchAonAnnotation.arrow_patchcurrently has no visible effect because the arrow position is not recomputed after updatingpatchA.What problem does it solve?
This change ensures that when
patchAis modified, the annotation is marked as stale. This triggers a recomputation of the arrow position during rendering, fixing the issue where the arrow does not align or clip correctly with the associated text.What is the reasoning for this implementation?
The arrow position is initially computed correctly, but subsequent updates to
patchAare not reflected visually. By marking the annotation as stale whenpatchAchanges, we ensure that the rendering pipeline updates the arrow position accordingly.AI Disclosure
AI assistance was used in a manner for understanding parts of the codebase and validating the approach. The implementation and reasoning were completed and verified independently.