hi @ibrewster

Thanks for the PR, and sorry for not replying to your post on the mail list. I'm in the middle of switching jobs so things on my end have been crazy.

First, love the diff; love removing some of the complexity. The static code checker flagged some issues:

I'll try and do some more testing here soon... a lot of folks have been contributing recently, and my PR queue is quite extensive 😬

Interesting. I think I disagree with the static code checker here, however. It's true that viewMask and pts are initialized inside an if block that may not be executed. Presumably this is what the static code checker is flagging on. However they are only used inside another if block, where the condition matches the one on the initialization block. As such, if they are not initialized, they will not be used, since the conditions are the same.

That said, it appears I can do the initial initialization at least of those variables outside of the if block without causing problems. While that feels somewhat kludgy to me, I believe it should be safe enough. I'll update the pull request.

Random thought: I don't know how the static code checker works - might it be afraid of a race condition, whereby the content of self.opts['pxMode'] gets changed between the first and second if block? If so, perhaps a better way to solve this is to only check the value once, storing the result of self.opts['pxMode'] is True in a variable that I use for both if blocks? Might that satisfy the static code checker?

Maybe I need to look into how to run it locally prior to making commits... :-D

static code checker definitely has some false positives in this regard; ... usually, a tell-tale sign of it tripping is that if a variable is created during an if-block; and it's used outside that if-block later. I'll try and take a closer look at it later 👍🏻

NOTE: This pull request also eliminates the if ... useCache section of the pxMode=True paint block. I think that with the QPicture being cached, this is no longer needed, but testing is in order :-)

I gave ScatterPlotSpeedTest.py a run on Windows.
(Python 3.8.10 / PyQt 6.2.3 / Qt 6.2.4 / NumPy 1.22.3)
The PixmapFragments codepath (that was removed by this PR) does have a positive impact on performance.

Well, that stinks, although I guess it's not that surprising - rendering a vector to a pixmap once and just drawing the pixmap is faster than rendering the vector every time. I actually had to do exactly that to get acceptable performance for a different application.

Maybe a quality flag? Or I'll just manually apply this to my own copy only, since I do need the high-quality output. Oh well. Back to the drawing board!

I am not sure, when you are exporting to PDF, are you entering codepath A or codepath B?
Does codepath B render the way you want?

if self.opts['useCache'] and self._exportOpts is False:
    # Draw symbols from pre-rendered atlas
    # codepath A
else:
    # render each symbol individually
    # codepath B

Interesting... yes, if I force useCache to False such that it runs codepath B, things do render correctly. Unfortunately, setting useCache=False in the plot command does not do the trick, as that setting apparently doesn't make it through to the ScatterPlotItem - ScatterPlotItem is initialized with default options on line 305 of PlotDataItem.py, and the list of kwargs that is passed to the setData function doesn't include that option either. In fact, from what I can tell, the useCache kwarg passed to the plot function is completely ignored. So maybe that's the approach that needs to be taken here - get the plot function to recognize the useCache argument and pass it through to the ScatterPlotItem?

Ok, changed to just passing the useCache option through to ScatterPlotItem, and tested to have the desired result. In the attached screenshot, both the colored lines and the triangle symbol in the middle are plotted with pxMode=True, but only the triangle volcano symbol has useCache=True in the arguments.

Plus, since all I did this time was allow an already-existing option to be passed through to ScatterPlotItem from the plot command, this change really shouldn't have any effect on performance (unless you pass that option, of course!).

Granted, this completely changes the nature of the pull request, though the end goal is the same...

.

Ha, that diff definitely shrank! Would you mind updating the PR title (as that is used for automatically generating the release notes)?

Also unrelated to the diff in the PR, but since you mentioned exporting to PDF, the Orange project does that with this code: https://github.com/biolab/orange-widget-base/blob/master/orangewidget/utils/PDFExporter.py

We can't use that yet as we support Qt 5.12 for the time being, but just linking as it might be relevant to you.

Yeah, just exposing that pre-existing option was definitely simpler, code-wise, than my first mis-guided approach!

Thanks for that link. I can't use it directly, as I have more than just the plot that I am exporting (I build up a whole widget with labels, scales, keys, etc), but there were still some useful portions in there that I can use to improve my process. There may well be ways to pull to pull all my widgets into pyqtgraph (is there a color scale widget? I'll have to check...), but at least when I first made this, I found simply using separate widgets for the multiple keys, etc I needed easier.

It's possible to influence the code to enter codepath "B" with existing master.
Granted, we have to "manually" access the scatter object within the PlotDataItem.

import pyqtgraph as pg
from pyqtgraph.Qt import QtCore, QtGui, QtWidgets

pg.mkQApp()

# deliberately create a wrong qpath for symbol
# correct symbol needs to be of size (1, 1)
qpath = QtGui.QPainterPath()
qpath.addEllipse(-1, -1, 2, 2)

pw = pg.PlotWidget()
pw.show()
pdi = pw.plot(range(5), symbol=qpath)

# enabling either of the following will enter codepath "B"
# and draw a circle
# if neither are enabled, a square gets drawn
pdi.scatter.setExportMode(True)
pdi.scatter.opts['useCache'] = False

pg.exec()

@ibrewster The diff LGTM, the only thing I ask is that you update the docstring optimization options section to detail the use of useCache. I was going to suggest linking to the docstring entry in ScatterPlotItem but it's not detailed there either 😬

Sure thing. We have a new volcano acting up this week, resulting in a higher-than-normal number of software development requests, and next week I'll be out-of-town at PyCon, but hopefully I should be able to get to it within a week or so after that! 😀

@ibrewster feel free to message me on twitter/slack if you're having trouble finding time to add that in in the coming days. I would be fine to add the docstring to this PR (assuming I get some time too 😆 ) if it means this change would be included in the next release (which I'm hoping will be sooner than later).

@ibrewster hoping that volcano has settled down ... hoping to do a release in the not too distant future, would love this PR to get in. If you're going to be busy for a bit let me know and I'll insert that blurb in the docstring.

thanks for the PR @ibrewster LGTM, merging!