Feature #748 - undo redo keyframes #1885
Conversation
To avoid memory leaks when calling createState too many times, without applying the state
This is a temporary issue that will be obsolete once we support restoring the layer state
So we can set the input ptr to null again
chchwy
changed the title
|
Just taking my first steps with Pencil2D and this turns out to be one of the most painful missing features: Copy&Paste of keyframes is a tricky operation and often goes wrong. That on its own would be annoying. The fact that the operation is then not undoable is horribly painful because it can leave the timeline completely messed up. Hope this feature gets completed at some point! |
The scenario is you call create state from - Move tool, move selection - Meanwhile you move the keyframe to a different layer - You then deselect which creates a new keyframe due to no keyframe being there - this deletes mCurrentState but that is not the same ptr as the one in MoveTool.. as such it is now dangling - Then we get into MoveTool::mouseReleaseEvent which tries to create a record of the undoState but that has now become garbage. Now it is only the caller who can clear the ptr rather, although it is still handled automatically due to record
The mCurrentState was flawed in the way that it assumed you would always have a createState followed by record(...) which is not always the case... the scenario i ran into was: creating a selection and then moving some keyframes, leaving the scrubbed position with no keyframe, this would then call createState, deleting the current mCurrentState in the manager (and leaving the one stored in MoveTool dangling) which when you then tried to apply the selection, it would crash because the UndoSaveState had been deleted memory wise but the ptr wouldn't know that. The solution i came up with to fix this is that we instead of getting access to the raw ptr, we get a handle now called SAVESTATE_ID. The id is used to fetch a given saveState stored temporarily in undoRedoManager. This now also means that we can create multiple save states and apply all of them without having to think about dealing with their lifetime. In order to still allow the caller to add additional state to the record, I have also introduced a new struct called UserSaveState which is meant to work as a mega struct in the way that it can store all kinds of data but you should only add the data you actually need.
|
When i wrote this PR and also back when I started on the undo/redo rewrite I had my doubts about undoSaveState implementation, specifically the way we deal with the lifetime of the current save state and delete it. I was never fully happy with that implementation and just recently bumped into the reason why it was indeed a bad idea. Dangling ptrs... I knew about the problem but at the time i couldn't figure out a better solution, so I left it there... With this PR and especially the latest changes, I think I have finally come up with a solution that I like and is safer to use. Instead of getting access to the raw ptr, we get a handle to the save state called SAVESTATE_ID. The id is used to fetch a given saveState stored temporarily in undoRedoManager. This now also means that we can create multiple save states and apply all of them without having to think about dealing with their lifetime. Once the state has been recorded, it will deleted automatically. In most cases the save state will almost always be accompanied by record(...) right away but I did bump into a few scenarios I hadn't thought of, which showed the flaws in the previous implementation. By keeping a list of save states rather than only one, we fix this problem. I thought about using a circular buffer instead since it's not likely we'll need more than 5 slots but then again, KISS until proven otherwise... With that I think the PR is ready to be reviewed. |
2 participants
This PR adds the following to the new undo/redo system:
In addition I had to make some changes to the structure of UndoRedoManager, as it wasn't as flexible as I liked it to be. The result of this means you can now add additional state changes to the UndoSaveState once it has been created. This is for example used to add data which is mostly proprietary to the TimeLineCells timeline interaction methods and can therefore not be fetched from within the state methods of UndoRedoManager.
The way to add custom data is through a new UserSaveState struct that can be created and stored on given SaveState based on a new handle called SAVESTATE_ID. Rather than getting direct access to the UndoSaveState ptr, we now instead only get access to an identifier. By doing so we prevent cases where the UndoSaveState could be dangling.
To add user data to the undoState, you call
UndoRedoManager::addUserState(<id>, myState)I've also introduced a set of guards to avoid crashing when calling undo/redo on data that's no longer available. For example due to the implementation currently not having delete layer undo/redo functionality, if you tried to undo a keyframe operation after such an operation, the application would crash.
The way to prevent that currently is to mark the command as obsolete, which removes it from the stack.
closes #748, #1935