experimental line drawing mode for thick lines#2011
Conversation
|
@markotoplak this was something you previously mentioned being interested in. If you could give this PR a spin that would be greatly appreciated! |
b8ec390 to
f5e702c
Compare
|
Tagging some other folks that have expressed interest in lines with > 1.0 pixel thickness. |
|
From #533 (comment), there is a link to https://stackoverflow.com/questions/46206296/performance-issues-with-pyqtgraph By adding plot_item = pg.PlotCurveItem(x, y, pen=pen, skipFiniteCheck=True) |
|
Thanks for working on this. I'll check it out! |
PlotSpeedTest.py (width=2) showed a drop from 97 fps to 67 fps when switching to RoundCap. I am thinking perhaps this mode should be made even more opt-in by way of |
|
If we compare with QtCharts, their implementation uses I was able to trigger the various other plotting artifacts (Alpha and CapStyle) with PlotSpeedTest_qtcharts.py (https://gist.github.com/pijyoi/c8a2eb963a36a5d5bb14cc3b75a8bab0) |
|
The modified-to-use thick pens examples/crosshair.py is an example where this PR makes user-interaction feasible. |
|
It seems to me that the reason this Perhaps |
|
I fully support the idea of moving the logic/implementation to
Definitely, but this change should happen sooner rather than later, we're going to need a long deprecation period here. Suppose we should make sure there is a consensus that we migrate the |
|
I support moving stepmode to PlotDataItem. |
I don't think it's necessary to hide it behind EDIT: was on mobile, but wanted to expand more about my indecisiveness.... I see two major reasons for putting things behind
I think point 1, we do not need to worry about. The current implementation that this PR replaces is entirely non-functional. We would be hard pressed to make things worse. To point 2, we stumbled across the alternative drawing method as a way to get around this major performance hit. We're still trying to find out what the limitations are both on Qt's end, and with respect to integration with our library. We're going to move changing things as we test more use-cases, and we don't want end-users surprised here. Anyway, I stand by my statement here that I'm good with whatever @pijyoi thinks is appropriate 👍🏻 |
|
My mild concerns are exactly the 2 points listed by @j9ac9k . That said, now that 0.12.3 has been released, it should be safe to remove |
|
|
|
I think the expectation would be that ShadowPen should become quite a bit more usable, since it is kind of pointless with a single-pixel width. :) (We should double-check that, though. I played around with it, but didn't benchmark for speed.) @sem-geologist , could you let us know what application you might have for a stepMode plot with many points? |
|
I managed to get the thickpen codepath to handle import pyqtgraph as pg
import numpy as np
pg.mkQApp()
pw = pg.PlotWidget()
pw.show()
rng = np.random.default_rng()
y = rng.random(10000)
x = np.arange(len(y)+1)
killer_opts = dict(fillLevel=0, fillOutline=True, brush=(0,0,255,150))
pw.plot(x, y, stepMode="center", pen=pg.mkPen(width=2))
pg.exec()What kills performance but is out of scope of this PR are the fill-related options in |
There is one usage of shadow pen in examples/PlotWidget.py with number of samples set to 100. |
|
@NilsNemitz , stepMode is good to have for spectral data (what is what I am doing mostly, X-ray spectra I'm talking about). This normally is not more than 4096 (12-bit ADC, thus such round number) channels, where each represents different radiation energy window. It actually is something like histogram, but has exactly strictly set width and number of the "bin"s, and is intended to be zoomed-in and out during visual inspection of data, as normally there can be hundreds of peaks. |
|
@pijyoi , In any case, the plot seems to run slower by a factor of four or so after adding Now, this is clearly a far out fringe case, but it might support adding an override to bypass the thick line code. Maybe a "doNotSegment" flag might work? And just because I already looked it up: # segmenting the curve slows gradient brushes, and is expected to do the same for other patterns
and pen.brush().style() == QtCore.Qt.BrushStyle.SolidPattern |
|
@sem-geologist , In my opinion, part of the awkwardness of stepMode is that PlotCurveItem is typically tasked with throwing raw data at the screen as fast as possible. For stepMode, extra points have to be inserted, which isn't really part of that. The end result is this weird compromise where you have to pre-mangle the data, so that PlotCurveItem can post-mangle it into the plot you want. PlotDataItem has a more structured separation of input, processed, and ready-to-show data, and would be much better suited to the task. How much of a slowdown are you seeing with PlotDataItem? For a single curve with many points, I am getting maybe about 15% slowdown here, but there might be additional per-plot overhead that breaks things in your application. The next time you come across an example that breaks PlotDataItem particularly hard, could you post it? |
After setting it to 100_000 samples, the optimization makes it go from "doesn't even show for 17 seconds" to "I can actually interact with this." This makes the shadowPen usable in so many additional situations that we should probably fix the issue that LegendItem doesn't even know it exists. :) |
@NilsNemitz has confirmed a performance penalty for using As already mentioned I can see how PlotDataItem would feel heavy, given the size of the docs-page, doc-strings, and so on; but that is considered the primary entrance point for data into pyqtgraph, and we accept data in various data-structures, so with that, there is going to be a fair amount of code no matter what. That said, the current implementation of stepMode is in PlotCurveItem, and we try not to make breaking changes to the public API unless necessary, and via some kind of deprecation warning process. In other words, I wouldn't let a potential future deprecation warning interfere with your planned development. Step mode will be remaining in the library, it just might move to PlotDataItem at some point in the future is all. |
|
@j9ac9k , |
|
All four connect modes are supported now. There is in fact no in-library usage of |
|
Following script is to verify that import pyqtgraph as pg
from pyqtgraph.Qt import QtGui
pg.mkQApp()
pw = pg.PlotWidget()
pw.show()
curve = pg.PlotCurveItem()
pw.addItem(curve)
ey = [v * 1e-12 for v in [1.1, 1, 0.9]]
x = []
y = []
path = QtGui.QPainterPath()
for idx, e in enumerate(ey):
x0 = float(idx)
y0 = 1.0 + e/2
x1 = float(idx)
y1 = 1.0 - e/2
x.extend([x0, x1])
y.extend([y0, y1])
path.moveTo(x0, y0)
path.lineTo(x1, y1)
curve.setData(x, y, connect="pairs", pen='r', shadowPen=pg.mkPen('c', width=10))
print(path.elementCount())
for idx in range(path.elementCount()):
elem = path.elementAt(idx)
print(elem.type, elem.x, elem.y)
pg.exec() |
|
For edge-case testing this PR, considering modifying def _shouldUseDrawLineSegments(self, pen):
return (
pen.widthF() > 1.0It fails |
Would you like me to make a new image? |
No, it was expected to fail. The segments are correct, it's just the joints are very slightly different. |
Ahh, I misread your quote as the test failing; but clearly it's not as CI is all green. It is failing for line thickness being 0.0 and the |
|
Just ran the speed test for line plot update, @pijyoi I can't believe what I'm seeing; over 100 fps with line plot width > 2 (with default settings); this is absolutely amazing, thank you so much for your work on this. moving the stepMode out to a static method is great, as this will make it easier to migrate to PlotDataItem in the future so a huge 👍🏻 from me there too. The one question I have is what do you think about moving LineSegments to |
Changed this in the application I develop internally at my company; it works exactly the same 👍🏻 |
functions.py is currently too bloated. There are some large tables and some specialised niche functions in it that should move out to slim it down. What parts of the library do you have in mind? Couldn't they use PlotCurveItem? I suppose this decision to refactor can be left to the first PR that wants to make use of it? The class is also slightly low-level. It returns a list of objects that don't own their memory. So we may not want to make it so accessible to outside the library. |
|
I just wanted to say that I very much agree with @pijyoi 's assessment here. functions.py has a lot disconnected stuff in it, and we should stop adding to it without need. I have run into trouble with circular imports when I really only wanted one or two of the functions that functions.py brings in. Even if PlotDataItem might take over some or all of this later, the code could still live in PlotCurveItem.py, since that will always need to get imported anyway. I don't really see much need to generate the current stepmode datasets outside the internal code. If we decide to offer that functionality later, it should probably bring a wider set of options and an extended API; I don't think it would be very productive to accidentally formalize the current API by putting it somewhere accessible. |
Completely agree, @outofculture and I have been ranting to each other about this.
First thought was
That's a good point, that comment does scream of pre-mature optimization.
Good point. I'm good w/ this PR as is. |
drawLines has been integrated into PlotCurveItem
|
Sneaking in some changes to PlotSpeedTest.py
(When merging, please use squash) |
|
@pijyoi I can't think you enough for tackling this issue. When we were soliciting for feedback with what areas of the library could use improvement, this issue was the most referenced, by far. Thanks for jumping on this. Code looks good, and I couldn't break it so I'm going to squash and merge. |



Proof of concept using drawLines code from PlotSpeedTest.py.
Drawing thick lines probably involves a polygon flood fill by Qt, which gets slower the more complicated the polygon gets.
So the idea here is to draw many many short line segments. This has its own overhead but is far less than before.
To see it in action in PlotSpeedTest.py,
check enableExperimental,set to line width more than 1and check skipFiniteCheck.It's only meant to support fully finite data.I don't use thick lines myself, so feel free to take over this PR.