MrStevns
commented
4 years ago Good proposal and I agree, there's not much structure to the current layout and as such you have to invent a new type and figure out where to place it, while if we made a more generic structure, then it would be easier to expand and improve in the future.
As for your xml, you're still repeating 'frame' and 'position', is that intentional?
I don't think it makes sense to make a keyframe type of each comment element, they're always used in the same context so they should be read as such.
it's true there could be problems with attributes, because of the way xml handles special characters. Taking that into consideration I think it's better to create a new level, 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"/>
<comments>
<dialogue>Hello world!</dialogue>
<slug>Hello slug</slug>
<action>Hello action</action>
</comments>
</keyframe>
</layer>I'm not sure about the KeyframeContent vs Casting between classes, if it means there would be a performance penalty but the rest sounds good to me
davidlamhauge
scribblemaniac
Jose-Moreno
chchwy
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:
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:
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:
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:
Now the redundant frame information is eliminated. In the code, the KeyFrame class might look something like this:
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.