Page MenuHome

Image Undo, fixes T61263
ClosedPublic

Authored by Campbell Barton (campbellbarton) on Sep 29 2019, 6:18 PM.

Details

Summary

The main change from this patch is image undo stores all tiles for each step.

Previously, undo steps only stored changes which relied on the the image cache being persistent.

This resolves T61263, which was error prone since the image cache can be freed.

Summary

  • While each undo step stores pre/post image buffer made up of tiles, shared between undo steps.
  • This adds support for images changing size in undo steps (added an image scale operator to test this works).
  • Paint tiles have been split out into a separate tile type, because they're used quite different from undo-image tiles.
  • Painting won't be negatively impacted, any delay from having to store extra undo information will occur once the paint-stroke is completed.

Implications

  • Every undo step now contains a list of image buffers, allowing undo to track resizing operations, not just brush strokes or color adjustments.
  • This uses more memory, once an image has undo steps assosiate dwith it - the image buffer is stored at least twice, since we don't rely on applying changes to the cache (which may be freed at run-time). While using more memory is a down-side, it allows the cache to be freed without corrupting undo - T61263. With the advantage that undo can support operators can change the image size (resize / crop).

Remaining Issues

  • Memfile undo can still cause the cache to be freed and the image to get re-loaded.

    Solving this requires loading an image cache to use the undo tiles as a source instead of the on-disk image data (or re-generating the image).
  • Currently we don't have a good way to access multiple, different ImBuf's per image. In practice this doesn't seem to be used in the case of painting.

    To properly support this we could have an ImageUser in each image-undo-buffer and assume the ImageUser.scene is always the active scene.

Improvements

  • Track differences between the current state and the undo state, so only the different tiles need to be copied onto the image (could also use to only update modified regions on the GPU).
  • Support undoing arbitrary image operations by detecting changes and only storing tiles that change (currently no image operations would use this, however it would be useful if we support more image operations).

Details for Review

  • Public functions to paint-tiles should be renamed from undo_tiles to paint_tiles to avoid confusion, this can be done in a separate commit.
  • We could remove temporary image buffer use (support copying from/to raw buffers - without an ImBuf), since having to create the temporary buffer isn't very convenient.

    I looked into this, it means duplicating quite a bit of IMB_rectblend, twice - since we would need to copy from/to a raw buffer. An alternative would be to create an ImBuf in stack memory and assign it's members, while this could work I'd rather do this separately from this patch.
  • The resize operator would be committed separately, if at all, it's just useful for testing.

Diff Detail

Repository
rB Blender
Branch
TEMP-UNDO-PAINT-REWRITE
Build Status
Buildable 5185
Build 5185: arc lint + arc unit

Event Timeline

Campbell Barton (campbellbarton) retitled this revision from Image Undo, fix for T61263 to Image Undo, fixes T61263.Sep 29 2019, 6:38 PM
Campbell Barton (campbellbarton) edited the summary of this revision. (Show Details)

Rename image.scale -> image.resize (matches transform)

source/blender/editors/sculpt_paint/paint_image_undo.c
251–254

Sounds a bit fragile. Do you think we can do some assert() to catch possible race conditions? Or, maybe, consider lock cheap enough and always acquire it?

409

image_dims?

source/blender/editors/space_image/image_ops.c
2909

DEG_id_tag_update(&ima->id, 0); to inform textures and such about the change.

Campbell Barton (campbellbarton) marked an inline comment as done.
  • Rename 'dims' -> 'image_dims'

Update based on feedback.

source/blender/editors/sculpt_paint/paint_image_undo.c
251–254

Not sure why we would assert?


This logic is kept unchanged from before, not against further changes but think it's safe to keep as-is and make changes here as a separate patch.

This uses more memory, once an image has undo steps assosiate dwith it - the image buffer is stored at least twice, since we don't rely on applying changes to the cache (which may be freed at run-time). While using more memory is a down-side, it allows the cache to be freed without corrupting undo.

I think we can avoid that duplicate memory usage, but I don't consider it a blocker.

Currently we don't have a good way to access multiple, different ImBuf's per image. In practice this doesn't seem to be used in the case of painting.

Multiple layers/passes are not supported for painting indeed, fine to not support this in undo until that changes.

source/blender/editors/space_image/image_ops.c
2931

Size is going to 0 by default, which I guess will fail in exec. I think we can initialize to something arbitrary like 1024.

2935

Update this comment.

This revision is now accepted and ready to land.Oct 1 2019, 3:46 PM