Fix rip tool issues related to BM_Make_Ngon generating a copy of an original face with a face loop cycle starting on a different loop.

2 files changed, 43 insertions, 17 deletions

diff --git a/source/blender/bmesh/intern/bmesh_construct.c b/source/blender/bmesh/intern/bmesh_construct.c
index f4eb454e7a5..f263e4115d4 100644
--- a/source/blender/bmesh/intern/bmesh_construct.c
+++ b/source/blender/bmesh/intern/bmesh_construct.c
@@ -36,6 +36,7 @@
 #include "BKE_utildefines.h"
 
 #include "BLI_array.h"
+#include "BLI_utildefines.h"
 
 #include "DNA_meshdata_types.h"
 #include "DNA_mesh_types.h"
@@ -248,6 +249,11 @@ void BM_Face_CopyShared(BMesh *bm, BMFace *f) {
  *
  * The edges are not required to be ordered, simply to to form
  * a single closed loop as a whole
+ *
+ * Note that while this function will work fine when the edges
+ * are already sorted, if the edges are always going to be sorted,
+ * BM_Make_Face should be considered over this function as it
+ * avoids some unnecessary work.
 */
 #define VERT_BUF_SIZE 100
 BMFace *BM_Make_Ngon(BMesh *bm, BMVert *v1, BMVert *v2, BMEdge **edges, int len, int nodouble)
@@ -258,6 +264,7 @@ BMFace *BM_Make_Ngon(BMesh *bm, BMVert *v1, BMVert *v2, BMEdge **edges, int len,
 	BLI_array_staticdeclare(verts, VERT_BUF_SIZE);
 	BMFace *f = NULL;
 	BMEdge *e;
+	BMVert *ev1, *ev2;
 	int overlap = 0, i, /* j,*/ v1found, reverse;
 
 	/*this code is hideous, yeek.  I'll have to think about ways of
@@ -273,9 +280,18 @@ BMFace *BM_Make_Ngon(BMesh *bm, BMVert *v1, BMVert *v2, BMEdge **edges, int len,
 		bmesh_api_setflag(edges[i], _FLAG_MF);
 	}
 
-	BLI_array_append(verts, edges[0]->v1);
+	ev1 = edges[0]->v1;
+	ev2 = edges[0]->v2;
 
-	v = edges[0]->v2;
+	if (v1 == ev2) {
+		/* Swapping here improves performance and consistency of face
+		   structure in the special case that the edges are already in
+		   the correct order and winding */
+		SWAP(BMVert *, ev1, ev2);
+	}
+
+	BLI_array_append(verts, ev1);
+	v = ev2;
 	e = edges[0];
 	do {
 		BMEdge *e2 = e;
@@ -562,7 +578,7 @@ BMesh *BM_Copy_Mesh(BMesh *bmold)
 
 	/*copy over edit selection history*/
 	for (ese=bmold->selected.first; ese; ese=ese->next) {
-		void *ele;
+		void *ele = NULL;
 
 		if (ese->type == BM_VERT)
 			ele = vtable[BM_GetIndex(ese->data)];
@@ -571,6 +587,9 @@ BMesh *BM_Copy_Mesh(BMesh *bmold)
 		else if (ese->type == BM_FACE) {
 			ele = ftable[BM_GetIndex(ese->data)];
 		}
+		else {
+			BLI_assert(0);
+		}
 		
 		if (ele)
 			BM_store_selection(bm, ele);
diff --git a/source/blender/bmesh/operators/edgesplitop.c b/source/blender/bmesh/operators/edgesplitop.c
index d73d8f5551a..ca98226c100 100644
--- a/source/blender/bmesh/operators/edgesplitop.c
+++ b/source/blender/bmesh/operators/edgesplitop.c
@@ -65,13 +65,14 @@ typedef struct EdgeTag {
 #define FACE_DEL	1
 #define FACE_NEW	2
 
-static BMFace *remake_face(BMesh *bm, EdgeTag *etags, BMFace *f, BMVert **verts)
+#define EDGE_BUF_SIZE 100
+static BMFace *remake_face(BMesh *bm, EdgeTag *etags, BMFace *f, BMVert **verts, BMEdge **edges)
 {
 	BMIter liter1, liter2;
 	EdgeTag *et;
 	BMFace *f2;
 	BMLoop *l, *l2;
-	BMEdge **edges = (BMEdge**) verts; /*he he, can reuse this, sneaky! ;)*/
+	BMEdge *e;
 	BMVert *lastv1, *lastv2, *v1, *v2;
 	int i;
 
@@ -81,16 +82,16 @@ static BMFace *remake_face(BMesh *bm, EdgeTag *etags, BMFace *f, BMVert **verts)
 	v1 = verts[0];
 	v2 = verts[1];
 	for (i=0; i<f->len-1; i++) {
-		edges[i] = BM_Make_Edge(bm, verts[i], verts[i+1], NULL, 1);
-
-		if (!edges[i]) {
+		e = BM_Make_Edge(bm, verts[i], verts[i+1], NULL, 1);
+		if (!e) {
 			return NULL;
 		}
+		edges[i] = e;
 	}
 
 	edges[i] = BM_Make_Edge(bm, lastv1, lastv2, NULL, 1);
 
-	f2 = BM_Make_Ngon(bm, v1, v2, edges, f->len, 0);
+	f2 = BM_Make_Face(bm, verts, edges, f->len);
 	if (!f2) {
 		return NULL;
 	}
@@ -110,9 +111,14 @@ static BMFace *remake_face(BMesh *bm, EdgeTag *etags, BMFace *f, BMVert **verts)
 			if (!et->newe1) {
 				et->newe1 = l2->e;
 			}
-			else {
+			else if (!et->newe2) {
 				et->newe2 = l2->e;
 			}
+			else {
+				/* Only two new edges should be created from each original edge
+				   for edge split operation */
+				BLI_assert(et->newe1 == l2->e || et->newe2 == l2->e);
+			}
 
 			if (BMO_TestFlag(bm, l->e, EDGE_SEAM)) {
 				BMO_SetFlag(bm, l2->e, EDGE_SEAM);
@@ -209,8 +215,10 @@ void bmesh_edgesplitop_exec(BMesh *bm, BMOperator *op)
 	BMFace *f, *f2;
 	BMLoop *l, *nextl, *prevl, *l2, *l3;
 	BMEdge *e, *e2;
-	BLI_array_declare(verts);
 	BMVert *v, *v2, **verts = NULL;
+	BLI_array_declare(verts);
+	BMEdge **edges = NULL;
+	BLI_array_declare(edges);
 	int i, j;
 
 	BMO_Flag_Buffer(bm, op, "edges", EDGE_SEAM, BM_EDGE);
@@ -261,22 +269,20 @@ void bmesh_edgesplitop_exec(BMesh *bm, BMOperator *op)
 		BLI_array_empty(verts);
 		BLI_array_growitems(verts, f->len);
 		memset(verts, 0, sizeof(BMVert*)*f->len);
+
+		BLI_array_empty(edges);
+		BLI_array_growitems(edges, f->len);
 		
 		i = 0;
 		BM_ITER(l, &liter, bm, BM_LOOPS_OF_FACE, f) {
 			if (!BMO_TestFlag(bm, l->e, EDGE_SEAM)) {
 				if (!verts[i]) {
 
-					/* WARNING, commented because of bug [#28581] in rip tool
-					 * I couldn't find any cases where this is needed, without
-					 * it rip tool at least works fine - campbell */
-#if 0
 					et = etags + BM_GetIndex(l->e);
 					if (ETV(et, l->v, l)) {
 						verts[i] = ETV(et, l->v, l);
 					}
 					else
-#endif
 					{
 						verts[i] = l->v;
 					}
@@ -383,7 +389,7 @@ void bmesh_edgesplitop_exec(BMesh *bm, BMOperator *op)
 		}
 #endif
 
-		f2 = remake_face(bm, etags, f, verts); /* clobbers 'verts', */
+		f2 = remake_face(bm, etags, f, verts, edges); /* clobbers 'verts', */
 		if (!f2) {
 			continue;
 		}
@@ -411,6 +417,7 @@ void bmesh_edgesplitop_exec(BMesh *bm, BMOperator *op)
 	BMO_Flag_To_Slot(bm, op, "edgeout2", EDGE_RET2, BM_EDGE);
 
 	BLI_array_free(verts);
+	BLI_array_free(edges);
 	if (etags) MEM_freeN(etags);
 }