-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
ENH: Add a playback button to the notebook 3d backend #8741
ENH: Add a playback button to the notebook 3d backend #8741
Conversation
It seems like you should only set "interval" to get the right refresh rate (e.g., 1/60 or 1/30), and the callback should be responsible for picking the correct time points. It seems like this should be the same for the Qt and IPywidgets backends actually -- and then as long as the callback tracks/knows 1) when the last frame was rendered (or if it's the first frame), 2) what the desired time dilation is, and 3) what the current time is ( |
Oh you're right! All this code is in mne-python/mne/viz/_brain/_brain.py Lines 778 to 802 in fb22fa6
|
The |
It's ready to go on my end 🚀 |
Works great! FWIW on my local Ubuntu-based 3.9 after
Onto the minor issues:
|
One nitpick from the peanut gallery: "toggle visibility" I think would be more clear if it said "toggle controls". Otherwise this is looking really nice. |
This PR requires quite the update. I think the challenge now changed to finding a GUI-agnostic way (Qt/Ipywidget) to playback. |
Instead of making it agnostic I'd use our GUI abstraction layer to deal with the differences, and use a Qt timer and ipywidget object (Play?) as necessary in those backends |
The test fails because |
Since I return it as a |
I chose the simplest approach which is to connect the playback buttons to the time slider bidirectionally. The known issues:
|
In my experiments, the bidirectional connection was not reliable. For example, using the playback to start, changing the slider and coming back to the playback, it will start from the old position: I can mitigate by duplicating
But it brings lag when the slider is dragged (it snaps the value of the slider). output.mp4 |
TL;DR: The realtime approach may not be possible considering the current limitations so I think the approach based on I took the time to fix 3) and the solution explained in jupyter-widgets/ipywidgets#1775 really helped. I shared the prototype in be61d64 for reference but I ended up reverting because I am not satisfied with the overall experience. The number of calls to |
Some ability to play back is better than none, even if it's effectively a slideshow and the |
You can have a look @larsoner, I'm ready for reviews. |
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.
Otherwise LGTM
@@ -348,7 +363,7 @@ def show(self): | |||
except RuntimeError: | |||
# pyvista>=0.30.0 | |||
viewer = self.plotter.show( | |||
jupyter_backend="ipyvtk_simple", return_viewer=True) | |||
jupyter_backend="ipyvtklink", return_viewer=True) |
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.
Should this be a try/except
as well?
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.
Since 0.30.0
pyvista does not use ipyvtk_simple
anymore
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.
Yes but I'm thinking that it should also work for people with 0.29
if possible
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.
I plan to bump/update those requirements in #9341
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.
If it's only a line or two to keep it working, it would be nice not to force people to upgrade
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.
But if they have ipyvtk_simple
and pyvista < 0.30
it should work, right? For example:
kwargs = dict()
if LooseVersion(pyvista.__version__) < LooseVersion('0.30'):
kwargs['jupyter_backend' ] = 'ipyvtk_simple'
else:
kwargs['jupyter_backend'] = 'ipyvtklink'
viewer = self.plotter.show(return_viewer=True, **kwargs)
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 would be nice not to force people to upgrade
I understand
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.
I'll rework this part to use LooseVersion
instead.
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.
The API changed too, the following won't work:
self.plotter.show(jupyter_backend="ipyvtk_simple", return_viewer=True)
try: | ||
# pyvista<0.30.0 | ||
self.picked_renderer = \ | ||
self.plotter.iren.FindPokedRenderer(x, y) | ||
except AttributeError: | ||
# pyvista>=0.30.0 | ||
self.picked_renderer = \ | ||
self.plotter.iren.interactor.FindPokedRenderer(x, y) |
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.
Like this block works for >= 0.30 and < 0.30
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.
But this one does not depend on another package.
If the user does not have ipyvtklink
but has pyvista 0.30.0
, it just won't work.
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.
That's why I think bumping is the least painful solution
Thanks @GuillaumeFavelier ! |
* upstream/main: MRG, ENH: Make _get_hpi_info public (mne-tools#9369) ENH: Add a playback button to the notebook 3d backend (mne-tools#8741) better docs for permutation_cluster_test (mne-tools#9365) MRG: Add fNIRS to html output (mne-tools#9367) When plotting GFP comparison in Report, don't show sensor layout by default (mne-tools#9366) DOC: Update Mayavi troubleshooting section (mne-tools#9362) more tutorial tweaks (mne-tools#9359)
* upstream/main: FIX: make epoch cropping idempotent (mne-tools#9378) MRG, ENH: Add NIRSport support (mne-tools#9348) MRG, ENH: Make _get_hpi_info public (mne-tools#9369) ENH: Add a playback button to the notebook 3d backend (mne-tools#8741) better docs for permutation_cluster_test (mne-tools#9365) MRG: Add fNIRS to html output (mne-tools#9367) When plotting GFP comparison in Report, don't show sensor layout by default (mne-tools#9366) DOC: Update Mayavi troubleshooting section (mne-tools#9362) more tutorial tweaks (mne-tools#9359) MRG, MAINT: Use native GitHub Actions skip (mne-tools#9361) MAINT: Clean up crufty code [circle front] (mne-tools#9358) API: Complete deprecations (mne-tools#9356) Add qdarkstyle, darkdetect to environment.yml [circle full] (mne-tools#9357) FIX: Fix FIX: Add
ipywidgets
buttons for playback. Here how it looks like:output.mp4
Known issue
Does not depend on
playback_speed
but on the widget'sinterval
value.backgroundjobs
from ENH: Notebook 3d backend improvements (ipyvtklink) #8704 (comment) without success so far. I'll give it a try again later this afternoon.It's an item of #8704.