PlotItemOverlay: an alt approach for #1359

[goodboy]

This is a competing approach to #1359 for "multi-axis" support.

The main difference is not introducing any new apis or inherited types (PlotWidget) and to instead use existing subsystems (with minor internal changes) and a composed PlotItem overlay approach which keeps support for custom user defined subtypes (such as ViewBox, other GraphicsObject, etc.).

---

Notes:

• this is not yet at feature parity with Added multi axis plots #1359
  • no 'left', 'bottom' axis layout stacking yet (but this is trivial)
    • prettty sure the QGraphicsAnchorLayout is the right tool for this job 😉
  • no label retrieval and layout insertion yet (also trivial)
  • there is a bug with click-drag interaction on axes which seems to be something that needs to be fixed in the ViewBox-AxisItem rats nest of "linked" handlers 😂
• yes i know you probably don't like the Python 3.9+ annotations, we can remove them once we're ready to make the code hard to read and ready to land 😉

---

Improvements over #1359

• each plot item keeps it's own ViewBox which means multiple implementations can be use together (or passed in by the user)
• no ad-hoc rerouting of interaction signals (where possible) to some singleton view
• linkable axes controls for either x, y or both

The current issues there last I checked were:

• initial autorange is not correct
• multiple signals received by the charts in the back when interacting with the top level one making them move more than expected
  • figured this out! turns out you just have to ignore calls to any handler that was a broadcasted call from some source ViewBox, the details of why this is an issue is gets fairly involved:
    • using ViewBox.linkView() does it's own set of signal/slot-broadcasting configuration which can result in handlers being called twice, which in this case is true since ViewBox.linkView() will also call .setXRange()..

Afaict both of these are solved in this approach. Minus unimplemented features, the only bug here is the axis click-drag support which seems to be an internal bug to do with signal relaying.

The trick for overlaying view box code was definitely used from #1359 which you'll see commented.

See the list of out-standings in the module for more details.

---

Changes to ViewBox and PlotItem

I will make comments alongside the code below.

---

Very interested to get a detailed discussion on this approach 🏄🏼 😂 🥸
ping @j9ac9k @outofculture @pijyoi

[goodboy]

Ignore this stuff since it's from the history in #2160.

[goodboy]

This is a pretty serious design thing I noticed.
I don't grok at all why all axes must always be created other then that when you don't create them the 'bottom' axis geometry seems to still be used to in the view calculations resulting in .viewRect() style calcs to compute wrong?!

[goodboy]

The semantics of set IMO should be write, not mutate. If you want to add then we should have a addAxisItems().

[goodboy]

I don't understand any of these comments.
There's weird defaults for weird defaults, special cases for skipping this axis and that axis.

It's all highly confusing and doesn't represent the semantics of the method name.

[goodboy]

We can probably just call a decorator version of this around all (user-defined) signal handlers right?

[goodboy]

Woops was supposed to be set to False; though I guess that's default 😂

[goodboy]

We should be able to drop this if we make PlotItem.setAxisItems() actually be a pure write operation.

[goodboy]

Can all be dropped.

[goodboy]

can also all be dropped.

[goodboy]

Gah missing some patches from #1359, notably ViewBox.sigMouseDragged and functions.connect_lambda.

[goodboy]

Lol, Ok so apparently the history from #1359 was more extensive then I thought and is needed to make the example work out of the box.

Might try to get this working on master if not will bring in that history.

[goodboy]

There it's back to where I expected.

[goodboy]

there is a bug with click-drag interaction on axes which seems to be something that needs to be fixed in the ViewBox-AxisItem rats nest of "linked" handlers

Ahh think I know what this is; we need a relay signal for axis adjustments and likely independently as in #1359 as well. Though I'm confused why it didn't work on that history either?

[goodboy]

Pulled verbatim from original commit on #1359.
Still need to cherry the last couple commits from that branch that adjusted docs iirc.

[goodboy]

Actually, so now having tested this a bunch in a parent project, I'm pretty sure it's not needed?

@0000matteo0000 what was the original leak you found which motivated using this?

[0000matteo0000]

storing self in a lambda or in any object and then storing that object in self (self.var = {"self": self}) will break python garbage collection. this is how i test it:

import weakref
import gc

import numpy as np
import pyqtgraph as pg
from pyqtgraph.Qt import QtWidgets
app = pg.mkQApp()
pg.setConfigOption('background', 'w')
pg.setConfigOption('foreground', 'k')

mpw = pg.MultiAxisPlotWidget()

# LEGEND
mpw.addLegend(offset=(0, 0))
# TITLE
mpw.setTitle('MultiAxisPlotWidget Example')
# AXYS
mpw.addAxis('sx1', 'bottom', text='Samples1', units='sx1')
mpw.addAxis('sx2', 'bottom', text='Samples2', units='sx2')
mpw.addAxis('sx3', 'bottom', text='Samples3', units='sx3')
mpw.addAxis('sy1', 'left', text='Data1', units='sy1')
mpw.addAxis('sy2', 'left', text='Data2', units='sy2')
mpw.addAxis('sy3', 'left', text='Data3', units='sy3')
# CHARTS
mpw.addChart('Dataset 1', xAxisName='sx1', yAxisName='sy1')[0].setData(np.array(np.sin(np.linspace(0, 2 * np.pi, num=1000))))
mpw.addChart('Dataset 2', xAxisName='sx2', yAxisName='sy1')[0].setData(np.array(np.sin(np.linspace(0, 2 * np.pi, num=1000))) * 2)
mpw.addChart('Dataset 3', xAxisName='sx2', yAxisName='sy2')[0].setData(np.array(np.sin(np.linspace(0, 4 * np.pi, num=500))) * 3)
mpw.addChart('Dataset 4', xAxisName='sx3', yAxisName='sy3')[0].setData(np.array(np.sin(np.linspace(0, 4 * np.pi, num=500))) * 3)
# make and display chart
mpw.makeLayout()
mpw.update()
mpw.show()
pg.exec()

refs = [
    weakref.ref(mpw),
    weakref.ref(mpw.pi),
    weakref.ref(mpw.vb),
    weakref.ref(mpw.layout),
    weakref.ref(mpw.pi.legend),
    weakref.ref(mpw.charts['Dataset 1']),
    weakref.ref(mpw.axes['sx1']),
]
del mpw
for ref in refs:
    print(ref(), 'should be None')
gc.collect()
for ref in refs:
    print(ref(), 'should be None')

[goodboy]

typo: duplicate wording.

[goodboy]

I want to see it work like this.

[0000matteo0000]

i looked at your code and tested your example, it seems it needs some work still, and it's not quite mature, but i think there is some good intent.
fixing the spaghetti in axisitem and viewbox i agree should be done, but we need to keep in mind that changing the api is probably not an option since it would mean reworking many other classes that depend on them.
i still think #1359's approach is better for the simple reason it almost does not touch the internals of the library except for adding some signals and some minor edits.
the most critical point in simplifying my approach is changing how events and signals are routed,
events some time just call other event-handling methods in other classes instead of emitting signals that are then connected to such methods properly, and some signals do not include information to support multiple axes correctly.
this is why implementing multiple axis was such a pain and still the issues in my pr remain.
i think your approach is a little over-confident and you should not try to remake what i made already and instead you focus on fixing the pipework in the core library so that the widget in my pr does not need the _connect_signals magic.
mostly this would mean, as i previously hinted, making each event throw signals, connect those signals to the correct slots, making sure that viewbox and axisitem are actually separate entities.
another reason to implement a multi-plotitem instead of multi-viewbox is that this means all the features in the plotitem are pretty much supported out of the box with minor tweaks, this means legend, auto-scale button, grids, and many other things that are eye candy maybe but needed for shure and currently working in my pr (you surely noticed i almost do not touch the plotitems after creation).
supporting these features fully would mean changing plotitem in such a way so that it supports basically two modes: master and slave, so that the master plotitem will accept other slave plotitems with most of their features delegated to the master one that should then track the state of the slave ones and act accordingly.
this changes in conjunction with the, i think, simple to use api of the widget in my pr should make the multi axis feature complete.

[goodboy]

i looked at your code and tested your example, it seems it needs some work still, and it's not quite mature, but i think there is some good intent.

Thanks 🏄🏼 !

i think your approach is a little over-confident and you should not try to remake what i made already and instead you focus on fixing the pipework in the core library so that the widget in my pr does not need the _connect_signals magic.

Yes! I totally agree. I actually have narrowed down the main issue to a couple things:

• pyqtgraph basically modifies the normal behaviour of Qt's event routing by nearly re-implementing all of QGraphicsScene and event processing semantics 😂 (the original reason for this seems very very legacy). This re-implementation is partially the source of why my overlay/relay approach doesn't work as easily as you'd expect - QEvent.accept() actually short circuits the custom (written in python) event loop in pyqtgraph which is not the normal Qt processing behaviour IIRC.
• Further, all of ^ could be bypassed if we can overlay the view boxes in a given order - currently this is happening in the exact opposite of the desired overlay order (i.e. in my example 'Data 2' is the last overlay and its ViewBox receives events first from the event loop...)
  • if we can get the root to be the view box that accepts input first then this view box relay approach will work very well.
  • there is some additional odd behavior in the event loop i haven't found the source of where if you re-.emit() an event from one of the views it is actually cutting the mouse drag state short? Super super weird.

mostly this would mean, as i previously hinted, making each event throw signals, connect those signals to the correct slots, making sure that viewbox and axisitem are actually separate entities.
another reason to implement a multi-plotitem instead of multi-viewbox is that this means all the features in the plotitem are pretty much supported out of the box with minor tweaks, this means legend, auto-scale button, grids, and many other things that are eye candy maybe but needed for shure and currently working in my pr (you surely noticed i almost do not touch the plotitems after creation).

Totally agree. This was exactly my original quiff with the other PR. We already have APIs for all the axis management and graph annotations/labels - we should leverage that and just make the event and layout system work around it.

making each event throw signals, connect those signals to the correct slots, making sure that viewbox and axisitem are actually separate entities.

I haven't pushed it yet, but the signals you added for mouse drag and wheel I think should be mouseDragRelay and mouseWheelRelay and these can then be hooked by any widget looking to plug into the view's input handling. In this case it's other views / plot items and we can simply add a optional kwarg to the mouseDragEvent such as relayed_from: ViewBox which the handler can use to detect if the signal was sent from some other view (ideally a lone broadcaster / producer which is the top level widget which first receives input events).

supporting these features fully would mean changing plotitem in such a way so that it supports basically two modes: master and slave, so that the master plotitem will accept other slave plotitems with most of their features delegated to the master one that should then track the state of the slave ones and act accordingly.

Yup exactly. I would prefer to call it focussed and overlaid plot items since there should be one top level widget that broadcasts to the others.

this changes in conjunction with the, i think, simple to use api of the widget in my pr should make the multi axis feature complete.

Sounds good 😎 . I want to keep working on this anyway just to improve my own knowledge of the pyqtgraph code base core and I need it for my own parent project.

I'll keep you posted on progress for sure :)
Also, it might be good if you hopped in the slack room so we can interact in real-time if you want.

[goodboy]

Yeah just to clarify the main problem that i'm seeing with the event loop even just to remind myself:

• the scenes containing each view are layered in the view in the opposite desired order such that the last overlayed view is receiving events first.
  • no idea yet how to change this, but if we can set a scene order up front then this whole event loop issue (to be described below) should go away
• when the last overlayed view receives the first event, if it MouseDragEven.ignore()'s it, that will short circuilt the processing for that view. It also seems that the event loop keeps memory of that and won't re-fire that same event to that view due to the some kind of either "drag items" state tracking or bad event loop logic (though I don't want to claim too much yet until I've stepped through it proper).

[goodboy]

I ended up taking a slightly different approach with the label stacking (since I personally think the defaults are far too big and invasive) but for all other intents and purposes this is working as expected minus one outstanding issue: when you link all the axes panning via click-drag results in N times the movement per mouse input move. I haven't quite figured out the source of this issue but it's likely due to recursive calls in the ViewBox mouse handler that somehow aren't filtered correctly.

For those interested the first draft implementation is in pikers/piker#257.
I will keep this open because the example is likely a key driver to getting this brought back into this repo but for now we will not be working on this branch as the client project it is needed for is much faster moving with fewer legacy Python support constraints.

Please feel free to reach out when ready and we'll do our best to resume this 😄

[goodboy]

As an update, I've reworked this original code to discard all signal/slot rewiring to instead use a python broadcast loop in pikers/piker#414.

This turned out to be vastly easier to understand and implement and appears to have zero performance hit.
The new implementation solves all prior interaction quirks/bugs and clears up the differences between axis linking versus event handler broadcasting allowing for much finer grained controls over pub-sub mechanics particularly when an event handler itself fires signals.