Skip to content

Animation: KisKeyframeChannel / KisKeyframe & Subclasses Refactor

Emmet O'Neill requested to merge (removed):ani/refactor into master

This branch refactors a few core pieces of Krita's animation systems with the goal of making them easier to use, easier to test, easier to build on top of and hopefully a bit more stable. This refactor is a work in progress and there is undoubtedly more that can be done, so let us know if you think there is more we can do to clean things up or make these classes easier to work with. =]

(NOTE: The base and raster stuff is mostly finished and should be working well. The scalar/curves stuff is unstable.)

Our initial proposal can be found here, and here's a rundown on what's changed so far.

Key Changes:

1. We've removed as much clutter as possible from the KisKeyframeChannel and KisKeyframe base classes.

These classes previously contained quite a few pieces of data and methods that were specific to a given subclass (like KisRasterKeyframeChannel or KisScalarKeyframeChannel). We want to make sure that our base classes are as lean and easy to use as possible and contain only the parts that are actually needed throughout the entire class hierarchy.

2. We've made significant changes to the interfaces of KisKeyframeChannel and KisKeyframe.

Clients of a given channel now communicate mostly in terms of time (in frames--indices for the channel's internal times-keyframes map), only requesting a virtual KisKeyframeSP when they need to access the value of a given frame.

We've also revamped and simplified the signals that KisKeyframeChannel exposes, and swapped the requestUpdate function with a new sigChannelUpdated signal as a more flexible and dogmatic way of calling back into the node that owns the channel.

3. As the basis for our new channel interface, we've rewritten most of the unit tests in kis_keyframing_tests.cpp so far.

We've made sure that the new tests cover at least the same verification points as the old tests, and we've actually added quite a few more, which will hopefully improve stability down the road. (We wrote the test first. 😉)

4. We've automated the allocation and freeing "physical" paint device frameIDs with the creation of "virtual" keyframes inside a raster channel.

We've gone with a kind of "RAII" style approach to raster keyframes, where a the paint device creates and returns a new frameID each time a unique KisRasterKeyframe is constructed while automatically freeing the frameID when the KisRasterKeyframe is destructed. This means (a) that we no longer require the channel or its user to clean up frame-specific data and (b) it should be easier to use shared pointers to KisRasterKeyframes to implement reference counted clone frames. We hope. =]

5. We've tried to reduce code duplication as much as reasonably possible and have created static functions for channel-to-channel move/copy/swap operations.

Operations like swap and copy are able to use the move operation internally, and single-channel operations are simply convenience methods that wrap the multi-channel operations.

6. We've updated and simplified how undo-redo works within the KisKeyframeChannel.

Because of changes 4 and 5 we are able to simplify how undo redo works, as commands have less responsibility for cleaning up and no longer have to be passed through as many layers of classes. Only top level, user-facing actions (add, move, copy, remove, etc.) need specific undoredo commands, and the code should work well and be easy to understand even if they aren't there.

7. Made KisRasterKeyframe's copy constructor private, exposing a "duplicate" method instead.

We've done this to prevent any implicit copies, which will eventually help clarify the difference between duplicate frames and cloned frames.

Also, various implementations were updated as we saw fit and code has been commented, renamed, and moved around as needed for clarity.

I'm sure I'm forgetting a thing or two...

Where to look:

Because there have been quite a few changes, renames and chunks of code moved around, it might be kind of hard to dive straight into the diff.

Instead I'd recommend that you start with libs/image/tests/kis_keyframing_test.cpp to get a general sense of how the new KisKeyframeChannel interface works.

What to test:

  1. Raster animation on the Animation Timeline Docker. Creating, drawing, moving, removing, copying keyframes, and so on. Everything should undo/redo correctly.

  2. Opacity animation on the Animation Curves Docker. We should be able to add/remove/move keyframes, change interpolation and tangent modes, tweak curves, etc. Everything should undo/redo correctly.

  3. Unit tests in libs/image/tests/kis_keyframing_test should all pass!

Designed and implemented with @eoinoneill. Reviewed by @dkazakov. Thanks!

Edited by Emmet O'Neill

Merge request reports