[Bf-blender-cvs] [c9d6737e3e9] master: BKE UndoSys refactor: deduplicate and simplify code, sanitize naming.
Bastien Montagne
noreply at git.blender.org
Wed Feb 3 11:11:49 CET 2021
Commit: c9d6737e3e9a00bc297511af4a4d1cf2ed6f8cce
Author: Bastien Montagne
Date: Wed Jan 27 16:42:50 2021 +0100
Branches: master
https://developer.blender.org/rBc9d6737e3e9a00bc297511af4a4d1cf2ed6f8cce
BKE UndoSys refactor: deduplicate and simplify code, sanitize naming.
Now we only use 'undo' or 'redo' in function names when the direction is
clear (and we assert about it). Otherwise, use 'load' instead.
When passing an undo step to BKE functions, consider calling code has
done its work and is actually passing the target step (i.e. the final
step intended to be loaded), instead of assuming we have to load the
step before/after it.
Also deduplicate and simplify a lot of core undo code in BKE, now
`BKE_undosys_step_load_data_ex` is the only place where all the complex
logic of undo/redo loop (to handle several steps in a row) is placed. We also
only use a single loop there, instead of the two existing ones in
previous code.
Note that here we consider that when we are loading the current active
step, we are undoing. This makes sense in that doing so //may// undo
some changes (ideally it should never do so), but should never, ever
redo anything.
`BKE_undosys_step_load_from_index` also gets heavily simplified, it's
not basically a shallow wrapper around
`BKE_undosys_step_load_from_index`.
And some general update of variable names, commenting, etc.
Part of T83806.
Differential Revision: https://developer.blender.org/D10227
===================================================================
M source/blender/blenkernel/BKE_undo_system.h
M source/blender/blenkernel/intern/undo_system.c
M source/blender/editors/undo/ed_undo.c
===================================================================
diff --git a/source/blender/blenkernel/BKE_undo_system.h b/source/blender/blenkernel/BKE_undo_system.h
index 0603ef85cc1..4c3b21482ea 100644
--- a/source/blender/blenkernel/BKE_undo_system.h
+++ b/source/blender/blenkernel/BKE_undo_system.h
@@ -203,23 +203,32 @@ UndoStep *BKE_undosys_step_find_by_name_with_type(UndoStack *ustack,
UndoStep *BKE_undosys_step_find_by_type(UndoStack *ustack, const UndoType *ut);
UndoStep *BKE_undosys_step_find_by_name(UndoStack *ustack, const char *name);
+int BKE_undosys_step_calc_direction(const UndoStack *ustack,
+ const UndoStep *us_target,
+ const UndoStep *us_reference);
+
+bool BKE_undosys_step_load_data_ex(UndoStack *ustack,
+ struct bContext *C,
+ UndoStep *us_target,
+ UndoStep *us_reference,
+ const bool use_skip);
+bool BKE_undosys_step_load_data(UndoStack *ustack, struct bContext *C, UndoStep *us_target);
+void BKE_undosys_step_load_from_index(UndoStack *ustack, struct bContext *C, const int index);
+
bool BKE_undosys_step_undo_with_data_ex(UndoStack *ustack,
struct bContext *C,
UndoStep *us,
bool use_skip);
-bool BKE_undosys_step_undo_with_data(UndoStack *ustack, struct bContext *C, UndoStep *us);
+bool BKE_undosys_step_undo_with_data(UndoStack *ustack, struct bContext *C, UndoStep *us_target);
bool BKE_undosys_step_undo(UndoStack *ustack, struct bContext *C);
bool BKE_undosys_step_redo_with_data_ex(UndoStack *ustack,
struct bContext *C,
UndoStep *us,
bool use_skip);
-bool BKE_undosys_step_redo_with_data(UndoStack *ustack, struct bContext *C, UndoStep *us);
+bool BKE_undosys_step_redo_with_data(UndoStack *ustack, struct bContext *C, UndoStep *us_target);
bool BKE_undosys_step_redo(UndoStack *ustack, struct bContext *C);
-bool BKE_undosys_step_load_data(UndoStack *ustack, struct bContext *C, UndoStep *us);
-
-void BKE_undosys_step_undo_from_index(UndoStack *ustack, struct bContext *C, int index);
UndoStep *BKE_undosys_step_same_type_next(UndoStep *us);
UndoStep *BKE_undosys_step_same_type_prev(UndoStep *us);
diff --git a/source/blender/blenkernel/intern/undo_system.c b/source/blender/blenkernel/intern/undo_system.c
index 5f99f629f42..643510cf652 100644
--- a/source/blender/blenkernel/intern/undo_system.c
+++ b/source/blender/blenkernel/intern/undo_system.c
@@ -251,42 +251,10 @@ static void undosys_stack_validate(UndoStack *ustack, bool expect_non_empty)
BLI_assert(!BLI_listbase_is_empty(&ustack->steps));
}
}
-
-/* Return whether `us_item` is before (-1), after (1) or same as (0) `us_anchor` step. */
-static int undosys_stack_order(const UndoStack *ustack,
- const UndoStep *us_anchor,
- const UndoStep *us_item)
-{
- const int index_anchor = BLI_findindex(&ustack->steps, us_anchor);
- const int index_item = BLI_findindex(&ustack->steps, us_item);
- BLI_assert(index_anchor >= 0);
- BLI_assert(index_item >= 0);
-
- return (index_item == index_anchor) ? 0 : (index_item < index_anchor) ? -1 : 1;
-}
-
-# define ASSERT_VALID_UNDO_STEP(_ustack, _us_undo) \
- { \
- const UndoStep *_us_anchor = (_ustack)->step_active; \
- BLI_assert(_us_anchor == NULL || \
- (undosys_stack_order((_ustack), _us_anchor, (_us_undo)) <= 0)); \
- } \
- (void)0
-
-# define ASSERT_VALID_REDO_STEP(_ustack, _us_redo) \
- { \
- const UndoStep *_us_anchor = (_ustack)->step_active; \
- BLI_assert(_us_anchor == NULL || \
- (undosys_stack_order((_ustack), _us_anchor, (_us_redo)) >= 0)); \
- } \
- (void)0
-
#else
static void undosys_stack_validate(UndoStack *UNUSED(ustack), bool UNUSED(expect_non_empty))
{
}
-# define ASSERT_VALID_UNDO_STEP(_ustack, _us_undo)
-# define ASSERT_VALID_REDO_STEP(_ustack, _us_redo)
#endif
UndoStack *BKE_undosys_stack_create(void)
@@ -701,37 +669,91 @@ UndoStep *BKE_undosys_step_find_by_type(UndoStack *ustack, const UndoType *ut)
return NULL;
}
-bool BKE_undosys_step_undo_with_data_ex(UndoStack *ustack,
- bContext *C,
- UndoStep *us,
- bool use_skip)
+/**
+ * Return direction of the undo/redo from `us_reference` (or `ustack->step_active` if NULL), and
+ * `us_target`.
+ *
+ * \note If `us_reference` and `us_target` are the same, we consider this is an undo.
+ *
+ * \return -1 for undo, 1 for redo, 0 in case of error.
+ */
+int BKE_undosys_step_calc_direction(const UndoStack *ustack,
+ const UndoStep *us_target,
+ const UndoStep *us_reference)
+{
+ if (us_reference == NULL) {
+ us_reference = ustack->step_active;
+ }
+
+ BLI_assert(us_reference != NULL);
+
+ /* Note that we use heuristics to make this lookup as fast as possible in most common cases,
+ * assuming that:
+ * - Most cases are just undo or redo of one step from active one.
+ * - Otherwise, it is typically faster to check future steps since active one is usually close
+ * to the end of the list, rather than its start. */
+ /* NOTE: in case target step is the active one, we assume we are in an undo case... */
+ if (ELEM(us_target, us_reference, us_reference->prev)) {
+ return -1;
+ }
+ if (us_target == us_reference->next) {
+ return 1;
+ }
+
+ /* Search forward, and then backward. */
+ for (UndoStep *us_iter = us_reference->next; us_iter != NULL; us_iter = us_iter->next) {
+ if (us_iter == us_target) {
+ return 1;
+ }
+ }
+ for (UndoStep *us_iter = us_reference->prev; us_iter != NULL; us_iter = us_iter->prev) {
+ if (us_iter == us_target) {
+ return -1;
+ }
+ }
+
+ BLI_assert(!"Target undo step not found, this should not happen and may indicate an undo stack corruption");
+ return 0;
+}
+
+/**
+ * Undo/Redo until the given `us_target` step becomes the active (currently loaded) one.
+ *
+ * \note Unless `us_target` is a 'skipped' one and `use_skip` is true, `us_target` will become the
+ * active step.
+ *
+ * \note In case `use_skip` is true, the final target will always be **beyond** the given one (if
+ * the given one has to be skipped).
+ *
+ * \param us_reference If NULL, will be set to current active step in the undo stack. Otherwise, it
+ * is assumed to match the current state, and will be used as basis for the
+ * undo/redo process (i.e. all steps in-between `us_reference` and `us_target`
+ * will be processed).
+ */
+bool BKE_undosys_step_load_data_ex(UndoStack *ustack,
+ bContext *C,
+ UndoStep *us_target,
+ UndoStep *us_reference,
+ const bool use_skip)
{
UNDO_NESTED_ASSERT(false);
- if (us == NULL) {
- CLOG_ERROR(&LOG, "called with a NULL step");
+ if (us_target == NULL) {
+ CLOG_ERROR(&LOG, "called with a NULL target step");
return false;
}
undosys_stack_validate(ustack, true);
- /* We expect to get next-from-actual-target step here (i.e. active step in case we only undo
- * once)?
- * FIXME: this is very confusing now that we may have to undo several steps anyway, this function
- * should just get the target final step, not assume that it is getting the active one by default
- * (or the step after the target one when undoing more than one step). */
- UndoStep *us_target = us->prev;
- if (us_target == NULL) {
- CLOG_ERROR(&LOG, "could not find a valid target step");
+ if (us_reference == NULL) {
+ us_reference = ustack->step_active;
+ }
+ if (us_reference == NULL) {
+ CLOG_ERROR(&LOG, "could not find a valid initial active target step as reference");
return false;
}
- ASSERT_VALID_UNDO_STEP(ustack, us_target);
- /* This will be active once complete. */
- UndoStep *us_active = us_target;
- if (use_skip) {
- while (us_active && us_active->skip) {
- us_active = us_active->prev;
- }
- }
+ /* This considers we are in undo case if both `us_target` and `us_reference` are the same. */
+ const int undo_dir = BKE_undosys_step_calc_direction(ustack, us_target, us_reference);
+ BLI_assert(undo_dir != 0);
/* This will be the active step once the undo process is complete.
*
@@ -740,7 +762,7 @@ bool BKE_undosys_step_undo_with_data_ex(UndoStack *ustack,
UndoStep *us_target_active = us_target;
if (use_skip) {
while (us_target_active != NULL && us_target_active->skip) {
- us_target_active = us_target_active->prev;
+ us_target_active = (undo_dir == -1) ? us_target_active->prev : us_target_active->next;
}
}
if (us_target_active == NULL) {
@@ -748,42 +770,47 @@ bool BKE_undosys_step_undo_with_data_ex(UndoStack *ustack,
return false;
}
- CLOG_INFO(
- &LOG, 1, "addr=%p, name='%s', type='%s'", us_target, us_target->name, us_target->type->name);
+ CLOG_INFO(&LOG,
+ 1,
+ "addr=%p, name='%s', type='%s', undo_dir=%d",
+ us_target,
+ us_target->name,
+ us_target->type->name,
+ undo_dir);
- /* Undo steps until we reach original given target, if we do have a current active step.
+ /* 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? */
- if (ustack->step_active != NULL) {
- for (UndoStep *us_iter = ustack->step_active; us_iter != us_target; us_iter = us_it
@@ Diff output truncated at 10240 characters. @@
More information about the Bf-blender-cvs
mailing list