modeler normals don't match render normals

there's a before and after shot at
http://jbit.net/~sparky/nprob/

Essentially the modeler doesn't take into account that some faces might be set to flat (the way the render does it).
This inconsistency is bad in various contexts. Normal maps is one (terrible) but also BGE and exports don't get the same results as the renderer which is also bad. This is my suggested fix.

Btw, the before shot uses the modeler normals (which as you can see are no good).

[brechtvl, 2011-03-15 13:59:52]

To be honest, I don't like this SCalcNormalInterface at all, way too much code
and abstraction. 

Why not simply keep the loops per mesh type, to clear vertex normals and
normalize them, and then have one utility function that is used from all the
face loop? Something like:

void accumulate_vertex_normals(int nverts, const float verts[][3], float
vnormals[][3], float fnormal[3]);

[sparky, 2011-03-15 14:10:45]

No that won't work. The structures are way too different. For instance edit
mesh contains it's
data in a linked list which has forced me to make even more abstraction
actually.
I did it this way to avoid having to allocate more memory yet at the same
time
the implementation calculatenormal.c has full control.
We can change anything we want in there. Angle weighed, not angle weighted,
check for degenerate prims, whatever. Anything we might ever want to add or
remove
from the method is now confined to one place which is a huge plus.
Even if we were to do the accumulation only it would still require most of
the functions.
There are afterall big differences between the various mesh types.


On Tue, Mar 15, 2011 at 6:59 AM, <brechtvanlommel@gmail.com> wrote:

> To be honest, I don't like this SCalcNormalInterface at all, way too
> much code and abstraction.
>
> Why not simply keep the loops per mesh type, to clear vertex normals and
> normalize them, and then have one utility function that is used from all
> the face loop? Something like:
>
> void accumulate_vertex_normals(int nverts, const float verts[][3], float
> vnormals[][3], float fnormal[3]);
>
>
> http://codereview.appspot.com/4280049/
>

[brechtvl, 2011-03-15 14:22:52]

On 2011/03/15 14:10:45, mikkelsen7 wrote:
> We can change anything we want in there. Angle weighed, not angle weighted,
> check for degenerate prims, whatever. Anything we might ever want to add or
> remove
> from the method is now confined to one place which is a huge plus.

Well, I don't think we should account for everything you might possibly want to
do, but it's not even clear what you might need this abstraction for in the
future. All these examples you give can be done inside a function like
accumulate_vertex_normals.

These iterators are hard to follow & debug, I much prefer readable slightly
duplicated code over this.

[sparky, 2011-03-15 14:28:55]

So you'd be happy with it the way it is if I took out the mem-clear and the
normalization
thus getting rid of the ability to iterate over the vertices?



On Tue, Mar 15, 2011 at 7:22 AM, <brechtvanlommel@gmail.com> wrote:

> On 2011/03/15 14:10:45, mikkelsen7 wrote:
>
>> We can change anything we want in there. Angle weighed, not angle
>>
> weighted,
>
>> check for degenerate prims, whatever. Anything we might ever want to
>>
> add or
>
>> remove
>> from the method is now confined to one place which is a huge plus.
>>
>
> Well, I don't think we should account for everything you might possibly
> want to do, but it's not even clear what you might need this abstraction
> for in the future. All these examples you give can be done inside a
> function like accumulate_vertex_normals.
>
> These iterators are hard to follow & debug, I much prefer readable
> slightly duplicated code over this.
>
>
> http://codereview.appspot.com/4280049/
>

[brechtvl, 2011-03-15 14:32:13]

On 2011/03/15 14:28:55, mikkelsen7 wrote:
> So you'd be happy with it the way it is if I took out the mem-clear and the
> normalization
> thus getting rid of the ability to iterate over the vertices?

I'd like to see all iterators removed. I don't see the need to share anything
more than a single function with a dozen lines of code.

[brechtvl, 2011-03-15 14:36:38]

On 2011/03/15 14:32:13, brechtvl wrote:
> On 2011/03/15 14:28:55, mikkelsen7 wrote:
> > So you'd be happy with it the way it is if I took out the mem-clear and the
> > normalization
> > thus getting rid of the ability to iterate over the vertices?
> 
> I'd like to see all iterators removed. I don't see the need to share anything
> more than a single function with a dozen lines of code.

I forgot, a function like accumulate_vertex_normals would also have a parameter
to indicate if the face is smooth or not, to share that behavior too. You can
also add functions to clear and normalize the vertex normal, to encapsulate that
better too.

[sparky, 2011-03-15 14:51:05]

But it has to iterate over the faces twice.

Removing the iteration from the abstraction would conflict with that.


On Tue, Mar 15, 2011 at 7:36 AM, <brechtvanlommel@gmail.com> wrote:

> On 2011/03/15 14:32:13, brechtvl wrote:
>
>> On 2011/03/15 14:28:55, mikkelsen7 wrote:
>> > So you'd be happy with it the way it is if I took out the mem-clear
>>
> and the
>
>> > normalization
>> > thus getting rid of the ability to iterate over the vertices?
>>
>
>  I'd like to see all iterators removed. I don't see the need to share
>>
> anything
>
>> more than a single function with a dozen lines of code.
>>
>
> I forgot, a function like accumulate_vertex_normals would also have a
> parameter to indicate if the face is smooth or not, to share that
> behavior too. You can also add functions to clear and normalize the
> vertex normal, to encapsulate that better too.
>
>
> http://codereview.appspot.com/4280049/
>

[brechtvl, 2011-03-15 15:02:21]

On 2011/03/15 14:51:05, mikkelsen7 wrote:
> But it has to iterate over the faces twice.
> 
> Removing the iteration from the abstraction would conflict with that.

Is the second loop really necessary, those normals should never be used? Maybe
for export.. If it is necessary, just add two loops then.

[sparky, 2011-03-15 15:22:50]

I'll make a version of the patch without any code sharing instead.



On Tue, Mar 15, 2011 at 8:02 AM, <brechtvanlommel@gmail.com> wrote:

> On 2011/03/15 14:51:05, mikkelsen7 wrote:
>
>> But it has to iterate over the faces twice.
>>
>
>  Removing the iteration from the abstraction would conflict with that.
>>
>
> Is the second loop really necessary, those normals should never be used?
> Maybe for export.. If it is necessary, just add two loops then.
>
>
> http://codereview.appspot.com/4280049/
>

[sparky, 2011-03-15 16:20:15]

Index: render/intern/source/convertblender.c
===================================================================
--- render/intern/source/convertblender.c	(revision 35505)
+++ render/intern/source/convertblender.c	(working copy)
@@ -499,6 +499,11 @@
 }
 
 
+
+/****************************************************************
+************ tangent space generation interface *****************
+****************************************************************/
+
 typedef struct
 {
 	ObjectRen *obr;
@@ -599,7 +604,6 @@
 			VertRen *v3= vlr->v3;
 			VertRen *v4= vlr->v4;
 			float n1[3], n2[3], n3[3], n4[3];
-			float fac1, fac2, fac3, fac4=0.0f;
 			
 			sub_v3_v3v3(n1, v2->co, v1->co);
 			normalize_v3(n1);
@@ -608,10 +612,6 @@
 			if(v4==NULL) {
 				sub_v3_v3v3(n3, v1->co, v3->co);
 				normalize_v3(n3);
-
-				fac1= saacos(-n1[0]*n3[0]-n1[1]*n3[1]-n1[2]*n3[2]);
-				fac2= saacos(-n1[0]*n2[0]-n1[1]*n2[1]-n1[2]*n2[2]);
-				fac3= saacos(-n2[0]*n3[0]-n2[1]*n3[1]-n2[2]*n3[2]);
 			}
 			else {
 				sub_v3_v3v3(n3, v4->co, v3->co);
@@ -619,27 +619,22 @@
 				sub_v3_v3v3(n4, v1->co, v4->co);
 				normalize_v3(n4);
 
-				fac1= saacos(-n4[0]*n1[0]-n4[1]*n1[1]-n4[2]*n1[2]);
-				fac2= saacos(-n1[0]*n2[0]-n1[1]*n2[1]-n1[2]*n2[2]);
-				fac3= saacos(-n2[0]*n3[0]-n2[1]*n3[1]-n2[2]*n3[2]);
-				fac4= saacos(-n3[0]*n4[0]-n3[1]*n4[1]-n3[2]*n4[2]);
-
-				v4->n[0] +=fac4*vlr->n[0];
-				v4->n[1] +=fac4*vlr->n[1];
-				v4->n[2] +=fac4*vlr->n[2];
+				v4->n[0] +=vlr->n[0];
+				v4->n[1] +=vlr->n[1];
+				v4->n[2] +=vlr->n[2];
 			}
 
-			v1->n[0] +=fac1*vlr->n[0];
-			v1->n[1] +=fac1*vlr->n[1];
-			v1->n[2] +=fac1*vlr->n[2];
+			v1->n[0] +=vlr->n[0];
+			v1->n[1] +=vlr->n[1];
+			v1->n[2] +=vlr->n[2];
 
-			v2->n[0] +=fac2*vlr->n[0];
-			v2->n[1] +=fac2*vlr->n[1];
-			v2->n[2] +=fac2*vlr->n[2];
+			v2->n[0] +=vlr->n[0];
+			v2->n[1] +=vlr->n[1];
+			v2->n[2] +=vlr->n[2];
 
-			v3->n[0] +=fac3*vlr->n[0];
-			v3->n[1] +=fac3*vlr->n[1];
-			v3->n[2] +=fac3*vlr->n[2];
+			v3->n[0] +=vlr->n[0];
+			v3->n[1] +=vlr->n[1];
+			v3->n[2] +=vlr->n[2];
 			
 		}
 		if(do_nmap_tangent || do_tangent) {
@@ -3236,6 +3231,7 @@
 	int a, a1, ok, vertofs;
 	int end, do_autosmooth=0, totvert = 0;
 	int use_original_normals= 0;
+	int iCalcNormalsInRender = 0;	// false by default
 
 	me= ob->data;
 
@@ -3313,6 +3309,7 @@
 	
 	ma= give_render_material(re, ob, 1);
 
+
 	if(ma->material_type == MA_TYPE_HALO) {
 		make_render_halos(re, obr, me, totvert, mvert, ma, orco);
 	}
@@ -3321,8 +3318,17 @@
 		for(a=0; a<totvert; a++, mvert++) {
 			ver= RE_findOrAddVert(obr, obr->totvert++);
 			VECCOPY(ver->co, mvert->co);
+			ver->n[0]=mvert->no[0];
+			ver->n[1]=mvert->no[1];
+			ver->n[2]=mvert->no[2];
+			//dm->getVertNo(dm, a, ver->n);
 			if(do_autosmooth==0)	/* autosmooth on original unrotated data to prevent
differences between frames */
+			{
 				mul_m4_v3(mat, ver->co);
+				mul_transposed_m3_v3(imat, ver->n);
+				normalize_v3(ver->n);
+				negate_v3(ver->n);
+			}
   
 			if(orco) {
 				ver->orco= orco;
@@ -3370,6 +3376,9 @@
 					end= dm->getNumFaces(dm);
 					mface= dm->getFaceArray(dm);
 
+					if(need_nmap_tangent!=0 && CustomData_get_layer_index(&dm->faceData,
CD_TANGENT) == -1)
+						DM_add_tangent_layer(dm);
+					
 					for(a=0; a<end; a++, mface++) {
 						int v1, v2, v3, v4, flag;
 						
@@ -3400,10 +3409,14 @@
 									len= normal_tri_v3( vlr->n,mv[mf->v3].co, mv[mf->v2].co,
mv[mf->v1].co);
 							}
 							else {
-								if(vlr->v4) 
+								/*if(vlr->v4) 
 									len= normal_quad_v3( vlr->n,vlr->v4->co, vlr->v3->co, vlr->v2->co,
vlr->v1->co);
 								else 
-									len= normal_tri_v3( vlr->n,vlr->v3->co, vlr->v2->co, vlr->v1->co);
+									len= normal_tri_v3( vlr->n,vlr->v3->co, vlr->v2->co, vlr->v1->co);*/
+								len = dm->getFaceNo(dm, a, vlr->n);
+								mul_transposed_m3_v3(imat, vlr->n);
+								len *= normalize_v3(vlr->n);
+								negate_v3(vlr->n);
 							}
 
 							vlr->mat= ma;
@@ -3415,7 +3428,7 @@
 								CustomDataLayer *layer;
 								MTFace *mtface, *mtf;
 								MCol *mcol, *mc;
-								int index, mtfn= 0, mcn= 0;
+								int index, mtfn= 0, mcn= 0, mtng=0;
 								char *name;
 
 								for(index=0; index<dm->faceData.totlayer; index++) {
@@ -3432,6 +3445,22 @@
 										mcol= (MCol*)layer->data;
 										memcpy(mc, &mcol[a*4], sizeof(MCol)*4);
 									}
+									else if(layer->type == CD_TANGENT && mtng < 1)
+									{
+										if(need_nmap_tangent!=0)
+										{
+											const float * tangent = (const float *) layer->data;
+											int t;
+											int nr_verts = v4!=0 ? 4 : 3;
+											float * ftang = RE_vlakren_get_nmap_tangent(obr, vlr, 1);
+											for(t=0; t<nr_verts; t++)
+											{
+												QUATCOPY(ftang+t*4, tangent+a*16+t*4);
+												mul_mat3_m4_v3(mat, ftang+t*4);
+												normalize_v3(ftang+t*4);
+											}
+										}
+									}
 								}
 							}
 						}
@@ -3447,6 +3476,7 @@
 				MEdge *medge;
 				struct edgesort *edgetable;
 				int totedge= 0;
+				iCalcNormalsInRender=1;
 				
 				medge= dm->getEdgeArray(dm);
 				
@@ -3489,6 +3519,7 @@
 	
 	if(!timeoffset) {
 		if (test_for_displace(re, ob ) ) {
+			iCalcNormalsInRender = 1;
 			calc_vertexnormals(re, obr, 0, 0);
 			if(do_autosmooth)
 				do_displacement(re, obr, mat, imat);
@@ -3497,11 +3528,13 @@
 		}
 
 		if(do_autosmooth) {
+			iCalcNormalsInRender = 1;
 			autosmooth(re, obr, mat, me->smoothresh);
 		}
 
-		calc_vertexnormals(re, obr, need_tangent, need_nmap_tangent);
-
+		if(iCalcNormalsInRender!=0 || need_tangent!=0)
+			calc_vertexnormals(re, obr, need_tangent, need_nmap_tangent);
+		
 		if(need_stress)
 			calc_edge_stress(re, obr, me);
 	}
Index: blenkernel/intern/cdderivedmesh.c
===================================================================
--- blenkernel/intern/cdderivedmesh.c	(revision 35505)
+++ blenkernel/intern/cdderivedmesh.c	(working copy)
@@ -179,6 +179,54 @@
 	no_r[2] = no[2]/32767.f;
 }
 
+static void cdDM_getRealVertNo(DerivedMesh *dm, int face_index, int vert_num,
float no_r[3])
+{
+	CDDerivedMesh *cddm = (CDDerivedMesh*) dm;
+	const int smoothnormal = (cddm->mface[face_index].flag & ME_SMOOTH);
+	if(!smoothnormal)	// flat
+	{
+		dm->getFaceNo(dm, face_index, no_r);
+	}
+	else
+	{
+		unsigned int indices[] = {	cddm->mface[face_index].v1,
cddm->mface[face_index].v2,
+								cddm->mface[face_index].v3, cddm->mface[face_index].v4 };
+		dm->getVertNo(dm, indices[vert_num], no_r);
+		normalize_v3(no_r);
+	}
+}
+
+static float cdDM_getFaceNo(DerivedMesh *dm, int index, float no_r[3])
+{
+	float * nors = dm->getFaceDataArray(dm, CD_NORMAL);
+	float len;
+
+	if(nors)
+	{
+		VECCOPY(no_r, &nors[3*index]);
+		len = len_v3(no_r);
+	}
+	else
+	{
+		CDDerivedMesh *cddm = (CDDerivedMesh*) dm;
+		float * p0, * p1, * p2;
+		const int iGetNrVerts = cddm->mface[index].v4!=0 ? 4 : 3;
+		unsigned int indices[] = {	cddm->mface[index].v1, cddm->mface[index].v2,
+								cddm->mface[index].v3, cddm->mface[index].v4 };
+		p0 = cddm->mvert[indices[0]].co; p1 = cddm->mvert[indices[1]].co; p2 =
cddm->mvert[indices[2]].co;
+		if(iGetNrVerts==4)
+		{
+			float * p3 = cddm->mvert[indices[3]].co;
+			len = normal_quad_v3( no_r, p0, p1, p2, p3);
+		}
+		else {
+			len = normal_tri_v3(no_r, p0, p1, p2);
+		}
+
+	}
+	return len;
+}
+
 static ListBase *cdDM_getFaceMap(Object *ob, DerivedMesh *dm)
 {
 	CDDerivedMesh *cddm = (CDDerivedMesh*) dm;
@@ -1480,6 +1528,8 @@
 	dm->getVertCos = cdDM_getVertCos;
 	dm->getVertCo = cdDM_getVertCo;
 	dm->getVertNo = cdDM_getVertNo;
+	dm->getRealVertNo = cdDM_getRealVertNo;
+	dm->getFaceNo = cdDM_getFaceNo;
 
 	dm->getPBVH = cdDM_getPBVH;
 	dm->getFaceMap = cdDM_getFaceMap;
@@ -1798,7 +1848,7 @@
 	int i;
 	int numVerts = dm->numVertData;
 	int numFaces = dm->numFaceData;
-	MFace *mf;
+	MFace *mfaces;
 	MVert *mv;
 
 	if(numVerts == 0) return;
@@ -1817,22 +1867,41 @@
 										 NULL, dm->numFaceData);
 
 	/* calculate face normals and add to vertex normals */
-	mf = CDDM_get_faces(dm);
-	for(i = 0; i < numFaces; i++, mf++) {
-		float *f_no = face_nors[i];
+	mfaces = CDDM_get_faces(dm);
+	for(i = 0; i < numFaces; i++) {
+		MFace * mf = &mfaces[i];
+		if((mf->flag&ME_SMOOTH)!=0)
+		{
+			float *f_no = face_nors[i];
 
-		if(mf->v4)
-			normal_quad_v3( f_no,mv[mf->v1].co, mv[mf->v2].co, mv[mf->v3].co,
mv[mf->v4].co);
-		else
-			normal_tri_v3( f_no,mv[mf->v1].co, mv[mf->v2].co, mv[mf->v3].co);
+			if(mf->v4)
+				normal_quad_v3( f_no,mv[mf->v1].co, mv[mf->v2].co, mv[mf->v3].co,
mv[mf->v4].co);
+			else
+				normal_tri_v3( f_no,mv[mf->v1].co, mv[mf->v2].co, mv[mf->v3].co);
 		
-		add_v3_v3(temp_nors[mf->v1], f_no);
-		add_v3_v3(temp_nors[mf->v2], f_no);
-		add_v3_v3(temp_nors[mf->v3], f_no);
-		if(mf->v4)
-			add_v3_v3(temp_nors[mf->v4], f_no);
+			add_v3_v3(temp_nors[mf->v1], f_no);
+			add_v3_v3(temp_nors[mf->v2], f_no);
+			add_v3_v3(temp_nors[mf->v3], f_no);
+			if(mf->v4)
+				add_v3_v3(temp_nors[mf->v4], f_no);
+		}
 	}
 
+	for(i = 0; i < numFaces; i++) {
+		MFace * mf = &mfaces[i];
+		if((mf->flag&ME_SMOOTH)!=0)
+		{
+			float *f_no = face_nors[i];
+
+			if(temp_nors[mf->v1][0]==0 && temp_nors[mf->v1][1]==0 &&
temp_nors[mf->v1][2]==0) VECCOPY(temp_nors[mf->v1], f_no);
+			if(temp_nors[mf->v2][0]==0 && temp_nors[mf->v2][1]==0 &&
temp_nors[mf->v2][2]==0) VECCOPY(temp_nors[mf->v2], f_no);
+			if(temp_nors[mf->v3][0]==0 && temp_nors[mf->v3][1]==0 &&
temp_nors[mf->v3][2]==0) VECCOPY(temp_nors[mf->v3], f_no);
+			if(mf->v4)
+				if(temp_nors[mf->v4][0]==0 && temp_nors[mf->v4][1]==0 &&
temp_nors[mf->v4][2]==0) VECCOPY(temp_nors[mf->v4], f_no);
+		}
+	}
+
+
 	/* normalize vertex normals and assign */
 	for(i = 0; i < numVerts; i++, mv++) {
 		float *no = temp_nors[i];
Index: blenkernel/intern/mesh.c
===================================================================
--- blenkernel/intern/mesh.c	(revision 35505)
+++ blenkernel/intern/mesh.c	(working copy)
@@ -1280,19 +1280,35 @@
 
 	for (i=0; i<numFaces; i++) {
 		MFace *mf= &mfaces[i];
-		float *f_no= &fnors[i*3];
+		if((mf->flag&ME_SMOOTH)!=0)
+		{
+			float *f_no= &fnors[i*3];
 
-		if (mf->v4)
-			normal_quad_v3( f_no,mverts[mf->v1].co, mverts[mf->v2].co,
mverts[mf->v3].co, mverts[mf->v4].co);
-		else
-			normal_tri_v3( f_no,mverts[mf->v1].co, mverts[mf->v2].co,
mverts[mf->v3].co);
+			if (mf->v4)
+				normal_quad_v3( f_no,mverts[mf->v1].co, mverts[mf->v2].co,
mverts[mf->v3].co, mverts[mf->v4].co);
+			else
+				normal_tri_v3( f_no,mverts[mf->v1].co, mverts[mf->v2].co,
mverts[mf->v3].co);
 		
-		add_v3_v3(tnorms[mf->v1], f_no);
-		add_v3_v3(tnorms[mf->v2], f_no);
-		add_v3_v3(tnorms[mf->v3], f_no);
-		if (mf->v4)
-			add_v3_v3(tnorms[mf->v4], f_no);
+			add_v3_v3(tnorms[mf->v1], f_no);
+			add_v3_v3(tnorms[mf->v2], f_no);
+			add_v3_v3(tnorms[mf->v3], f_no);
+			if (mf->v4)
+				add_v3_v3(tnorms[mf->v4], f_no);
+		}
 	}
+	for (i=0; i<numFaces; i++) {
+		MFace *mf= &mfaces[i];
+		if((mf->flag&ME_SMOOTH)==0)
+		{
+			float *f_no= &fnors[i*3];
+			if(tnorms[mf->v1][0]==0 && tnorms[mf->v1][1]==0 && tnorms[mf->v1][2]==0)
VECCOPY(tnorms[mf->v1], f_no);
+			if(tnorms[mf->v2][0]==0 && tnorms[mf->v2][1]==0 && tnorms[mf->v2][2]==0)
VECCOPY(tnorms[mf->v2], f_no);
+			if(tnorms[mf->v3][0]==0 && tnorms[mf->v3][1]==0 && tnorms[mf->v3][2]==0)
VECCOPY(tnorms[mf->v3], f_no);
+			if(mf->v4)
+				if(tnorms[mf->v4][0]==0 && tnorms[mf->v4][1]==0 && tnorms[mf->v4][2]==0)
VECCOPY(tnorms[mf->v4], f_no);
+		}
+	}
+
 	for (i=0; i<numVerts; i++) {
 		MVert *mv= &mverts[i];
 		float *no= tnorms[i];
Index: blenkernel/BKE_DerivedMesh.h
===================================================================
--- blenkernel/BKE_DerivedMesh.h	(revision 35505)
+++ blenkernel/BKE_DerivedMesh.h	(working copy)
@@ -206,9 +206,15 @@
 	/* Fill the array (of length .getNumVerts()) with all vertex locations */
 	void (*getVertCos)(DerivedMesh *dm, float (*cos_r)[3]);
 
-	/* Get vertex normal, undefined if index is not valid */
+	/* Get smooth vertex normal, undefined if index is not valid */
 	void (*getVertNo)(DerivedMesh *dm, int index, float no_r[3]);
 
+	/* Get real vertex normal, takes faces set to hard into account */
+	void (*getRealVertNo)(DerivedMesh *dm, int face_index, int vert_num, float
no_r[3]);
+
+	/* Get face normal, returns the length before normalization */
+	float (*getFaceNo)(DerivedMesh *dm, int index, float no_r[3]);
+
 	/* Get a map of vertices to faces
 	 */
 	struct ListBase *(*getFaceMap)(struct Object *ob, DerivedMesh *dm);
Index: editors/mesh/editmesh_lib.c
===================================================================
--- editors/mesh/editmesh_lib.c	(revision 35505)
+++ editors/mesh/editmesh_lib.c	(working copy)
@@ -2007,18 +2007,32 @@
 	}
 
 	for(efa= em->faces.first; efa; efa=efa->next) {
-		if(efa->v4) {
-			normal_quad_v3( efa->n,efa->v1->co, efa->v2->co, efa->v3->co, efa->v4->co);
-			cent_quad_v3(efa->cent, efa->v1->co, efa->v2->co, efa->v3->co, efa->v4->co);
-			add_v3_v3(efa->v4->no, efa->n);
+		if((efa->flag&ME_SMOOTH)!=0)
+		{
+			if(efa->v4) {
+				normal_quad_v3( efa->n,efa->v1->co, efa->v2->co, efa->v3->co, efa->v4->co);
+				cent_quad_v3(efa->cent, efa->v1->co, efa->v2->co, efa->v3->co,
efa->v4->co);
+				add_v3_v3(efa->v4->no, efa->n);
+			}
+			else {
+				normal_tri_v3( efa->n,efa->v1->co, efa->v2->co, efa->v3->co);
+				cent_tri_v3(efa->cent, efa->v1->co, efa->v2->co, efa->v3->co);
+			}
+			add_v3_v3(efa->v1->no, efa->n);
+			add_v3_v3(efa->v2->no, efa->n);
+			add_v3_v3(efa->v3->no, efa->n);
 		}
-		else {
-			normal_tri_v3( efa->n,efa->v1->co, efa->v2->co, efa->v3->co);
-			cent_tri_v3(efa->cent, efa->v1->co, efa->v2->co, efa->v3->co);
+	}
+
+	for(efa= em->faces.first; efa; efa=efa->next) {
+		if((efa->flag&ME_SMOOTH)==0)
+		{
+			if(efa->v1->no[0]==0 && efa->v1->no[1]==0 && efa->v1->no[2]==0)
VECCOPY(efa->v1->no, efa->n);
+			if(efa->v2->no[0]==0 && efa->v2->no[1]==0 && efa->v2->no[2]==0)
VECCOPY(efa->v2->no, efa->n);
+			if(efa->v3->no[0]==0 && efa->v3->no[1]==0 && efa->v3->no[2]==0)
VECCOPY(efa->v3->no, efa->n);
+			if(efa->v4)
+				if(efa->v4->no[0]==0 && efa->v4->no[1]==0 && efa->v4->no[2]==0)
VECCOPY(efa->v4->no, efa->n);
 		}
-		add_v3_v3(efa->v1->no, efa->n);
-		add_v3_v3(efa->v2->no, efa->n);
-		add_v3_v3(efa->v3->no, efa->n);
 	}
 
 	/* following Mesh convention; we use vertex coordinate itself for normal in
this case */

[sparky, 2011-03-15 17:37:26]

Index: source/blender/render/intern/source/convertblender.c
===================================================================
--- source/blender/render/intern/source/convertblender.c	(revision 35505)
+++ source/blender/render/intern/source/convertblender.c	(working copy)
@@ -499,6 +499,11 @@
 }
 
 
+
+/****************************************************************
+************ tangent space generation interface *****************
+****************************************************************/
+
 typedef struct
 {
 	ObjectRen *obr;
@@ -599,7 +604,6 @@
 			VertRen *v3= vlr->v3;
 			VertRen *v4= vlr->v4;
 			float n1[3], n2[3], n3[3], n4[3];
-			float fac1, fac2, fac3, fac4=0.0f;
 			
 			sub_v3_v3v3(n1, v2->co, v1->co);
 			normalize_v3(n1);
@@ -608,10 +612,6 @@
 			if(v4==NULL) {
 				sub_v3_v3v3(n3, v1->co, v3->co);
 				normalize_v3(n3);
-
-				fac1= saacos(-n1[0]*n3[0]-n1[1]*n3[1]-n1[2]*n3[2]);
-				fac2= saacos(-n1[0]*n2[0]-n1[1]*n2[1]-n1[2]*n2[2]);
-				fac3= saacos(-n2[0]*n3[0]-n2[1]*n3[1]-n2[2]*n3[2]);
 			}
 			else {
 				sub_v3_v3v3(n3, v4->co, v3->co);
@@ -619,27 +619,22 @@
 				sub_v3_v3v3(n4, v1->co, v4->co);
 				normalize_v3(n4);
 
-				fac1= saacos(-n4[0]*n1[0]-n4[1]*n1[1]-n4[2]*n1[2]);
-				fac2= saacos(-n1[0]*n2[0]-n1[1]*n2[1]-n1[2]*n2[2]);
-				fac3= saacos(-n2[0]*n3[0]-n2[1]*n3[1]-n2[2]*n3[2]);
-				fac4= saacos(-n3[0]*n4[0]-n3[1]*n4[1]-n3[2]*n4[2]);
-
-				v4->n[0] +=fac4*vlr->n[0];
-				v4->n[1] +=fac4*vlr->n[1];
-				v4->n[2] +=fac4*vlr->n[2];
+				v4->n[0] +=vlr->n[0];
+				v4->n[1] +=vlr->n[1];
+				v4->n[2] +=vlr->n[2];
 			}
 
-			v1->n[0] +=fac1*vlr->n[0];
-			v1->n[1] +=fac1*vlr->n[1];
-			v1->n[2] +=fac1*vlr->n[2];
+			v1->n[0] +=vlr->n[0];
+			v1->n[1] +=vlr->n[1];
+			v1->n[2] +=vlr->n[2];
 
-			v2->n[0] +=fac2*vlr->n[0];
-			v2->n[1] +=fac2*vlr->n[1];
-			v2->n[2] +=fac2*vlr->n[2];
+			v2->n[0] +=vlr->n[0];
+			v2->n[1] +=vlr->n[1];
+			v2->n[2] +=vlr->n[2];
 
-			v3->n[0] +=fac3*vlr->n[0];
-			v3->n[1] +=fac3*vlr->n[1];
-			v3->n[2] +=fac3*vlr->n[2];
+			v3->n[0] +=vlr->n[0];
+			v3->n[1] +=vlr->n[1];
+			v3->n[2] +=vlr->n[2];
 			
 		}
 		if(do_nmap_tangent || do_tangent) {
@@ -3236,6 +3231,7 @@
 	int a, a1, ok, vertofs;
 	int end, do_autosmooth=0, totvert = 0;
 	int use_original_normals= 0;
+	int iCalcNormalsInRender = 0;	// false by default
 
 	me= ob->data;
 
@@ -3313,6 +3309,7 @@
 	
 	ma= give_render_material(re, ob, 1);
 
+
 	if(ma->material_type == MA_TYPE_HALO) {
 		make_render_halos(re, obr, me, totvert, mvert, ma, orco);
 	}
@@ -3321,8 +3318,17 @@
 		for(a=0; a<totvert; a++, mvert++) {
 			ver= RE_findOrAddVert(obr, obr->totvert++);
 			VECCOPY(ver->co, mvert->co);
+			ver->n[0]=mvert->no[0];
+			ver->n[1]=mvert->no[1];
+			ver->n[2]=mvert->no[2];
+			//dm->getVertNo(dm, a, ver->n);
 			if(do_autosmooth==0)	/* autosmooth on original unrotated data to prevent
differences between frames */
+			{
 				mul_m4_v3(mat, ver->co);
+				mul_transposed_m3_v3(imat, ver->n);
+				normalize_v3(ver->n);
+				negate_v3(ver->n);
+			}
   
 			if(orco) {
 				ver->orco= orco;
@@ -3370,6 +3376,9 @@
 					end= dm->getNumFaces(dm);
 					mface= dm->getFaceArray(dm);
 
+					if(need_nmap_tangent!=0 && CustomData_get_layer_index(&dm->faceData,
CD_TANGENT) == -1)
+						DM_add_tangent_layer(dm);
+					
 					for(a=0; a<end; a++, mface++) {
 						int v1, v2, v3, v4, flag;
 						
@@ -3400,10 +3409,14 @@
 									len= normal_tri_v3( vlr->n,mv[mf->v3].co, mv[mf->v2].co,
mv[mf->v1].co);
 							}
 							else {
-								if(vlr->v4) 
+								/*if(vlr->v4) 
 									len= normal_quad_v3( vlr->n,vlr->v4->co, vlr->v3->co, vlr->v2->co,
vlr->v1->co);
 								else 
-									len= normal_tri_v3( vlr->n,vlr->v3->co, vlr->v2->co, vlr->v1->co);
+									len= normal_tri_v3( vlr->n,vlr->v3->co, vlr->v2->co, vlr->v1->co);*/
+								len = dm->getFaceNo(dm, a, vlr->n);
+								mul_transposed_m3_v3(imat, vlr->n);
+								len *= normalize_v3(vlr->n);
+								negate_v3(vlr->n);
 							}
 
 							vlr->mat= ma;
@@ -3415,7 +3428,7 @@
 								CustomDataLayer *layer;
 								MTFace *mtface, *mtf;
 								MCol *mcol, *mc;
-								int index, mtfn= 0, mcn= 0;
+								int index, mtfn= 0, mcn= 0, mtng=0;
 								char *name;
 
 								for(index=0; index<dm->faceData.totlayer; index++) {
@@ -3432,6 +3445,22 @@
 										mcol= (MCol*)layer->data;
 										memcpy(mc, &mcol[a*4], sizeof(MCol)*4);
 									}
+									else if(layer->type == CD_TANGENT && mtng < 1)
+									{
+										if(need_nmap_tangent!=0)
+										{
+											const float * tangent = (const float *) layer->data;
+											int t;
+											int nr_verts = v4!=0 ? 4 : 3;
+											float * ftang = RE_vlakren_get_nmap_tangent(obr, vlr, 1);
+											for(t=0; t<nr_verts; t++)
+											{
+												QUATCOPY(ftang+t*4, tangent+a*16+t*4);
+												mul_mat3_m4_v3(mat, ftang+t*4);
+												normalize_v3(ftang+t*4);
+											}
+										}
+									}
 								}
 							}
 						}
@@ -3447,6 +3476,7 @@
 				MEdge *medge;
 				struct edgesort *edgetable;
 				int totedge= 0;
+				iCalcNormalsInRender=1;
 				
 				medge= dm->getEdgeArray(dm);
 				
@@ -3489,6 +3519,7 @@
 	
 	if(!timeoffset) {
 		if (test_for_displace(re, ob ) ) {
+			iCalcNormalsInRender = 1;
 			calc_vertexnormals(re, obr, 0, 0);
 			if(do_autosmooth)
 				do_displacement(re, obr, mat, imat);
@@ -3497,11 +3528,13 @@
 		}
 
 		if(do_autosmooth) {
+			iCalcNormalsInRender = 1;
 			autosmooth(re, obr, mat, me->smoothresh);
 		}
 
-		calc_vertexnormals(re, obr, need_tangent, need_nmap_tangent);
-
+		if(iCalcNormalsInRender!=0 || need_tangent!=0)
+			calc_vertexnormals(re, obr, need_tangent, need_nmap_tangent);
+		
 		if(need_stress)
 			calc_edge_stress(re, obr, me);
 	}
Index: source/blender/blenkernel/intern/cdderivedmesh.c
===================================================================
--- source/blender/blenkernel/intern/cdderivedmesh.c	(revision 35505)
+++ source/blender/blenkernel/intern/cdderivedmesh.c	(working copy)
@@ -179,6 +179,54 @@
 	no_r[2] = no[2]/32767.f;
 }
 
+static void cdDM_getRealVertNo(DerivedMesh *dm, int face_index, int vert_num,
float no_r[3])
+{
+	CDDerivedMesh *cddm = (CDDerivedMesh*) dm;
+	const int smoothnormal = (cddm->mface[face_index].flag & ME_SMOOTH);
+	if(!smoothnormal)	// flat
+	{
+		dm->getFaceNo(dm, face_index, no_r);
+	}
+	else
+	{
+		unsigned int indices[] = {	cddm->mface[face_index].v1,
cddm->mface[face_index].v2,
+								cddm->mface[face_index].v3, cddm->mface[face_index].v4 };
+		dm->getVertNo(dm, indices[vert_num], no_r);
+		normalize_v3(no_r);
+	}
+}
+
+static float cdDM_getFaceNo(DerivedMesh *dm, int index, float no_r[3])
+{
+	float * nors = dm->getFaceDataArray(dm, CD_NORMAL);
+	float len;
+
+	if(nors)
+	{
+		VECCOPY(no_r, &nors[3*index]);
+		len = len_v3(no_r);
+	}
+	else
+	{
+		CDDerivedMesh *cddm = (CDDerivedMesh*) dm;
+		float * p0, * p1, * p2;
+		const int iGetNrVerts = cddm->mface[index].v4!=0 ? 4 : 3;
+		unsigned int indices[] = {	cddm->mface[index].v1, cddm->mface[index].v2,
+								cddm->mface[index].v3, cddm->mface[index].v4 };
+		p0 = cddm->mvert[indices[0]].co; p1 = cddm->mvert[indices[1]].co; p2 =
cddm->mvert[indices[2]].co;
+		if(iGetNrVerts==4)
+		{
+			float * p3 = cddm->mvert[indices[3]].co;
+			len = normal_quad_v3( no_r, p0, p1, p2, p3);
+		}
+		else {
+			len = normal_tri_v3(no_r, p0, p1, p2);
+		}
+
+	}
+	return len;
+}
+
 static ListBase *cdDM_getFaceMap(Object *ob, DerivedMesh *dm)
 {
 	CDDerivedMesh *cddm = (CDDerivedMesh*) dm;
@@ -1480,6 +1528,8 @@
 	dm->getVertCos = cdDM_getVertCos;
 	dm->getVertCo = cdDM_getVertCo;
 	dm->getVertNo = cdDM_getVertNo;
+	dm->getRealVertNo = cdDM_getRealVertNo;
+	dm->getFaceNo = cdDM_getFaceNo;
 
 	dm->getPBVH = cdDM_getPBVH;
 	dm->getFaceMap = cdDM_getFaceMap;
@@ -1798,7 +1848,7 @@
 	int i;
 	int numVerts = dm->numVertData;
 	int numFaces = dm->numFaceData;
-	MFace *mf;
+	MFace *mfaces;
 	MVert *mv;
 
 	if(numVerts == 0) return;
@@ -1817,22 +1867,41 @@
 										 NULL, dm->numFaceData);
 
 	/* calculate face normals and add to vertex normals */
-	mf = CDDM_get_faces(dm);
-	for(i = 0; i < numFaces; i++, mf++) {
-		float *f_no = face_nors[i];
+	mfaces = CDDM_get_faces(dm);
+	for(i = 0; i < numFaces; i++) {
+		MFace * mf = &mfaces[i];
+		if((mf->flag&ME_SMOOTH)!=0)
+		{
+			float *f_no = face_nors[i];
 
-		if(mf->v4)
-			normal_quad_v3( f_no,mv[mf->v1].co, mv[mf->v2].co, mv[mf->v3].co,
mv[mf->v4].co);
-		else
-			normal_tri_v3( f_no,mv[mf->v1].co, mv[mf->v2].co, mv[mf->v3].co);
+			if(mf->v4)
+				normal_quad_v3( f_no,mv[mf->v1].co, mv[mf->v2].co, mv[mf->v3].co,
mv[mf->v4].co);
+			else
+				normal_tri_v3( f_no,mv[mf->v1].co, mv[mf->v2].co, mv[mf->v3].co);
 		
-		add_v3_v3(temp_nors[mf->v1], f_no);
-		add_v3_v3(temp_nors[mf->v2], f_no);
-		add_v3_v3(temp_nors[mf->v3], f_no);
-		if(mf->v4)
-			add_v3_v3(temp_nors[mf->v4], f_no);
+			add_v3_v3(temp_nors[mf->v1], f_no);
+			add_v3_v3(temp_nors[mf->v2], f_no);
+			add_v3_v3(temp_nors[mf->v3], f_no);
+			if(mf->v4)
+				add_v3_v3(temp_nors[mf->v4], f_no);
+		}
 	}
 
+	for(i = 0; i < numFaces; i++) {
+		MFace * mf = &mfaces[i];
+		if((mf->flag&ME_SMOOTH)!=0)
+		{
+			float *f_no = face_nors[i];
+
+			if(temp_nors[mf->v1][0]==0 && temp_nors[mf->v1][1]==0 &&
temp_nors[mf->v1][2]==0) VECCOPY(temp_nors[mf->v1], f_no);
+			if(temp_nors[mf->v2][0]==0 && temp_nors[mf->v2][1]==0 &&
temp_nors[mf->v2][2]==0) VECCOPY(temp_nors[mf->v2], f_no);
+			if(temp_nors[mf->v3][0]==0 && temp_nors[mf->v3][1]==0 &&
temp_nors[mf->v3][2]==0) VECCOPY(temp_nors[mf->v3], f_no);
+			if(mf->v4)
+				if(temp_nors[mf->v4][0]==0 && temp_nors[mf->v4][1]==0 &&
temp_nors[mf->v4][2]==0) VECCOPY(temp_nors[mf->v4], f_no);
+		}
+	}
+
+
 	/* normalize vertex normals and assign */
 	for(i = 0; i < numVerts; i++, mv++) {
 		float *no = temp_nors[i];
Index: source/blender/blenkernel/intern/mesh.c
===================================================================
--- source/blender/blenkernel/intern/mesh.c	(revision 35505)
+++ source/blender/blenkernel/intern/mesh.c	(working copy)
@@ -1280,19 +1280,35 @@
 
 	for (i=0; i<numFaces; i++) {
 		MFace *mf= &mfaces[i];
-		float *f_no= &fnors[i*3];
+		if((mf->flag&ME_SMOOTH)!=0)
+		{
+			float *f_no= &fnors[i*3];
 
-		if (mf->v4)
-			normal_quad_v3( f_no,mverts[mf->v1].co, mverts[mf->v2].co,
mverts[mf->v3].co, mverts[mf->v4].co);
-		else
-			normal_tri_v3( f_no,mverts[mf->v1].co, mverts[mf->v2].co,
mverts[mf->v3].co);
+			if (mf->v4)
+				normal_quad_v3( f_no,mverts[mf->v1].co, mverts[mf->v2].co,
mverts[mf->v3].co, mverts[mf->v4].co);
+			else
+				normal_tri_v3( f_no,mverts[mf->v1].co, mverts[mf->v2].co,
mverts[mf->v3].co);
 		
-		add_v3_v3(tnorms[mf->v1], f_no);
-		add_v3_v3(tnorms[mf->v2], f_no);
-		add_v3_v3(tnorms[mf->v3], f_no);
-		if (mf->v4)
-			add_v3_v3(tnorms[mf->v4], f_no);
+			add_v3_v3(tnorms[mf->v1], f_no);
+			add_v3_v3(tnorms[mf->v2], f_no);
+			add_v3_v3(tnorms[mf->v3], f_no);
+			if (mf->v4)
+				add_v3_v3(tnorms[mf->v4], f_no);
+		}
 	}
+	for (i=0; i<numFaces; i++) {
+		MFace *mf= &mfaces[i];
+		if((mf->flag&ME_SMOOTH)==0)
+		{
+			float *f_no= &fnors[i*3];
+			if(tnorms[mf->v1][0]==0 && tnorms[mf->v1][1]==0 && tnorms[mf->v1][2]==0)
VECCOPY(tnorms[mf->v1], f_no);
+			if(tnorms[mf->v2][0]==0 && tnorms[mf->v2][1]==0 && tnorms[mf->v2][2]==0)
VECCOPY(tnorms[mf->v2], f_no);
+			if(tnorms[mf->v3][0]==0 && tnorms[mf->v3][1]==0 && tnorms[mf->v3][2]==0)
VECCOPY(tnorms[mf->v3], f_no);
+			if(mf->v4)
+				if(tnorms[mf->v4][0]==0 && tnorms[mf->v4][1]==0 && tnorms[mf->v4][2]==0)
VECCOPY(tnorms[mf->v4], f_no);
+		}
+	}
+
 	for (i=0; i<numVerts; i++) {
 		MVert *mv= &mverts[i];
 		float *no= tnorms[i];
Index: source/blender/blenkernel/BKE_DerivedMesh.h
===================================================================
--- source/blender/blenkernel/BKE_DerivedMesh.h	(revision 35505)
+++ source/blender/blenkernel/BKE_DerivedMesh.h	(working copy)
@@ -206,9 +206,15 @@
 	/* Fill the array (of length .getNumVerts()) with all vertex locations */
 	void (*getVertCos)(DerivedMesh *dm, float (*cos_r)[3]);
 
-	/* Get vertex normal, undefined if index is not valid */
+	/* Get smooth vertex normal, undefined if index is not valid */
 	void (*getVertNo)(DerivedMesh *dm, int index, float no_r[3]);
 
+	/* Get real vertex normal, takes faces set to hard into account */
+	void (*getRealVertNo)(DerivedMesh *dm, int face_index, int vert_num, float
no_r[3]);
+
+	/* Get face normal, returns the length before normalization */
+	float (*getFaceNo)(DerivedMesh *dm, int index, float no_r[3]);
+
 	/* Get a map of vertices to faces
 	 */
 	struct ListBase *(*getFaceMap)(struct Object *ob, DerivedMesh *dm);
Index: source/blender/editors/mesh/editmesh_lib.c
===================================================================
--- source/blender/editors/mesh/editmesh_lib.c	(revision 35505)
+++ source/blender/editors/mesh/editmesh_lib.c	(working copy)
@@ -2007,18 +2007,32 @@
 	}
 
 	for(efa= em->faces.first; efa; efa=efa->next) {
-		if(efa->v4) {
-			normal_quad_v3( efa->n,efa->v1->co, efa->v2->co, efa->v3->co, efa->v4->co);
-			cent_quad_v3(efa->cent, efa->v1->co, efa->v2->co, efa->v3->co, efa->v4->co);
-			add_v3_v3(efa->v4->no, efa->n);
+		if((efa->flag&ME_SMOOTH)!=0)
+		{
+			if(efa->v4) {
+				normal_quad_v3( efa->n,efa->v1->co, efa->v2->co, efa->v3->co, efa->v4->co);
+				cent_quad_v3(efa->cent, efa->v1->co, efa->v2->co, efa->v3->co,
efa->v4->co);
+				add_v3_v3(efa->v4->no, efa->n);
+			}
+			else {
+				normal_tri_v3( efa->n,efa->v1->co, efa->v2->co, efa->v3->co);
+				cent_tri_v3(efa->cent, efa->v1->co, efa->v2->co, efa->v3->co);
+			}
+			add_v3_v3(efa->v1->no, efa->n);
+			add_v3_v3(efa->v2->no, efa->n);
+			add_v3_v3(efa->v3->no, efa->n);
 		}
-		else {
-			normal_tri_v3( efa->n,efa->v1->co, efa->v2->co, efa->v3->co);
-			cent_tri_v3(efa->cent, efa->v1->co, efa->v2->co, efa->v3->co);
+	}
+
+	for(efa= em->faces.first; efa; efa=efa->next) {
+		if((efa->flag&ME_SMOOTH)==0)
+		{
+			if(efa->v1->no[0]==0 && efa->v1->no[1]==0 && efa->v1->no[2]==0)
VECCOPY(efa->v1->no, efa->n);
+			if(efa->v2->no[0]==0 && efa->v2->no[1]==0 && efa->v2->no[2]==0)
VECCOPY(efa->v2->no, efa->n);
+			if(efa->v3->no[0]==0 && efa->v3->no[1]==0 && efa->v3->no[2]==0)
VECCOPY(efa->v3->no, efa->n);
+			if(efa->v4)
+				if(efa->v4->no[0]==0 && efa->v4->no[1]==0 && efa->v4->no[2]==0)
VECCOPY(efa->v4->no, efa->n);
 		}
-		add_v3_v3(efa->v1->no, efa->n);
-		add_v3_v3(efa->v2->no, efa->n);
-		add_v3_v3(efa->v3->no, efa->n);
 	}
 
 	/* following Mesh convention; we use vertex coordinate itself for normal in
this case */

[dfelinto, 2011-03-15 17:54:36]

To add a new patch you need to click in the link between "Patch Set 1" and
"Messages". It's called "Add Another Patch Set".

[sparky, 2011-03-15 18:01:04]

this patch fixes the same bug caused by multiple implementations but has no code
sharing.

[brechtvl, 2011-03-15 18:31:12]

Some comments on the code.

[brechtvl, 2011-03-15 18:31:32]

http://codereview.appspot.com/4280049/diff/8003/source/blender/blenkernel/BKE...
File source/blender/blenkernel/BKE_DerivedMesh.h (right):

http://codereview.appspot.com/4280049/diff/8003/source/blender/blenkernel/BKE...
source/blender/blenkernel/BKE_DerivedMesh.h:213: void
(*getRealVertNo)(DerivedMesh *dm, int face_index, int vert_num, float no_r[3]);
I guess this function is no longer needed?

http://codereview.appspot.com/4280049/diff/8003/source/blender/blenkernel/BKE...
source/blender/blenkernel/BKE_DerivedMesh.h:216: float (*getFaceNo)(DerivedMesh
*dm, int index, float no_r[3]);
If you're going to use this, it would need to be implemented for all types of
derived meshes. Probably it's easier to drop it.

http://codereview.appspot.com/4280049/diff/8003/source/blender/blenkernel/int...
File source/blender/blenkernel/intern/cdderivedmesh.c (right):

http://codereview.appspot.com/4280049/diff/8003/source/blender/blenkernel/int...
source/blender/blenkernel/intern/cdderivedmesh.c:1869: /* calculate face normals
and add to vertex normals */
We can use mesh_calc_normals here instead, since the arrays are the same. That's
one duplicate less.

http://codereview.appspot.com/4280049/diff/8003/source/blender/blenkernel/int...
File source/blender/blenkernel/intern/mesh.c (right):

http://codereview.appspot.com/4280049/diff/8003/source/blender/blenkernel/int...
source/blender/blenkernel/intern/mesh.c:1276: {
It would be good to have a comment here to keep it in sync with
recalc_editnormals and calc_vertexnormals, and do the same in those functions.

http://codereview.appspot.com/4280049/diff/8003/source/blender/blenkernel/int...
source/blender/blenkernel/intern/mesh.c:1284: {
I'm going to be pedantic here, but please follow the coding style from the
file/function as a minimum. Put { on the same line as if statement here and in
other places.

http://codereview.appspot.com/4280049/diff/8003/source/blender/blenkernel/int...
source/blender/blenkernel/intern/mesh.c:1304: if(tnorms[mf->v1][0]==0 &&
tnorms[mf->v1][1]==0 && tnorms[mf->v1][2]==0) VECCOPY(tnorms[mf->v1], f_no);
Can use is_zero_v3 here and in other places. Also copy_v3_v3 is preferred over
VECCOPY.

http://codereview.appspot.com/4280049/diff/8003/source/blender/render/intern/...
File source/blender/render/intern/source/convertblender.c (right):

http://codereview.appspot.com/4280049/diff/8003/source/blender/render/intern/...
source/blender/render/intern/source/convertblender.c:606: float n1[3], n2[3],
n3[3], n4[3];
Computing n1, n2, n3, n4 seems no longer necessary?

http://codereview.appspot.com/4280049/diff/8003/source/blender/render/intern/...
source/blender/render/intern/source/convertblender.c:3234: int
iCalcNormalsInRender = 0;	// false by default
Please follow coding style, e.g rename to recalc_normals.

http://codereview.appspot.com/4280049/diff/8003/source/blender/render/intern/...
source/blender/render/intern/source/convertblender.c:3324: //dm->getVertNo(dm,
a, ver->n);
Best to use normal_short_to_float_v3 here. In case of autosmooth it wouldn't be
normalized otherwise.

http://codereview.appspot.com/4280049/diff/8003/source/blender/render/intern/...
source/blender/render/intern/source/convertblender.c:3419: negate_v3(vlr->n);
As mentioned before, perhaps easiest to recalculate here and drop getFaceNo.

[brechtvl, 2011-03-15 18:39:32]

http://codereview.appspot.com/4280049/diff/8003/source/blender/blenkernel/int...
File source/blender/blenkernel/intern/cdderivedmesh.c (right):

http://codereview.appspot.com/4280049/diff/8003/source/blender/blenkernel/int...
source/blender/blenkernel/intern/cdderivedmesh.c:1869: /* calculate face normals
and add to vertex normals */
Also, it seems the last parameter in mesh_calc_normals is unused, so it can be
changed to not allocate the face normals but fill them in in the existing array
and avoid memcpy.

[sparky, 2011-03-15 18:51:07]

Now that my code has mostly been taken out I think I will leave it to
someone else to clean up some day in
the vertex normal calculation functions. But I can apply the changes you're
talking about to convertblender.c

In regards to getRealVertNo I think dropping it is a bad idea since it's
going to be needed for
the exporters soon but if you don't want it unless it's also implemented on
the other mesh types
I'll let it go. I can leave it to jesterking to put it in again when he's
ready.




On Tue, Mar 15, 2011 at 11:39 AM, <brechtvanlommel@gmail.com> wrote:

>
>
>
http://codereview.appspot.com/4280049/diff/8003/source/blender/blenkernel/int...
> File source/blender/blenkernel/intern/cdderivedmesh.c (right):
>
>
>
http://codereview.appspot.com/4280049/diff/8003/source/blender/blenkernel/int...
> source/blender/blenkernel/intern/cdderivedmesh.c:1869: /* calculate face
> normals and add to vertex normals */
> Also, it seems the last parameter in mesh_calc_normals is unused, so it
> can be changed to not allocate the face normals but fill them in in the
> existing array and avoid memcpy.
>
>
> http://codereview.appspot.com/4280049/
>

[sparky, 2011-03-16 14:50:26]

std fixes

[sparky, 2011-03-17 03:36:05]

forgot to move copy to norm inside the if(). But that's incl. in this patch.

[brechtvl, 2011-03-17 09:19:00]

After some tests with various models, it seems that angle weighted really is
much better in some cases. I can work on the patch myself to add a mesh option
for both methods.

One problem is we're still breaking compatibility either way, either it's in the
render engine, or it's in the game engine. Also what the default should be I'm
not sure, depends if angle weighted is really slow or not, will test.

[sparky, 2011-03-17 14:30:09]

The renderer is much more used and people using the renderer are much more
sensitive about
looks. The people using BGE most likely couldn't tell the diff and even if
they can getting
the better option is a good thing.
So what I would do is make the default setting angle weighted and then of
course have the modeler and the renderer both track the setting so they are
in sync.



On Thu, Mar 17, 2011 at 2:19 AM, <brechtvanlommel@gmail.com> wrote:

> After some tests with various models, it seems that angle weighted
> really is much better in some cases. I can work on the patch myself to
> add a mesh option for both methods.
>
> One problem is we're still breaking compatibility either way, either
> it's in the render engine, or it's in the game engine. Also what the
> default should be I'm not sure, depends if angle weighted is really slow
> or not, will test.
>
>
> http://codereview.appspot.com/4280049/
>

[sparky, 2011-03-19 14:21:34]

angle weighted plus removed a bug frm when I went frm patch 1 to patch 2 and
removed the code sharin

[sparky, 2011-03-19 14:53:36]

oops. must relocate one of the normalizes inside AccAtVertexNormals()

[brechtvl, 2011-03-19 20:00:30]

Looks ok to me, I've made some changes (but can't seem to attach the patch
here):
http://users.telenet.be/blendix/angle_weighted_vertex_normal.diff

This adds a version patch to recalculate normals on older files, and also has
some code style changes. Too late for me to commit today, can do it tomorrow
unless you find any more issues.

[sparky, 2011-03-19 21:19:47]

Brilliant! Thanks a lot.

Cheers,

Morten.



On Sat, Mar 19, 2011 at 1:00 PM, <brechtvanlommel@gmail.com> wrote:

> Looks ok to me, I've made some changes (but can't seem to attach the
> patch here):
> http://users.telenet.be/blendix/angle_weighted_vertex_normal.diff
>
> This adds a version patch to recalculate normals on older files, and
> also has some code style changes. Too late for me to commit today, can
> do it tomorrow unless you find any more issues.
>
>
> http://codereview.appspot.com/4280049/
>

[brechtvl, 2011-03-20 13:50:34]

Committed.