A: Why Refactor?

After a few months working with and around Krita's animation systems, @eoinoneill and I want to suggest and discuss a few potential changes.
While it's always possible to just build on top of what's already there, we both think that it will be easier and cleaner in the long to implement things like clones, cycles, animated transform masks, and other new features if we take the extra time now to clean up certain aspects of the Krita's animation system.
We can do this by defining clearer responsibilities and ownership for each piece, improving encapsulation between parts and making sure that interfaces are straight-forward and as easy to use as possible.

B: Suggested Changes

• Remove subclass-specific interface pollution from KisKeyframe and KisKeyframeChannel base classes.

A number of non-common methods and data have been added to the KisKeyframe and KisKeyframeChannel base classes.
This adds unnecessary clutter to the base interfaces and makes it harder to use and implement the derived classes. For example:

// KisKeyframeChannel.h //

    virtual bool hasScalarValue() const = 0;
    virtual qreal minScalarValue() const;
    virtual qreal maxScalarValue() const;
    virtual qreal scalarValue(const KisKeyframeSP keyframe) const;
    virtual void setScalarValue(KisKeyframeSP keyframe, qreal value, KUndo2Command *parentCommand = 0);

// KisKeyframeChannel.cpp //

qreal KisKeyframeChannel::scalarValue(const KisKeyframeSP keyframe) const
{
    Q_UNUSED(keyframe);

    return 0;
}

void KisKeyframeChannel::setScalarValue(KisKeyframeSP keyframe, qreal value, KUndo2Command *parentCommand)
{
    Q_UNUSED(keyframe);
    Q_UNUSED(value);
    Q_UNUSED(parentCommand);
}

// KisScalarKeyframeChannel.cpp //

qreal KisScalarKeyframeChannel::scalarValue(const KisKeyframeSP keyframe) const
 {
     KisScalarKeyframe *key = dynamic_cast<KisScalarKeyframe*>(keyframe.data());
     Q_ASSERT(key != 0);
     return key->value;
 }

void KisScalarKeyframeChannel::setScalarValue(KisKeyframeSP keyframe, qreal value, KUndo2Command *parentCommand)
 {
     QScopedPointer<KUndo2Command> tempCommand;
     if (!parentCommand) {
         tempCommand.reset(new KUndo2Command());
         parentCommand = tempCommand.data();
     }
 
     qreal oldValue = scalarValue(keyframe);
     KUndo2Command *cmd = new Private::SetValueCommand(this, keyframe, oldValue, value, parentCommand);
     cmd->redo();
 }

• KisKeyframeChannel should have sole responsibility for mapping "times" to "keyframes" & time state should be removed from KisKeyframe.

Right now, each KisKeyframe maintains an internal "time" state that describes its own location within the channel.
Not only is this duplication of data error prone as care has to be taken to keep things in sync, it implies a one-to-one relationship between virtual KisKeyframes and times, which complicates things clone frames and cycles.
It also creates the temptation to awkwardly use the KisKeyframe's internal time or circumvent the KisKeyframeChannel's interface:

bool KisKeyframeChannel::moveKeyframe(KisKeyframeSP keyframe, int newTime, KUndo2Command *parentCommand)
 {
     LAZY_INITIALIZE_PARENT_COMMAND(parentCommand);
 
    if (newTime == keyframe->time()) return false;
 
    KisKeyframeSP other = keyframeAt(newTime);
    if (other) {
        deleteKeyframeImpl(other, parentCommand, false);
     }
 
    const int srcTime = keyframe->time();

    KUndo2Command *cmd = new KisMoveFrameCommand(this, keyframe, srcTime, newTime, parentCommand);
     cmd->redo();
 
    if (srcTime == 0) {
        addKeyframe(srcTime, parentCommand);
     }
 
     return true;
}

Because the KisKeyframeChannel is effectively a container of KisKeyframes is shouldn't be necessary for the channel to ask a given keyframe for its internal memory of where it is in time.

• KisKeyframeChannel interface should operate on "time" indices whenever possible, and should only return KisKeyframes when the value of a keyframe is needed by the caller.

Related to the last point, this KisKeyframeChannel has quite a few methods which take in references to Keyframes (that it already owns) even though the operations only require access to time.
Here's a small example:

// Current:
-    bool deleteKeyframe(KisKeyframeSP keyframe, KUndo2Command *parentCommand = 0);
-    bool moveKeyframe(KisKeyframeSP keyframe, int newTime, KUndo2Command *parentCommand = 0);

// Improved:
+    bool deleteKeyframe(int time, KUndo2Command *parentCommand = 0);
+    bool moveKeyframe(int sourceTime, int targetTime, KUndo2Command *parentCommand = 0);

• Enforce separation between time, virtual keyframes & physical keyframes.

The KisPaintDevice is responsible for operations of "physical keyframes" using integer keyframeID handles, stored within a KisRasterKeyframe.
The KisKeyframeChannel is responsible for mapping units of time (frames numbers) to "virtual" KisKeyframes.

• KisRasterKeyframe RAII

A keyframeID is generated by the KisPaintDevice upon construction of a KisRasterKeyframeSP and freed/released upon destruction.