Skip to content

MRG: Add support for playback in time_viewer#7200

Merged
larsoner merged 13 commits intomne-tools:masterfrom
GuillaumeFavelier:time_viewer_playback
Jan 15, 2020
Merged

MRG: Add support for playback in time_viewer#7200
larsoner merged 13 commits intomne-tools:masterfrom
GuillaumeFavelier:time_viewer_playback

Conversation

@GuillaumeFavelier
Copy link
Contributor

@GuillaumeFavelier GuillaumeFavelier commented Jan 13, 2020

This PR adds basic support for brain/time playback in _TimeViewer. It's still a work in progress since for now only a simple call to set_time_point() is done every second with an increment of playback_speed. I had to modify the UI as well to make room for this new slider. Playback is started/stopped with the shortcut t.

ToDo

  • Support time dilation
  • Use space for playback shortcut
  • Snap fscale back to 1
  • Improve coverage

It's an item of #7162


Screenshot

2020-01-13_1920x1080

Animation

output
(t is pressed twice to start and stop the playback)

@codecov
Copy link

codecov bot commented Jan 13, 2020

Codecov Report

Merging #7200 into master will decrease coverage by 0.02%.
The diff coverage is 66.98%.

@@            Coverage Diff             @@
##           master    #7200      +/-   ##
==========================================
- Coverage   89.76%   89.74%   -0.03%     
==========================================
  Files         445      445              
  Lines       79874    79960      +86     
  Branches    12773    12786      +13     
==========================================
+ Hits        71702    71761      +59     
- Misses       5374     5398      +24     
- Partials     2798     2801       +3

@larsoner
Copy link
Member

Nice proof of concept, agree with the todo list

@GuillaumeFavelier
Copy link
Contributor Author

@larsoner I'm not familiar with PySurfer time dilation so I tried the following:

A callback checking on the time_elapsed every 10ms updates the brain depending on the playback_speed and find the current time index with: idx = np.argmin(np.abs(times - time))

But I'm not sure about the user experience, is it even close to what you expect?

@larsoner
Copy link
Member

I'm not familiar with PySurfer time dilation

@GuillaumeFavelier you should try it. There is no interactive element but you can see how the movies get created.

Basically a time dilation of 1 plays back in real-time (1 sec of data plays back over 1 sec of viewer time). A time dilation of 10 plays back 10 times slower than real time (0.1 sec of data plays back over 1 sec of viewer time). I would implement the same user interface because it's an intuitive way to think about playback speed.

self.playback = False
self.playback_speed = 1
self.time_elapsed = 0
self.plotter.add_key_event('t', self.toggle_playback)
Copy link
Member

Choose a reason for hiding this comment

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

I would use spacebar, it's what most media players use for play/pause

self.playback_speed = 1
self.time_elapsed = 0
self.plotter.add_key_event('t', self.toggle_playback)
self.plotter.add_callback(self.play, 10)
Copy link
Member

Choose a reason for hiding this comment

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

I would use 16.7ms as the interval (about 1./60.)

times = self.brain._data['time']
time_idx = self.brain._data['time_idx']
time = times[time_idx] + 1. / self.playback_speed
idx = np.argmin(np.abs(times - time))
Copy link
Member

Choose a reason for hiding this comment

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

After working out the user-time-to-data-time logic I let interp1d from scipy in 'linear' mode do the work of picking the correct data.

@GuillaumeFavelier
Copy link
Contributor Author

Update that introduces button widgets:

2020-01-14_1920x1080

@codecov
Copy link

codecov bot commented Jan 14, 2020

Codecov Report

Merging #7200 into master will increase coverage by 0.01%.
The diff coverage is 78%.

@@            Coverage Diff             @@
##           master    #7200      +/-   ##
==========================================
+ Coverage   89.76%   89.78%   +0.01%     
==========================================
  Files         445      445              
  Lines       79874    79943      +69     
  Branches    12773    12783      +10     
==========================================
+ Hits        71702    71773      +71     
+ Misses       5374     5367       -7     
- Partials     2798     2803       +5

@GuillaumeFavelier
Copy link
Contributor Author

This is the latest result:

output

@larsoner
Copy link
Member

If you are going to call it playback_speed (which is fine by me) it should probably go from 0.01 (1/100th speed) to 1 (realtime), i.e. the reciprocal of time_dilation. Sliding to the right should make things faster and sliding left should slow it down

@larsoner
Copy link
Member

And after using it, a few more:

  • Show/hide does not toggle the start button, which it should
  • In media players, "Stop" typically means "pause and return to start". So rather than using "start/stop" I would use "Play/Pause"
  • I would also check to see if VTK supports unicode and just use ⏯ and let the checkbox / behavior indicate the state, but if you really like changing the text then I'd use ▶️ and ⏸ (though this might be less clear than ⏯ depending on how well VTK renders these characters, if it does at all)
  • It looks like you're on a HiDPI display (macOS). Things look good for you but bad when using non-HiDPI displays. Using:
    $ QT_SCALE_FACTOR=2 MNE_3D_BACKEND=pyvista python -ui tutorials/source-modeling/plot_visualize_stc.py
    
    I get what you see:
    Screenshot from 2020-01-14 11-50-34
    But then using 1. as the scale factor, which is also effectively the default on my system:
    Screenshot from 2020-01-14 11-51-19
    So it seems like PyVista and/or our code needs to take into account the physical-to-logical pixel ratio to get e.g. the font-to-button distances correct. You can use the QT_SCALE_FACTOR=1 on your system to test what it would look like for most people. Might be good to get in the habit of testing both.

@larsoner
Copy link
Member

larsoner commented Jan 14, 2020

Also:

  • Changing fscale is an immediate change relative to the present values on the slider. In other words, that if I click and drag to a value, it updates fmin/fmid/fmax, and the next time I click and release fscale, it changes those new values based on the value of fmult. That means that the effective value of fscale after releasing and updating fmin/fmid/fmax should be restored to 1. In other words, once the user click-drag-releases the fscale slider, it should (after changing fmin/fmid/fmax) immediately snap back to the 1. position.

@GuillaumeFavelier
Copy link
Contributor Author

GuillaumeFavelier commented Jan 15, 2020

Show/hide does not toggle the start button, which it should

I encountered the same bug and I couldn't find a way to hide/show the button widget without first interacting with it. For reference, the temporary solution modifies the opacity of the button.

I would also check to see if VTK supports unicode

The properties of this button don't allow it but it would be possible to use an image as a texture for a state of the button (checked/unchecked).

It looks like you're on a HiDPI display (macOS)

I'm not using a HiDPI display, this type of button uses absolute coordinates in the window space, which is not consistent with the rest of the UI elements using relative coordinates. This means that the button won't get resized and its position to the bottom-left corner won't change either while you resize the window. It is possible to mitigate this with a callback that updates the information of the button accordingly, this is the role of the perform_maintenance() function. I think this is a lot of code dedicated to compensate the limitations of this type of button. I would prefer to invest this energy to find a better alternative.

Thanks for the feedback @larsoner, I think I will rollback to the shortcuts for now in order to move forward with this PR. I will report all your comments on #7162 in case we come back on the buttons again.

@GuillaumeFavelier
Copy link
Contributor Author

GuillaumeFavelier commented Jan 15, 2020

Now that I think about it. We can also use the Qt interface of the plotter itself instead of the rendering view. We could add a menu 'TimeViewer' and put all the toggles in here. What do you think?

This is 3 lines of code:

        menu = self.plotter.main_menu.addMenu('TimeViewer')
        menu.addAction('Toggle interface', self.toggle_interface)
        menu.addAction('Toggle playback', self.toggle_playback)

image

Or we could add them in the camera toolbar or create a new toolbar. It's another option.

@GuillaumeFavelier GuillaumeFavelier changed the title WIP: Add support for playback in time_viewer MRG: Add support for playback in time_viewer Jan 15, 2020
@larsoner
Copy link
Member

After much tweaking the colorbar limits (including with overlap) I think this is probably the best compromise of usability and compactness:

Screenshot from 2020-01-15 10-37-09

I also:

  • updated / simplified / fixed the logic for playback speed
  • changed the default to 0.05 which works well for `sample
  • added a .update() that makes the playback smoother
  • made it so that if you hit space after playback is over (i.e., it's on the last time point) it starts from the beginning -- makes iterative viewing much easier because you don't have to click back to the beginning anymore

I'm very happy with this version! These changes okay with you @GuillaumeFavelier ?

@GuillaumeFavelier
Copy link
Contributor Author

Magnificent!

self.playback = False
self.playback_speed = 1
self.playback_speed = default_playback_speed
self.time_elapsed = 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't need self.time_elapsed anymore

@larsoner larsoner merged commit a35f8cf into mne-tools:master Jan 15, 2020
@larsoner
Copy link
Member

Thanks @GuillaumeFavelier !

@agramfort
Copy link
Member

agramfort commented Jan 15, 2020 via email

AdoNunes pushed a commit to AdoNunes/mne-python that referenced this pull request Apr 6, 2020
* Add basic support for playback

* Try time dilation

* Add button widgets

* Rollback this part

* Add linear time interpolation

* Fix time management

* Fix time dilation

* Update range of playback speed

* Rollback to shortcuts

* Snap fscale back to 1

* Improve coverage

* FIX: Smoother animation

* Remove unused variable

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
AdoNunes pushed a commit to AdoNunes/mne-python that referenced this pull request Apr 6, 2020
* Add basic support for playback

* Try time dilation

* Add button widgets

* Rollback this part

* Add linear time interpolation

* Fix time management

* Fix time dilation

* Update range of playback speed

* Rollback to shortcuts

* Snap fscale back to 1

* Improve coverage

* FIX: Smoother animation

* Remove unused variable

Co-authored-by: Eric Larson <larson.eric.d@gmail.com>
@GuillaumeFavelier GuillaumeFavelier deleted the time_viewer_playback branch June 11, 2020 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants