Blender Cucumber to Trunk

[dfelinto, 2011-09-01 09:37:16]

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/pr...
File release/scripts/startup/bl_ui/properties_scene.py (right):

http://codereview.appspot.com/4961053/diff/1/release/scripts/startup/bl_ui/pr...
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_ma...
File source/blender/gpu/intern/gpu_material.c (right):

http://codereview.appspot.com/4961053/diff/1/source/blender/gpu/intern/gpu_ma...
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_scen...
File source/blender/makesdna/DNA_scene_types.h (right):

http://codereview.appspot.com/4961053/diff/1/source/blender/makesdna/DNA_scen...
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/r...
File source/blender/makesrna/intern/rna_scene.c (right):

http://codereview.appspot.com/4961053/diff/1/source/blender/makesrna/intern/r...
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/BlenderRoutine...
File source/gameengine/BlenderRoutines/BL_KetsjiEmbedStart.cpp (right):

http://codereview.appspot.com/4961053/diff/1/source/gameengine/BlenderRoutine...
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_B...
File source/gameengine/Converter/BL_BlenderDataConversion.cpp (right):

http://codereview.appspot.com/4961053/diff/1/source/gameengine/Converter/BL_B...
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_B...
source/gameengine/Converter/BL_BlenderDataConversion.cpp:244: //XXX clean up
please cleanup :)

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

http://codereview.appspot.com/4961053/diff/1/source/gameengine/GamePlayer/gho...
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_Text...
File source/gameengine/Ketsji/BL_Texture.cpp (right):

http://codereview.appspot.com/4961053/diff/1/source/gameengine/Ketsji/BL_Text...
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_Text...
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_Font...
File source/gameengine/Ketsji/KX_FontObject.cpp (right):

http://codereview.appspot.com/4961053/diff/1/source/gameengine/Ketsji/KX_Font...
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_Font...
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/b...
File source/gameengine/VideoTexture/blendVideoTex.cpp (right):

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

[Kupoman, 2011-09-08 05:46:32]

Addressed a few of the comments from Dalai.

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

http://codereview.appspot.com/4961053/diff/1/release/scripts/startup/bl_ui/pr...
release/scripts/startup/bl_ui/properties_scene.py:37: bl_label = "Scene"
On 2011/09/01 09:37:16, dfelinto wrote:
> should you add GAME_ENGINE instead of removing the pool completely?
> (to make it easier for someone extending it to support a new engine)

Not sure what happened here. Fixed.

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

http://codereview.appspot.com/4961053/diff/1/source/blender/gpu/intern/gpu_ma...
source/blender/gpu/intern/gpu_material.c:77: DYN_LAMP_PERSMAT = 8
On 2011/09/01 09:37:16, dfelinto wrote:
> keep the comma, it helps for svn blame (so new changes don't interfere with
this
> line)

Must have had something there and deleted too much. Fixed.

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

http://codereview.appspot.com/4961053/diff/1/source/gameengine/BlenderRoutine...
source/gameengine/BlenderRoutines/BL_KetsjiEmbedStart.cpp:234:
KX_KetsjiEngine::SetExitKey(ConvertKeyCode(startscene->gm.exitkey));
On 2011/09/01 09:37:16, dfelinto wrote:
> why not using ketsjiengine->SetExitKey() ?
> and can ConvertKeyCode be inside the SetExitKey()?
> [even if the function is static]

Not sure why I did that, Fixed here and the corresponding code for the player.
If I recall correctly the ConvertKeyCode is necessary to account for differences
in the external and internal players.

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

http://codereview.appspot.com/4961053/diff/1/source/gameengine/Converter/BL_B...
source/gameengine/Converter/BL_BlenderDataConversion.cpp:219: #if 0
On 2011/09/01 09:37:16, dfelinto wrote:
> why to commit an #if 0 code? was this copied over?

Yes, this entire map declaration was copied over.

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

http://codereview.appspot.com/4961053/diff/1/source/gameengine/GamePlayer/gho...
source/gameengine/GamePlayer/ghost/GPG_ghost.cpp:415: U.gameflags |=
USER_DISABLE_VBO;
On 2011/09/01 09:37:16, dfelinto wrote:
> add a comment explaining (e.g. disabling Blender VBO because BGE creates its
own
> VBOs)

Fixed

[dfelinto, 2011-09-13 17:54:14]

just noticed a big problem in the DNA changes.

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

http://codereview.appspot.com/4961053/diff/7001/source/blender/makesdna/DNA_s...
source/blender/makesdna/DNA_scene_types.h:443: short mode, flag, matmode;
Overall comment in the DNA:
you cannot do changes in the middle of the DNA struct (unless to replace a pad
by a real property).
I can explain why on irc if you want.

So the changes here need to be rearranged.

[dfelinto, 2012-01-04 11:00:39]

small comment on collision mask code

http://codereview.appspot.com/4961053/diff/7001/source/gameengine/Physics/Bul...
File source/gameengine/Physics/Bullet/CcdPhysicsController.h (right):

http://codereview.appspot.com/4961053/diff/7001/source/gameengine/Physics/Bul...
source/gameengine/Physics/Bullet/CcdPhysicsController.h:224: UserFilter = 1<<8 |
1<<9 | 1<<10 | 1<<11 | 1<<12 | 1<<13 | 1<<14 | 1<<15,
why those hardcoded values, what do they relate to?