[Bf-blender-cvs] [1b4d5c7a355] master: Undo System: avoid redundant decoding on undo

[Campbell Barton]

Commit: 1b4d5c7a35597a70411515f721a405416244b540
Author: Campbell Barton
Date:   Tue Jul 13 17:28:07 2021 +1000
Branches: master
https://developer.blender.org/rB1b4d5c7a35597a70411515f721a405416244b540

Undo System: avoid redundant decoding on undo

In most cases the undo system was loading undo steps twice.

This was needed since some undo systems (sculpt, paint, text)
require stepping out of the current undo step.

Use a flag to limit this to the undo systems that need it.

This improves performance for other undo systems.
This gives around 1.96x speedup in edit-mesh for high-poly objects.

Reviewed by: mont29

Ref D11893

===================================================================

M	source/blender/blenkernel/BKE_undo_system.h
M	source/blender/blenkernel/intern/undo_system.c
M	source/blender/editors/sculpt_paint/sculpt_undo.c
M	source/blender/editors/space_image/image_undo.c
M	source/blender/editors/space_text/text_undo.c

===================================================================

diff --git a/source/blender/blenkernel/BKE_undo_system.h b/source/blender/blenkernel/BKE_undo_system.h
index efac5d9097f..2973a432723 100644
--- a/source/blender/blenkernel/BKE_undo_system.h
+++ b/source/blender/blenkernel/BKE_undo_system.h
@@ -162,6 +162,13 @@ typedef enum UndoTypeFlags {
    * \note Callback is still supposed to properly deal with a NULL context pointer.
    */
   UNDOTYPE_FLAG_NEED_CONTEXT_FOR_ENCODE = 1 << 0,
+
+  /**
+   * When the active undo step is of this type, it must be read before loading other undo steps.
+   *
+   * This is typically used for undo systems that store both before/after states.
+   */
+  UNDOTYPE_FLAG_DECODE_ACTIVE_STEP = 1 << 1,
 } UndoTypeFlags;
 
 /* Expose since we need to perform operations on specific undo types (rarely). */
diff --git a/source/blender/blenkernel/intern/undo_system.c b/source/blender/blenkernel/intern/undo_system.c
index fe2aa701c63..03bd9323f39 100644
--- a/source/blender/blenkernel/intern/undo_system.c
+++ b/source/blender/blenkernel/intern/undo_system.c
@@ -721,6 +721,24 @@ eUndoStepDir BKE_undosys_step_calc_direction(const UndoStack *ustack,
   return STEP_INVALID;
 }
 
+/**
+ * When reading undo steps for undo/redo,
+ * some extra checks are needed when so the correct undo step is decoded.
+ */
+static UndoStep *undosys_step_iter_first(UndoStep *us_reference, const eUndoStepDir undo_dir)
+{
+  if (us_reference->type->flags & UNDOTYPE_FLAG_DECODE_ACTIVE_STEP) {
+    /* Reading this step means an undo action reads undo twice.
+     * This should be avoided where possible, however some undo systems require it.
+     *
+     * Redo skips the current state as this represents the currently loaded state. */
+    return (undo_dir == -1) ? us_reference : us_reference->next;
+  }
+
+  /* Typical case, skip reading the current undo step. */
+  return (undo_dir == -1) ? us_reference->prev : us_reference->next;
+}
+
 /**
  * Undo/Redo until the given `us_target` step becomes the active (currently loaded) one.
  *
@@ -786,15 +804,10 @@ bool BKE_undosys_step_load_data_ex(UndoStack *ustack,
             us_target->type->name,
             undo_dir);
 
-  /* Undo/Redo steps until we reach given target step (or beyond if it has to be skipped), from
-   * given reference step.
-   *
-   * NOTE: Unlike with redo case, where we can expect current active step to fully reflect current
-   * data status, in undo case we also do reload the active step.
-   * FIXME: this feels weak, and should probably not be actually needed? Or should also be done in
-   * redo case? */
+  /* Undo/Redo steps until we reach given target step (or beyond if it has to be skipped),
+   * from given reference step. */
   bool is_processing_extra_skipped_steps = false;
-  for (UndoStep *us_iter = (undo_dir == -1) ? us_reference : us_reference->next; us_iter != NULL;
+  for (UndoStep *us_iter = undosys_step_iter_first(us_reference, undo_dir); us_iter != NULL;
        us_iter = (undo_dir == -1) ? us_iter->prev : us_iter->next) {
     BLI_assert(us_iter != NULL);
 
diff --git a/source/blender/editors/sculpt_paint/sculpt_undo.c b/source/blender/editors/sculpt_paint/sculpt_undo.c
index fe7029c7457..f21c900941b 100644
--- a/source/blender/editors/sculpt_paint/sculpt_undo.c
+++ b/source/blender/editors/sculpt_paint/sculpt_undo.c
@@ -1608,7 +1608,7 @@ void ED_sculpt_undosys_type(UndoType *ut)
   ut->step_decode = sculpt_undosys_step_decode;
   ut->step_free = sculpt_undosys_step_free;
 
-  ut->flags = 0;
+  ut->flags = UNDOTYPE_FLAG_DECODE_ACTIVE_STEP;
 
   ut->step_size = sizeof(SculptUndoStep);
 }
diff --git a/source/blender/editors/space_image/image_undo.c b/source/blender/editors/space_image/image_undo.c
index 082f66b57af..cc6effd0f71 100644
--- a/source/blender/editors/space_image/image_undo.c
+++ b/source/blender/editors/space_image/image_undo.c
@@ -1006,7 +1006,7 @@ void ED_image_undosys_type(UndoType *ut)
    * specific case, see `image_undosys_step_encode` code. We cannot specify
    * `UNDOTYPE_FLAG_NEED_CONTEXT_FOR_ENCODE` though, as it can be called with a NULL context by
    * current code. */
-  ut->flags = 0;
+  ut->flags = UNDOTYPE_FLAG_DECODE_ACTIVE_STEP;
 
   ut->step_size = sizeof(ImageUndoStep);
 }
diff --git a/source/blender/editors/space_text/text_undo.c b/source/blender/editors/space_text/text_undo.c
index f55db8c3cc9..80af7d8c9f6 100644
--- a/source/blender/editors/space_text/text_undo.c
+++ b/source/blender/editors/space_text/text_undo.c
@@ -265,7 +265,7 @@ void ED_text_undosys_type(UndoType *ut)
 
   ut->step_foreach_ID_ref = text_undosys_foreach_ID_ref;
 
-  ut->flags = UNDOTYPE_FLAG_NEED_CONTEXT_FOR_ENCODE;
+  ut->flags = UNDOTYPE_FLAG_NEED_CONTEXT_FOR_ENCODE | UNDOTYPE_FLAG_DECODE_ACTIVE_STEP;
 
   ut->step_size = sizeof(TextUndoStep);
 }