Basic "play" functionality (animate a dimension)

[tlambert03]

Description

This PR adds a "play" method to qt_dims.QtDims to animate changing the slice setpoint (e.g. for a time axis). I'm sure this was something you were going to implement eventually but I didn't see any discussion yet in the issue tracker... This was just how I implemented it, so feel free to "freeze" or disregard this PR if there were reasons for holding off on this functionality until later. Currently there is no button in the GUI Window, just a keybinding (ctrl-alt-P), and a couple new methods on QtDims. Similarly, changing playback speed or the axis being animated requires using the method directly.

Some other things that should be addressed before any sort of merge...

• The QTimer driving the animation appears to halt on any mouseover event in the main window causing a delay in animation. That might be fine (might even be the desired compromise for performance), but I thought it was worth mentioning. I suppose it could run in a different thread to prevent that?
• currently, only the QtDims.stop() method halts the animation. So if the user changes the number of displayed dims, the "animated" axis may no longer be visible, yet the timer and the corresponding computation continue. this can create a performance/lag issue.

Type of change

• New feature (non-breaking change which adds functionality)

References

https://doc.qt.io/qt-5/qtimer.html

How has this been tested?

• the test suite (napari._qt.tests.test_qt_viewer.test_play_axis) for my feature covers playing the 0th axis in a 3D image, asserting that the play method works (i.e. the dims.point has changed the expected number of frames after some wait interval), and asserting that the stop method actually works to prevent additional set_point calls
• all tests pass with my change

Final checklist:

• My PR is the minimum possible work for the desired functionality
• I have commented my code, particularly in hard-to-understand areas
• I have added tests that prove my fix is effective or that my feature works
• I have made corresponding changes to the documentation

[sofroniewn]

This looks great @tlambert03! Thanks for the contribution - a play button is on our 0.3.0 roadmap #420 but no one was working on it yet, so great timing!

A couple quick comments - using another thread so the QTimer is not blocking might be a good idea, but I can also see reasons why it might be best to just let it play without being able to change anything. How do @royerloic @jni feel about the expected behavior?

If we do want to use another thread @AhmetCanSolak might have ideas on how to do this best too.

I'll also play around with some example datasets and see how it feels too.

Your comment about QtDims.stop is also interesting - we might want to hook up an event from the viewer to trigger stopping when the layers data changes. Right now we have a function viewer._on_layers_change that gets called when changes like that happen, we might want to make the viewer emit an event layers_change that will stop an animation, but again maybe that relates to what expectations we have about interactivity during playing.

If you want to add play buttons in this PR too that would be great - you'll need to look at our contributing guidelines for adding icons, but we can also merge without it and then make a followup PR later if not.

[tlambert03]

OK, made the changes mentioned above, and I moved the animation timer to it's own QThread and I think it works much better (relatively smooth animation even during 3D rotation for instance). I also added a new layers_change emitter to the viewer, call it on the viewer._on_layers_change, connected it to the QtDims.stop() method where all the other viewer.layers.events were getting connected in QtViewer. It works to stop the animation when adding layers or changing dims displays. Please let me know if I did that correctly, I definitely don't want to mess up any of the lower level model.

@AhmetCanSolak, I realize now that you were going to take on the play button thing, (sorry!) so please have a look when you get a chance and let me know what you would do differently... In particular, you might have thoughts on the issues below...

Remaining problems:

• If I add a lot of 3D image layers and the per-timepoint rendering burden gets higher, triggering playback can bring the whole thing to a grinding halt, sometimes requiring a force quit. I don't expect it to keep up with the frame rate (in this case 10 fps was requested) but it should at least fail gracefully. I guess we might need to debounce the animation timer's call to set_point somehow, so that the timer doesn't spam the viewer with requests that it can't keep up with
• I chose to create and destroy the animation QThread each time playback is started and stopped (rather than keeping a thread open the whole time just in case someone plays). One issue is that if the user quits the program while the animation is running, the QThread doesn't have time to call the timer.deleteLater cleanup, a warning is thrown (WARNING: QObject::killTimer: Timers cannot be stopped from another thread) and the program exits with a segfault. I guess we need to hook into the QT aboutToQuit() signal... but I haven't figured that out yet.

I'll try to take a look at adding icons and play buttons soon...

[sofroniewn]

for the aboutToQuit you should be able to put the shutdown call inside closeEvent(self, event) which is at the bottom of the qt_main_window.py file and where we shutdown the console.

Gracefully failing / debouncing the animation timer sounds good.

The layers_change event looks good.

[tlambert03]

I made some debouncing/performance changes that I think worked pretty well. The QtDims now has a _play_ready attribute that gets set to false right before sending a set_point change to the Dims object ... and then it waits for that to cascade all the way to the SceneCanvas.event.draw event before re-enabling QtDims._play_ready. Any frame update requests sent in the meantime by the AnimationThread get ignored, (but the desired frame is not lost). The net effect is to keep up the "effective" requested frame rate, even if the graphics view can't deliver it, without choking the main thread.

The gif below shows the GUI remains responsive to start/stop requests and layer events even with 6 3D stacks "playing" as fast as 1000fps (obviously vispy isn't keeping up, but the gui remains responsive).

unfortunately, I broke my tests, since now it's waiting on a vispy draw event that never occurs in the test and so it appears to advance only a single frame. I could either mock the set_point function, or just relax the test...

[sofroniewn]

That's amazing!! If you can try using mock that would be appreciated. You can see how I did it in #629 for an example using the patch.object

    @patch.object(visual, '_on_data_change', wraps=visual._on_data_change)
    def test_ndisplay_change(mocked_method, ndisplay=3):
        viewer.dims.ndisplay = ndisplay
        mocked_method.assert_called_once()

[sofroniewn]

@tlambert03 just gave this a try - everything is working great!

One question though - when you click in the slider during an animation it only skips backward / forward one point instead of jumping to that point. You can click and drag on the slider to get back to that point, but I feel like the functionality would be more useful if it just automatically jumped. Was there a reason you made it just skip one instead of jump?

One of the usage modes I am imagining is that you're playing and then you see something cool and you want to just click the slider and have it automatically jump back to that point so you can watch it again. You can't quite do that so easily right now.

I also seem to remember seeing a comment asking for help thinking about the tests, but I can't find it so you may have deleted it. Sorry that's such a pain right now. My recommendation is to relax so we can merge - maybe leave the more strict test in there as a comment though so we can always revisit in the future if something breaks.

[tlambert03]

Thanks a lot @sofroniewn

One question though - when you click in the slider during an animation it only skips backward / forward one point instead of jumping to that point. You can click and drag on the slider to get back to that point, but I feel like the functionality would be more useful if it just automatically jumped. Was there a reason you made it just skip one instead of jump?

see the "note" in the second paragraph of my comment above: #607 (comment)
I totally agree: that is probably what the user expects/wants when clicking on the slider. But that's actually not what currently happens regardless of whether the slider is animated or not (i.e. that's just how the program works at the moment). Since it's not specific to play behavior, I was thinking it might be cleaner to address that behavior in a separate PR, and I'm happy to open it immediately. I guess the main question is whether we want to keep the current behavior (clicking on slider moves one frame) when the slider is not animated, but change the behavior (clicking on slider moves to specified frame) when QtDims.is_playing == True, or whether you want it changed globally.

yeah, I deleted the request for help; I thought about a new approach and didn't want anyone wasting their time. The new approach is working a bit better, but still failing. I'm determined to get it working and will post when ready for final review.

[sofroniewn]

Ah - totally miss understood that comment about the current behavior - I got it now. I actually thought the current behavior was to jump, not just increment left / right by one, even with no animation. I completely agree this should be a separate PR, and I'd wait until this one merges to open it. I think it would be nice if jumped for both non-animation and animation usage.

Good luck with the tests. Let us know when you are satisfied with them and then I will merge

[tlambert03]

I think the tests are stable now. I re-triggered the cirrus tests a few more times on my account and they work (excluding windows). In case anyone is curious, I focused on testing qt_dims.AnimationThread in isolation, in particular the advance() method which holds most of the logic... with just a couple loose tests to make sure that QtDims.play() triggers and stops the thread appropriately. I'm happy with the functionality and tests at this point and leave it to your review.

@jni may want to comment again on #607 (comment)

sorry everyone for the many commits and emails this PR probably caused!

things to do in another PR:

• add play buttons to GUI sliders (related to Labeled axes #644)... will also need a way to control options like fps and range
• change slider-clicking behavior

[sofroniewn]

This looks finished to me @tlambert03 - I think that seems like a good strategy on the tests.

I will leave this until end of day today in case @jni wants to weigh in once more and resolve his requested changes, but judging from his comment I think he trusts this is the right direction, and I will merge at that point

Requested changes have now been made and PR has been ready for 24hrs