Why do you use MAX_DIR_LIGHTS, MAX_POINT_LIGHTS, and MAX_SPOT_LIGHTS with their respective "number of lights" when you could just use something like NumberOfDir, NumberOfPoint, and NumberOfSpot in the for-loop?

I would recommend that you test out both approaches, make some measurements, and see which one performs better for you:

float3 lightCounts;
...
[loop]
for (int i = 0; i < MAX_SPOT_LIGHTS; ++i)
{
[branch]
if( (float)i >= lightCounts.z )
break;

int3 lightCounts;
...
[loop]
for (int i = 0; i < lightCounts.z; ++i)
{

Shader-model 3 level hardware doesn't actually have support for general purpose int variables at runtime (they're emulated using floats), but some cards may have some special integer loop registers...

I'd also recommend always checking your HLSL assembly output occasionally. I've had a loop before that looked like:

for (int i = 0; i < myVariable; ++i)
{...

But the compiled assembly actually represented:

for (int i = 0; i <= 255; ++i)
{ if( (float)i >= myVariable ) break;
...

Or another one where the compiler converted it to a loop-to-255-with-break as above, but then unrolled it:

if( myVariable > 0 ) ...
if( myVariable > 1 ) ...
and so on, until
if( myVariable > 254 ) ...

Which is why I don't trust SM3 loops very much...

I'd also recommend making use of the [loop], [branch] and [flatten] attributes above for and if statements. This will force the compiler to do what you tell it, instead of relying on it to guess what you want it to do.


You could also try the idea suggested by phil_t of having multiple versions (permutations) of the shader for different loop values:

const int numSpotLights = ...;//compile the shader many times, with different values here
...
[unroll]
for (int i = 0; i < numSpotLights; ++i)
{

This will be, by far, the most efficient method, because you can completely remove the loop/branching from the shader, and just have the compiler unroll the loop the right amount of times. There's a bit of additional complexity on the CPU in choosing the right shader/permutation/technique for each object though.

-The "length" calculation is an attempt to reduce the number of calculations if the pixil is outside the light's range. Would the branching with an "if" statement encapsulating all those equations (that follow) be slower than doing the calculations with 0 ?

That's an optimization question, which means the only way to answer it is to test (profile) and find out.

Branching on SM3-level hardware is pretty expensive. I would guess, as a very rough rule of thumb, than an if statement is roughly equivalent to about a dozen math statements. So, for the branch to be a useful optimization, then on average, it needs to skip a dozen operations.

If it only skips a dozen half the time, but skips nothing the other half the time, then it will be costing you more than it saves.
If it skips two dozen half the time and skips nothing the other half the time, then you break even.
If it skips a dozen the majority of the time, then it's worthwhile.

Also, what about cubic lighting? Is that inefficient? It would look better for flashlights....

Can you explain what you mean by cubic lighting? Do you mean using a cube-map / environment map?

-Is there any time I would need to have the ambient from the light be a factor? I have a global ambient....

In your original code, you're not even using each light's ambient value, you're only using global ambient

If you did want each light to be able to contribute ambient lighting, then you don't have to do this in your shader. On the CPU, you can add together all the ambient values from each light source, and then put the result into the global-ambient variable.