Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Fix Cursor widget drawing behavior with blitting support #29971

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

Closed
wants to merge 6 commits into from

Conversation

livlutz
Copy link

@livlutz livlutz commented Apr 26, 2025

PR summary

In class cursor,when the mouse moves out of the ax, redrawing takes a long time,
it solves the issue with the redrawing method, which takes too long when the mouse moves out of the ax, after fixing it should be more efficient

closes #29731
Summarising changes :
"Fix Cursor widget drawing behavior with blitting support"

PR checklist

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a week or so, please leave a new comment below and that should bring it to our attention. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

@livlutz livlutz closed this Apr 26, 2025
@livlutz livlutz reopened this Apr 26, 2025
@livlutz
Copy link
Author

livlutz commented May 10, 2025

Hello, it's just that I haven't heard from my contribution yet and it's been almost 2 weeks. The github bot suggested I leave a comment here so it would bring attention to any reviewers who are willing to check it and give me some feedback. So I would just like to ask if it is possible to review my PR soon so I could try to fix anything if needed.

Hope to get some feedback soon!

@rcomer
Copy link
Member

rcomer commented May 10, 2025

Hi @livlutz thank you for your contribution and sorry you did not get a response sooner.

Is this PR in response to issue #29731? If so, please edit your summary above to include "closes #29731".

I am not familiar with this module but I think the suggestion at #29731 (comment) was to make this method consistent with the one on MultiCursor, which just returns if we are outside the axes. Did you try that? If so, what happened?

@livlutz
Copy link
Author

livlutz commented May 10, 2025

Thanks for the feedback!

Yes, it is related to #29731! I'll change it on my summary shortly.

I did not try to fix it using MultiCursor though, I used self.canvas.blit() instead...should I take another look and try to use MultiCursor this time?

@rcomer
Copy link
Member

rcomer commented May 11, 2025

As I said I'm not familiar with this module so definitely no expert. So I had a play with the code examples Cursor and MultiCursor to teach myself something about it. For Cursor, when we go outside the axes, the crosshairs disappear. For MultiCursor, when we go outside the axes, the line remains at the side of the axes. So I think the different approaches in the code are deliberate, and we do need this loop to explicitly turn off the lines.

I also tried the Cursor example with your branch. It seems to work correctly as-is, but if I change it to set useblit=False, I get errors when I go outside the axes (because self.background is None).

Copy link
Member

@rcomer rcomer left a comment

Choose a reason for hiding this comment

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

I made a few comments to help you think about the change. I am inexperienced with blitting concepts so feel free to say so if you think I'm mistaken. I have just been looking at this example to try to understand what is happening.

lib/matplotlib/widgets.py Outdated Show resolved Hide resolved
self.canvas.restore_region(self.background)
else:
self.canvas.draw()
self.canvas.restore_region(self.background)
Copy link
Member

Choose a reason for hiding this comment

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

self.background may be None here, in which case this fails. I think it is always None if useblit is False.

else:
self.canvas.draw()
self.canvas.restore_region(self.background)
self.canvas.blit(self.ax.bbox)
Copy link
Member

Choose a reason for hiding this comment

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

Since useblit may be False here, I think it does not make sense to use blit.

@@ -1934,8 +1934,15 @@ def onmove(self, event):
self.linev.set_visible(False)
self.lineh.set_visible(False)
if self.needclear:
if self.useblit and self.background is not None:
Copy link
Member

Choose a reason for hiding this comment

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

If both needclear and useblit are True, is it possible for background to be None? Looking at the code I do not think so, but maybe I missed something.

@livlutz
Copy link
Author

livlutz commented May 13, 2025

Hello, I tried to apply the suggested changes and it looks a bit better, could you try to take a look again and see if it is alright?

@rcomer
Copy link
Member

rcomer commented May 14, 2025

I'm afraid I do not understand your changes. Can you explain how you chose what goes in the different branches of the if-loops?

Also, please test your changes locally. Unfortunately there are no unit tests for this class, but you can run some example code that uses it. When you run an example against your branch, does the behaviour look right? Is it the same as without your changes? If not, fix it.

I'm going to mark this as "draft" for now. Please mark it as "ready for review" when you are ready. In the meantime, if you need help, feel free to ask questions here. Or you may prefer to ask them in our incubator gitter room.

@rcomer rcomer marked this pull request as draft May 14, 2025 16:13
@melissawm melissawm moved this to Waiting for author in First Time Contributors May 22, 2025
@github-actions github-actions bot added the Documentation: examples files in galleries/examples label May 24, 2025
@livlutz livlutz closed this May 24, 2025
@livlutz livlutz deleted the livia branch May 24, 2025 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Waiting for author
Development

Successfully merging this pull request may close these issues.

[MNT]: in class cursor,when the mouse moves out of the ax, redrawing takes a long time
2 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.