[Bf-codereview] Blender Cucumber to Trunk (issue 4961053)

[dfelinto at gmail.com]

first batch of review.
I stopped right after KX_Font and KX_KetsjiEngine.
next I will review light and VBO code


http://codereview.appspot.com/4961053/diff/1/release/scripts/startup/bl_ui/properties_scene.py
File release/scripts/startup/bl_ui/properties_scene.py (right):

http://codereview.appspot.com/4961053/diff/1/release/scripts/startup/bl_ui/properties_scene.py#newcode37
release/scripts/startup/bl_ui/properties_scene.py:37: bl_label = "Scene"
should you add GAME_ENGINE instead of removing the pool completely?
(to make it easier for someone extending it to support a new engine)

http://codereview.appspot.com/4961053/diff/1/source/blender/gpu/intern/gpu_material.c
File source/blender/gpu/intern/gpu_material.c (right):

http://codereview.appspot.com/4961053/diff/1/source/blender/gpu/intern/gpu_material.c#newcode77
source/blender/gpu/intern/gpu_material.c:77: DYN_LAMP_PERSMAT = 8
keep the comma, it helps for svn blame (so new changes don't interfere
with this line)

http://codereview.appspot.com/4961053/diff/1/source/blender/makesdna/DNA_scene_types.h
File source/blender/makesdna/DNA_scene_types.h (right):

http://codereview.appspot.com/4961053/diff/1/source/blender/makesdna/DNA_scene_types.h#newcode455
source/blender/makesdna/DNA_scene_types.h:455: short fullscreen,
use_desktop, xplay, yplay, freqplay;
I would name "use_desktop" something more generic to be used later as a
flag (playerflag? flag2?). Or even use the GameData.flag and create a
GAME_PLAYER_USE_DESKTOP (now I forgot if 1 << 16 still fits in the
short)

http://codereview.appspot.com/4961053/diff/1/source/blender/makesrna/intern/rna_scene.c
File source/blender/makesrna/intern/rna_scene.c (right):

http://codereview.appspot.com/4961053/diff/1/source/blender/makesrna/intern/rna_scene.c#newcode1748
source/blender/makesrna/intern/rna_scene.c:1748: prop=
RNA_def_property(srna, "exit_key", PROP_ENUM, PROP_NONE);
I still wonder if it's possible/good idea to have combined keys to ESC.
I remember this being an issue when this was implemented, but don't
recall the final word on that.

http://codereview.appspot.com/4961053/diff/1/source/gameengine/BlenderRoutines/BL_KetsjiEmbedStart.cpp
File source/gameengine/BlenderRoutines/BL_KetsjiEmbedStart.cpp (right):

http://codereview.appspot.com/4961053/diff/1/source/gameengine/BlenderRoutines/BL_KetsjiEmbedStart.cpp#newcode234
source/gameengine/BlenderRoutines/BL_KetsjiEmbedStart.cpp:234:
KX_KetsjiEngine::SetExitKey(ConvertKeyCode(startscene->gm.exitkey));
why not using ketsjiengine->SetExitKey() ?
and can ConvertKeyCode be inside the SetExitKey()?
[even if the function is static]

http://codereview.appspot.com/4961053/diff/1/source/gameengine/Converter/BL_BlenderDataConversion.cpp
File source/gameengine/Converter/BL_BlenderDataConversion.cpp (right):

http://codereview.appspot.com/4961053/diff/1/source/gameengine/Converter/BL_BlenderDataConversion.cpp#newcode219
source/gameengine/Converter/BL_BlenderDataConversion.cpp:219: #if 0
why to commit an #if 0 code? was this copied over?

http://codereview.appspot.com/4961053/diff/1/source/gameengine/Converter/BL_BlenderDataConversion.cpp#newcode244
source/gameengine/Converter/BL_BlenderDataConversion.cpp:244: //XXX
clean up
please cleanup :)

http://codereview.appspot.com/4961053/diff/1/source/gameengine/GamePlayer/ghost/GPG_ghost.cpp
File source/gameengine/GamePlayer/ghost/GPG_ghost.cpp (right):

http://codereview.appspot.com/4961053/diff/1/source/gameengine/GamePlayer/ghost/GPG_ghost.cpp#newcode415
source/gameengine/GamePlayer/ghost/GPG_ghost.cpp:415: U.gameflags |=
USER_DISABLE_VBO;
add a comment explaining (e.g. disabling Blender VBO because BGE creates
its own VBOs)

http://codereview.appspot.com/4961053/diff/1/source/gameengine/Ketsji/BL_Texture.cpp
File source/gameengine/Ketsji/BL_Texture.cpp (right):

http://codereview.appspot.com/4961053/diff/1/source/gameengine/Ketsji/BL_Texture.cpp#newcode225
source/gameengine/Ketsji/BL_Texture.cpp:225: for (int i=0;
i<ibuf->dds_data.nummipmaps && (width||height); ++i)
Is part of this code to assure 1D images have 1 pixels in one of the
dimensions? If so can a 1D image has height? I would expect it to always
store the data as width.

http://codereview.appspot.com/4961053/diff/1/source/gameengine/Ketsji/BL_Texture.cpp#newcode232
source/gameengine/Ketsji/BL_Texture.cpp:232: size =
((width+3)/4)*((height+3)/4)*blocksize;
where does this +3/4 come from? comments here would help.

http://codereview.appspot.com/4961053/diff/1/source/gameengine/Ketsji/KX_FontObject.cpp
File source/gameengine/Ketsji/KX_FontObject.cpp (right):

http://codereview.appspot.com/4961053/diff/1/source/gameengine/Ketsji/KX_FontObject.cpp#newcode122
source/gameengine/Ketsji/KX_FontObject.cpp:122: /* Allow for some logic
brick control */
As a to-do it would be really good to have a 'text' String Game Property
always there (undeletable). as a get/set function it could get/set the
FontObject.body (i.e. the text).

http://codereview.appspot.com/4961053/diff/1/source/gameengine/Ketsji/KX_FontObject.cpp#newcode138
source/gameengine/Ketsji/KX_FontObject.cpp:138: /* Get a working copy of
the OpenGLMatrix to use */
So far the code is not working when the Font Object has a scale
different than 1.0 (with Blender default font or another ttf).

I would like to hear from Diego Borghetti his take on it anyways. We
should be using more blf imo.

http://codereview.appspot.com/4961053/diff/1/source/gameengine/VideoTexture/blendVideoTex.cpp
File source/gameengine/VideoTexture/blendVideoTex.cpp (right):

http://codereview.appspot.com/4961053/diff/1/source/gameengine/VideoTexture/blendVideoTex.cpp#newcode211
source/gameengine/VideoTexture/blendVideoTex.cpp:211:
PyModule_AddIntConstant(m, (char*)"SOURCE_ERROR", SourceError);
need to add this to the documentation (rst file)

http://codereview.appspot.com/4961053/