fix FancyArrowPatch picker fails depending on arrowstyle #10776
Conversation
|
Every arrowstyle(Curve, Bracket or others) contains one to three sub paths which represent the head arrow (if present), line, and tail arrow (if present) respectively. Because whether a location should respond to a mouse event depends on the result of the contains function. So I check whether any sub path contains the point separately(separate by the starting code=1) in contains function. |
|
Hi @tacaswell, I push a temp fix of this bug and add a test, could you take a look at it? Thank you. |
|
👍 on sorting out what is wrong here! This bug had me stumped. |
|
Something like is probably faster (moving all the logic to numpy and taking advantage of the lazy map). But +1 to the PR in general. |
|
Hi @anntzer , thanks for your suggestion! I have updated the code. |
e885826 to
28bc4f8
Compare
|
hi @tacaswell , is there anything else need to be fixed to make it better? |
| subpath.contains_point( | ||
| (mouseevent.x, mouseevent.y), self.get_transform(), radius) | ||
| for subpath in map( | ||
| Path, np.split(vertices, idxs), np.split(codes, idxs))), {} |
There was a problem hiding this comment.
I think I finally see what this is doing, but, can this be deconvolved to ease in maintenance and understanding? Other devs are free to approve if they are OK with this, and its just my python ignorance, but its not the clearest return statement in the code base.
There was a problem hiding this comment.
Could you reformat this to
subpaths = map(Path, np.split(vertices, idxs), np.split(codes, idxs)))
contains = any(
subpath.contains_point(
(mouseevent.x, mouseevent.y), self.get_transform(), radius)
for subpath in subpaths))
return contains, {}
There was a problem hiding this comment.
I think it is OK to leave as-is.
|
Power-cycled to re-trigger CI |
|
@zhoubecky Are you still interested in working on this and can you look into why the tests are failing? I suspect a re-base may help. |
|
@tacaswell sorry I just saw your message. I will take a look this weekend. |
|
@tacaswell I have checked the failure message and it shows ImageComparisonFailure, the expected image name is arrow_contains_point-expected.png but the actual is arrow_contains_point.png. Change the image name and the tests should pass. I will double check and fix it. I need some time to reinstall the developing environment. Sorry for the late. |
eca521e to
4347ee6
Compare
4347ee6 to
d8a1c97
Compare
|
@zhoubecky I've just updated the test image for you on your branch - hopefully this should all pass now, and if it does I'll give it a review! |
|
Power-cycled to re-run against current master. |
| subpath.contains_point( | ||
| (mouseevent.x, mouseevent.y), self.get_transform(), radius) | ||
| for subpath in map( | ||
| Path, np.split(vertices, idxs), np.split(codes, idxs))), {} |
There was a problem hiding this comment.
Could you reformat this to
subpaths = map(Path, np.split(vertices, idxs), np.split(codes, idxs)))
contains = any(
subpath.contains_point(
(mouseevent.x, mouseevent.y), self.get_transform(), radius)
for subpath in subpaths))
return contains, {}
…ing on arrowstyle
|
Thanks @zhoubecky ! Sorry this took almost a year to get in, hopefully we will hear from you again. |
…776-on-v3.1.x Backport PR #10776 on branch v3.1.x (fix FancyArrowPatch picker fails depending on arrowstyle)
PR Summary
fix FancyArrowPatch picker fails depending on arrowstyle #8384