Fix some issues related to frame copying & change pcl working directory #1896
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
c1d7e93 to
dad6e6c
Compare
dad6e6c to
7ca086c
Compare
chchwy
approved these changes
Feb 18, 2025
|
Thanks for the I have reviewed the PR and it looks good. I will merge this into the master branch and then cherry pick the fix to 0.7.1 branch. |
Since the frames stored in clipboardmanager are clones, they are not deleted by any layer and must be managed by clipboard manager.
The old approach added a 12 character suffix to the source frame's filename. This works fine when cloning an normal frame, but cloning cloned frames would result in multiple suffixes, rapidly increaing the length of the file name. While untested, I suspect this could cause serious issues when there is a small limit on the file name or path length (such as on Windows).
Non-modified frames can be unloaded. If the copied frame is loaded when copied and then later unloaded, it can end up with no data in a file and no data in memory, wiping the frame.
Now that the backing file is properly copied, we do not need to set duplicated keyframes as modified. If the source frame is modified, the cloned frame will be modified as well and remain in memory. If the source frame is not modified, then it can be copied without loading it into memory, or unloaded after copying.
This change is being made so that modifications to the working directory are not equivalent to saving the project when working with the pcl format.
7ca086c to
f20c53b
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Upon reviewing the code used to copy keyframes, multiple issues were found. These issues are:
Memory leak in ClipboardManager
A memory leak in ClipboardManager where cloned frames are not deleted. Normally KeyFrame deletion is handled by Layer, but since the KeyFrames held in the clipboard are not part of a layer, deleting them must be handled separately.
Filename exceeds the path length limit
Suffixes are always added to BitmapImages. This works as expected when cloning regular images, however when cloned images are cloned, the resulting image's filename has two suffixes. Repeat this, adding 13 characters each time, and the filename can quickly exceed the maximum length of a filename on some systems. This issue exists for various methods for copying bitmap images, but it is most obvious with copy and pasted keyframes because they are cloned twice (once to the clipboard, and once when pasting).
Frame being wiped
Pasted BitmapImages using the keyframe copy/paste can sometimes be in a state vulnerable to being wiped. What was happening here is that the clone function only copied the backing file (the .png file in the working directory corresponding to the KeyFrame) for a BitmapImage if the key is not loaded. But we force every key to be loaded when copying, so the backing files are never copied. This has been not an issue with other frame copying actions because we set the modification state of all duplicated frames to true, forcing them to stay in memory until saving.
However, the recently added copy selected frames feature forgot to set them to modified, so if the pasted frames were subsequently attempted to be unloaded by ActiveFramePool, then they would be deleted from memory because they were not marked as modified. However recall that the backing file was never copied, so the frame ends up in a state where the image data is no longer in memory and has not been saved to the disk 😱 .
A simple solution would have been to set keys to modified when pasting, just as we have done for duplicating keys or layers. However, the better solution, implemented here, is to copy the files if they have not been modified, then we don't even need to load frames to copy them, let alone keep them all in memory until saving.
This last fix raised a new problem: where to put the files for cloned frames before saving. When faced with a similar problem for movie importing, we created a new temporary directory for each import. I didn't think this would be a good solution for duplicating frames given that copying frames could happen quite often.
So I wanted to simply write the files with a temporary name to the working directory. This raised a new issue: for pcl projects, the working directory is the data directory of the save (ie. the .pcl.data directory). So by writing these temporary files, we are effectively partially saving the project. If the users decide later they do not want to save the project, these temporary files remain in the data directory indefinitely.
So, I've also added another significant change to this PR to address this. Now, when loading a pcl project, the data directory will be copied to the temporary working directory, and when saving, the files will be written back to the data directory. This fixes the aforementioned issue with the temporary files, and opens up opportunities for many new features to use the data directory (lazy-loading resources, proxy files, unloading dirty frames, improving data recovery, versioning, ...I have many ideas for this).
Right now I have opted for the simplest possible implementation so that it can be merged easier, but there is lots of room to improve how pcl projects are loaded/saved. Considering it's a legacy format, I think this functional implementation is sufficient for now.