-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Update _TimeViewer's plotter only when necessary #7517
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
Update _TimeViewer's plotter only when necessary #7517
Conversation
|
Can you try this @hoechenberger ? It would be nice if this could at least improve your situation. |
| self.time_call(idx, update_widget=True) | ||
| if time_point == max_time: | ||
| self.playback = False | ||
| self.plotter.update() # critical for smooth animation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This always should have been indented one more level :( But if you now have time_call effectively trigger the .update(), then indeed it makes sense to remove it entirely now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was necessary after PyVista 0.24, probably something can be improved upstream
|
@GuillaumeFavelier I tested this with: In [1]: %pwd
Out[1]: '/Users/hoechenberger/Development/mne-python/examples/inverse'
In [2]: import mne
In [3]: mne.viz.set_3d_backend('pyvista')
Using pyvista 3d backend.
In [4]: %run plot_vector_mne_solution.pyAnd I get a mayavi window… I think there might be a PyVista window shortly popping up and then disappearing, but I'm not sure (maybe it's the mayavi window all along, changing some of its properties) |
|
@hoechenberger #7060 is not merged yet |
|
can you make sure you have pyvista >= 0.24?
… |
|
The vector PR has not been merged, you need to test a standard STC for this PR |
|
Oh sorry, I thought it had been merged. Losing track with all these PRs and amazing new features by @GuillaumeFavelier :D This PR fixes the CPU hogging issue for me! |
Fantastic! |
larsoner
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed it seems to work great
Codecov Report
@@ Coverage Diff @@
## master #7517 +/- ##
==========================================
- Coverage 90.12% 90.08% -0.05%
==========================================
Files 452 453 +1
Lines 82885 82767 -118
Branches 13100 13062 -38
==========================================
- Hits 74703 74560 -143
- Misses 5347 5381 +34
+ Partials 2835 2826 -9 |
|
@GuillaumeFavelier can you fix the conflict? then I'll merge |
|
I'm on it |
|
CIs were happy before merge and still are locally so I'll go ahead and merge, thanks @GuillaumeFavelier |
|
I'll check how it goes directly on |
This PR updates the render window only when it is expected or necessary (meaning after a user interaction) instead of being called constantly at a very small interval. I expect this to lower the burden on the CPU.
Note: My CPU usage for a standard script went from ~25-30% to around 1% on idle.
It's an item of #7162