Animation Timeline Docker: Insert Keyframes with Timing.
ClosedPublic

Authored by emmetoneill on May 13 2018, 5:34 AM.

Details

Summary

This patch adds timing functionality to the Animation Timeline Docker's "Insert N Keyframes" menu actions. Out of necessity, it also replaces the create-on-demand QInputDialog with a new TimelineInsertKeyframesDialog that was designed as a drop-in replacement that allows for getting more than a single variable of user input (i.e. frame count and timing) and which could easy be given more functionality in the future.

The motivation behind this patch was to improve the "Insert N Keyframes Right/Left" action workflow by giving the animator control of frame timing. Previously, those actions worked by adding a number of immediately adjacent frames based on the user's desired number of frames to insert.

In animation, keyframes that are immediately adjacent to each other are described as being "on 1s", but other timings are also common - "on 2s", "on 3s", etc. - in which drawings are held for different amounts of time. Animations typically maintain a particular rhythm for a span of frames, but it's also very common for different parts of a single animation to switch timings.

This patch improves upon existing functionality to facilitate animating on 1s, 2s, 3s, and others. Now, a Krita user can specify a timing of 2 to quickly and easily insert keyframes "on 2s", a timing of 3 to insert "on 3s", and so on, allowing even longer timings if desired.

( Pair-programmed on a sunny Portland Saturday with @eoinoneill, of course! =] )

Test Plan
  1. Right-click a position on the animation Timeline docker.
  1. Select "Keyframes > Insert N Keyframes Right" or "Keyframes > Insert N Keyframes Left".
  1. A new dialog windows should pop up, asking for a number of frames and a timing.
  1. Test different values as well as both the "Cancel" and "OK" buttons and check for predictable behavior in a variety of cases.

Diff Detail

Repository
R37 Krita
Lint
Lint Skipped
Unit
Unit Tests Skipped
emmetoneill requested review of this revision.May 13 2018, 5:34 AM
emmetoneill created this revision.
emmetoneill edited the summary of this revision. (Show Details)May 13 2018, 6:27 AM

I added @Bollebib onto this so he can give feedack on the useful-ness of this

I added @Bollebib onto this so he can give feedack on the useful-ness of this

Sure.

dkazakov requested changes to this revision.May 15 2018, 1:21 PM

Hi, @emmetoneill!

I really love this patch! The idea is very useful, I wonder how we didn't come up to it before :)

There are two small bugs in the patch, though:

  1. [blocker] Right now, the the action always works as "Insert Column Right/Left", that is, it adds frames to all the layers in the timeline. It happens dues to a accidental true passed to a function (check my inline comment).
  2. [wish] Could you also check why "Insert Column" functionality doesn't use your new dialog? I guess it should just be changed in TimelineFramesView::slotInsertColumns{Left,Right}Custom().

Otherwise, I really love the idea of the patch and looking forward to seeing it in master! :)

plugins/dockers/animation/timeline_frames_view.cpp
1391

There is a spurious true parameter that should actually be false :)

Right now if you have two visible layers in timeline, this action works as "insert columns" instead of "insert frames".

PS:
Perhaps we should change these booleans to QFlags in the future, but it is out of scope of this patch of course :)

This revision now requires changes to proceed.May 15 2018, 1:21 PM

Update:

  • Fixed bug that caused unwanted creation of columns. Whoops!
  • Added timing and GUI changes to column insertion.
emmetoneill marked an inline comment as done.EditedMay 15 2018, 6:41 PM

@dkazakov, thanks for the kind words and feedback. I'm glad you like it. I went ahead and made the changes that you requested and I actually properly tested the column insertion this time! Even though I saw the code for columns before, I actually wasn't fully aware of / didn't fully understand what the feature was or how to use it because I never tried clicking on the timeline. That's my mistake though, as it's a useful feature!

I also have some ideas about how we might be able to improve both the user experience as well as the code here, at some point:

  • We might be able to simplify the right-click menu options by replacing the two "Insert N Keyframes Left/Right" with a single "Insert Multiple Keyframes" menu item. We could then add a Right/Left radio button option within the new insert keyframes dialog. Of course the same thing could be done for columns insertion, replacing "Insert N Columns Left/Right" with a single "Insert Multiple Columns". Of course, just like the current insert keyframes dialog, the state would be saved across each use.
  • From a code standpoint, there is a lot of duplicate/similar code that I think could be reduced if we were to similarly parameterize things like functions with things like direction (which could be an enum for clarity, or a bool for simplicity). I understand that some of the more explicit functions may need to exist in order to hook them up to buttons or actions, but in my opinion those could be simply pass-through to more general functions. I'm not sure if I'm doing a great job describing what I'm talking about here... haha

Anyway, I'd be happy to go through with some of those changes in the near future, probably in a separate diff/cr though.

  • From a code standpoint, there is a lot of duplicate/similar code that I think could be reduced if we were to similarly parameterize things like functions with things like direction (which could be an enum for clarity, or a bool for simplicity). I understand that some of the more explicit functions may need to exist in order to hook them up to buttons or actions, but in my opinion those could be simply pass-through to more general functions. I'm not sure if I'm doing a great job describing what I'm talking about here... haha

Just a quick note: there are so many separate functions with duplications, because all these functions are connected to signals as slots :) This can be easily solved with QSignalMapper though :)

dkazakov requested changes to this revision.May 16 2018, 8:24 PM

Hi, @emmetoneill!

The frame insertion bug is now fixed!

There is still one small issue: both 'Insert N Columns Left' and 'Insert N Columns Right' insert columns to the left of the current selection now. It looks like it is my code duplication issue, sorry :)

Please update the patch with a fix and I will push it! :)

This revision now requires changes to proceed.May 16 2018, 8:24 PM

Stupid mistake there, sorry about that! Seems to be all fixed up now and working correctly. Thanks for catching that, Dmitry.

This revision was not accepted when it landed; it landed in state Needs Review.May 17 2018, 5:38 AM
This revision was automatically updated to reflect the committed changes.