Bug Fix: DraggableBase not to move unselected object #7894
Conversation
Not to move unselected object
|
It would be nice to have a reproducible failure first (I cannot reproduce the issue following the instructions at #5839 (comment) either with 1.5.3 or 2.0). |
|
I tried on 2.0. import numpy as np
import matplotlib as mpl
mpl.use('tkagg')
import matplotlib.pyplot as plt
x = np.arange(50)
y = x**2
text = [str(i) for i in zip(x, y)]
plt.scatter(x, y)
for a, b, c in zip(x, y, text):
plt.annotate(c, xy=(a, b), xytext=(a, b+2), arrowprops=dict(arrowstyle="->", color='r', lw=0.5)).draggable()
plt.show()Please move |
|
I do not understand why this fix works (it does). In the example you give, when clicking in a dense zone, multiple pickevents are consecutively fired, one per artist in the vincinity of the click. Indeed, you'd prefer only one of them to be fired. But I do not see how your patch ensures that. |
|
Multiple pickevents fired simultaneously, but each And once The cause of the issue is |
|
Thanks for the PR. This hasn't seen action for a long time so marking stale. Note that PRs sometimes fall through the cracks, so if this is important, please ping for more reviews. |
|
Closing to due to lack of action. Not sure if this is still a bug or not, but feel free to re-open if you come back to it. |
|
I can't repro the issue on master, bisecting says this was fixed in #10776 which seems at least possible. |
|
I pulled in this fix to stop the addition of unselected objects during the drag operation. It worked, but there was a strange side-effect. Sometimes, the annotation I tried to drag didn't move at all. I found that another annotation, so far removed that it was not even visible on the current display moved My application has date + time-of-day on the Y axis, and a value on the X axis. I typically display one day of readings in my graph. The user can scroll backwards or forwards in time. I average about 5 annotations per day, and have more than a years worth of readings. To summarize, I have thousands of annotations, but only a small subset (maybe 5 - 20) is displayed to the user at any time. Debugging revealed that the default selection routine, artist_picker(), operates on data which has been transformed from Data coordinates to Display coordinates. This transformation places every annotation within the current display space. As a result, when I tried to grab a particular annotation which is currently visible, artist_picker() would generate a large set of close annotations. In my testing, it sometimes found 50 annotations which passed the proximity test, even though the current display only showed 5 annotations, and only one of those was close. With fukatani's fix, a somewhat randomly selected first of these annotations would be the only annotation selected for dragging. It's not entirely random. The order of the set of close annotations tends to be consistent at any particular mouse position. If you just try again to drag your target annotation, it will usually fail in the same way. The more annotations you have in your data set, the less likely that one you care about dragging is actually moved. After figuring out what was going wrong, I developed the following selection function with a fix for this matplotlib bug. In addition, I made another modification to improve selection for my application. You may or may not want to include this change in your application. |
|
A lot of this changed with #10776; can you check whether your patch is still needed with mpl 3.1? |
|
I tested with mpl 3.1.1, which includes (#10776), and found that this matplotlib release resolves fukatani's issue, and also the first part of my patch. Selection response time seems a bit faster when the first part of my patch is included. In my test scenario, I had about 500 annotations. With my filter, roughly 495 of those were eliminated after a single X axis comparison, because they were located outside of the current view. When I disabled the first part of my patch, the time cost of calling the updated version of contains(), for each of those annotations outside of the current view, had to be payed. The second part of my patch is not addressed within mpl 3.1.1. After disabling that second part, I tried to drag an inner annotation which was eclipsed by an outer annotation, and both annotations moved. When the second part of my patch is included with mpl 3.1.1, the area of collision between two or more annotations is greatly reduced, so it's less likely to accidentally drag multiple annotations. However, it's still possible in the case that the Text part of multiple annotations are coincident. This is where I really prefer the behavior change in fukatani's patch which restricts the selection down to a single annotation. Often, the reason I'm trying to drag an annotation is to eliminate a collision with another annotation. Moving both of them doesn't help resolve that situation. I prefer dragging just one annotation, even if it's the "wrong one". I can move it out of the way to separate it from the annotation I really want to move, and then separately drag them to their final positions. In my application, all of the annotations have Text associated with them, but I've realized that it's possible that someone could have annotations with no Text, so I've slightly updated the code in my second part to handle this scenario. This update will allow users to drag annotation arrows with no Text. |
|
If I understand correctly, the first part of your patch is basically assuming that the user would not want to pick annotations which point to somewhere outside of the field of view, even if the annotation itself is visible (which happens if annotation_clip is False). Certainly that seems a reasonable optimization, but needs to handle various additional cases (annotation_clip being False, xy being in axes rather than data coordinates, etc.). As for dragging multiple axes, I would rather have a more general mechanism for mouse events to be "swallowed" by a single event handler and not propagated to following ones, rather than having to code that separately in every handler. |
|
Good point. All of my annotations were using Data coordinates, and had clipping turned on. To generalize, the code would have to be modified to possibly transform the coordinate system, and to check for the clipping status. This would, of course, add overhead and reduce the level of time optimization. It's hard to know whether it's worthwhile, or not. I've been trying to come up with a way of measuring the time between the user click and the end of the selection, but haven't found one yet. I agree that it would be great to have a general mechanism to swallow a mouse event by a single handler. |
See #5839
Current draggable object can be moved unintentionally.
The reason of the issue is that
evt.artistchanged during dragging and objects to be moved are added.To fix it,
self.got_artistmust not change during dragging.i.e. once
evt.artist != self.ref_artist, self.got_artist will never becomeTrue.