Setting 'Undo' steps to '1' causes weirdness. #42531

New Issue

Joel Godin · 2014-11-07T03:44:19+01:00

Joel Godin commented

2014-11-07 03:44:19 +01:00

System Information
OSX 10.10 AMD Radeon HD 6770M 512 MB

Blender Version
Broken: 2.72.2 fb820c0

Short description of error
Setting undo to 1 in preferences then subdividing object massively increases faces, even if you reduce the number of subdivisions.

Exact steps for others to reproduce the error
1)Open preferences > editing
2)Set 'undo' steps to 1
3) in 3d view, select default cube, tab into edit mode.
4) Hit 'W' > select 'subdivide'
5) In the panel on the left enter number of subdivisions to be 4 (or any number, I used 4) hit enter
6) Go back into that field and enter 1
7) you now have 2400 faces ( it increased)
Also, if you just re-enter 1 again, subdivisions increase again.

I was hesitant to submit this bug because I'm sure most people have more than 1 step of undo,
but then I thought it's possible the faulty underlying code could cause problems somewhere else too.

**System Information** OSX 10.10 AMD Radeon HD 6770M 512 MB **Blender Version** Broken: 2.72.2 fb820c0 **Short description of error** Setting undo to 1 in preferences then subdividing object massively increases faces, even if you reduce the number of subdivisions. **Exact steps for others to reproduce the error** 1)Open preferences > editing 2)Set 'undo' steps to 1 3) in 3d view, select default cube, tab into edit mode. 4) Hit 'W' > select 'subdivide' 5) In the panel on the left enter number of subdivisions to be 4 (or any number, I used 4) hit enter 6) Go back into that field and enter 1 7) you now have 2400 faces ( it increased) Also, if you just re-enter 1 again, subdivisions increase again. I was hesitant to submit this bug because I'm sure most people have more than 1 step of undo, but then I thought it's possible the faulty underlying code could cause problems somewhere else too.

Joel Godin commented

2014-11-07 03:44:19 +01:00

Changed status to: 'Open'

Joel Godin commented

2014-11-07 03:44:19 +01:00

Added subscriber: @FloridaJo

Lukas commented

2014-11-07 08:29:48 +01:00

Added subscriber: @luckyxxl

Lukas commented

2014-11-07 08:29:48 +01:00

I can confirm this (Blender 2.72b, Debian Wheezy amd64, NVIDIA GTX 760).

Lukas commented

2014-11-07 08:35:45 +01:00

It even gets more weird when adding a new uv-sphere in edit mode and changing the number of segments. The sphere is duplicated with the old one being deselected.

In object mode however, the left panel is greyed-out. Maybe this should also happen in edit mode?

It even gets more weird when adding a new uv-sphere in edit mode and changing the number of segments. The sphere is duplicated with the old one being deselected. In object mode however, the left panel is greyed-out. Maybe this should also happen in edit mode?

Bastien Montagne commented

2014-11-07 08:46:26 +01:00

Added subscriber: @mont29

Bastien Montagne commented

2014-11-07 08:46:26 +01:00

Yes, we should always have at least two or three undosteps in fact, some operators are macros and need them…

Bastien Montagne self-assigned this 2014-11-07 08:46:51 +01:00

Bastien Montagne commented

2014-11-07 09:02:54 +01:00

Added subscribers: @Sergey, @ideasman42

Bastien Montagne commented

2014-11-07 09:02:54 +01:00

Patch below just ensure that when undosteps are not zero (i.e. disabled), we always have at least 4 - should be enough to ensure all op can be redone, at least.

Sergey, Campbell, not so familiar with undo stack, is this patch acceptable?

P160: #42531

diff --git a/source/blender/blenkernel/intern/blender.c b/source/blender/blenkernel/intern/blender.c
index adfe43c..53b9b55 100644
--- a/source/blender/blenkernel/intern/blender.c
+++ b/source/blender/blenkernel/intern/blender.c
@@ -633,13 +633,14 @@ void BKE_write_undo(bContext *C, const char *name)
 {
 	uintptr_t maxmem, totmem, memused;
 	int nr /*, success */ /* UNUSED */;
+	const int undosteps = USERPREF_UNDOSTEPS(U.undosteps);
 	UndoElem *uel;
 	
 	if ((U.uiflag & USER_GLOBALUNDO) == 0) {
 		return;
 	}
 
-	if (U.undosteps == 0) {
+	if (undosteps == 0) {
 		return;
 	}
 	
@@ -661,7 +662,7 @@ void BKE_write_undo(bContext *C, const char *name)
 	uel = undobase.last;
 	while (uel) {
 		nr++;
-		if (nr == U.undosteps) break;
+		if (nr == undosteps) break;
 		uel = uel->prev;
 	}
 	if (uel) {
@@ -684,7 +685,7 @@ void BKE_write_undo(bContext *C, const char *name)
 
 		/* calculate current filepath */
 		counter++;
-		counter = counter % U.undosteps;
+		counter = counter % undosteps;
 	
 		BLI_snprintf(numstr, sizeof(numstr), "%d.blend", counter);
 		BLI_make_file_string("/", filepath, BLI_temp_dir_session(), numstr);
diff --git a/source/blender/editors/physics/particle_edit.c b/source/blender/editors/physics/particle_edit.c
index 9a3433b..3c1a693 100644
--- a/source/blender/editors/physics/particle_edit.c
+++ b/source/blender/editors/physics/particle_edit.c
@@ -4176,6 +4176,7 @@ void PE_undo_push(Scene *scene, const char *str)
 	PTCacheEdit *edit= PE_get_current(scene, OBACT);
 	PTCacheUndo *undo;
 	int nr;
+	const int undosteps = USERPREF_UNDOSTEPS(U.undosteps);
 
 	if (!edit) return;
 
@@ -4197,7 +4198,7 @@ void PE_undo_push(Scene *scene, const char *str)
 	undo= edit->undo.last;
 	while (undo) {
 		nr++;
-		if (nr==U.undosteps) break;
+		if (nr == undosteps) break;
 		undo= undo->prev;
 	}
 	if (undo) {
diff --git a/source/blender/editors/sculpt_paint/paint_undo.c b/source/blender/editors/sculpt_paint/paint_undo.c
index 20e3155..1540881 100644
--- a/source/blender/editors/sculpt_paint/paint_undo.c
+++ b/source/blender/editors/sculpt_paint/paint_undo.c
@@ -83,6 +83,7 @@ static void undo_stack_push_begin(UndoStack *stack, const char *name, UndoRestor
 {
 	UndoElem *uel;
 	int nr;
+	const int undosteps = USERPREF_UNDOSTEPS(U.undosteps);
 	
 	/* Undo push is split up in begin and end, the reason is that as painting
 	 * happens more tiles/nodes are added to the list, and at the very end we
@@ -110,7 +111,7 @@ static void undo_stack_push_begin(UndoStack *stack, const char *name, UndoRestor
 	uel = stack->elems.last;
 	while (uel) {
 		nr++;
-		if (nr == U.undosteps) break;
+		if (nr == undosteps) break;
 		uel = uel->prev;
 	}
 	if (uel) {
@@ -127,13 +128,14 @@ static void undo_stack_push_end(UndoStack *stack)
 	UndoElem *uel;
 	uintptr_t totmem, maxmem;
 	int totundo = 0;
+	const int undosteps = USERPREF_UNDOSTEPS(U.undosteps);
 
 	/* first limit to undo steps */
 	uel = stack->elems.last;
 
 	while (uel) {
 		totundo++;
-		if (totundo > U.undosteps) break;
+		if (totundo > undosteps) break;
 		uel = uel->prev;
 	}
 
diff --git a/source/blender/editors/util/editmode_undo.c b/source/blender/editors/util/editmode_undo.c
index ef95e4c..a9090ef 100644
--- a/source/blender/editors/util/editmode_undo.c
+++ b/source/blender/editors/util/editmode_undo.c
@@ -118,6 +118,7 @@ void undo_editmode_push(bContext *C, const char *name,
 	Object *obedit = CTX_data_edit_object(C);
 	void *editdata;
 	int nr;
+	const int undosteps = USERPREF_UNDOSTEPS(U.undosteps);
 	uintptr_t memused, totmem, maxmem;
 
 	/* at first here was code to prevent an "original" key to be inserted twice
@@ -146,7 +147,7 @@ void undo_editmode_push(bContext *C, const char *name,
 	uel = undobase.last;
 	while (uel) {
 		nr++;
-		if (nr == U.undosteps) break;
+		if (nr == undosteps) break;
 		uel = uel->prev;
 	}
 	if (uel) {
diff --git a/source/blender/editors/util/undo.c b/source/blender/editors/util/undo.c
index 189a938..d50c2a5 100644
--- a/source/blender/editors/util/undo.c
+++ b/source/blender/editors/util/undo.c
@@ -77,6 +77,8 @@
 
 void ED_undo_push(bContext *C, const char *str)
 {
+	const int undosteps = USERPREF_UNDOSTEPS(U.undosteps);
+
 	wmWindowManager *wm = CTX_wm_manager(C);
 	Object *obedit = CTX_data_edit_object(C);
 	Object *obact = CTX_data_active_object(C);
@@ -85,7 +87,7 @@ void ED_undo_push(bContext *C, const char *str)
 		printf("%s: %s\n", __func__, str);
 
 	if (obedit) {
-		if (U.undosteps == 0) return;
+		if (undosteps == 0) return;
 		
 		if (obedit->type == OB_MESH)
 			undo_push_mesh(C, str);
@@ -101,7 +103,7 @@ void ED_undo_push(bContext *C, const char *str)
 			undo_push_armature(C, str);
 	}
 	else if (obact && obact->mode & OB_MODE_PARTICLE_EDIT) {
-		if (U.undosteps == 0) return;
+		if (undosteps == 0) return;
 
 		PE_undo_push(CTX_data_scene(C), str);
 	}
diff --git a/source/blender/makesdna/DNA_userdef_types.h b/source/blender/makesdna/DNA_userdef_types.h
index d8653e1..150cc8f 100644
--- a/source/blender/makesdna/DNA_userdef_types.h
+++ b/source/blender/makesdna/DNA_userdef_types.h
@@ -862,6 +862,9 @@ typedef enum eUserpref_VirtualPixel {
 	VIRTUAL_PIXEL_DOUBLE = 1,
 } eUserpref_VirtualPixel;
 
+/* If undo is enabled, we need a minimum number of those to ensure proper execution, see #42531. */
+#define USERPREF_UNDOSTEPS(_steps) (((_steps) > 0) ? (((_steps) < 4) ? 4 : (_steps)) : 0)
+
 #ifdef __cplusplus
 }
 #endif

Patch below just ensure that when undosteps are not zero (i.e. disabled), we always have at least 4 - should be enough to ensure all op can be redone, at least. Sergey, Campbell, not so familiar with undo stack, is this patch acceptable? [P160: #42531](https://archive.blender.org/developer/P160.txt) ``` diff --git a/source/blender/blenkernel/intern/blender.c b/source/blender/blenkernel/intern/blender.c index adfe43c..53b9b55 100644 --- a/source/blender/blenkernel/intern/blender.c +++ b/source/blender/blenkernel/intern/blender.c @@ -633,13 +633,14 @@ void BKE_write_undo(bContext *C, const char *name) { uintptr_t maxmem, totmem, memused; int nr /*, success */ /* UNUSED */; + const int undosteps = USERPREF_UNDOSTEPS(U.undosteps); UndoElem *uel; if ((U.uiflag & USER_GLOBALUNDO) == 0) { return; } - if (U.undosteps == 0) { + if (undosteps == 0) { return; } @@ -661,7 +662,7 @@ void BKE_write_undo(bContext *C, const char *name) uel = undobase.last; while (uel) { nr++; - if (nr == U.undosteps) break; + if (nr == undosteps) break; uel = uel->prev; } if (uel) { @@ -684,7 +685,7 @@ void BKE_write_undo(bContext *C, const char *name) /* calculate current filepath */ counter++; - counter = counter % U.undosteps; + counter = counter % undosteps; BLI_snprintf(numstr, sizeof(numstr), "%d.blend", counter); BLI_make_file_string("/", filepath, BLI_temp_dir_session(), numstr); diff --git a/source/blender/editors/physics/particle_edit.c b/source/blender/editors/physics/particle_edit.c index 9a3433b..3c1a693 100644 --- a/source/blender/editors/physics/particle_edit.c +++ b/source/blender/editors/physics/particle_edit.c @@ -4176,6 +4176,7 @@ void PE_undo_push(Scene *scene, const char *str) PTCacheEdit *edit= PE_get_current(scene, OBACT); PTCacheUndo *undo; int nr; + const int undosteps = USERPREF_UNDOSTEPS(U.undosteps); if (!edit) return; @@ -4197,7 +4198,7 @@ void PE_undo_push(Scene *scene, const char *str) undo= edit->undo.last; while (undo) { nr++; - if (nr==U.undosteps) break; + if (nr == undosteps) break; undo= undo->prev; } if (undo) { diff --git a/source/blender/editors/sculpt_paint/paint_undo.c b/source/blender/editors/sculpt_paint/paint_undo.c index 20e3155..1540881 100644 --- a/source/blender/editors/sculpt_paint/paint_undo.c +++ b/source/blender/editors/sculpt_paint/paint_undo.c @@ -83,6 +83,7 @@ static void undo_stack_push_begin(UndoStack *stack, const char *name, UndoRestor { UndoElem *uel; int nr; + const int undosteps = USERPREF_UNDOSTEPS(U.undosteps); /* Undo push is split up in begin and end, the reason is that as painting * happens more tiles/nodes are added to the list, and at the very end we @@ -110,7 +111,7 @@ static void undo_stack_push_begin(UndoStack *stack, const char *name, UndoRestor uel = stack->elems.last; while (uel) { nr++; - if (nr == U.undosteps) break; + if (nr == undosteps) break; uel = uel->prev; } if (uel) { @@ -127,13 +128,14 @@ static void undo_stack_push_end(UndoStack *stack) UndoElem *uel; uintptr_t totmem, maxmem; int totundo = 0; + const int undosteps = USERPREF_UNDOSTEPS(U.undosteps); /* first limit to undo steps */ uel = stack->elems.last; while (uel) { totundo++; - if (totundo > U.undosteps) break; + if (totundo > undosteps) break; uel = uel->prev; } diff --git a/source/blender/editors/util/editmode_undo.c b/source/blender/editors/util/editmode_undo.c index ef95e4c..a9090ef 100644 --- a/source/blender/editors/util/editmode_undo.c +++ b/source/blender/editors/util/editmode_undo.c @@ -118,6 +118,7 @@ void undo_editmode_push(bContext *C, const char *name, Object *obedit = CTX_data_edit_object(C); void *editdata; int nr; + const int undosteps = USERPREF_UNDOSTEPS(U.undosteps); uintptr_t memused, totmem, maxmem; /* at first here was code to prevent an "original" key to be inserted twice @@ -146,7 +147,7 @@ void undo_editmode_push(bContext *C, const char *name, uel = undobase.last; while (uel) { nr++; - if (nr == U.undosteps) break; + if (nr == undosteps) break; uel = uel->prev; } if (uel) { diff --git a/source/blender/editors/util/undo.c b/source/blender/editors/util/undo.c index 189a938..d50c2a5 100644 --- a/source/blender/editors/util/undo.c +++ b/source/blender/editors/util/undo.c @@ -77,6 +77,8 @@ void ED_undo_push(bContext *C, const char *str) { + const int undosteps = USERPREF_UNDOSTEPS(U.undosteps); + wmWindowManager *wm = CTX_wm_manager(C); Object *obedit = CTX_data_edit_object(C); Object *obact = CTX_data_active_object(C); @@ -85,7 +87,7 @@ void ED_undo_push(bContext *C, const char *str) printf("%s: %s\n", __func__, str); if (obedit) { - if (U.undosteps == 0) return; + if (undosteps == 0) return; if (obedit->type == OB_MESH) undo_push_mesh(C, str); @@ -101,7 +103,7 @@ void ED_undo_push(bContext *C, const char *str) undo_push_armature(C, str); } else if (obact && obact->mode & OB_MODE_PARTICLE_EDIT) { - if (U.undosteps == 0) return; + if (undosteps == 0) return; PE_undo_push(CTX_data_scene(C), str); } diff --git a/source/blender/makesdna/DNA_userdef_types.h b/source/blender/makesdna/DNA_userdef_types.h index d8653e1..150cc8f 100644 --- a/source/blender/makesdna/DNA_userdef_types.h +++ b/source/blender/makesdna/DNA_userdef_types.h @@ -862,6 +862,9 @@ typedef enum eUserpref_VirtualPixel { VIRTUAL_PIXEL_DOUBLE = 1, } eUserpref_VirtualPixel; +/* If undo is enabled, we need a minimum number of those to ensure proper execution, see #42531. */ +#define USERPREF_UNDOSTEPS(_steps) (((_steps) > 0) ? (((_steps) < 4) ? 4 : (_steps)) : 0) + #ifdef __cplusplus } #endif ```

Campbell Barton commented

2014-11-07 10:15:07 +01:00

if one undo step isn't useful, use an RNA set function to bump up to 2 when 1 is set, this removes some complexity. (and risk of accidentally using U.undosteps directly)

if one undo step isn't useful, use an RNA set function to bump up to 2 when 1 is set, this removes some complexity. (and risk of accidentally using `U.undosteps` directly)

blender-admin commented

2014-11-07 10:31:15 +01:00

This issue was referenced by 5604a3d31d

This issue was referenced by 5604a3d31ddb01c6856f9e85c87111e155137984

Bastien Montagne commented

2014-11-07 10:31:43 +01:00

Changed status from 'Open' to: 'Resolved'

Bastien Montagne closed this issue

2014-11-07 10:31:43 +01:00

Bastien Montagne commented

2014-11-07 10:31:43 +01:00

Closed by commit 5604a3d31d.

Closed by commit 5604a3d31d.

Sign in to join this conversation.

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Setting 'Undo' steps to '1' causes weirdness. #42531