Undo / Redo Bitmap is not properly working

Question

I have been stuck on this for a few days now and it's really beyond me why I can't get something so simple to work. I have a small drawing applicaiton, quite similiar to the Android FingerPaint demo, but I am saving each Bitmap after a drawing happens. This is because I plan to add a fill tool to my application later, and it's easier just to load the newly filled bitmap onto the canvas, instead of figuring out how to save the fill as a stroke to re-draw onto the canvas.

The drawing works great - the problem is, I can't undo and redo. I have tried with the code below, but nothing gets undone or redone. In theory, on undo, the mBitmapsDrawn should remove it's most recent Bitmap and push that onto the mBitmapsUndone stack, and then the method loadSurfaceBitmap() is called to draw the Bitmap that was saved before the one we just undid.

For redo, the mBitmapsUndone stack should pop the top Bitmap and push it back into mBitmapsDrawn, to then be re-loaded onto the Canvas by loadSurfaceBitmap().

When clicking redo/undo, no changes to the visual canvas happen, but the sizes of the data structures change (since I have log statemenets checking before and after, and they decrease/increase in size. So I think my problem is in the actual rendering of the latest bitmap in mBitmapsDrawn after a redo/undo happens.

private Stack<Bitmap> mBitmapsDrawn = new Stack<>();
private Stack<Bitmap> mBitmapsUndone = new Stack<>();

protected void touchStart(float x, float y) {
       
        mBitmapsUndone.clear();

        mPath.reset();
        mPath.moveTo(x, y);
        mX = x;
        mY = y;
        onDraw();
    }

    protected void touchMove(float x, float y) {
        float dx = Math.abs(x - mX);
        float dy = Math.abs(y - mY);
        if (dx >= TOUCH_TOLERANCE || dy >= TOUCH_TOLERANCE) {
            mPath.quadTo(mX, mY, (x + mX)/2, (y + mY)/2);
            mX = x;
            mY = y;
            onDraw();
        }
    }

    protected void touchUp() {
        mPath.lineTo(mX, mY);

        // Add path to list of paths to draw
        onDraw();
        mBitmapsDrawn.push(mBitmap);
        loadSurfaceBitmap();

        // Kill this so we don't double draw
        mPath = new Path();

        mPaint = new Paint(mPaint);

        mIsDrawing = false;
    }

    protected void onDraw() {
        if (mCanvas == null) {
            return;
        }

        mCanvas.drawPath(mPath, mPaint);
    }

    protected void loadSurfaceBitmap() {
        if (mBitmapsDrawn.size() != 0) { 
              mBitmap = mBitmapsDrawn.peek();
        } else {
              mBitmap = mBgBitmap;
        }
        Canvas surfaceCanvas = mSurfaceHolder.lockCanvas();
        surfaceCanvas.drawBitmap(mBitmap, 0, 0, null);
        mSurfaceHolder.unlockCanvasAndPost(surfaceCanvas);
    }

    protected void undo() {
        int undoSize = mBitmapsUndone.size();
        int drawnSize = mBitmapsDrawn.size();
        if (drawnSize > 0) {
            Bitmap removedBitmap = mBitmapsDrawn.pop();
            mBitmapsUndone.push(removedBitmap);
            loadSurfaceBitmap();
        }
    }

protected void redo() {
        int undoSize = mBitmapsUndone.size();
        int pathsSize = mBitmapsDrawn.size();
        if (undoSize > 0) {
            Bitmap mostRecentUndo = mBitmapsUndone.pop();
            mBitmapsDrawn.push(mostRecentUndo);
            loadSurfaceBitmap();
        }
    }

TLDR: I'm having issues popping a stack / adding to an ArrayList when undo'ing / redo'ing in my application, where I should be changing the bitmap that is loaded on the surface, but it never happens.

Edit: Updated code per the answer, still no luck. Redo doesn't change the visual bitmap on the surface.

Answer 1

If your drawing works fine it doesn't seem like there's a rendering problem - it's all calling loadSurfaceBitmap() the same way, right? I don't know if handling a touch event makes a difference and if you need to call invalidate() otherwise - I've never poked at surface canvases like this.

Your undo code is a little strange though

Bitmap removedBitmap = mBitmapsUndone.remove(drawnSize - 1);
mPathsUndone.push(removedBitmap);
loadSurfaceBitmap();

You're removing a bitmap from your Undone stack when you should be removing it from your Drawn list - I'm assuming that's why you're doing remove(size - 1) instead of just pop(), because you meant to remove from the List instead of the Stack (why not just use two Stacks anyway?).

Then you place it onto something called mPathsUndone which is maybe a typo from renaming Paths to Bitmaps for the question? If so your undo code is removing a bitmap from the undo stack, then putting it back on top - otherwise it's going somewhere mysterious. (You're also using the size of the drawn list to get an index for the undo stack so that will cause big problems too)

Your redo code looks fine to me, I'm guessing the only problem is that your undo code wasn't actually adding anything to its stack, so there's nothing to redo. I'm guessing this might fix it:

protected void undo() {
    int undoSize = mBitmapsUndone.size();
    int drawnSize = mBitmapsDrawn.size();
    if (drawnSize > 0) {
        Bitmap removedBitmap = mBitmapsDrawn.remove(drawnSize - 1);
        mBitmapsUndone.push(removedBitmap);
        loadSurfaceBitmap();
    }
}

I feel like using stacks for both would be more intuitive since it's a nice pop one, push the other flow, but that's up to you!

---

edit here's some code updating the file you posted, something like this should work:

protected void onDraw() {
    if (mCanvas == null) {
        return;
    }
    // a new drawing event needs a new bitmap layer to draw it on,
    // starting with the contents of the current layer
    mBitmap = mBitmap.copy(mBitmap.getConfig(), true);
    // we also need a canvas for the new bitmap so we can draw on it
    mCanvas = new Canvas(mBitmap);

    mCanvas.drawPath(mPath, mFgPaint);
}

Personally I'd probably move the "add this bitmap to mBitmapsDrawn" code here too, as well as the "draw the current bitmap to the surface" code - you're doing those in touchUp but I feel like they're more directly related to the action of drawing, and touchUp is more about finalising a touch gesture and initiating the draw call. Up to you!

Here's how I'd probably organise this, if it helps. Since every draw event creates a new bitmap, you don't need to keep a reference to a bitmap's Canvas - you'll have to create a new one anyway. I'd do the rest this way:

getCurrentBitmap() {
    return the top of mBitmapsDrawn, or if empty, the background bitmap
}

drawCurrentBitmap() {
    calls getCurrentBitmap(), draws it to the Surface
}

undo() {
    pops a bitmap off Drawn and pushes onto Undo, calls drawCurrentBitmap()
}

redo() {
    pops a bitmap off Undo and pushes onto Drawn, calls drawCurrentBitmap()
}

drawPath() {
    call getCurrentBitmap to get a source image
    create a new bitmap from that
    create a canvas for it and draw to it
    push the bitmap onto Drawn
    clear Undo
    call drawCurrentBitmap
}

So everything's really about manipulating the drawn and undo stacks, and every time the drawn one changes, you call a function that displays the image on top of it. And having a getCurrentBitmap function puts that logic in one place, so everything that updates the stack doesn't also need to worry about updating mBitmap with the appropriate value.

I'll also say, storing bitmaps like this works fine, but it's pretty inefficient - your memory usage could start to pile up quickly! Since you're doing drawing operations, you might want to think about storing representations of those instead, so you can get the "current" image by replaying all the past draw operations in order.

You could get fancy and cache a bitmap of the result of all but the most recent operations, and replay the most recent ones on top of that to make redrawing faster - but you'd still have all those operations in your history, almost infinite undo. Just something to consider maybe!