viliusle / miniPaint

online image editor
http://viliusle.github.io/miniPaint/
Other
2.62k stars 610 forks source link

[Performance] Lag during history creation - Maybe rework undo #206

Closed Giwayume closed 3 years ago

Giwayume commented 3 years ago

When I load a 1000x1000 image and use the "select" tool to move it around, I'm seeing noticeable fps stuttering at the start of the move. After about half a second it moves smoothly, it's just at the very beginning of movement that it lags.

Performance monitor points to this line: https://github.com/viliusle/miniPaint/blob/master/src/js/tools/select.js#L133

The save function takes a 64ms hit.

I think a proper undo system would be beneficial here, where you save only the changes made by the user in the history stack (after the action is completed), instead of trying to save everything at every history event.

An example of this would be to have an "action" class for every possible action that can be accomplished in the application, which stores only the necessary data to both "do" and "undo" the action (this enables redo as well).

Example

class Layer_create_action {
    constructor(new_layer) {
        this.new_layer = new_layer;
    }
    do() {
        config.layers.push(this.new_layer);
        return this;
    }
    undo() {
        config.layers.pop();
    }
}

Running the action

config.history.push(
    new Layer_create_action({ name: 'New Layer' }).do()
);

This is a simple example but hopefully communicates the idea.

Giwayume commented 3 years ago

I think I'm going to look at this next as this seems to be a fairly big problem. I want to get a gauge for how much work it would be to convert the entire application to a context-specific undo history.

viliusle commented 3 years ago

Trying to use smart undo, like you mentioned, can take too much time to implement.

Right now on save() everything is copied to cache, so on undo current layers are overwritten by last version of archive. It could be improved by only saving current layer to cache if possible.

p.s. redo was available long time ago and disabled at some point, because it created performance problems.

Giwayume commented 3 years ago

Trying to use smart undo, like you mentioned, can take too much time to implement.

It's wide reaching, but I think it's significantly less work than the text tool changes I made. I'm testing it out.

Most of the work that needs versioning seems to center around a couple of functions in base-layers.js

viliusle commented 3 years ago

@todo for me: Optimize check_hit_region, getImageData() takes too much time on selecting object, if there are big images on screen. (dont check pixels, but use layer borders for big images)