Fixing the lights, something does not look legit in the code

[illwieckz]

I have some doubt about our lighting being correct. While doing #301 (merge lightmap/lightgrid world and entity lighting) I spent a lot of time reading our lighting code and I found something weird.

Let's explain:

This is lighting code for BSP surface with lightmap color and deluxemap direction like we did before and we will still do after #301.

	// Compute light direction in world space from deluxe map.
	vec4 deluxe = texture2D(u_DeluxeMap, var_TexLight);
	vec3 lightDir = normalize(2.0 * deluxe.xyz - 1.0);

	// Compute light color from world space lightmap.
	vec3 lightColor = texture2D(u_LightMap, var_TexLight).rgb;
	
	// Divide by cosine term to restore original light color.
	lightColor /= clamp(dot(normalize(var_Normal), lightDir), 0.3, 1.0);
	
	color.rgb = vec3(0.0);
			
	// Blend static light.
	computeLight(lightDir, normal, viewDir, lightColor, diffuse, material, color);

	// Blend dynamic lights.
	computeDLights(var_Position, normal, viewDir, diffuse, material, color);

This is lighting code for Entity surface with lightgrid color and lightgrid direction like we did before and will still do after #301. This is also lighting code for BSP surface with lightgrid color and lightgrid direction after #301.

	// Compute light direction in world space from light grid.
	vec4 texel = texture3D(u_LightGrid2, lightGridPos);
	vec3 lightDir = normalize(texel.xyz - (128.0 / 255.0));  
		
	// Compute light color from lightgrid.
	vec3 ambientColor, lightColor;
	ReadLightGrid(texture3D(u_LightGrid1, lightGridPos), ambientColor, lightColor);

	color.rgb = ambientColor * r_AmbientScale * diffuse.rgb;

	// Blend static light.
	computeLight(lightDir, normal, viewDir, lightColor, diffuse, material, color);

	// Blend dynamic lights.
	computeDLights(var_Position, normal, viewDir, diffuse, material, color);

This is lighting code for BSP surface with lightgrid color and without lightgrid direction neither deluxemap direction, this code was used before #301 to lit the map when lightmapping (and deluxe mapping) was disabled.

	// Compute light color from lightgrid.
	vec3 ambientColor, lightColor;
	ReadLightGrid(texture3D(u_LightGrid1, lightGridPos), ambientColor, lightColor);
	
	color.rgb = lightColor.rgb * diffuse.rgb;

	// Blend dynamic lights.
	computeDLights(var_Position, normal, viewDir, diffuse, material, color);

So, what's the issue ? We see that:

• in first case, we assign color with zero (pure black) and call computeLight with color, diffuse and lightColor;
• in second case, we assign color with the result of some operation with diffuse and ambientColor, then call computeLight with color, diffuse and lightColor;
• in third case, we assign color with the result of some operation with diffuse and lightColor, then do not call computeLight with color, diffuse and lightColor.

So, I have a strong feeling the second case is doing wrong things: it looks like either we compute color from diffuse and something else, either we set color to black and call computeLight. Doing both looks suspicious.

@gimhael, what's your opinion on this?

[gimhael]

The light grid stores separate ambient and directional light intesities, and computeLight only deals with the directional light computation, so the ambient part is computed in the main shader.

The light map stores a precomputed mix of ambient and directional light and it is not possible to separate those two components again, so the shader assumes that the light is 100% directional, i.e. the ambient part is zero.

This could probably be improved if we read the ambientScale from the light grid and use that to split the lightmap value into directional and ambient components.

I would not take the ambient light from the light grid directly, because the light map already contains the ambient lighting and so it would be added twice.

[illwieckz]

Oh, so basically this line:

color.rgb = vec3(0.0);

is strictly equivalent to:

color.rgb = ambientColor * r_AmbientScale * diffuse.rgb;

when ambientColor is zero.

I would not take the ambient light from the light grid directly, because the light map already contains the ambient lighting and so it would be added twice.

So in the case we would read light direction from lightgrid and light color from lightmap, we would have to ditch the ambient from lightgrid because the light color from lightmap already have it, right?

Something like this:

	// Compute light direction in world space from light grid.
	vec4 texel = texture3D(u_LightGrid2, lightGridPos);
	vec3 lightDir = normalize(texel.xyz - (128.0 / 255.0));  

	// Compute light color from world space lightmap.
	vec3 lightColor = texture2D(u_LightMap, var_TexLight).rgb;
	
	// Divide by cosine term to restore original light color.
	lightColor /= clamp(dot(normalize(var_Normal), lightDir), 0.3, 1.0);

	// color.rgb = 0.0 * diffuse.rgb
	color.rgb = vec3(0.0);
			
	// Blend static light.
	computeLight(lightDir, normal, viewDir, lightColor, diffuse, material, color);

	// Blend dynamic lights.
	computeDLights(var_Position, normal, viewDir, diffuse, material, color);

[gimhael]

Yes, the light map actually stores only ambient + directed * dot(N,L), while the light grid has ambient, directed and light direction. The ambient and directed light values are "compressed" into a single rgb value for ambient + directed and a scalar in alpha that holds the relative amount of ambient light in that sum.

Assuming the ambient part in the texture map is similar to the ambient part in the light grid, one could read that ambientScale value from the light grid alpha channel and assign lightColor * ambientScale to the ambient color and lightColor * (1.0 - ambientScale) to the directional colour.

That would require another texture3D fetch unless we move the ambientScale into the 2nd light grid texture, but I guess it wouldn't be too bad if we read another texture.

[illwieckz]

This is not a bug. Thanks for the explanations.

It's just that the code was so optimized it was very hard to track down the various collapsing steps that led to this code.