Simple Ray Tracer, Diffuse Shading problems c++

Question

I'm writing a basic Ray-tracer in effort to better understand the whole thing. I've come across an issue that's been holding me back for a little while now, Diffuse shading of a sphere. I've used a formula from the following source to calculate Sphere intersections, and the diffuse shading.

http://www.ccs.neu.edu/home/fell/CSU540/programs/RayTracingFormulas.htm

My code which calculates the shading (An attempted replication of the source code at the link) is shown before. For the most part the calculations appear correct on some spheres, at times, however depending on the lights position depends upon how correct/broken the spheres shading appears to be.

TVector intersect   (ray.getRayOrigin().getVectX() + t * (ray.getRayDirection().getVectX() - ray.getRayOrigin().getVectX()),
                    ray.getRayOrigin().getVectY() + t * (ray.getRayDirection().getVectY() - ray.getRayOrigin().getVectY()),
                    ray.getRayOrigin().getVectZ() + t * (ray.getRayDirection().getVectZ() - ray.getRayOrigin().getVectZ()));

//Calculate the normal at the intersect point
TVector NormalIntersect (intersect.getVectX() - (position.getVectX()/r), 
                        intersect.getVectY() - (position.getVectY()/r),
                        intersect.getVectZ() - (position.getVectZ()/r));

NormalIntersect = NormalIntersect.normalize();

//Find unit vector from intersect(x,y,z) to the light(x,y,z)
TVector L1 (light.GetPosition().getVectX() - intersect.getVectX(), 
            light.GetPosition().getVectY() - intersect.getVectY(),
            light.GetPosition().getVectZ() - intersect.getVectZ());
L1 = L1.normalize();
double Magnitude = L1.magnitude();

TVector UnitVector(L1.getVectX() / Magnitude,
                   L1.getVectY() / Magnitude,
                   L1.getVectZ() / Magnitude);

//Normalized or not, the result is the same
UnitVector = UnitVector.normalize();

float Factor = (NormalIntersect.dotProduct(UnitVector));
float kd = 0.9;             //diffuse-coefficient
float ka = 0.1;             //Ambient-coefficient
Color pixelFinalColor(kd * Factor * (color.getcolorRed())  +  (ka * color.getcolorRed()) ,
                      kd * Factor * (color.getcolorGreen())  + (ka * color.getcolorGreen())  ,
                      kd * Factor * (color.getcolorBlue()) +  (ka * color.getcolorBlue()) ,1);

As you can see from the picture, some spheres appear to be shaded correctly, while others are completely broken. At first I thought the issue may lie with the UnitVector Calculation, however when I looked over it I was unable to find issue. Can anyone see the reason as to the problem?

Note: I'm using OpenGl to render my scene.

Update: I'm still having a few problems however I think they've mostly been solved thanks to the help of you guys, and a few alterations to how I calculate the unit vector. Updates shown below. Many thanks to everyone who gave their answers.

TVector UnitVector (light.GetPosition().getVectX() - intersect.getVectX(), 
                    light.GetPosition().getVectY() - intersect.getVectY(),
                    light.GetPosition().getVectZ() - intersect.getVectZ());

UnitVector = UnitVector.normalize();
float Factor = NormalIntersect.dotProduct(UnitVector);

//Set Pixel Final Color
Color pixelFinalColor(min(1,kd * Factor * color.getcolorRed())  +  (ka * color.getcolorRed()) ,
                      min(1,kd * Factor * color.getcolorGreen())  + (ka * color.getcolorGreen())  ,
                      min(1,kd * Factor * color.getcolorBlue()) +  (ka * color.getcolorBlue()) ,1);

Answer 1

you misplaced braces in normal calculation it should be

TVector NormalIntersect ((intersect.getVectX() - position.getVectX())/r, 
                         (intersect.getVectY() - position.getVectY())/r,
                         (intersect.getVectZ() - position.getVectZ())/r);

Answer 2

Firstly, if your getRayDirection() is doing what it says it is, it's producing a direction, and not a gaze point, so you should not be subtracting the ray origin from it, which is a point. (Although directions and points are both represented by vectors, it does not make sense to add a point to a direction)

Also, you are normalising L1 and then taking its magnitude and dividing each of its components by that magnitude, to produce UnitVector, which you then call normalize on again. This is unnecessary: The magnitude of L1 after the first normalization is 1, you're normalizing the same vector 3 times, just use L1.

The last issue, is that of clamping. The variable you call Factor is the value cos(th) where th is the angle between the direction of light and the normal. But cos(th) has a range of [1,-1] and you want a range of only [0,1], so you must clamp Factor to that range:

Factor = max(0, min( NormalIntersect.dotProduct(UnitVector), 1));

(And remove the min calls in your production of color).

This clamping is necessary for faces whose normals are facing away from the light, which will have negative cos(th) values. The angle between their normal and the direction of light are greater than pi/2. Intuitively, they should appear as dark as possible with respect to the light in question, so we clamp them to 0).

Here's my final version of the code, which should work. I'm going to work on the assumption that you have scale(float) and operator +(const TVector &) etc defined on your TVector class, because they will quite plainly make your life easier. Also, for brevity, I'm going to call NormalIntersect, just normal:

TVector 
    intersect = ray.getRayOrigin() + ray.getRayDirection().scale(t),
    normal    = (intersect - position).normalize(),
    L1        = (light.GetPosition() - intersect).normalize();

float
    diffuse   = max(0, min(normal.dotProduct(L1), 1)),
    kd        = 0.1,
    ka        = 0.9;

Color pixelFinalColor = color.scale(kd * diffuse + ka);