WIP Auto save in background (wihtout window popping up)

[chchwy]

A new autosave implementation. The goal is to minimize the interruption and run the auto-save mostly in background.

The main differences are:

1. When an auto-save is triggered, write only the main.xml and the current bitmap/vector frame to the working directory, rather than running a complete save process.
2. Using a background worker thread writing files to disks without blocking the main thread.
3. No window popup anymore.

This is the first attempt to use a background thread in Pencil2D. Basically, I followed the QThread official example to implement this feature. The BackgroundTasks object is where all the async tasks are managed, and the actual work is done in BackgroundWorker running on another thread. Async tasks are driven by Qt's cross-thread signal/slot connections between BackgroundTasks and BackgroundWorker.

To avoid a data race between threads, the data preparation, including generating XML doc object of main.xml and bitmap/vector frame data copy are still in the main thread. The worker thread then takes the data and does the time-consuming part: writing to the disk.

I have done some benchmarking with a 1000-frame project.

Here is the time measured in milliseconds for each step:

1. Preparing data: 4-5ms
2. Writing main.xml to disk: 16-34ms
3. Writing a bitmap frame PNG to disk: 8-100ms (depends on the image size)
4. Writing a vector frame .VEC to disk: 4-30ms (depends on the complexity of the vector image)

As you can see, only step 1 is in the main thread, so the new auto-save will only block users around 4-5ms. step 2 to step 4 is in the background so users will not be aware of it.

I hope this improvement can encourage everyone to enable the auto-save without being interrupted regularly.

[Jose-Moreno]

@chchwy Excellent idea! If we could pair this in the future with a simple automated file backup system that could be enabled on-demand, I think we could reduce Pencil2D's recurring support issue (e.g The classic "Help my file got corrupted and I lost 1 month of work.")

Additionally, after a lot of thought, to me a backup system only needs to leave the previous file states up to a specified number of copies in either a special subset of the Pencil2D temp folder or the actual folder where the original file is saved to.

This way each time the user saves over their own file, it will first duplicate the existing copy and then proceed to save over. No need to overcomplicate it further than that.

Edit: I sent the comment too quickly. I wanted to say that i'll gladly stress-test this as soon as possible with some test files I have to see if there are any glaring issues on the behavior.

Hopefully this can be reviewed for the next release too, but I know there are some backlog issues that are being sorted right now so let's face one thing at a time 💪

[chchwy]

Some trivial comments about indentation but aside from that I think it looks good from a technical point of view. I haven't done any multi-threading in c++ yet but I do know the background/worker architecture, and it looks fine to me. Usually I'd create a backgroundTask and worker class for each type of background operation, so having a generic BackgroundTask/BackgroundWorker class potentially being used for multiple things is new to me. I assume your thoughts about calling it BackgroundTask and BackgroundWorker is because you want them to be used for other background operations and not only file saving? if not I think we should consider renaming them to something like ProjectWriteBackgroundTask and ProjectWriteBackgroundWorker

Thanks for catching the indentation problem. That reminds me that I didn't configure my IDE correctly.

In Qt, the async functions are triggered by the signal/slot mechanism, so it makes less sense to me to create a new class just for a background operation. One slot means one type of async background operation. Also there is no such cumbersome like overriding a run() with a common thread/worker interface. I personally think it should be good if the function name can reflect the purpose.

I have some thoughts about the functionality and what and when to save. I agree that doing the entire save operation may not be required all the time however the auto save process should preferably save the files that you've modified recently, if it only looks at the current frame, and you've just modified another frame and then gone back, then that frame will never be saved, unless you explicitly manually press save, which imo. goes a bit against the idea of auto saving.

Have you thought about using KeyFrame::isModified and go over the entire list of keyframes?

You got me. Will do that.

[scribblemaniac]

I have not taken the time to review the code or functionality of this, just the discussion here. I want to say that I think the idea of doing a partial save is not a good idea. If you save only the current changed frame and main.xml for example, then you potentially could have a main.xml that is not in sync with the actual data folder. For instance, if the project was loaded up from that, new frames would be there but would be blank, making it seem like the project was corrupted when in fact the last save just didn't save everything :frowning_face:

Saving in a background process however would be nice, if it is done in a way where we are careful to avoid race conditions (ex. passing a deep copy, or shudder locks).

[chchwy]

I have not taken the time to review the code or functionality of this, just the discussion here. I want to say that I think the idea of doing a partial save is not a good idea. If you save only the current changed frame and main.xml for example, then you potentially could have a main.xml that is not in sync with the actual data folder. For instance, if the project was loaded up from that, new frames would be there but would be blank, making it seem like the project was corrupted when in fact the last save just didn't save everything ☹️

Sorry, I don't quite get it. Could you explain more about the corrupted project? What could be inconsistent if the auto save does save all the changed frames?

Saving in a background process however would be nice, if it is done in a way where we are careful to avoid race conditions (ex. passing a deep copy, or shudder locks).

That's a big topic since Pencil2D don't have any multi-thread data protection. Adding locks needs to change a significant amount of code. Personally, I don't really want to make this PR too big.

A deep copy of the whole project is simply too costly, both in memory use and time. Probably the project needs to be broken down into smaller pieces and then written to disk a little at a time.

I am still thinking what's the best way to do a full save in background. Please tell me if anyone has a good idea.

[scribblemaniac]

Sorry, I don't quite get it. Could you explain more about the corrupted project? What could be inconsistent if the auto save does save all the changed frames?

You've got it backwards, I'm saying that it would be inconsistent if the autosave does not save all changed frames (including new frames, deleted frames, imported frames, etc.).

That's a big topic since Pencil2D don't have any multi-thread data protection. Adding locks needs to change a significant amount of code.

No doubt it's a lot of complexity and I don't necessarily think that's the best way to go. However, it may not be as difficult as we think. We could build this into the "StateManager" that was proposed as an extension to the current work done on the undo/redo system. That class could handle locking for all write-access modifications with a basic structure for modifying operations like this:

function modifying_operation() {
    // Do preparation

    get_lock(frameNum);

    // Make changes
    
    release_lock(frameNum);

    // Do cleanup
}

[chchwy]

Sorry, I don't quite get it. Could you explain more about the corrupted project? What could be inconsistent if the auto save does save all the changed frames?

You've got it backwards, I'm saying that it would be inconsistent if the autosave does not save all changed frames (including new frames, deleted frames, imported frames, etc.).

Ah ok. get it.

That's a big topic since Pencil2D don't have any multi-thread data protection. Adding locks needs to change a significant amount of code.

No doubt it's a lot of complexity and I don't necessarily think that's the best way to go. However, it may not be as difficult as we think. We could build this into the "StateManager" that was proposed as an extension to the current work done on the undo/redo system. That class could handle locking for all write-access modifications with a basic structure for modifying operations like this:

function modifying_operation() {
    // Do preparation

    get_lock(frameNum);

    // Make changes
    
    release_lock(frameNum);

    // Do cleanup
}

However, modifications may not come only from users. One example is BitmapImage::autoCrop(). It could potentially modify the image at any time.

[scribblemaniac]

However, modifications may not come only from users. One example is BitmapImage::autoCrop(). It could potentially modify the image at any time.

Well those operations can go through the statemanager too. Would have to think about that a bit more.

[chchwy]

True. And Ideally, the auto crop could be done by a background thread, too. It's a rather expensive operation.

[chchwy]

I am closing this PR as encountered some technical difficulties. I will reopen this PR after I figure it out.