Undo implementation

[nidoro]

Hello,
I'm getting unexpected results from my undo implementation. This is how I'm doing it:

1. Initialize (global) empty array of strokes:
let strokes = []

2. When strokerecorded is fired, push stroke to array:

atrament.addEventListener('strokerecorded', function(obj) {
    if (!atrament.recordPaused) {
        strokes.push(obj.stroke);
    }
});

3. When the undo button is pressed, do the following:

function undoStroke() {
    strokes.pop();
    atrament.clear();
    atrament.recordPaused = true;
    for (let stroke of strokes) {
        // set drawing options
        atrament.mode = stroke.mode;
        atrament.weight = stroke.weight;
        atrament.smoothing = stroke.smoothing;
        atrament.color = stroke.color;
        atrament.adaptiveStroke = stroke.adaptiveStroke;

        // don't want to modify original data
        const points = stroke.points.slice();

        const firstPoint = points.shift();
        // beginStroke moves the "pen" to the given position and starts the path
        atrament.beginStroke(firstPoint.x, firstPoint.y);

        let prevPoint = firstPoint;
        while (points.length > 0) {
            const point = points.shift();

            // the `draw` method accepts the current real coordinates
            // (i. e. actual cursor position), and the previous processed (filtered)
            // position. It returns an object with the current processed position.
            const { x, y } = atrament.draw(point.x, point.y, prevPoint.x, prevPoint.y);

            // the processed position is the one where the line is actually drawn to
            // so we have to store it and pass it to `draw` in the next step
            prevPoint = { x, y };
        }

        // endStroke closes the path
        atrament.endStroke(prevPoint.x, prevPoint.y);
    }
    atrament.recordPaused = false;
}

But this is what I get:

Notice how after I press undo, the last line disappears, as expected, but the other two lines are extended a little bit.

Can you help me with this?
I'm on Linux. This happens on both Firefox 72.0.1 and Google Chrome 89.0.4389.90.

Thank you for the library!

[Gnarly-Charley]

Hi @nidoro

I ran into the same problem. In my case it seemed to be solved after modifying the while loop from "while (points.length > 0)" to "while (points.length > 1)". I haven't quite figured out why this works yet, but for now this is good enough for me.

[jakubfiala]

@nidoro thank you for submitting the issue! Apologies for only getting to this now, I haven't been able to dedicate time to this lib for a while. I'll test your use case, try to reproduce the bug and fix it in the next few weeks.

[feored]

Thanks for this example @nidoro . I modified it slightly to add support for fills, and fixed a few things that I assume have changed since your post (Point.point.x instead of point.x, ...). Not battle tested or anything but might save someone else a bit of trouble.

        this.sketchpad.addEventListener('strokerecorded', (obj) => {
            if (!this.sketchpad.recordPaused) {
                obj.stroke.type = "stroke";
                this.strokes.push(obj.stroke);
            }
        });

        this.sketchpad.addEventListener('fillstart', ({ x, y }) => {
            var obj = {};
            obj.type = "fill";
            obj.fillColor = this.sketchpad.color;
            obj.x = x;
            obj.y = y;
            this.strokes.push(obj);
        });

undo(){
            this.strokes.pop();
            this.sketchpad.clear();
            this.sketchpad.recordPaused = true;
            for (let stroke of this.strokes) {
                if (stroke.type == "stroke"){
                    // set drawing options
                    this.sketchpad.mode = stroke.mode;
                    this.sketchpad.weight = stroke.weight;
                    this.sketchpad.smoothing = stroke.smoothing;
                    this.sketchpad.color = stroke.color;
                    this.sketchpad.adaptiveStroke = stroke.adaptiveStroke;

                    // don't want to modify original data
                    const points = stroke.points.slice();

                    const firstPoint = points.shift().point;
                    // beginStroke moves the "pen" to the given position and starts the path
                    this.sketchpad.beginStroke(firstPoint.x, firstPoint.y);

                    let prevPoint = firstPoint;
                    while (points.length > 0) {
                        const point = points.shift();

                        // the `draw` method accepts the current real coordinates
                        // (i. e. actual cursor position), and the previous processed (filtered)
                        // position. It returns an object with the current processed position.
                        const { x, y } = this.sketchpad.draw(point.point.x, point.point.y, prevPoint.x, prevPoint.y);

                        // the processed position is the one where the line is actually drawn to
                        // so we have to store it and pass it to `draw` in the next step
                        prevPoint = { x, y };
                    }

                    // endStroke closes the path
                    this.sketchpad.endStroke(prevPoint.x, prevPoint.y);
                } else {
                    this.sketchpad.color = stroke.fillColor;
                    const startColor = Array.from(this.sketchpad.context.getImageData(stroke.x, stroke.y, 1, 1).data);
                    this.sketchpad._floodFill(stroke.x, stroke.y, startColor);
                }
                
            }
            this.sketchpad.recordPaused = false;
        }

[jakubfiala]

@nidoro I may be nearly 3 years late, but I believe this bug has now been fixed via db7242e :) it looks like the stroke segments recorded when beginStroke and endStroke are called were redundant. Specifically the segment recorded during endStroke caused the re-drawn stroke to be slightly longer than the original.

I am now working on a big update which will be released as v4 in the next few weeks. This fix will be part of it.

Thank you again for the bug report and apologies for being very, very late!

[nidoro]

Hi @jakubfiala, no need for apologies, that's the nature of open source. I'm currently not using atrament in any of my projects, but it's good to know that, if I ever need it, this will be fixed. Thank a lot!

[Innders]

@feored This works pretty well but there is one things that might trip some people up.

The redraw loop will set the brush style for each undo and redo which is correct but then it will leave the brush style in that state when you next go to draw again.

Maybe this is a feature not a bug but you will most likely want to restore the original brush styles to what they were when you started redrawing the history.

Here is my implementation...

 // Store original brush settings
const original = {
  mode: annotations.mode,
  weight: annotations.weight,
  smoothing: annotations.smoothing,
  color: annotations.color,
  adaptiveStroke: annotations.adaptiveStroke,
};

annotations.clear();
annotations.recordPaused = true;

const history = historyMap.get(pageNumber) || [];

// Replay all strokes up to the target index
for (let i = 0; i <= targetIndex; i++) {
  const stroke = history[i];
  if (!stroke?.segments?.length) continue;

  // Set stroke properties
  annotations.mode = stroke.mode;
  annotations.weight = stroke.weight;
  annotations.smoothing = stroke.smoothing;
  annotations.color = stroke.color;
  annotations.adaptiveStroke = stroke.adaptiveStroke;

  // Create a copy of segments to avoid modifying original data
  const segments = [...stroke.segments];
  const firstPoint = segments.shift().point;
  annotations.beginStroke(firstPoint.x, firstPoint.y);

  let prevPoint = firstPoint;
  while (segments.length > 0) {
    const point = segments.shift().point;
    const { x, y } = annotations.draw(
      point.x,
      point.y,
      prevPoint.x,
      prevPoint.y
    );
    prevPoint = { x, y };
  }

  annotations.endStroke(prevPoint.x, prevPoint.y);
}

annotations.recordPaused = false;

// Restore original brush settings
// timeout because it breaks without?!
setTimeout(() => {
  annotations.mode = original.mode;
}, 100);
annotations.weight = original.weight;
annotations.smoothing = original.smoothing;
annotations.color = original.color;
annotations.adaptiveStroke = original.adaptiveStroke;

Interestingly if the original mode was eraser and you undo a brush stroke it won't work properly unless the restoring of the mode is in a timeout, kind of weird, any ideas @jakubfiala ?

[jakubfiala]

@Innders hmm that's odd, I can't think of a reason that would be necessary - Atrament doesn't execute any asynchronous code (other than passing messages between the fill worker and the main thread). Perhaps there's some other code in your application that updates the mode?