BGE Harmony Branch

The purpose of the Harmony project is to improve the Blender Game Engine’s rendering of light and shadows. The first phase of improvements will focus on the rendering and use of shadows. The scope of this phase includes a revised shadow panel, point light shadows, variance shadow maps, shadow cookies, and shadow color.

Changes from original scope:
* Point light shadows are not fully functional and have therefore not been added
* Sun light orthographic shadows have been added

[brechtvl, 2012-02-29 13:57:22]

https://codereview.appspot.com/5491068/diff/2001/release/scripts/startup/bl_u...
File release/scripts/startup/bl_ui/properties_data_lamp.py (right):

https://codereview.appspot.com/5491068/diff/2001/release/scripts/startup/bl_u...
release/scripts/startup/bl_ui/properties_data_lamp.py:221:
col.label("Algorithm:")
Algorithm is not really a term to use in the user interface, a bit too
programmer-y.

https://codereview.appspot.com/5491068/diff/2001/release/scripts/startup/bl_u...
release/scripts/startup/bl_ui/properties_data_lamp.py:222: col.prop(lamp,
"shadow_map_type", text="", toggle=True)
Also in blender shadow map is called shadow buffer, maybe this property name
should be consistent with that.

https://codereview.appspot.com/5491068/diff/2001/source/blender/gpu/CMakeList...
File source/blender/gpu/CMakeLists.txt (right):

https://codereview.appspot.com/5491068/diff/2001/source/blender/gpu/CMakeList...
source/blender/gpu/CMakeLists.txt:56:
intern/shaders/gpu_shader_sep_gaussian_blur_frag.glsl.c
If you're going to make a separate directory for glsl shaders, you can move all
of them there. Also perhaps move it at the same level as intern/ instead of
nested.

https://codereview.appspot.com/5491068/diff/2001/source/blender/gpu/intern/gp...
File source/blender/gpu/intern/gpu_extensions.c (right):

https://codereview.appspot.com/5491068/diff/2001/source/blender/gpu/intern/gp...
source/blender/gpu/intern/gpu_extensions.c:89: } GG = {1, 0, 0, 0, 0, 0};
Does this work, shouldn't it be ",  {0, 0}"? I would expect this to give a
compile error with some compilers.

https://codereview.appspot.com/5491068/diff/2001/source/blender/gpu/intern/gp...
source/blender/gpu/intern/gpu_extensions.c:594: GPUTexture
*GPU_texture_create_shadow_map(int size, char err_out[256])
I guess this is for a specific type of shadow map which uses 2 components rather
than 1, maybe function name should reflect that.

https://codereview.appspot.com/5491068/diff/2001/source/blender/gpu/intern/gp...
source/blender/gpu/intern/gpu_extensions.c:873: GPUShader *blur_shader =
GPU_shader_get_builtin_shader(GPU_SHADER_SEP_GAUSSIAN_BLUR);
There should be a NULL pointer check before using blur_shader, it might fail to
compile for some reason.

https://codereview.appspot.com/5491068/diff/2001/source/blender/gpu/intern/gp...
source/blender/gpu/intern/gpu_extensions.c:878:
glBindFramebufferEXT(GL_FRAMEBUFFER_EXT, blurfb->object);
Is there a particular reason why GPU_framebuffer_texture_bind is not used?

https://codereview.appspot.com/5491068/diff/2001/source/blender/gpu/intern/gp...
source/blender/gpu/intern/gpu_extensions.c:1261: void
GPU_shader_free_builtin_shader(GPUBuiltinShader shader)
I'd make this a function that frees all builtin shaders, avoids having to keep
calls from gpu_codegen.c in sync.

https://codereview.appspot.com/5491068/diff/2001/source/blender/gpu/intern/gp...
File source/blender/gpu/intern/gpu_material.c (right):

https://codereview.appspot.com/5491068/diff/2001/source/blender/gpu/intern/gp...
source/blender/gpu/intern/gpu_material.c:625: static void
shade_light_cookies(GPUMaterial *mat, GPULamp *lamp, GPUNodeLink **rgb)
This function name is totally cryptic to me, this is just evaluating lamp
textures?

https://codereview.appspot.com/5491068/diff/2001/source/blender/gpu/intern/gp...
source/blender/gpu/intern/gpu_material.c:630: MTex *mtex = NULL;
A bit pedantic, but there's sort of a convention in blender to first define the
pointers, and then ints/floats, etc.

https://codereview.appspot.com/5491068/diff/2001/source/blender/gpu/intern/gp...
source/blender/gpu/intern/gpu_material.c:735: shade_light_cookies(mat, lamp,
&shr->diff);
This should not work shr->diff I think? Since that's the accumulated lighting
from all lamps. Why not apply the textures to shadfac earlier in the function?

https://codereview.appspot.com/5491068/diff/2001/source/blender/gpu/intern/gp...
source/blender/gpu/intern/gpu_material.c:763: shade_light_cookies(mat, lamp,
&rgb);
Same here, this seems a strange place to be modifying shadfac, just do it
earlier?

https://codereview.appspot.com/5491068/diff/2001/source/blender/makesrna/inte...
File source/blender/makesrna/intern/rna_lamp.c (right):

https://codereview.appspot.com/5491068/diff/2001/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_lamp.c:724: static EnumPropertyItem
prop_shadbuftype_items[] = {
These enum items should be removed here.

https://codereview.appspot.com/5491068/diff/2001/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_lamp.c:804: prop= RNA_def_property(srna,
"frustum_size", PROP_FLOAT, PROP_NONE);
Maybe name this shadow_frustum_size.

https://codereview.appspot.com/5491068/diff/2001/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_lamp.c:806: RNA_def_property_ui_range(prop,
0.001, 100.0, 2, 1);
The max here seems a bit low.

https://codereview.appspot.com/5491068/diff/2001/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_lamp.c:807: RNA_def_property_ui_text(prop,
"Frustum Size", "Size of the frustum used for creating the shadow map");
Could mention here and other places the property only works in game engine.

[Moguri, 2012-03-05 08:36:38]

Most everything else has been pretty simple/straight-forward fixes, but here's a
few I had some more questions/comments on.

https://codereview.appspot.com/5491068/diff/2001/source/blender/gpu/intern/gp...
File source/blender/gpu/intern/gpu_extensions.c (right):

https://codereview.appspot.com/5491068/diff/2001/source/blender/gpu/intern/gp...
source/blender/gpu/intern/gpu_extensions.c:89: } GG = {1, 0, 0, 0, 0, 0};
Actually the ", 0" doesn't even end up affecting the shaders part of GG, since
the sixth member of GG is npotdisabled (and only six members are explicitly
specified). All members after npotdisabled are set to 0. Maybe the line should
be "} GG = {1, 0};"?

On 2012/02/29 13:57:22, brechtvl wrote:
> Does this work, shouldn't it be ",  {0, 0}"? I would expect this to give a
> compile error with some compilers.

https://codereview.appspot.com/5491068/diff/2001/source/blender/gpu/intern/gp...
source/blender/gpu/intern/gpu_extensions.c:873: GPUShader *blur_shader =
GPU_shader_get_builtin_shader(GPU_SHADER_SEP_GAUSSIAN_BLUR);
Should something be printed to the console if blur_shader is NULL? Some sort of
warning perhaps? Is there a specific function to use, or can printf() be used in
this situation?

On 2012/02/29 13:57:22, brechtvl wrote:
> There should be a NULL pointer check before using blur_shader, it might fail
to
> compile for some reason.

https://codereview.appspot.com/5491068/diff/2001/source/blender/gpu/intern/gp...
source/blender/gpu/intern/gpu_extensions.c:878:
glBindFramebufferEXT(GL_FRAMEBUFFER_EXT, blurfb->object);
I didn't want to push more matrices onto the stack (and I think OpenGL even
errored out on me here), so I bound the FBO myself without pushing matrices.
I'll make sure to add a comment.

On 2012/02/29 13:57:22, brechtvl wrote:
> Is there a particular reason why GPU_framebuffer_texture_bind is not used?

[brechtvl, 2012-03-05 15:46:52]

https://codereview.appspot.com/5491068/diff/2001/source/blender/gpu/intern/gp...
File source/blender/gpu/intern/gpu_extensions.c (right):

https://codereview.appspot.com/5491068/diff/2001/source/blender/gpu/intern/gp...
source/blender/gpu/intern/gpu_extensions.c:89: } GG = {1, 0, 0, 0, 0, 0};
Yes, shortening the line to {1, 0} is ok.

https://codereview.appspot.com/5491068/diff/2001/source/blender/gpu/intern/gp...
source/blender/gpu/intern/gpu_extensions.c:873: GPUShader *blur_shader =
GPU_shader_get_builtin_shader(GPU_SHADER_SEP_GAUSSIAN_BLUR);
Printing something to the console is ok, it should already print the glsl
compile error too. Mainly it should not crash.

[Moguri, 2012-03-06 03:53:36]

Okay, so I was looking into fixing up the lamp texture code. This was previously
trying to mimic Unity's light "cookies", which alter shadows, but then the
feature expanded to just light textures in general. So, I cleaned it up to not
depend on shadow code. Instead now, it modifies the light's color value. The
downside to this is that a lamp's dynpersmat was only properly updated when
shadow buffers were bound (persmat was updated, which allowed dynpersmat to be
calculated correctly), which made the feature not work with shadows disabled. I
moved the matrix update code into another function that is now also called in
GPU_material_bind_uniforms(), but now the matrix updates (moved to
GPU_lamp_update_buffer_mats()) are called twice for shadow buffer updates
(binding shadow buffers and then binding uniforms).

Here's a patch of gpu_material.c against the Harmony branch:
http://www.pasteall.org/29768/diff

Any thoughts?

[Moguri, 2012-03-06 03:58:36]

Here is my current solution, which I think should do the trick:
if(material->dynproperty & DYN_LAMP_PERSMAT)
{
	if (!GPU_lamp_has_shadow_buffer(lamp)) /* The lamp matrices are already updated
if we're using shadow buffers */
		GPU_lamp_update_buffer_mats(lamp);
	mult_m4_m4m4(lamp->dynpersmat, lamp->persmat, viewinv);
}

Any objections to this? It avoids multiple calls to
GPU_lamp_update_buffer_mats(), but it seems to avoid the actual problem: the
matrix data is needed in two different places (camera setup and shaders).

On 2012/03/06 03:53:36, Moguri wrote:
> Okay, so I was looking into fixing up the lamp texture code. This was
previously
> trying to mimic Unity's light "cookies", which alter shadows, but then the
> feature expanded to just light textures in general. So, I cleaned it up to not
> depend on shadow code. Instead now, it modifies the light's color value. The
> downside to this is that a lamp's dynpersmat was only properly updated when
> shadow buffers were bound (persmat was updated, which allowed dynpersmat to be
> calculated correctly), which made the feature not work with shadows disabled.
I
> moved the matrix update code into another function that is now also called in
> GPU_material_bind_uniforms(), but now the matrix updates (moved to
> GPU_lamp_update_buffer_mats()) are called twice for shadow buffer updates
> (binding shadow buffers and then binding uniforms).
> 
> Here's a patch of gpu_material.c against the Harmony branch:
> http://www.pasteall.org/29768/diff
> 
> Any thoughts?

[brechtvl, 2012-04-30 16:15:16]

With the latest commits to svn, LGTM.