[Bug]: scroll_event is broken after motion_notify_event in WXAgg

komoto48g commented 2 years ago

Bug summary

Scrolling the mouse wheel returns a broken xy value. After any key is pressed, the xy value will be correct during no motion.

Code for reproduction

import matplotlib
from matplotlib.backends.backend_wxagg import FigureCanvasWxAgg as FigureCanvas
from matplotlib.backends.backend_wxagg import NavigationToolbar2WxAgg as Toolbar
from matplotlib.figure import Figure
import wx

class Plugin(wx.Panel):
    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)

        self.figure = Figure(facecolor='white', figsize=(.1,.1)) # inches
        self.canvas = FigureCanvas(self, -1, self.figure)
        self.toolbar = Toolbar(self.canvas)

        self.SetSizer(wx.BoxSizer(wx.VERTICAL))
        self.Sizer.AddMany((
            (self.canvas, 1, wx.EXPAND|wx.ALL, 0),
            (self.toolbar, 0, wx.EXPAND|wx.ALL, 2),
        ))

        self.canvas.mpl_connect('scroll_event', print)
        self.canvas.mpl_connect('motion_notify_event', print)

        self.figure.clear()
        self.figure.add_subplot(111)

if __name__ == '__main__':
    app = wx.App()
    frm = wx.Frame(None)
    frm.panel = Plugin(frm)
    frm.Show()
    app.MainLoop()

Actual outcome

When you scroll on the canvas, you will see corrupted data such as the following:

scroll_event: xy=(32761, 231) xydata=(None, None) button=down dblclick=False inaxes=None`
                  ^^^^^ corrupted

Expected outcome

After any key is pressed (but no motion), you will see a message such as the following:

scroll_event: xy=(153, 107) xydata=(0.35..., 0.45...) button=up dblclick=False inaxes=AxesSubplot(0.125,0.11;0.775x0.77)
                  ^^^ ok

Additional information

wxPython 4.1.1 msw (phoenix) wxWidgets 3.1.5 backend_bases.py:1211: (3.5.x) backend_bases.py:1229: (3.4.x)

class Event:
    def __init__(self, name, canvas, guiEvent=None):
        self.name = name
        self.canvas = canvas
        self.guiEvent = guiEvent
        ^^^^^^^^^^^^^^^^^^^^^^^^ Don't keep reference to the wx.Event object.
                                 !! Otherwise it would be a potential bug.

To fix this issue temporarily, change it to dummy data. So far, self.guiEvent doesn't seem to be referenced anywhere.

-        self.guiEvent = guiEvent
+        self.guiEvent = None

A related issue is discussed in the Phoenix issue tracker. https://github.com/wxWidgets/Phoenix/issues/2034

Operating system

Windows 10

Matplotlib Version

3.4.0, 3.5.1

Matplotlib Backend

WXAgg

Python version

3.8.6, 3.9.9

Jupyter version

No response

Installation

pip

anntzer commented 2 years ago

guiEvent is public API and would need a deprecation period; furthermore, looking at the linked wx issue, it seems like this is a bug on wx's side? Alternatively we can use the evt.__class__(evt) hack mentioned in the thread, but "official" guidance from the wx project would be nice.

komoto48g commented 2 years ago

Thank you for reviewing.

it seems like this is a bug on wx's side?

I am not sure about this, so I posted this topic in [Discuss wxPython] and asked for feedback or official guidance.

DietmarSchwertberger commented 2 years ago

Whether there is a bug involved is not yet clear. I have had a look at the C++ code, but that's very complex. There could be an issue in the wxWidgets <-> sip interaction, but that's beyond my capabilities... Anyway, it's no good style to keep references to short lived objects. For PaintDC it's explicitely documented that no reference is to be stored. For Event, it's not (yet) documented.

The version evt.__class__(evt) or evt.Clone() would care for case that someone copies guiEvent, but I would say this is not a useful use case.

I think the best solution would be delete the reference at the end of the matplotlib event handling.

E.g. in backend_bases.py motion_notify_event add one of these lines at the end:

        event.guiEvent = None
or:
        del event.guiEvent

I would recommend the del call. This should be done for all Event types, even if there is currently no issue. Unfortunately there seems to be no hook which would cover all event handlers at once. (Adding Event.__del__ would not work.)

komoto48g commented 2 years ago

Thank you for taking the time to review.

The version evt.__class__(evt) or evt.Clone() would care for case that someone copies guiEvent, but I would say this is not a useful use case.

I think the best solution would be delete the reference at the end of the matplotlib event handling.

If someone holds the reference, even if del event.guiEvent is called, it will only decrement the ref-counter. I recognized that the behavior of wx is by design, not a bug. I think the best solution would be to make guiEvent API deprecated.

There are resonable reasons to make (problematic) guiEvent API deprecated.

For developers, guiEvent is not referenced anywhere in the matplotlib/WXAgg module, thus it has little impact.

For users, guiEvent is not very useful because it is easy to chain the event and refer it, e.g.,

self.canvas.mpl_connect('motion_notify_event', self.on_motion_notify)
self.canvas.Bind(wx.EVT_MOTION, self.OnMotionNotify)
...

anntzer commented 2 years ago

FWIW I have relied on guiEvent e.g. at https://github.com/anntzer/mplinorm/blob/f0d340b8cc63b7b000e18a466f53671ac92e7b69/lib/mplinorm.py#L163-L194. There may be workarounds but some documentation on how to achieve them would be nice (e.g. in my case I need a correspondence between the motion_notify_event handled by Matplotlib and the EVT_MOTION from wx).

tacaswell commented 2 years ago

I am pretty :-1: on deprecating evt.guiEvent because that is our escape hatch to get the event from the underlying UI through to callbacks registered to Matplotlib. It is indeed a brittle API (as the type is dependent on which backend you are using), but it is still useful and present on all of the GUI backends.

The source of our problem is https://github.com/matplotlib/matplotlib/blob/96af8c52c7beb373c1970cd71916e0c6e1231db7/lib/matplotlib/backend_bases.py#L1298 and https://github.com/matplotlib/matplotlib/blob/96af8c52c7beb373c1970cd71916e0c6e1231db7/lib/matplotlib/backend_bases.py#L1332-L1356 which cache the last event as a class level attribute on Location event when we enter / leave an Axes.

I think possible fixes here are:

clone the Event while stripping the GUI event when we stash LocationEvent.lastevent (bunch of work, maybe not easy to do given that this logic is currently called from the __init__ method...maybe that should move?)
do not forward WX guiEvents on to the user (odd in that it makes wx different than the other GUI backends, but would only affect wx users and keep the fallout contained)
deprecate LocationEvent.lastevent and only stash the last axes?

I am not a fan of deleting or setting to None the guiEvent attribute as if the user has held onto the mpl event object, mutating it under them seems rude.

anntzer commented 2 years ago

Deprecating lastevent certainly seems reasonable.

matplotlib / matplotlib