vispy / jupyter_rfb

Remote Frame Buffer for Jupyter
https://jupyter-rfb.readthedocs.io
MIT License
58 stars 10 forks source link

Use QWheelEvent.pixelDelta() when appropriate #48

Closed j9ac9k closed 2 years ago

j9ac9k commented 2 years ago

Greetings from pyqtgraph-land,

We have started testing the usage of this plugin, and it's remarkable how much of the heavy lifting this does, this is an amazing tool, thanks for creating/maintaining/sharing it! If we meet in person at a conference or something, I will gladly purchase a beverage of your choice!

I have run into one issue, and I'm not sure if it's an issue in the pyqtgraph code-base or with jupyter_rfb extension, and that is mouse events via trackpad on macOS are very jumpy/not-smooth. The QWheelEvent docs give some guidance on the description of when QWheelEvent.pixelDelta() may want to be used instead of QWheelEvent.angleDelta() With QWheelEvent.pixelDelta() the distances are much smaller than .angleDelta() which is likely why zooming behavior feels so jumpy.

That said, I'm not sure if the issue is in the pyqtgraph code-base or not. I'll be happy to do testing on my end, especially if others don't have access to a macOS platform.

Here is how our implementation is at it stands:

import pyqtgraph as pg
from pyqtgraph.Qt import QtCore, QtGui, QtWidgets
import numpy as np
import jupyter_rfb

class PlotWidget(jupyter_rfb.RemoteFrameBuffer):
    def __init__(self, **kwds):
        super().__init__(**kwds)
        self.gfxView = pg.GraphicsView()
        self.plotItem = pg.PlotItem()
        self.plotItem.setMenuEnabled(False)
        self.gfxView.setCentralItem(self.plotItem)
        self.logical_size = 640, 480
        self.pixel_ratio = 1.0
        # self.gfxView.resize(*self.logical_size)
        # self.gfxView.show()       # showing pops up a window, even for offscreen
        # self.gfxView.resizeEvent(None)

    def get_frame(self):
        w, h = self.logical_size
        dpr = self.pixel_ratio
        buf = np.empty((int(h * dpr), int(w * dpr), 4), dtype=np.uint8)
        qimg = pg.functions.ndarray_to_qimage(buf, QtGui.QImage.Format.Format_RGBX8888)
        qimg.fill(QtCore.Qt.GlobalColor.transparent)
        qimg.setDevicePixelRatio(dpr)
        painter = QtGui.QPainter(qimg)
        gv = self.gfxView
        gv.render(painter, gv.viewRect(), gv.rect())
        painter.end()
        return buf

    def handle_event(self, event):
        event_type = event["event_type"]

        MB = QtCore.Qt.MouseButton
        KM = QtCore.Qt.KeyboardModifier

        if event_type == "resize":
            self.logical_size = int(event["width"]), int(event["height"])
            self.pixel_ratio = event["pixel_ratio"]
            self.gfxView.resize(*self.logical_size)
            self.gfxView.resizeEvent(None)
        elif event_type in ["pointer_down", "pointer_up", "pointer_move"]:
            pos = QtCore.QPointF(event["x"], event["y"])
            mblut = [MB.NoButton, MB.LeftButton, MB.RightButton, MB.MiddleButton]
            btn = mblut[event["button"]]
            btns = MB.NoButton
            for x in event["buttons"]:
                btns |= mblut[x]
            kmlut = dict(Shift=KM.ShiftModifier, Control=KM.ControlModifier, Alt=KM.AltModifier)
            mods = KM.NoModifier
            for x in event["modifiers"]:
                mods |= kmlut[x]
            QET = QtCore.QEvent.Type
            typlut = dict(pointer_down=QET.MouseButtonPress, pointer_up=QET.MouseButtonRelease, pointer_move=QET.MouseMove)
            typ = typlut[event_type]
            evt = QtGui.QMouseEvent(typ, pos, pos, btn, btns, mods)
            QtCore.QCoreApplication.sendEvent(self.gfxView.viewport(), evt)
            self.request_draw()            
        elif event_type == "wheel":
            pos = QtCore.QPointF(event["x"], event["y"])
            pixdel = QtCore.QPoint()
            angdel = QtCore.QPoint(int(event["dx"]), int(event["dy"]))
            btns = MB.NoButton
            mods = KM.NoModifier
            phase = QtCore.Qt.ScrollPhase.NoScrollPhase
            inverted = False
            evt = QtGui.QWheelEvent(pos, pos, pixdel, angdel, btns, mods, phase, inverted)
            QtCore.QCoreApplication.sendEvent(self.gfxView.viewport(), evt)
            self.request_draw()

#QAPP = pg.mkQApp()
if QtWidgets.QApplication.instance() is None:
    QAPP = QtWidgets.QApplication([])
djhoese commented 2 years ago

Do you see this in all browsers? I would guess that this would be something on the javascript side wouldn't it?

https://github.com/vispy/jupyter_rfb/blob/73fd4cdd94427bc7add86b38c8f374cbc0ca9dfd/js/lib/widget.js#L260-L287

j9ac9k commented 2 years ago

Do you see this in all browsers? I would guess that this would be something on the javascript side wouldn't it?

I just tried safari and chrome; I can't tell if it's working as intended and there is just some kind of performance penalty in place because of "web"; I don't have a good way to query what the properties of the QGraphicsSceneWheelEvent are that are passed into our ViewBox:

https://github.com/pyqtgraph/pyqtgraph/blob/master/pyqtgraph/graphicsItems/ViewBox/ViewBox.py#L1208-L1221

I can say that the .delta() method does display the "correct" values (whether using a trackpad or using a mouse) when this is run outside of jupyter notebook.

j9ac9k commented 2 years ago

I created some videos, but the jumpiness isn't quite as noticeable

jupyter_rfb

https://user-images.githubusercontent.com/646398/139903057-5c98b441-5bcc-4961-8d3a-213483b8f5ee.mov

pyqtgraph example

https://user-images.githubusercontent.com/646398/139903410-e5646779-b768-4ef6-8839-bdf5d034164c.mov

almarklein commented 2 years ago

Hi Ogi! Thanks for working on on integrating jupyter_rfb with PyQtGraph!

it's remarkable how much of the heavy lifting this does

That has been the plan, and its great to hear that it has worked out :)

I have run into one issue, and I'm not sure if it's an issue in the pyqtgraph code-base or with jupyter_rfb extension, and that is mouse events via trackpad on macOS are very jumpy/not-smooth

There may be two things at play here:

  1. The value of dy corresponding to one "scroll step". This value is notoriously different between GUI toolkits, so I can imagine that the value from JS does not match Qt. This is something that I hope we can document better: what values of dx and dy one can expect, and how then can be scaled to match e.g. Qt.

  2. To prevent a scroll action from spamming the IO and server, the scroll events are throttled. See the code that David mentioned. It's now set at 100ms, which means that a scroll events happen no more than 10x per second.

Do you have an idea which of these (or a combination) is the main problem? It should be fine to decrease that throttling a bit. If (1) is also an issue, you could simply multiply the value of dy. Would be nice to find some theory of what number to multiply with though :)

j9ac9k commented 2 years ago

From eyeballing it, I suspect I'm hitting the rate limitation here, as the steps appear to be in line with an image that updates at 10fps.

How would I go about modifying the throttling here? Would I just open the Widget.js file and manually edit it, or is there a preferred solution?

almarklein commented 2 years ago

Well, for investigating this issue that should be fine. Note that you need to have jupyter_rfb installed in developer mode though: https://jupyter-rfb.readthedocs.io/en/latest/contributing.html

If you could do that to find out what value works smooth enough, e.g. 50 or 20, then we should probably just change that here.

j9ac9k commented 2 years ago

Well, for investigating this issue that should be fine. Note that you need to have jupyter_rfb installed in developer mode though: https://jupyter-rfb.readthedocs.io/en/latest/contributing.html

If you could do that to find out what value works smooth enough, e.g. 50 or 20, then we should probably just change that here.

Having effectively never done javascript, configuring jupyter_rfb in development mode sure produced a lot of console output! Thanks for the great instructions, it worked like a champ.

I changed line following line

https://github.com/vispy/jupyter_rfb/blob/73fd4cdd94427bc7add86b38c8f374cbc0ca9dfd/js/lib/widget.js#L284

to

                window.setTimeout(send_wheel_event, 17);

This number representing ~60 fps which is in line with what most monitors can do. I can confirm that the zoom smoothness is now perfect.

I recognize this is a much bigger impact on the server end, I wish I could provide guidance/input on what the "cost" of having more rapid upgrades here. I'd be happy to do any further testing if it would be of help.

https://user-images.githubusercontent.com/646398/140007205-a7fcc4f7-3df4-4c75-9afa-4eca187504af.mov

j9ac9k commented 2 years ago

I'm thinking more and more about this, and this is definitely a trackpad issue, ...trackpads send a series of wheel events with very small distances, but very rapidly. A typical mouse wheel operates a bit more incrementally at large (but fixed) distances; I'm not sure it would be worth increasing the toll on the server on behalf of trackpad users...

almarklein commented 2 years ago

Its ok if we set it to 20 which is the same as what is used for pointer events. It was set to a higher value, because I have not tested touchpad scrolling much, and mouse scrolling suffers much less from the throttling.

So would this fix the original issue? Can you feed dy (without scaling the value) into Qt's event system and get behavior that feels the same as running natively?

j9ac9k commented 2 years ago

So would this fix the original issue? Can you feed dy (without scaling the value) into Qt's event system and get behavior that feels the same as running natively?

I think changing the timeout alone fixes the issue. Here is a better video to highlight the smoothness when using the trackpad with a timeout set to 17.

https://user-images.githubusercontent.com/646398/140137091-a6f3f874-c7d6-4380-965e-c880159874c3.mov

Qt's event system is able to differentiate between regular mouse wheel events and trackpad events; and when the QWheelEvent is converted to a QGraphicsSceneWheelEvent it seems to handle the conversion in such a way that our scaling formula such that we don't have to differentiate between the two different sources.

In the implementation in our PR to implement this, we do not apply any scaling to dy

https://github.com/pyqtgraph/pyqtgraph/blob/5d50c4dbb6a089d09789af17d593f272ee89b0df/pyqtgraph/jupyter/GraphicsView.py#L85-L95

It seems to "just work", not sure if we can extract whether the wheel event is "inverted" or not, that may be beneficial as well; but that's a separate issue.

almarklein commented 2 years ago

not sure if we can extract whether the wheel event is "inverted" or not

Inverted?

j9ac9k commented 2 years ago

not sure if we can extract whether the wheel event is "inverted" or not

Inverted?

https://doc.qt.io/qt-5/qwheelevent.html#inverted

This is most common on machines that have reverse direction for trackpad scrolling. On macOS this is called "natural" scroll direction. When testing out jupyter_rfb, we realized that pyqtgraph doesn't actually look at this property, despite being present in QWheelEvent and more importantly, QGraphicsSceneWheelEvent (we'll likely modify pyqtgraph to look at that).

That said, I'm not seeing a reference to the inverted property in the JavaScript WheelEvent docs so probably nothing for jupyter_rfb to do.

pijyoi commented 2 years ago

Would like to add that this "inversion" is neither limited to macOS nor to trackpads. From https://doc.qt.io/qt-5/qwheelevent.html#angleDelta, A positive value indicates that the wheel was rotated forwards away from the user; a negative value indicates that the wheel was rotated backwards toward the user..

With the events from jupyter_rfb, the dy values are reversed. i.e. I get negative values when the wheel is rotated forwards away from the user. This also manifests itself with a trackpad, pinch-out / pinch-in zoom has the reverse effect from a regular Desktop application.

I don't think using the inverted property and relying on the downstream application to read it to react appropriately would be the correct solution.

almarklein commented 2 years ago

Whether or not the delta is inverted is an OS system setting. I think this is also why you cannot see in JS whether its reversed.

we realized that pyqtgraph doesn't actually look at this property

I don't think you should - if this is how the system is configured, then that's what you should work with, otherwise you break the expectations of the user. The only exception is when you specifically want to know what way the wheel turned.

almarklein commented 2 years ago

This also manifests itself with a trackpad, pinch-out / pinch-in zoom has the reverse effect from a regular Desktop application.

Oh this is interesting! When you pinch, its much more obvious what should be zoom-in and zoom-out. A pinch gesture is translated (by the OS/GUI/browser) into a wheel event.

But are you saying that the dy values from jupyter_rfb are reversed compared to the dy values from a wheel event when pinching in Qt?

almarklein commented 2 years ago

Ouch, I can confirm this. Will do some more research and update here.

What I found on Windows 10, with natural-touchpad-scroll:

Qt:          pinch-zoom pos, wheel-away pos, pad-away neg
glfw:        pinch-zoom pos, wheel-away pos, pad-away neg
jupyter_rfb: pinch-zoom neg, wheel-away neg, pad-away pos

Can someone please confirm (some of) this on MacOS?

Notes:

Discussion:

What I think happened, is that in the early days (with JS mousewheel_event) different browsers used different values for the deltas. This was fixed in the wheel_event API, but its different from what most GUI toolkits apply.

I think it makes sense for jupyter_rfb to follow the JS standard. This means that in your PyQtGraph Jupyter widget, you'd need to use -dy.

almarklein commented 2 years ago

Looks like dx is also reversed, btw.

almarklein commented 2 years ago

PR in progress: #49

Also note that JS (and jupyter_rfb) use wheel steps of 96, whereas Qt uses wheel steps of 120, so you might want to scale with -1.25.

j9ac9k commented 2 years ago

Can someone please confirm (some of) this on MacOS?

I'll try and do this later today; my day is sort of packed 😬

almarklein commented 2 years ago

No rush :)

j9ac9k commented 2 years ago

@almarklein would you still like me to check the natural touchpad scroll on macOS? I may need some help figuring out in jupyter_rfb which way it reads

almarklein commented 2 years ago

Yeah it would be good to have an extra confirmation, to see if the scrolling with jupyter_rfb is the same as running pyqtgraph natively, if the dy is reversed.