From bc090387ace9cf041455fa01e68d61551c47e18f Mon Sep 17 00:00:00 2001 From: Bastien Montagne Date: Thu, 12 Nov 2020 10:41:33 +0100 Subject: Fix T82388: Sculpt mode: Unexpected undo behavior. Issue exposed by rB4c7b1766a7f1. Main idea is that non-memfile first undo step should check into previous memfile and tag the ID it is editing as `future_changed`. That way, when we go back and undo to the memfile, said IDs are properly detected as changed and re-read from the memfile. Otherwise, undo system sees them as unchanged, and just re-use the current data instead. Note that currently only Sculpt mode seems affected (probably because it is storing the mode switch itself as a Sculpt undo step instead of a memfile one), but similar action might be needed in some other cases too. Maniphest Tasks: T82388 Differential Revision: https://developer.blender.org/D9510 --- source/blender/editors/include/ED_undo.h | 1 + source/blender/editors/sculpt_paint/paint_hide.c | 4 +-- source/blender/editors/sculpt_paint/paint_mask.c | 4 +-- source/blender/editors/sculpt_paint/sculpt.c | 10 +++---- source/blender/editors/sculpt_paint/sculpt_cloth.c | 2 +- .../blender/editors/sculpt_paint/sculpt_detail.c | 2 +- .../blender/editors/sculpt_paint/sculpt_dyntopo.c | 4 +-- .../blender/editors/sculpt_paint/sculpt_face_set.c | 8 ++--- .../editors/sculpt_paint/sculpt_filter_color.c | 2 +- .../editors/sculpt_paint/sculpt_filter_mask.c | 4 +-- .../editors/sculpt_paint/sculpt_filter_mesh.c | 2 +- .../blender/editors/sculpt_paint/sculpt_intern.h | 2 +- .../editors/sculpt_paint/sculpt_mask_expand.c | 2 +- .../editors/sculpt_paint/sculpt_transform.c | 2 +- source/blender/editors/sculpt_paint/sculpt_undo.c | 13 ++++++-- source/blender/editors/undo/memfile_undo.c | 35 ++++++++++++++++++++++ 16 files changed, 70 insertions(+), 27 deletions(-) diff --git a/source/blender/editors/include/ED_undo.h b/source/blender/editors/include/ED_undo.h index 989854872f3..7b643e7c764 100644 --- a/source/blender/editors/include/ED_undo.h +++ b/source/blender/editors/include/ED_undo.h @@ -87,6 +87,7 @@ void ED_undosys_type_free(void); /* memfile_undo.c */ struct MemFile *ED_undosys_stack_memfile_get_active(struct UndoStack *ustack); +void ED_undosys_stack_memfile_id_changed_tag(struct UndoStack *ustack, struct ID *id); #ifdef __cplusplus } diff --git a/source/blender/editors/sculpt_paint/paint_hide.c b/source/blender/editors/sculpt_paint/paint_hide.c index e5d682c27d9..da627c6b7db 100644 --- a/source/blender/editors/sculpt_paint/paint_hide.c +++ b/source/blender/editors/sculpt_paint/paint_hide.c @@ -366,10 +366,10 @@ static int hide_show_exec(bContext *C, wmOperator *op) /* Start undo. */ switch (action) { case PARTIALVIS_HIDE: - SCULPT_undo_push_begin("Hide area"); + SCULPT_undo_push_begin(ob, "Hide area"); break; case PARTIALVIS_SHOW: - SCULPT_undo_push_begin("Show area"); + SCULPT_undo_push_begin(ob, "Show area"); break; } diff --git a/source/blender/editors/sculpt_paint/paint_mask.c b/source/blender/editors/sculpt_paint/paint_mask.c index ad7b29ae33a..4dd7ecb09ca 100644 --- a/source/blender/editors/sculpt_paint/paint_mask.c +++ b/source/blender/editors/sculpt_paint/paint_mask.c @@ -169,7 +169,7 @@ static int mask_flood_fill_exec(bContext *C, wmOperator *op) BKE_pbvh_search_gather(pbvh, NULL, NULL, &nodes, &totnode); - SCULPT_undo_push_begin("Mask flood fill"); + SCULPT_undo_push_begin(ob, "Mask flood fill"); MaskTaskData data = { .ob = ob, @@ -707,7 +707,7 @@ static bool sculpt_gesture_is_vertex_effected(SculptGestureContext *sgcontext, P static void sculpt_gesture_apply(bContext *C, SculptGestureContext *sgcontext) { SculptGestureOperation *operation = sgcontext->operation; - SCULPT_undo_push_begin("Sculpt Gesture Apply"); + SCULPT_undo_push_begin(CTX_data_active_object(C), "Sculpt Gesture Apply"); operation->sculpt_gesture_begin(C, sgcontext); diff --git a/source/blender/editors/sculpt_paint/sculpt.c b/source/blender/editors/sculpt_paint/sculpt.c index 087cee52585..c33dfecd52c 100644 --- a/source/blender/editors/sculpt_paint/sculpt.c +++ b/source/blender/editors/sculpt_paint/sculpt.c @@ -7698,7 +7698,7 @@ static bool sculpt_stroke_test_start(bContext *C, struct wmOperator *op, const f sculpt_update_cache_invariants(C, sd, ss, op, mouse); - SCULPT_undo_push_begin(sculpt_tool_name(sd)); + SCULPT_undo_push_begin(ob, sculpt_tool_name(sd)); return true; } @@ -8057,7 +8057,7 @@ static int sculpt_symmetrize_exec(bContext *C, wmOperator *op) * as deleted, then after symmetrize operation all BMesh elements * are logged as added (as opposed to attempting to store just the * parts that symmetrize modifies). */ - SCULPT_undo_push_begin("Dynamic topology symmetrize"); + SCULPT_undo_push_begin(ob, "Dynamic topology symmetrize"); SCULPT_undo_push_node(ob, NULL, SCULPT_UNDO_DYNTOPO_SYMMETRIZE); BM_log_before_all_removed(ss->bm, ss->bm_log); @@ -8297,7 +8297,7 @@ void ED_object_sculptmode_enter_ex(Main *bmain, bool has_undo = wm->undo_stack != NULL; /* Undo push is needed to prevent memory leak. */ if (has_undo) { - SCULPT_undo_push_begin("Dynamic topology enable"); + SCULPT_undo_push_begin(ob, "Dynamic topology enable"); } SCULPT_dynamic_topology_enable_ex(bmain, depsgraph, scene, ob); if (has_undo) { @@ -8413,7 +8413,7 @@ static int sculpt_mode_toggle_exec(bContext *C, wmOperator *op) * while it works it causes lag when undoing the first undo step, see T71564. */ wmWindowManager *wm = CTX_wm_manager(C); if (wm->op_undo_depth <= 1) { - SCULPT_undo_push_begin(op->type->name); + SCULPT_undo_push_begin(ob, op->type->name); } } } @@ -9206,7 +9206,7 @@ static int sculpt_mask_by_color_invoke(bContext *C, wmOperator *op, const wmEven mouse[1] = event->mval[1]; SCULPT_cursor_geometry_info_update(C, &sgi, mouse, false); - SCULPT_undo_push_begin("Mask by color"); + SCULPT_undo_push_begin(ob, "Mask by color"); const int active_vertex = SCULPT_active_vertex_get(ss); const float threshold = RNA_float_get(op->ptr, "threshold"); diff --git a/source/blender/editors/sculpt_paint/sculpt_cloth.c b/source/blender/editors/sculpt_paint/sculpt_cloth.c index 20b164fa80c..f8165890cc4 100644 --- a/source/blender/editors/sculpt_paint/sculpt_cloth.c +++ b/source/blender/editors/sculpt_paint/sculpt_cloth.c @@ -1548,7 +1548,7 @@ static int sculpt_cloth_filter_invoke(bContext *C, wmOperator *op, const wmEvent /* Needs mask data to be available as it is used when solving the constraints. */ BKE_sculpt_update_object_for_edit(depsgraph, ob, true, true, false); - SCULPT_undo_push_begin("Cloth filter"); + SCULPT_undo_push_begin(ob, "Cloth filter"); SCULPT_filter_cache_init(C, ob, sd, SCULPT_UNDO_COORDS); ss->filter_cache->automasking = SCULPT_automasking_cache_init(sd, NULL, ob); diff --git a/source/blender/editors/sculpt_paint/sculpt_detail.c b/source/blender/editors/sculpt_paint/sculpt_detail.c index da51d3184b5..cdfcdbc4660 100644 --- a/source/blender/editors/sculpt_paint/sculpt_detail.c +++ b/source/blender/editors/sculpt_paint/sculpt_detail.c @@ -122,7 +122,7 @@ static int sculpt_detail_flood_fill_exec(bContext *C, wmOperator *UNUSED(op)) float object_space_constant_detail = 1.0f / (sd->constant_detail * mat4_to_scale(ob->obmat)); BKE_pbvh_bmesh_detail_size_set(ss->pbvh, object_space_constant_detail); - SCULPT_undo_push_begin("Dynamic topology flood fill"); + SCULPT_undo_push_begin(ob, "Dynamic topology flood fill"); SCULPT_undo_push_node(ob, NULL, SCULPT_UNDO_COORDS); while (BKE_pbvh_bmesh_update_topology( diff --git a/source/blender/editors/sculpt_paint/sculpt_dyntopo.c b/source/blender/editors/sculpt_paint/sculpt_dyntopo.c index 22bcbcc3bf1..87e0ea7f6a9 100644 --- a/source/blender/editors/sculpt_paint/sculpt_dyntopo.c +++ b/source/blender/editors/sculpt_paint/sculpt_dyntopo.c @@ -291,7 +291,7 @@ void sculpt_dynamic_topology_disable_with_undo(Main *bmain, /* May be false in background mode. */ const bool use_undo = G.background ? (ED_undo_stack_get() != NULL) : true; if (use_undo) { - SCULPT_undo_push_begin("Dynamic topology disable"); + SCULPT_undo_push_begin(ob, "Dynamic topology disable"); SCULPT_undo_push_node(ob, NULL, SCULPT_UNDO_DYNTOPO_END); } SCULPT_dynamic_topology_disable_ex(bmain, depsgraph, scene, ob, NULL); @@ -311,7 +311,7 @@ static void sculpt_dynamic_topology_enable_with_undo(Main *bmain, /* May be false in background mode. */ const bool use_undo = G.background ? (ED_undo_stack_get() != NULL) : true; if (use_undo) { - SCULPT_undo_push_begin("Dynamic topology enable"); + SCULPT_undo_push_begin(ob, "Dynamic topology enable"); } SCULPT_dynamic_topology_enable_ex(bmain, depsgraph, scene, ob); if (use_undo) { diff --git a/source/blender/editors/sculpt_paint/sculpt_face_set.c b/source/blender/editors/sculpt_paint/sculpt_face_set.c index 6eb51c77aef..f803bccde43 100644 --- a/source/blender/editors/sculpt_paint/sculpt_face_set.c +++ b/source/blender/editors/sculpt_paint/sculpt_face_set.c @@ -328,7 +328,7 @@ static int sculpt_face_set_create_exec(bContext *C, wmOperator *op) return OPERATOR_CANCELLED; } - SCULPT_undo_push_begin("face set change"); + SCULPT_undo_push_begin(ob, "face set change"); SCULPT_undo_push_node(ob, nodes[0], SCULPT_UNDO_FACE_SETS); const int next_face_set = SCULPT_face_set_next_available_get(ss); @@ -684,7 +684,7 @@ static int sculpt_face_set_init_exec(bContext *C, wmOperator *op) return OPERATOR_CANCELLED; } - SCULPT_undo_push_begin("face set change"); + SCULPT_undo_push_begin(ob, "face set change"); SCULPT_undo_push_node(ob, nodes[0], SCULPT_UNDO_FACE_SETS); const float threshold = RNA_float_get(op->ptr, "threshold"); @@ -829,7 +829,7 @@ static int sculpt_face_sets_change_visibility_exec(bContext *C, wmOperator *op) const int mode = RNA_enum_get(op->ptr, "mode"); const int active_face_set = SCULPT_active_face_set_get(ss); - SCULPT_undo_push_begin("Hide area"); + SCULPT_undo_push_begin(ob, "Hide area"); PBVH *pbvh = ob->sculpt->pbvh; PBVHNode **nodes; @@ -1276,7 +1276,7 @@ static void sculpt_face_set_edit_modify_face_sets(Object *ob, if (!nodes) { return; } - SCULPT_undo_push_begin("face set edit"); + SCULPT_undo_push_begin(ob, "face set edit"); SCULPT_undo_push_node(ob, nodes[0], SCULPT_UNDO_FACE_SETS); sculpt_face_set_apply_edit(ob, abs(active_face_set), mode, modify_hidden); SCULPT_undo_push_end(); diff --git a/source/blender/editors/sculpt_paint/sculpt_filter_color.c b/source/blender/editors/sculpt_paint/sculpt_filter_color.c index 07986bbb032..f3c07a86201 100644 --- a/source/blender/editors/sculpt_paint/sculpt_filter_color.c +++ b/source/blender/editors/sculpt_paint/sculpt_filter_color.c @@ -290,7 +290,7 @@ static int sculpt_color_filter_invoke(bContext *C, wmOperator *op, const wmEvent return OPERATOR_CANCELLED; } - SCULPT_undo_push_begin("color filter"); + SCULPT_undo_push_begin(ob, "color filter"); BKE_sculpt_color_layer_create_if_needed(ob); diff --git a/source/blender/editors/sculpt_paint/sculpt_filter_mask.c b/source/blender/editors/sculpt_paint/sculpt_filter_mask.c index 8fa20aaae50..ddad6bef7fd 100644 --- a/source/blender/editors/sculpt_paint/sculpt_filter_mask.c +++ b/source/blender/editors/sculpt_paint/sculpt_filter_mask.c @@ -212,7 +212,7 @@ static int sculpt_mask_filter_exec(bContext *C, wmOperator *op) int num_verts = SCULPT_vertex_count_get(ss); BKE_pbvh_search_gather(pbvh, NULL, NULL, &nodes, &totnode); - SCULPT_undo_push_begin("Mask filter"); + SCULPT_undo_push_begin(ob, "Mask filter"); for (int i = 0; i < totnode; i++) { SCULPT_undo_push_node(ob, nodes[i], SCULPT_UNDO_MASK); @@ -440,7 +440,7 @@ static int sculpt_dirty_mask_exec(bContext *C, wmOperator *op) } BKE_pbvh_search_gather(pbvh, NULL, NULL, &nodes, &totnode); - SCULPT_undo_push_begin("Dirty Mask"); + SCULPT_undo_push_begin(ob, "Dirty Mask"); for (int i = 0; i < totnode; i++) { SCULPT_undo_push_node(ob, nodes[i], SCULPT_UNDO_MASK); diff --git a/source/blender/editors/sculpt_paint/sculpt_filter_mesh.c b/source/blender/editors/sculpt_paint/sculpt_filter_mesh.c index 349e492a496..11af63c6e47 100644 --- a/source/blender/editors/sculpt_paint/sculpt_filter_mesh.c +++ b/source/blender/editors/sculpt_paint/sculpt_filter_mesh.c @@ -703,7 +703,7 @@ static int sculpt_mesh_filter_invoke(bContext *C, wmOperator *op, const wmEvent SCULPT_boundary_info_ensure(ob); } - SCULPT_undo_push_begin("Mesh Filter"); + SCULPT_undo_push_begin(ob, "Mesh Filter"); SCULPT_filter_cache_init(C, ob, sd, SCULPT_UNDO_COORDS); diff --git a/source/blender/editors/sculpt_paint/sculpt_intern.h b/source/blender/editors/sculpt_paint/sculpt_intern.h index 1a596909d8d..c5db078617f 100644 --- a/source/blender/editors/sculpt_paint/sculpt_intern.h +++ b/source/blender/editors/sculpt_paint/sculpt_intern.h @@ -1085,7 +1085,7 @@ void SCULPT_cache_free(StrokeCache *cache); SculptUndoNode *SCULPT_undo_push_node(Object *ob, PBVHNode *node, SculptUndoType type); SculptUndoNode *SCULPT_undo_get_node(PBVHNode *node); SculptUndoNode *SCULPT_undo_get_first_node(void); -void SCULPT_undo_push_begin(const char *name); +void SCULPT_undo_push_begin(struct Object *ob, const char *name); void SCULPT_undo_push_end(void); void SCULPT_undo_push_end_ex(const bool use_nested_undo); diff --git a/source/blender/editors/sculpt_paint/sculpt_mask_expand.c b/source/blender/editors/sculpt_paint/sculpt_mask_expand.c index 5a921383ac3..39432dbbca4 100644 --- a/source/blender/editors/sculpt_paint/sculpt_mask_expand.c +++ b/source/blender/editors/sculpt_paint/sculpt_mask_expand.c @@ -378,7 +378,7 @@ static int sculpt_mask_expand_invoke(bContext *C, wmOperator *op, const wmEvent BKE_pbvh_search_gather(pbvh, NULL, NULL, &ss->filter_cache->nodes, &ss->filter_cache->totnode); - SCULPT_undo_push_begin("Mask Expand"); + SCULPT_undo_push_begin(ob, "Mask Expand"); if (create_face_set) { SCULPT_undo_push_node(ob, ss->filter_cache->nodes[0], SCULPT_UNDO_FACE_SETS); diff --git a/source/blender/editors/sculpt_paint/sculpt_transform.c b/source/blender/editors/sculpt_paint/sculpt_transform.c index 479f70b5ff1..c1281c98deb 100644 --- a/source/blender/editors/sculpt_paint/sculpt_transform.c +++ b/source/blender/editors/sculpt_paint/sculpt_transform.c @@ -70,7 +70,7 @@ void ED_sculpt_init_transform(struct bContext *C) copy_v3_v3(ss->init_pivot_pos, ss->pivot_pos); copy_v4_v4(ss->init_pivot_rot, ss->pivot_rot); - SCULPT_undo_push_begin("Transform"); + SCULPT_undo_push_begin(ob, "Transform"); BKE_sculpt_update_object_for_edit(depsgraph, ob, false, false, false); ss->pivot_rot[3] = 1.0f; diff --git a/source/blender/editors/sculpt_paint/sculpt_undo.c b/source/blender/editors/sculpt_paint/sculpt_undo.c index f4f30c903aa..af2aad14008 100644 --- a/source/blender/editors/sculpt_paint/sculpt_undo.c +++ b/source/blender/editors/sculpt_paint/sculpt_undo.c @@ -1334,10 +1334,17 @@ SculptUndoNode *SCULPT_undo_push_node(Object *ob, PBVHNode *node, SculptUndoType return unode; } -void SCULPT_undo_push_begin(const char *name) +void SCULPT_undo_push_begin(Object *ob, const char *name) { UndoStack *ustack = ED_undo_stack_get(); + if (ob != NULL) { + /* If possible, we need to tag the object and its geometry data as 'changed in the future' in + * the previous undo step if it's a memfile one. */ + ED_undosys_stack_memfile_id_changed_tag(ustack, &ob->id); + ED_undosys_stack_memfile_id_changed_tag(ustack, ob->data); + } + /* Special case, we never read from this. */ bContext *C = NULL; @@ -1525,7 +1532,7 @@ static void sculpt_undosys_step_free(UndoStep *us_p) void ED_sculpt_undo_geometry_begin(struct Object *ob, const char *name) { - SCULPT_undo_push_begin(name); + SCULPT_undo_push_begin(ob, name); SCULPT_undo_push_node(ob, NULL, SCULPT_UNDO_GEOMETRY); } @@ -1638,7 +1645,7 @@ void ED_sculpt_undo_push_multires_mesh_begin(bContext *C, const char *str) Object *object = CTX_data_active_object(C); - SCULPT_undo_push_begin(str); + SCULPT_undo_push_begin(object, str); SculptUndoNode *geometry_unode = SCULPT_undo_push_node(object, NULL, SCULPT_UNDO_GEOMETRY); geometry_unode->geometry_clear_pbvh = false; diff --git a/source/blender/editors/undo/memfile_undo.c b/source/blender/editors/undo/memfile_undo.c index 2df26abe8b3..ace82f82a78 100644 --- a/source/blender/editors/undo/memfile_undo.c +++ b/source/blender/editors/undo/memfile_undo.c @@ -24,7 +24,9 @@ #include "BLI_utildefines.h" #include "BLI_ghash.h" +#include "BLI_listbase.h" +#include "DNA_ID.h" #include "DNA_node_types.h" #include "DNA_object_enums.h" #include "DNA_object_types.h" @@ -52,6 +54,8 @@ #include "undo_intern.h" +#include + /* -------------------------------------------------------------------- */ /** \name Implements ED Undo System * \{ */ @@ -307,4 +311,35 @@ struct MemFile *ED_undosys_stack_memfile_get_active(UndoStack *ustack) return NULL; } +/** + * If the last undo step is a memfile one, find the first memchunk matching given ID (using its + * seesion uuid), and tag it as 'changed in the future'. + * + * Since non-memfile undos cannot automatically set this flag in the previous step as done with + * memfile ones, this has to be called manually by relevant undo code. + * + * \note Only current known case for this is undoing a switch from Pbject to Sculpt mode (see + * T82388). + * + * \note Calling this ID by ID is not optimal, as it will loop over all memchunks until it find + * expected one. If this becomes an issue we'll have to add a mapping from session uuid to first + * memchunk in #MemFile itself (currently we only do that in #MemFileWriteData when writing a new + * step). + */ +void ED_undosys_stack_memfile_id_changed_tag(UndoStack *ustack, ID *id) +{ + UndoStep *us = ustack->step_active; + if (id == NULL || us == NULL || us->type != BKE_UNDOSYS_TYPE_MEMFILE) { + return; + } + + MemFile *memfile = &((MemFileUndoStep *)us)->data->memfile; + LISTBASE_FOREACH (MemFileChunk *, mem_chunk, &memfile->chunks) { + if (mem_chunk->id_session_uuid == id->session_uuid) { + mem_chunk->is_identical_future = false; + break; + } + } +} + /** \} */ -- cgit v1.2.3