Fix T39196, Dynamic Topology Undo Applied to Wrong Mesh
authorAntony Riakiotakis <kalast@gmail.com>
Tue, 13 May 2014 17:59:54 +0000 (20:59 +0300)
committerAntony Riakiotakis <kalast@gmail.com>
Tue, 13 May 2014 19:05:23 +0000 (22:05 +0300)
Undoing nodes that do not belong to the current object will cause the
saved bmesh log entry to be reverted instead. This entry can belong to
another object though.
This is easy to fix by enforcing name matching (this was borrowed by
edit mode but can definitely be improved) between current object name
and undo node name and deleting older entries.

However there are complications. Deleting dyntopo entries in this way
can leave a brush stroke as first dyntopo log entry. This can present
issues if we attempt to delete that entry since it's deleted mesh
elements may now have had their ids (which would still be valid at the
time) cleaned up. This can result in crashing if we attempt to resculpt
on the mesh. To fix this I have disabled releasing the deleted entries.

This entanglement between bm_log and undo is quite volatile but I hope
the system works better now.

Also minor cleanup, fix unneeded check warning

source/blender/blenlib/intern/boxpack2d.c
source/blender/bmesh/intern/bmesh_log.c
source/blender/bmesh/intern/bmesh_log.h
source/blender/editors/include/ED_sculpt.h
source/blender/editors/sculpt_paint/paint_image.c
source/blender/editors/sculpt_paint/paint_image_proj.c
source/blender/editors/sculpt_paint/paint_intern.h
source/blender/editors/sculpt_paint/paint_undo.c
source/blender/editors/sculpt_paint/sculpt_undo.c
source/blender/editors/space_image/image_ops.c
source/blender/editors/util/undo.c

index 5d01706ebb19b907007bfb8e31f2a278af11ed63..91495ce3c9cfa370b25437fa181df9e0d194f8e3 100644 (file)
@@ -91,7 +91,7 @@ typedef struct BoxVert {
 
 BLI_INLINE int quad_flag(unsigned int q)
 {
-       BLI_assert(q < 4 && q >= 0);
+       BLI_assert(q < 4);
        return (1 << q);
 }
 
index 7d3bc0e16b36b1a2b0aa3b4e09413c7834301afd..0bb1a892ffeeaab4246e37d1ce176ea5d89e0239 100644 (file)
@@ -489,6 +489,29 @@ BMLog *BM_log_create(BMesh *bm)
        return log;
 }
 
+void BM_log_cleanup_entry(BMLogEntry *entry)
+{
+       BMLog *log = entry->log;
+
+       if (log) {
+               /* Take all used IDs */
+               bm_log_id_ghash_retake(log->unused_ids, entry->deleted_verts);
+               bm_log_id_ghash_retake(log->unused_ids, entry->deleted_faces);
+               bm_log_id_ghash_retake(log->unused_ids, entry->added_verts);
+               bm_log_id_ghash_retake(log->unused_ids, entry->added_faces);
+               bm_log_id_ghash_retake(log->unused_ids, entry->modified_verts);
+               bm_log_id_ghash_retake(log->unused_ids, entry->modified_faces);
+
+               /* delete entries to avoid releasing ids in node cleanup */
+               BLI_ghash_clear(entry->deleted_verts, NULL, NULL);
+               BLI_ghash_clear(entry->deleted_faces, NULL, NULL);
+               BLI_ghash_clear(entry->added_verts, NULL, NULL);
+               BLI_ghash_clear(entry->added_faces, NULL, NULL);
+               BLI_ghash_clear(entry->modified_verts, NULL, NULL);
+       }
+}
+
+
 /* Allocate and initialize a new BMLog using existing BMLogEntries
  *
  * The 'entry' should be the last entry in the BMLog. Its prev pointer
@@ -684,8 +707,27 @@ void BM_log_entry_drop(BMLogEntry *entry)
                 * entry. Since the entry is at the beginning of the undo
                 * stack, and it's being deleted, those elements can never be
                 * restored. Their IDs can go back into the pool. */
-               bm_log_id_ghash_release(log, entry->deleted_faces);
-               bm_log_id_ghash_release(log, entry->deleted_verts);
+
+               /* This would never happen usually since first entry of log is
+                * usually dyntopo enable, which, when reverted will free the log
+                * completely. However, it is possible have a stroke instead of
+                * dyntopo enable as first entry if nodes have been cleaned up
+                * after sculpting on a different object than A, B.
+                *
+                * The steps are:
+                * A dyntopo enable - sculpt
+                * B dyntopo enable - sculpt - undo (A objects operators get cleaned up)
+                * A sculpt (now A's log has a sculpt operator as first entry)
+                *
+                * Causing a cleanup at this point will call the code below, however
+                * this will invalidate the state of the log since the deleted vertices
+                * have been reclaimed already on step 2 (see BM_log_cleanup_entry)
+                *
+                * Also, design wise, a first entry should not have any deleted vertices since it
+                * should not have anything to delete them -from-
+                */
+               //bm_log_id_ghash_release(log, entry->deleted_faces);
+               //bm_log_id_ghash_release(log, entry->deleted_verts);
        }
        else if (!entry->next) {
                /* Release IDs of elements that are added by this entry. Since
index 7a26506439f61e160ec422f417fcd5c0857dc76d..2147b5c64b48e1086a93bf8901718f8fbee0309c 100644 (file)
@@ -51,6 +51,9 @@ void BM_log_mesh_elems_reorder(BMesh *bm, BMLog *log);
 /* Start a new log entry and update the log entry list */
 BMLogEntry *BM_log_entry_add(BMLog *log);
 
+/* Mark all used ids as unused for this node */
+void BM_log_cleanup_entry(BMLogEntry *entry);
+
 /* Remove an entry from the log */
 void BM_log_entry_drop(BMLogEntry *entry);
 
index 8fcb228803b1b15a78197332eca5a6711a4f8db4..e89b0c53a10f7ce67c5acdba35e6473c67e2d74a 100644 (file)
@@ -61,7 +61,7 @@ typedef void (*UndoFreeCb)(struct ListBase *lb);
 
 int ED_undo_paint_step(struct bContext *C, int type, int step, const char *name);
 void ED_undo_paint_step_num(struct bContext *C, int type, int num);
-const char *ED_undo_paint_get_name(int type, int nr, int *active);
+const char *ED_undo_paint_get_name(struct bContext *C, int type, int nr, int *active);
 void ED_undo_paint_free(void);
 int ED_undo_paint_valid(int type, const char *name);
 bool ED_undo_paint_empty(int type);
index 5e2bdcb3c93f0e110b498171b62e11d06c42316b..575b7fc4062b6e0adc813ddb04d87f484b90ae9b 100644 (file)
@@ -491,7 +491,7 @@ static PaintOperation *texture_paint_init(bContext *C, wmOperator *op, float mou
 
        settings->imapaint.flag |= IMAGEPAINT_DRAWING;
        ED_undo_paint_push_begin(UNDO_PAINT_IMAGE, op->type->name,
-                             ED_image_undo_restore, ED_image_undo_free);
+                                                 ED_image_undo_restore, ED_image_undo_free);
 
        {
                UnifiedPaintSettings *ups = &settings->unified_paint_settings;
index 492857bd0bb6b55def4add20c28210e05a617535..f69c7e0ed41fd3568170fd204404a5094ad07212 100644 (file)
@@ -4337,7 +4337,7 @@ static int texture_paint_camera_project_exec(bContext *C, wmOperator *op)
        scene->toolsettings->imapaint.flag |= IMAGEPAINT_DRAWING;
 
        ED_undo_paint_push_begin(UNDO_PAINT_IMAGE, op->type->name,
-                             ED_image_undo_restore, ED_image_undo_free);
+                                                 ED_image_undo_restore, ED_image_undo_free);
 
        /* allocate and initialize spatial data structures */
        project_paint_begin(&ps);
index d8f7a3d8e054fc0f4248bc13c9a58e75d3caa80a..b420c8d209c2186b991f89a8edc85bc5d397bbac 100644 (file)
@@ -229,6 +229,7 @@ typedef enum BrushStrokeMode {
 /* paint_undo.c */
 struct ListBase *undo_paint_push_get_list(int type);
 void undo_paint_push_count_alloc(int type, int size);
+bool sculpt_undo_cleanup(struct bContext *C, struct ListBase *lb);
 
 /* paint_hide.c */
 
index f3946e30b832754e33d1ba3c28b31eb82cea6f6a..5bd6526c27046895e7762334187273c58a7febd4 100644 (file)
@@ -53,14 +53,17 @@ typedef struct UndoElem {
        UndoFreeCb free;
 } UndoElem;
 
+typedef bool (*UndoCleanupCb)(struct bContext *C, ListBase *lb);
+
 typedef struct UndoStack {
        int type;
        ListBase elems;
        UndoElem *current;
+       UndoCleanupCb cleanup;
 } UndoStack;
 
-static UndoStack ImageUndoStack = {UNDO_PAINT_IMAGE, {NULL, NULL}, NULL};
-static UndoStack MeshUndoStack = {UNDO_PAINT_MESH, {NULL, NULL}, NULL};
+static UndoStack ImageUndoStack = {UNDO_PAINT_IMAGE, {NULL, NULL}, NULL, NULL};
+static UndoStack MeshUndoStack = {UNDO_PAINT_MESH, {NULL, NULL}, NULL, sculpt_undo_cleanup};
 
 /* Generic */
 
@@ -171,10 +174,39 @@ static void undo_stack_push_end(UndoStack *stack)
        }
 }
 
+static void undo_stack_cleanup(UndoStack *stack, bContext *C)
+{
+       UndoElem *uel = stack->elems.first;
+       bool stack_reset = false;
+
+       if (stack->cleanup) {
+               while (uel) {
+                       if (stack->cleanup(C, &uel->elems)) {
+                               UndoElem *uel_tmp = uel->next;
+                               if (stack->current == uel) {
+                                       stack->current = NULL;
+                                       stack_reset = true;
+                               }
+                               undo_elem_free(stack, uel);
+                               BLI_freelinkN(&stack->elems, uel);
+                               uel = uel_tmp;
+                       }
+                       else
+                               uel = uel->next;
+               }
+               if (stack_reset) {
+                       stack->current = stack->elems.last;
+               }
+       }
+}
+
 static int undo_stack_step(bContext *C, UndoStack *stack, int step, const char *name)
 {
        UndoElem *undo;
 
+       /* first cleanup any old undo steps that may belong to invalid data */
+       undo_stack_cleanup(stack, C);
+
        if (step == 1) {
                if (stack->current == NULL) {
                        /* pass */
@@ -315,12 +347,17 @@ static char *undo_stack_get_name(UndoStack *stack, int nr, int *active)
        return NULL;
 }
 
-const char *ED_undo_paint_get_name(int type, int nr, int *active)
+const char *ED_undo_paint_get_name(bContext *C, int type, int nr, int *active)
 {
-       if (type == UNDO_PAINT_IMAGE)
+
+       if (type == UNDO_PAINT_IMAGE) {
+               undo_stack_cleanup(&ImageUndoStack, C);
                return undo_stack_get_name(&ImageUndoStack, nr, active);
-       else if (type == UNDO_PAINT_MESH)
+       }
+       else if (type == UNDO_PAINT_MESH) {
+               undo_stack_cleanup(&MeshUndoStack, C);
                return undo_stack_get_name(&MeshUndoStack, nr, active);
+       }
        return NULL;
 }
 
index 57e852db796519bce2cd933b39b2a0d394f84335..9beb11ec2f4c4022873a094882b750d3f643b446 100644 (file)
@@ -526,9 +526,11 @@ static void sculpt_undo_free(ListBase *lb)
                }
                if (unode->mask)
                        MEM_freeN(unode->mask);
+
                if (unode->bm_entry) {
                        BM_log_entry_drop(unode->bm_entry);
                }
+
                if (unode->bm_enter_totvert)
                        CustomData_free(&unode->bm_enter_vdata, unode->bm_enter_totvert);
                if (unode->bm_enter_totedge)
@@ -540,6 +542,24 @@ static void sculpt_undo_free(ListBase *lb)
        }
 }
 
+bool sculpt_undo_cleanup(bContext *C, ListBase *lb) {
+       Object *ob = CTX_data_active_object(C);
+       SculptUndoNode *unode;
+
+       unode = lb->first;
+
+       if (strcmp(unode->idname, ob->id.name) != 0) {
+               for (unode = lb->first; unode; unode = unode->next) {
+                       if (unode->bm_entry)
+                               BM_log_cleanup_entry(unode->bm_entry);
+               }
+
+               return true;
+       }
+
+       return false;
+}
+
 SculptUndoNode *sculpt_undo_get_node(PBVHNode *node)
 {
        ListBase *lb = undo_paint_push_get_list(UNDO_PAINT_MESH);
@@ -859,7 +879,7 @@ SculptUndoNode *sculpt_undo_push_node(Object *ob, PBVHNode *node,
 void sculpt_undo_push_begin(const char *name)
 {
        ED_undo_paint_push_begin(UNDO_PAINT_MESH, name,
-                             sculpt_undo_restore, sculpt_undo_free);
+                                                 sculpt_undo_restore, sculpt_undo_free);
 }
 
 void sculpt_undo_push_end(void)
index 57be15f23fb426d45793e03d36f7237c712da6b2..6f42a0b8928b332b6361c254a0c260ff5ecc25eb 100644 (file)
@@ -2034,7 +2034,7 @@ static int image_invert_exec(bContext *C, wmOperator *op)
 
        if (support_undo) {
                ED_undo_paint_push_begin(UNDO_PAINT_IMAGE, op->type->name,
-                                        ED_image_undo_restore, ED_image_undo_free);
+                                                                ED_image_undo_restore, ED_image_undo_free);
                /* not strictly needed, because we only imapaint_dirty_region to invalidate all tiles
                 * but better do this right in case someone copies this for a tool that uses partial redraw better */
                ED_imapaint_clear_partial_redraw();
index a4f2c36f250a3ac4de2e5b1c03a8bd5db90ab3a0..4d14323a20b517ac5126963eca2688b6677d13df 100644 (file)
@@ -489,7 +489,7 @@ static EnumPropertyItem *rna_undo_itemf(bContext *C, int undosys, int *totitem)
                        name = undo_editmode_get_name(C, i, &active);
                }
                else if (undosys == UNDOSYSTEM_IMAPAINT) {
-                       name = ED_undo_paint_get_name(UNDO_PAINT_IMAGE, i, &active);
+                       name = ED_undo_paint_get_name(C, UNDO_PAINT_IMAGE, i, &active);
                }
                else {
                        name = BKE_undo_get_name(i, &active);