Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix some issues related to frame copying & change pcl working directory #1896

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

scribblemaniac
Copy link
Member

Upon reviewing the code used to copy keyframes, multiple issues were found. These issues are:

  • 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.
  • 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).
  • 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 decides 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.

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.
@chchwy chchwy self-requested a review December 18, 2024 11:38
@chchwy chchwy self-assigned this Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Needs Review
Development

Successfully merging this pull request may close these issues.

2 participants