Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lazy create the rectangle selection item #2168

Merged
merged 3 commits into from Jun 18, 2022

Conversation

danielhrisca
Copy link
Contributor

In the current implementation for each ViewBox there will be a rectangle selection item. If the mouse mode is set to PanMode then this rectangle is unused.

With this PR the rectangle is only created if it is really needed.

self.sigStateChanged.emit(self)

def _add_rectangle_selection(self):
if self.reScaleBox is None and self.state['mouseMode'] == ViewBox.RectMode:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self.reScaleBox should be a typo for self.rbScaleBox?
As it is, a new QGraphicsRectItem is instantiated each time mouseMode changes to RectMode

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that was a typo 🤦‍♂️

@j9ac9k
Copy link
Member

j9ac9k commented Jan 13, 2022

Hi @danielhrisca thanks for the PR.

Only comment I have is should that hidden method remove the rectangular selection box if setMouseMode is set to PanMode, after initially being in RectMode. This PR I don't think will attempt to remove the selection rectangle in that condition.

@danielhrisca
Copy link
Contributor Author

Hi @danielhrisca thanks for the PR.

Only comment I have is should that hidden method remove the rectangular selection box if setMouseMode is set to PanMode, after initially being in RectMode. This PR I don't think will attempt to remove the selection rectangle in that condition.

My idea was that the CPU cycles were already used to create it, so removing it would only save some memory. Do you think it is better to remove it if the mouse mode is set to PanMode?

@j9ac9k
Copy link
Member

j9ac9k commented Jan 13, 2022

My idea was that the CPU cycles were already used to create it, so removing it would only save some memory. Do you think it is better to remove it if the mouse mode is set to PanMode?

We're talking few cycles and little memory here, so I don't think it makes a noticeable difference either way. I remember for your use-case you overlap many viewboxes, so i'm inclined to follow your lead here... removing the scale box would only occur on changing of the mouse mode, which I suspect is a time we have some cycles to spare. Either way, not going to hold up this PR on this, was just throwing out the idea is all 👍🏻

@j9ac9k
Copy link
Member

j9ac9k commented Jun 18, 2022

Blerg, should have merged this long ago, sorry for the wait @danielhrisca Thanks for the PR.

@j9ac9k j9ac9k merged commit 8e99198 into pyqtgraph:master Jun 18, 2022
@danielhrisca
Copy link
Contributor Author

Absolutely no problem 👍

@j9ac9k
Copy link
Member

j9ac9k commented Jun 18, 2022

FYI PyQt6 6.3.1 just introduced a new sip.array type which pyqtgraph master branch makes use of. It allows for noticeable faster ScatterPlots's and much faster calls to drawLines; and should be zero-copy operations for minimizing memory usage. I know you've started rolling out your own drawing mechanism, but if you're using QPainter.drawPixmapFragments or QPainter.drawLines and are using newer versions of PyQt6, you should absolutely take a look at our implementation.

Also if you want to get ahold of us on not the issue tracker, we now have a discord channel on the main python discord server here: https://discord.com/channels/267624335836053506/898139460821192724

@danielhrisca
Copy link
Contributor Author

Thanks for the tip. In my code I use drawPoints with a circular cap for the scatter plots, and drawPath with a cached version of generatePath https://github.com/danielhrisca/asammdf/blob/18c71009ef8b4784a55a560675eec66911cb1419/asammdf/gui/widgets/plot.py#L4589

In my work project we also use drawPolygon

@j9ac9k
Copy link
Member

j9ac9k commented Jun 18, 2022

Now that #2324 is merged, if you set enableExperimental to True, it's possible drawPainterPath will outperform drawPolygon now. While the sip.array object will work w/ drawPoints, the drawPoints method accepts a QPolygonF object which are are already able to construct very efficiently, so probably not much benefit in trying to use sip.array there with pyqtgraph.

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.

None yet

3 participants