Skip to content

experimental line drawing mode for thick lines#2011

Merged
j9ac9k merged 7 commits intopyqtgraph:masterfrom
pijyoi:thickpen
Oct 17, 2021
Merged

experimental line drawing mode for thick lines#2011
j9ac9k merged 7 commits intopyqtgraph:masterfrom
pijyoi:thickpen

Conversation

@pijyoi
Copy link
Copy Markdown
Contributor

@pijyoi pijyoi commented Oct 9, 2021

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 1 and 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.

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Oct 9, 2021

@markotoplak this was something you previously mentioned being interested in. If you could give this PR a spin that would be greatly appreciated!

@pijyoi pijyoi force-pushed the thickpen branch 2 times, most recently from b8ec390 to f5e702c Compare October 9, 2021 03:51
@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Oct 9, 2021

Tagging some other folks that have expressed interest in lines with > 1.0 pixel thickness.

@mliberty1 @jpgill86 @Tillsten

@pijyoi
Copy link
Copy Markdown
Contributor Author

pijyoi commented Oct 9, 2021

From #533 (comment), there is a link to https://stackoverflow.com/questions/46206296/performance-issues-with-pyqtgraph

By adding skipFiniteCheck=True to that example (to activate the changes made in this PR), "Plotting time" went from 3.925 secs to 0.106 secs

plot_item = pg.PlotCurveItem(x, y, pen=pen, skipFiniteCheck=True)

@markotoplak
Copy link
Copy Markdown
Contributor

Thanks for working on this. I'll check it out!

@NilsNemitz
Copy link
Copy Markdown
Contributor

First off: I love it :)

Second:
Here is an illustration that shows why Qt doesn't draw like that by default:
thick line flaws

Issues:

  • the ends of the individual line elements overlap, which becomes visible if the lines are not completely opaque.
  • there is no effort to connect the line elements. By default, Qt draws "square caps", which extends the line to overshoot the actual point by half a linewidth. If you look at the curved part of the orange line in the top right plot, you can see that the corners of the "caps" stick out of the line.
  • the individual line segments have no way to keep track of the dashing progress, and each element will tend to re-start with a solid segment. The lines in the example are all set to Qt.PenStyle.DashLine.

Suggestions:

  • The checked conditions in PlotCurveItem.py, L 593ff should include checking for opacity 1.0 and Qt.PenStyle.SolidLine.
  • The function mkPen in functions.py could include pen.setCapStyle( Qt.PenCapStyle.RoundCap ) for the case of width > 1, somewhere around L 398 .

The round caps will probably degrade performance somewhat, but they should remove the corner artifacts. (There's a commented out line to test this in the code below.) A totally overoptimized solution would probably alternate between segments with FlatCap ends and segments with RoundCap ends, so that the connecting circles are only drawn once.

Random thoughts:
Under some circumstances, it might be nice to use this drawing mode even with translucent lines, the overlapping RoundCaps make adorable point markers ( <3 ). But I don't think it is a good idea to clutter up the API with switches for this.

Demonstration code for fiddling around with:

import numpy as np
import pyqtgraph as pg
from PyQt5.QtCore import Qt

app = pg.mkQApp("Log Axis Example")

w = pg.GraphicsLayoutWidget(show=True)
w.resize(800,800)
w.setWindowTitle('pyqtgraph example: Log Axis, or How to Recognise Different Types of Curves from Quite a Long Way Away')

p0 = w.addPlot(0,0, title="Linear")
p1 = w.addPlot(0,1, title="X Semilog")
p2 = w.addPlot(1,0, title="Y Semilog")
p3 = w.addPlot(1,1, title="XY Log")
# configure logarithmic axis scaling:
p1.setLogMode(True, False)
p2.setLogMode(False, True)
p3.setLogMode(True, True)

x = np.logspace(-1, 1, 10) 
plotdata = ( # legend entry, color, and plotted equation:
    ('1 / 3x'    , '#ff9d47', 1./(3*x) ),
    ('sqrt x'    , '#b3cf00', 1/np.sqrt(x) ),
    ('exp. decay', '#00a0b5', 5 * np.exp(-x/1) ), 
    ('-log x'    , '#a54dff', - np.log10(x) )
)
p0.addLegend(offset=(-20,20)) # include legend only in top left plot
for p in (p0, p1, p2, p3):    # draw identical numerical data in all four plots
    p.showGrid(True, True)    # turn on grid for all four plots
    p.showAxes(True, size=(40,None)) # show a full frame, and reserve identical room for y labels
    for name, color, y in plotdata:  # draw all four curves as defined in plotdata
        qcol = pg.mkColor(color)
        qcol.setAlphaF(0.5)
        pen = pg.mkPen(color=qcol, width=20)
        pen.setStyle( Qt.PenStyle.DashLine )
        # pen.setCapStyle( Qt.PenCapStyle.RoundCap )
        p.plot( x,y, pen=pen, name=name )

w.show()

if __name__ == '__main__':
    pg.exec()

@pijyoi
Copy link
Copy Markdown
Contributor Author

pijyoi commented Oct 10, 2021

The round caps will probably degrade performance somewhat, but they should remove the corner artifacts.

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 setConfigOption('enableExperimental', True), so that any surprising behavior is at the request of the user.

@pijyoi
Copy link
Copy Markdown
Contributor Author

pijyoi commented Oct 10, 2021

If we compare with QtCharts, their implementation uses drawLine and the only condition they use is SolidLine.
#1910 (comment)

I was able to trigger the various other plotting artifacts (Alpha and CapStyle) with PlotSpeedTest_qtcharts.py (https://gist.github.com/pijyoi/c8a2eb963a36a5d5bb14cc3b75a8bab0)

@NilsNemitz
Copy link
Copy Markdown
Contributor

My opinion is that the gain is large enough to make this the default behavior when the conditions (solid, opaque) allow it. This covers the majority of things I would like to draw! The documentation should probably explain the different modes and requirements for the speed-up. I can help write that once we have the code worked out.

I do not think there is a user expectation for line end caps. This is not even an option that can be set through mkPen. And we have basically been telling everybody that thick lines don't work. :) So I would say that the main user expectation is fast plotting, and endcap styling is you get what you get. It would be nice if we could detect a "don't care" setting for the endcaps, but since we basically expect the user to supply a QPen, we cannot differentiate between "SquareCap as the default", and "SquareCap because the user picked it". The best we can do is the default in mkPen, since that can still be overridden manually by modifying the QPen before passing it on. (And we might add a parameter for the endcaps later on.)

The slowdown from RoundCap seems acceptable to me: This would only apply when drawing in the "thick pen", segmented line mode. And in that case it drops the speed to 70% of a 37-fold speed-up. If I calculate right, that means the "slow" segmented line mode with CircleCap still speeds up drawing by more than 25 times relative to the previous code.

But we can do better than my earlier suggestion:
How about defaulting to RoundCaps only when it makes a significant difference?
For a 45 degree line connection, the cap edges will stick out by about (sqrt(2) - 1) * w/2
For a width w <= 4, this is significantly less than a pixel.

Thus we could make mkPen default to RoundCap only when width > 4.

That gives the best speed for all "normal" linewidths, and avoids clearly visible artifacts for very wide lines. Below, orange and green are width=4 lines with square endcaps, purple and blue are width = 5 with round endcaps:
round caps at 5

Random thoughts: (( Do you have a good idea how to design a general API for optimization flags?
Otherwise, it might be helpful to just add a flag that controls this directly:
optimize_wide_line = None might be the default that checks for thickness, solid, opaque.
True might force this code path and allow using it with e.g. translucent lines. False would disable it.

In the long run, it would certainly make sense to have an overall "optimize = False" setting. That would make it easier to test if some specific bug results from the optimizations. It would also allow the user to turn them off if there are conflicting settings, or to keep their code working until we can fix a bug. ))

But fundamentally, this PR effectively enables a use case that wasn't very practical before, so I would see backwards compatibility as somewhat of a secondary concern. But I would love to hear some other opinions!

@pijyoi
Copy link
Copy Markdown
Contributor Author

pijyoi commented Oct 10, 2021

The modified-to-use thick pens examples/crosshair.py is an example where this PR makes user-interaction feasible.

@pijyoi
Copy link
Copy Markdown
Contributor Author

pijyoi commented Oct 11, 2021

It seems to me that the reason this drawLines mode can't support stepMode and requires skipFiniteCheck=True is that there is a mutation of data points within PlotCurveItem::generatePath. This modification of points goes directly into the QPainterPath and is not available further downstream.

Perhaps stepMode would better belong in PlotDataItem so that PlotCurveItem can be dedicated to pure line drawing. But that would require a much larger refactoring and also an API breakage. Maybe in another PR.

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Oct 11, 2021

stepMode ....didn't even cross my mind; yeah that's going to be an issue.

I fully support the idea of moving the logic/implementation to PlotDataItem.

Maybe in another PR.

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 stepMode logic to PlotDataItem.

@NilsNemitz
Copy link
Copy Markdown
Contributor

I support moving stepmode to PlotDataItem.
@j9ac9k , do you have an opinion about hiding this new thick pen mode behind "enableExperimental" ?

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Oct 11, 2021

I support moving stepmode to PlotDataItem.

@j9ac9k , do you have an opinion about hiding this new thick pen mode behind "enableExperimental" ?

I don't think it's necessary to hide it behind enableExperimental but I'm not opposed to it either. This is the sort of thing that I would go with whatever the author of the diff thought was best.

EDIT: was on mobile, but wanted to expand more about my indecisiveness.... I see two major reasons for putting things behind enableExperimental

  1. Making available alternative implementations where we don't have a degree of confidence that we would like to outright replace the existing functionality with it.
  2. We're not sure what the API criteria would look like, and want to prepare end-users for things changing with little notice.

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 👍🏻

@pijyoi
Copy link
Copy Markdown
Contributor Author

pijyoi commented Oct 11, 2021

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 enableExperimental.

@sem-geologist
Copy link
Copy Markdown
Contributor

stepMode to PlotDataItem? I just planned to use that (and I use mostly CurveItem, PlotDataItem is a bit too heavy for me) in my software, good I still had not. What I use is ShadowPen (for highlighting) will it be affected by this implementation?

@NilsNemitz
Copy link
Copy Markdown
Contributor

NilsNemitz commented Oct 11, 2021

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 thought about possible use cases, and most of what I could think of revolved around histograms, or similar plots with no more than a few hundred points. In that case, the overhead of PlotDataItem shouldn't matter much.

@pijyoi
Copy link
Copy Markdown
Contributor Author

pijyoi commented Oct 11, 2021

I managed to get the thickpen codepath to handle stepMode.
Demonstration code below:

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 killer_opts.

@pijyoi
Copy link
Copy Markdown
Contributor Author

pijyoi commented Oct 11, 2021

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.)

There is one usage of shadow pen in examples/PlotWidget.py with number of samples set to 100.
Try setting it to 10000 samples.

@sem-geologist
Copy link
Copy Markdown
Contributor

@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.
Often few and many (up to hundred) such curves are simultaneously plotted, so that relevant differences could be visually spotted.
Other usage cases could be when dealing with hyper-spectral images (every pixel contains spectra), re-plotting of curve with change of selected pixel should not lag.
StepMode is not a-must, but nice to have. X-ray data normally has peaks formed by couple to near hundred of channels, and visually without stepMode it is still possible to correctly identify the peaks. Step Mode is more "classic" thing which was always there on different kind of OEM software.
Additionally, often such spectra (stepped curve) should be possible to be filled.
Personally I found pyqtgraph's stepMode not very friendly to use, as that requires to mingle the original data (add one more x value, and shift all x values a half width of channel). If stepMode would be removed from CurveItem, I would not cry, probably I would simply completely drop it from my wish-list as that would get more complicated. Actually this would give me an excuse to forget this.

@NilsNemitz
Copy link
Copy Markdown
Contributor

NilsNemitz commented Oct 11, 2021

@pijyoi ,
sorry, this may be getting a little out of hand... But I just stumbled across the fact that the Color Gradient Plots example hates the thick line optimization. I guess there is overhead in figuring out the gradient information for each line segment?

In any case, the plot seems to run slower by a factor of four or so after adding skipFiniteCheck=True to the four PlotDataItem initializations (which is enough to enable the optimization there). I set the number of points to 20000 to slow it down enough to be visible here:
length = int( 3.0 * 20000. ) # length of display in samples

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:
adding this in L540 runs the thick line code only for pens with simple, flat colors.

# segmenting the curve slows gradient brushes, and is expected to do the same for other patterns
and pen.brush().style() == QtCore.Qt.BrushStyle.SolidPattern

@NilsNemitz
Copy link
Copy Markdown
Contributor

NilsNemitz commented Oct 11, 2021

@sem-geologist ,
Thank you very much! The key information is that these spectra get piled up, that is something that I simply haven't encountered before. I guess you would at least occasionally zoom in so that the 4096 steps actually become visible... or your screen just has a pretty nice resolution :)

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?
We do aim to keep the region where PlotCurveItem is fast enough, and PlotDataItem just isn't, as small as possible.

@NilsNemitz
Copy link
Copy Markdown
Contributor

There is one usage of shadow pen in examples/PlotWidget.py with number of samples set to 100. Try setting it to 10000 samples.

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. :)

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Oct 11, 2021

stepMode to PlotDataItem? I just planned to use that (and I use mostly CurveItem, PlotDataItem is a bit too heavy for me) in my software, good I still had not. What I use is ShadowPen (for highlighting) will it be affected by this implementation?

Hi @sem-geologist

@NilsNemitz has confirmed a performance penalty for using PlotDataItem vs. PlotCurveItem for some basic plots, we need to examine this more carefully, and we will before migrating functionality from one class to another; we're not fans of performance regressions.

As already mentioned PlotDataItem is the home for data that needs to undergo some kind of transformation to be viewed in the appropriate manner, whether that be displaying things in some kind of log-mode, applying downsampling methods, displaying derivative modes, phase-maps and so on. It makes sense to me that displaying step-mode of curves should reside in PlotDataItem as well, and PlotCurveItem is preserved for displaying a numpy array with minimal processing along the way.

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.

@sem-geologist
Copy link
Copy Markdown
Contributor

@j9ac9k ,
Thanks for clarification, it starts to make sense. I probably overdone myself and misunderstood the intentions of those two classes. I was sure I don't need the xy points (or points on node) and I saw that code of PlotCurveItem is more simple, thus thought it is the right class for my case (and it does not hurt when numpy array without any copy can be just plotted). But now I started to doubt, if PlotDataItem would not simplify some stuff, in particularly that I had implemented some custom logic per dataset to deal with scale unit change (i.e. for x it can be energy (eV), mechanical spectrometer position, or equivalent (sin theta) wavelength of x-rays (nm). Instead of doing conversions at datasets, I could subclass PlotDataItem and do that there where it would fit quite naturally.

@pijyoi
Copy link
Copy Markdown
Contributor Author

pijyoi commented Oct 13, 2021

All four connect modes are supported now.
'all', 'pairs', ndarray require skipFiniteCheck=True. 'finite' doesn't care.
This means that if you use PlotDataItem with its default connect='auto', it is not necessary to specify skipFiniteCheck=True in order to activate this mode. (PlotDataItem will call PlotCurveItem with either ('all', skipFiniteCheck=True) or ('finite', skipFiniteCheck=False))

There is in fact no in-library usage of PlotCurveItem with "pairs". ErrorBarItem uses arrayToQPath("pairs") directly. So the support for "pairs" mode is more for completeness.

@pijyoi
Copy link
Copy Markdown
Contributor Author

pijyoi commented Oct 14, 2021

Following script is to verify that drawLines does not suffer from the issue in #1457 and #1149.
Resultant QPainterPath will only have 5 elements instead of 6 due to the linked issues.
Plots with arrayToQPath and this PR are okay.

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()

@pijyoi pijyoi marked this pull request as ready for review October 14, 2021 13:48
@pijyoi
Copy link
Copy Markdown
Contributor Author

pijyoi commented Oct 15, 2021

For edge-case testing this PR, considering modifying 1.0 to 0.0 in the following snippet in PlotCurveItem.py and giving it a run on your pyqtgraph programs.

    def _shouldUseDrawLineSegments(self, pen):
        return (
            pen.widthF() > 1.0

It fails test_PlotCurveItem.py because of tiny line edge differences.

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Oct 15, 2021

It fails test_PlotCurveItem.py because of tiny line edge differences.

Would you like me to make a new image?

@pijyoi
Copy link
Copy Markdown
Contributor Author

pijyoi commented Oct 15, 2021

It fails test_PlotCurveItem.py because of tiny line edge differences.

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.

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Oct 15, 2021

It fails test_PlotCurveItem.py because of tiny line edge differences.

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 .drawLines being being used 👍🏻

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Oct 16, 2021

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 functions.py (or someplace more accessible) so we can draw thick lines using drawLines with greater ease from other parts of the library besides PlotCurveItem.

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Oct 16, 2021

For edge-case testing this PR, considering modifying 1.0 to 0.0 in the following snippet in PlotCurveItem.py and giving it a run on your pyqtgraph programs.

    def _shouldUseDrawLineSegments(self, pen):
        return (
            pen.widthF() > 1.0

It fails test_PlotCurveItem.py because of tiny line edge differences.

Changed this in the application I develop internally at my company; it works exactly the same 👍🏻

image

@pijyoi
Copy link
Copy Markdown
Contributor Author

pijyoi commented Oct 17, 2021

The one question I have is what do you think about moving LineSegments to functions.py (or someplace more accessible) so we can draw thick lines using drawLines with greater ease from other parts of the library besides PlotCurveItem.

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.

@NilsNemitz
Copy link
Copy Markdown
Contributor

NilsNemitz commented Oct 17, 2021

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.

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Oct 17, 2021

The one question I have is what do you think about moving LineSegments to functions.py (or someplace more accessible) so we can draw thick lines using drawLines with greater ease from other parts of the library besides PlotCurveItem.

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.

Completely agree, @outofculture and I have been ranting to each other about this.

What parts of the library do you have in mind? Couldn't they use PlotCurveItem?

First thought was ErrorBarItem (where we currently use arrayToQPath but it's probably better to modify ErrorBarItem to use a PlotCurveItem to draw the error bars. Was also considering GridItem and AxisItem as well, but those items don't frequently undergo updates, they generate a QPicture and then re-render that picture, so the benefit there is probably not worth the complexity.

I suppose this decision to refactor can be left to the first PR that wants to make use of it?

That's a good point, that comment does scream of pre-mature optimization.

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.

Good point.

I'm good w/ this PR as is.

@pijyoi
Copy link
Copy Markdown
Contributor Author

pijyoi commented Oct 17, 2021

Sneaking in some changes to PlotSpeedTest.py

  1. add "fill" parameter to demonstrate the huge performance hit due to it
  2. remove drawLines proof of concept code

(When merging, please use squash)

@j9ac9k
Copy link
Copy Markdown
Member

j9ac9k commented Oct 17, 2021

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Poor performance of plot with pen width larger than 1

5 participants