I want to propose a restructuring to how keyframes are represented in the code and the project files. I would like to hear what everyone things about this idea, but first some background.

'Recently' I was reviewing #1292 and there was one issue with it that bothered me, and that issue is with how the comments are saved. The solution is functional, but there are some issues which I'd like to go over. I'm not picking on @davidlamhauge here or anything like that, it is just a perfect motivation for the changes I want to propose. The main.xml generated by that branch looks something like this right now:

<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE PencilDocument>
<document>
  <projectdata>
    ...
  </projectdata>
  <object>
    ...
    <layer type="1" visibility="1" name="Bitmap Layer" id="3">
      <image topLeftX="0" topLeftY="0" src="003.001.png" frame="1"/>
    </layer>
  </object>
  <framecomments>
    <content slug="" dialogue="Hello world!" layer="2" frame="1" action=""/>
  </framecomments>
</document>

Now there are a couple things I think are wrong with this. First off, storing the strings as attributes is not ideal because they could theoretically be quite large and have have special characters which an element may be better suited for. So we could do something like this:

<framecomments>
  <content layer="2" frame="1">
    <dialogue>Hello world!</dialogue>
  </content>
</framecomments>

There's an attribute for the layer that these comments are for, but there is also that same information in the layer tag. It seems natural that the frame comments should be in the layer tag like so:

<layer type="1" visibility="1" name="Bitmap Layer" id="3">
  <image topLeftX="0" topLeftY="0" src="003.001.png" />
  <framecomment frame="1">
    <dialogue>Hello world!</dialogue>
  </framecomment>
</layer>

However after all this, there is still redundant data in the form of the frame number. This is where my proposal comes into play. Right now a keyframe and it's contents are synonymous. A BitmapImage or a SoundClip is literally a KeyFrame type in the code and the only indicator that a keyframe exists in the save file is whether there is an image take at that position or not. It's clear from the frame comments, as well as when considering potential future improvements such as easing, time based properties (ex. opacity fading in/out), or masks where the keyframes may consist of more than simply their trivial content. So what if we instead treated them as generic containers? For the file format it might start looking something like this:

<layer type="1" visibility="1" name="Bitmap Layer" id="3">
  <keyframe position="1">
    <image topLeftX="0" topLeftY="0" src="003.001.png" frame="1"/>
    <dialogue>Hello world!</dialogue>
  </keyframes>
</layer>

Now the redundant frame information is eliminated. In the code, the KeyFrame class might look something like this:

class KeyFrame
{
public:
    enum KeyframeType
    {
        BITMAP,
        VECTOR,
        MOVIE, // not supported yet
        SOUND,
        CAMERA,
        SLUG,
        DIALOGUE,
        ACTION
    };

    // Constructors, destructor and functions applicable to the keyframe as a whole here

    template <KeyframeContent T>
    T* getContent(KeyframeType type) {
        return static_cast<T>(mContents.find(type).value());
    }

    void setContent(KeyframeType type, KeyframeContent contents) {
        // Perhaps the parent layer could be called here to check for any restrictions (no sound on a bitmap layer, etc.)
        // Alternatively the Keyframe class could have subclasses for each layer type, but that sounds like it would bring back a lot of the test-and-cast code we currently have
        // Personally I think that it is layer logic and it makes sense for it to be handled there, although storing the parent layer could complicate some things as we would have to make sure that remains correct.
        mContents.insert(type, contents);
    }
private:
    QMap<KeyframeType, KeyframeContent*> mContents;

    // Any other necessary member variables for the above functions
};

Basically, a Keyframe would just hold a collection of objects that inherit from a new KeyframeContent type. The particular implementation above has a limitation of at most one of each KeyframeType per Keyframe container, but it doesn't necessarily have to be that way if it makes sense to allow multiple of some type on a single keyframe.

There are multiple advantages to this. First it would be more natural to parse the proposed xml structure above. Second it would make it easier to attach additional data to keyframes in the future. Just make a new KeyframeType entry and a sublass of KeyframeContent and you would be able to add that right away to keyframes and have them move, save, delete, etc. with the keyframe.

The main disadvantage is of course that this could take a fair amount of work to implement since keyframes are used in so many places. Fetching the KeyframeContents will also slow down some operations, although it should be minor as long as we aren't attaching a large number of different data objects to each keyframe. Changing the file format always has risks associated with it. These changes would be breaking changes to fine file format, so a version converter would need to be written into Pencil2D to open old project files. With a basic implementation there would not be backwards compatibility, but this could theoretically be achieved by adding the elements both inside and outside of the keyframe tags. It is worth noting that I believe file loading and saving needs to be rewritten anyway at some point to use Qt's xmlstreams so much of that work could be done at the same time as this.

I've probably missed a few points. I started to write this a while ago and just never got around to submitting it so I thought I would just polish it up to the best of my ability with what I can remember and put it out there to see what everyone's thoughts are on this.