[Bf-codereview] BGE Harmony Branch (issue 5491068)

[brechtvanlommel at gmail.com]

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

https://codereview.appspot.com/5491068/diff/2001/release/scripts/startup/bl_ui/properties_data_lamp.py#newcode221
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_ui/properties_data_lamp.py#newcode222
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/CMakeLists.txt
File source/blender/gpu/CMakeLists.txt (right):

https://codereview.appspot.com/5491068/diff/2001/source/blender/gpu/CMakeLists.txt#newcode56
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/gpu_extensions.c
File source/blender/gpu/intern/gpu_extensions.c (right):

https://codereview.appspot.com/5491068/diff/2001/source/blender/gpu/intern/gpu_extensions.c#newcode89
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/gpu_extensions.c#newcode594
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/gpu_extensions.c#newcode873
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/gpu_extensions.c#newcode878
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/gpu_extensions.c#newcode1261
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/gpu_material.c
File source/blender/gpu/intern/gpu_material.c (right):

https://codereview.appspot.com/5491068/diff/2001/source/blender/gpu/intern/gpu_material.c#newcode625
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/gpu_material.c#newcode630
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/gpu_material.c#newcode735
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/gpu_material.c#newcode763
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/intern/rna_lamp.c
File source/blender/makesrna/intern/rna_lamp.c (right):

https://codereview.appspot.com/5491068/diff/2001/source/blender/makesrna/intern/rna_lamp.c#newcode724
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/intern/rna_lamp.c#newcode804
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/intern/rna_lamp.c#newcode806
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/intern/rna_lamp.c#newcode807
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.

https://codereview.appspot.com/5491068/