Sign in to follow this  
deadstar

Calculating vertex normals - I'm doing something wrong

Recommended Posts

deadstar    536
I've been working at this damn problem on and off for months now. It's time I sat down and finally fixed it. The first source is the optimised version of the slow code below. The lighting on my terrain seems non-existant, it's pure white.
		//For calculating face normals
		SYM_VECTOR3 Point[3];

		//Loop through faces
		for (int f = 0; f < Face.size(); f++)
		{
			//Convert vertices of triangle to vectors
			Point[0] = Vertex[ Face[f].a ];
			Point[1] = Vertex[ Face[f].b ];
			Point[2] = Vertex[ Face[f].c ];

			//Calculate face normal
			Face[f].Normal = GetFaceNormal(Point[0], Point[1], Point[2]);

			//Add the face normal to the vertex normal of each vertex in this face
			Vertex[ Face[f].a ].Normal += Face[f].Normal;
			Vertex[ Face[f].b ].Normal += Face[f].Normal;
			Vertex[ Face[f].c ].Normal += Face[f].Normal;
		}

		//Loop through vertices
		for (int v = 0; v < Vertex.size(); v++)
		{
			//Normalise the summed normals
			Vertex[v].Normal.Normalise();
		}

My old code (which took several MINUTES to calculate for each model) worked ok. Terrain was smooth.

	//CALCULATE FACE NORMALS

	//For calculating normals
	SYM_VECTOR Point[3];

	//Calculate normals
	for (int i = 0; i < Face.size(); i++)
	{
		Point[0].i = Vertex[Face[i].a].x;
		Point[0].j = Vertex[Face[i].a].y;
		Point[0].k = Vertex[Face[i].a].z;

		Point[1].i = Vertex[Face[i].b].x;
		Point[1].j = Vertex[Face[i].b].y;
		Point[1].k = Vertex[Face[i].b].z;

		Point[2].i = Vertex[Face[i].c].x;
		Point[2].j = Vertex[Face[i].c].y;
		Point[2].k = Vertex[Face[i].c].z;

		//Calculate normal
		Face[i].Normal = GetFaceNormal(Point[2], Point[1], Point[0]);

		//Convert to array for OpenGL use
		Face[i].NormalArray[0] = Face[i].Normal.i;
		Face[i].NormalArray[1] = Face[i].Normal.j;
		Face[i].NormalArray[2] = Face[i].Normal.k;
	}



	//CALCULATE VERTEX NORMALS

	//Allocate a temp normal to store for calculation
	SYM_VECTOR TempNormal;

	//Loop through each vertex
	for (int v = 0; v < Vertex.size(); v++)
	{
		//Reset the temp normal
		///TODO: Implement '=' by scalar
		TempNormal.i = 0;
		TempNormal.j = 0;
		TempNormal.k = 0;

		//Keep count of normals found
		int Incident = 0;

		//Loop through each face
		for (int f = 0; f < Face.size(); f++)
		{
			//See if face uses the vertex
			if ((Vertex[Face[f].a] == Vertex[v]) || (Vertex[Face[f].b] == Vertex[v]) || (Vertex[Face[f].c] == Vertex[v]))
			{
				//This face uses the vertex, so store it's normal
				TempNormal += Face[f].Normal;

				//Count it
				Incident++;
			}
		}

		//Check if any instances have been found
		if (Incident != 0)
		{
			//Get average normal
			TempNormal = TempNormal / Incident;

			//Normalise the averaged normal
			Normalise(TempNormal);

			//Store it
			Vertex[v].Normal = TempNormal;

			//Convert to array for OpenGL use
			Vertex[v].NormalArray[0] = Vertex[v].Normal.i;
			Vertex[v].NormalArray[1] = Vertex[v].Normal.j;
			Vertex[v].NormalArray[2] = Vertex[v].Normal.k;
		}
	}

I'd appreciate it if anyone could hint me in the right direction. As far as I can see, the top code does exactly the same as the bottom, just faster.

Share this post


Link to post
Share on other sites
neonic    367
Try taking a look at these two posts:
http://www.gamedev.net/community/forums/topic.asp?topic_id=389021

&

http://www.gamedev.net/community/forums/topic.asp?topic_id=393131

I was having what seems to be the same problem as you and while I can't remember exactly which solution worked for me, I do know that one of those two posts has the correct method that I started using. So read through, understand and try to implement. It really helped me out of my problem.

Cheers.

Share this post


Link to post
Share on other sites
Woodoo    189
Two things you should check:

1. Are the vertex normals initialised to 0,0,0. In the "old code it didn't mattered if/how the normal was initialised.
2. in "old code" you set the vertex.normalArray which you don't do in the optimized version. (with the code you've posted it's impossible to tell if you made other changes so you don't need the normalArray anymore)

Share this post


Link to post
Share on other sites
deadstar    536
Quote:
Original post by Woodoo
2. in "old code" you set the vertex.normalArray which you don't do in the optimized version. (with the code you've posted it's impossible to tell if you made other changes so you don't need the normalArray anymore)


*Bangs head on table*

Well, that fixed the first problem! I must have done that yesterday accidentally.

Now the normals are actually showing up, this is what I was getting before they suddenly "disappeared":



And here is my code so far:



//For calculating face normals
SYM_VECTOR3 Point[3];

//Loop through faces
for (int f = 0; f < Face.size(); f++)
{
//Convert vertices of triangle to vectors
Point[0] = Vertex[ Face[f].a ];
Point[1] = Vertex[ Face[f].b ];
Point[2] = Vertex[ Face[f].c ];

//Calculate face normal
Face[f].Normal = GetFaceNormal(Point[0], Point[1], Point[2]);

//Convert to array for OpenGL use
Face[f].NormalArray[0] = Face[f].Normal.i;
Face[f].NormalArray[1] = Face[f].Normal.j;
Face[f].NormalArray[2] = Face[f].Normal.k;

//Add the face normal to the vertex normal of each vertex in this face
Vertex[ Face[f].a ].Normal += Face[f].Normal;
Vertex[ Face[f].b ].Normal += Face[f].Normal;
Vertex[ Face[f].c ].Normal += Face[f].Normal;

//Keep count of the number of normals added
Vertex[ Face[f].a ].NormalCounter++;
Vertex[ Face[f].b ].NormalCounter++;
Vertex[ Face[f].c ].NormalCounter++;
}

//Loop through vertices
for (int v = 0; v < Vertex.size(); v++)
{
//Average the normal
if(Vertex[v].NormalCounter != 0) Vertex[v].Normal = Vertex[v].Normal / Vertex[v].NormalCounter;

//Normalise the summed normals
Vertex[v].Normal.Normalise();

//Create opengl array
Vertex[v].NormalArray[0] = Vertex[v].Normal.i;
Vertex[v].NormalArray[1] = Vertex[v].Normal.j;
Vertex[v].NormalArray[2] = Vertex[v].Normal.k;
}



I get the same effect whether I average the vertex normal or not, or if I nomalise the face normal before calculating the vertex normal or not.

I've traced over the code in my head several times. I can't spot the flaw in the method. Here's what I (in theory) am doing:

1. Loop through faces
- Calculate the face normal
- (Optionally) normalise the face normal
- Add the face normal to the normal of each vertex attached to that face
- (Optionally) Increment a counter on the vertex for averaging later

2. Loop through vertices
- (Optionally) Average the vertex normal
- Normalise the vertex normal

It's an optimised version of the "old" method (which is explained in Enosch's post here: http://www.gamedev.net/community/forums/topic.asp?topic_id=393131)

And yes, the constructor of VECTOR3 initialises i, j and k to 0.

Share this post


Link to post
Share on other sites
Rasmadrak    196
I'm using a simple function that works great (for terrain at least).
Start at [x+1,y+1] and loop through this and the eight surrounding vertices.
The current normal becomes the sum of all normals divided by nine.

All vertices needs to have their normals supplied first, but this is created when loading the heightmap. Also edges aren't affected by normal-smoothing, but that shouldn't be a problem for most occasions.

Share this post


Link to post
Share on other sites
deadstar    536
It makes sense except for this line:

Quote:
Original post by Rasmadrak
All vertices needs to have their normals supplied first


Was it a typo, and you meant faces? Otherwise I don't see what you mean.

I'll give that a try, but I'm still unsure why my technique doesn't work :(

Share this post


Link to post
Share on other sites
Check your vertex winding. In your new code, you have


//Calculate face normal
Face[f].Normal = GetFaceNormal(Point[0], Point[1], Point[2]);



but your old code has:

//Calculate normal
Face[i].Normal = GetFaceNormal(Point[2], Point[1], Point[0]);



Check each normal before you sum them together. Make sure they are all pointing in the same direction (up in this case). A dot product test with (0,1,0) should work.

Share this post


Link to post
Share on other sites
deadstar    536
Thanks Paranoia, that was just trying reversing the winding at the time I posted the code, it's set back to normal now.

The face normals are correct. I'm using the same method to calculate face normals in both versions of the code (they are visually correct too).

I'm trying a testing measure: In the loop, if the vertex is equal to 47 I cout the face numbers attached to that vertex.

In the old code, vertex 47 is attached to faces 20, 21, 23, 50, 52 and 53.
in the new code, it just outputs face 23.

Somehow, the new code is only finding one face per vertex. I don't see what the problem is, for ever face I add the normal to Vertex[Face[i].a], Vertex[Face[i].b] AND Vertex[Face[i].c].

I'm really baffled :/

Share this post


Link to post
Share on other sites
Could it be because (in the new code) you are adding the normals to the vertex object but not counting the number of normals added and dividing by that number before normalizing?

Could you post the code you just said you were using to test?

Share this post


Link to post
Share on other sites
deadstar    536
There are a few articles discussing whether or not the normals should be averaged. It's just a slightly different visual effect, usually not worth the hassle.

The 'old' code works fine if I don't average them, so I've taken it out for simplicity (and to try and narrow the problem down).

Here is the code for debugging the old method:


//For calculating normals
SYM_VECTOR3 Point[3];

//Calculate face normals
for (int i = 0; i < Face.size(); i++)
{
Point[0] = Vertex[Face[i].a];
Point[1] = Vertex[Face[i].b];
Point[2] = Vertex[Face[i].c];

//Calculate normal
Face[i].Normal = GetFaceNormal(Point[0], Point[1], Point[2]);
}

//CALCULATE VERTEX NORMALS

//Allocate a temp normal to store for calculation
SYM_VECTOR3 TempNormal;

//Loop through each vertex
for (int v = 0; v < Vertex.size(); v++)
{
//Reset the temp normal
///TODO: Implement '=' by scalar
TempNormal.i = 0;
TempNormal.j = 0;
TempNormal.k = 0;

//Loop through each face
for (int f = 0; f < Face.size(); f++)
{
//See if face uses the vertex
if ((Vertex[Face[f].a] == Vertex[v]) || (Vertex[Face[f].b] == Vertex[v]) || (Vertex[Face[f].c] == Vertex[v]))
{
//This face uses the vertex, so store it's normal
TempNormal += Face[f].Normal;

if(v == 47) cout << "Face " << f << " uses vertex 47\n";
}
}

//Normalise the averaged normal
TempNormal.Normalise();

//Store it
Vertex[v].Normal = TempNormal;

//Convert to array for OpenGL use
Vertex[v].NormalArray[0] = Vertex[v].Normal.i;
Vertex[v].NormalArray[1] = Vertex[v].Normal.j;
Vertex[v].NormalArray[2] = Vertex[v].Normal.k;
}




This outputs the following:

Quote:

Face 20 uses vertex 47
Face 21 uses vertex 47
Face 23 uses vertex 47
Face 50 uses vertex 47
Face 52 uses vertex 47
Face 53 uses vertex 47


And the code for the new method:


//For calculating face normals
SYM_VECTOR3 Point[3];

//Loop through faces
for (int f = 0; f < Face.size(); f++)
{
//Convert vertices of triangle to vectors
Point[0] = Vertex[ Face[f].a ];
Point[1] = Vertex[ Face[f].b ];
Point[2] = Vertex[ Face[f].c ];

//Calculate face normal
Face[f].Normal = GetFaceNormal(Point[0], Point[1], Point[2]);

//Add the face normal to the vertex normal of each vertex in this face
Vertex[ Face[f].a ].Normal += Face[f].Normal;
Vertex[ Face[f].b ].Normal += Face[f].Normal;
Vertex[ Face[f].c ].Normal += Face[f].Normal;

if(Face[f].a == 47 || Face[f].b == 47 || Face[f].c == 47) cout << "Face " << f << " uses vertex 47\n";
}

//Loop through vertices
for (int v = 0; v < Vertex.size(); v++)
{
//Normalise the summed normals
Vertex[v].Normal.Normalise();

//Create opengl array
Vertex[v].NormalArray[0] = Vertex[v].Normal.i;
Vertex[v].NormalArray[1] = Vertex[v].Normal.j;
Vertex[v].NormalArray[2] = Vertex[v].Normal.k;
}




Which outputs:

Quote:

Face 23 uses vertex 47

Share this post


Link to post
Share on other sites
In the old code, what outputs if you change

[source lang=cpp]
if ((Vertex[Face[f].a] == Vertex[v]) || (Vertex[Face[f].b] == Vertex[v]) || (Vertex[Face[f].c] == Vertex[v]))



to

[source lang=cpp]
if ((Face[f].a == v) || (Face[f].b == v) || (Face[f].c == v))



?

Share this post


Link to post
Share on other sites
It might be that you have duplicates in the vertex list. If you're not storing each vertex uniquely, that would mean that normals would be added to the "same" vertex but in different places in the list. That might explain the problem.

Share this post


Link to post
Share on other sites
deadstar    536
Makes sense.

So for terrain I'll work out another way to calculate faces from the heightmap, and for my model format I'll look into implementing smoothing groups.

Thanks very much for your help!

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now

Sign in to follow this