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 rename optimization during save #1111

Merged
merged 2 commits into from Dec 4, 2018

Conversation

Projects
None yet
3 participants
@scribblemaniac
Copy link
Member

scribblemaniac commented Oct 19, 2018

During saving, there was some code to check if a frame changed position but was not modified so that a simple file rename could be used instead of a full image render and write.

However, there were many bugs that prevented this from working properly. I hope I have fixed them all. The most major one was caused by changes to the image loading, which would cause all frames to be loaded into memory during save. This fix will reduce memory consumption and increase the save speed by fixing this.

Thanks @Jose-Moreno for his extensive memory testing which brought this issue to my attention.

I have tested these changes in multiple scenarios and it is behaving as it should, but if I did make a mistake this could reintroduce the file wiping issues in the worst case scenario. So please review and test these changes thoroughly before merging.

Fix rename optimization during save
During saving, there was some code to check if a frame changed
position but was not modified so that a simple file rename could
be used instead of a full image render and write.

However, there were many bugs that prevented this from working
properly. I hope I have fixed them all. The most major one was
caused by changes to the image loading, which would cause
all frames to be loaded into memory during save. This fix will
reduce memory consumption and increase the save speed by fixing
this.

@scribblemaniac scribblemaniac requested a review from pencil2d/developers Oct 19, 2018

@Jose-Moreno

This comment has been minimized.

Copy link
Member

Jose-Moreno commented Oct 19, 2018

@scribblemaniac Ok I tried to compile it after getting the three files you committed but it seems QT won't let me as I'm getting about 30 errors and each time I try to build it it will halt. So i'd have to test it when there's a nightly i'm afraid Sigh, I'm sorry I made a mistake and was using the files in the wrong way. I've been able to get the files properly and I'll let you know if I get some bad behaviour.

Hopefully the others can review the code as well as I can only help by testing.

@Jose-Moreno

This comment has been minimized.

Copy link
Member

Jose-Moreno commented Oct 19, 2018

@scribblemaniac Hmm no I still can't build it after correcting my own mistake. Sorry
image
I might need to update this QT install but I don't have the time right now to be honest. If there's a nightly build I'll gladly test it later.

@chchwy

This comment has been minimized.

Copy link
Member

chchwy commented Oct 29, 2018

@Jose-Moreno
Please download this if you want to test this PR, thanks.
https://www.dropbox.com/s/cubqwtku7q293t3/pencil2d_pr1111.zip?dl=0

QDir sA, sB;
if ((sA=QFileInfo(b->fileName()).dir()) != (sB=dataFolder)) {
// Copy instead of move if the data folder itself has changed
QFile::copy(b->fileName(), tmpName);

This comment has been minimized.

Copy link
@chchwy

chchwy Oct 29, 2018

Member

Could you tell me in what scenario the data folder will change?

This comment has been minimized.

Copy link
@scribblemaniac

scribblemaniac Oct 29, 2018

Author Member

When you load a pclx file, and save it as a pcl file, the data folder will change from the temporary directory to the .data directory. @Jose-Moreno first noted problems arising from this scenario which appear to be fixed now.

@chchwy chchwy added this to the 0.6.3 milestone Dec 4, 2018

@chchwy chchwy merged commit fbade08 into pencil2d:master Dec 4, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@chchwy

This comment has been minimized.

Copy link
Member

chchwy commented Dec 4, 2018

Merged. Thanks @scribblemaniac

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.