[Bf-codereview] BGE: Material replacement for texface options (issue4289041)

[dfelinto at gmail.com]

"Done" most of the pointed issues - I sent a new patch. Let's see how it
works.


http://codereview.appspot.com/4289041/diff/1/release/scripts/ui/properties_material.py
File release/scripts/ui/properties_material.py (right):

http://codereview.appspot.com/4289041/diff/1/release/scripts/ui/properties_material.py#newcode621
release/scripts/ui/properties_material.py:621: split = layout.split()
On 2011/03/11 23:08:27, dingto wrote:
> No need to use split here.
> row = layout.row() will do the same job.

Done.

http://codereview.appspot.com/4289041/diff/1/release/scripts/ui/properties_material.py#newcode632
release/scripts/ui/properties_material.py:632:
row.prop(game,"face_orientation",text="")
I couldn't (and just tried again) to have the labels in one line, and
the enum in the next line. It gets more compact sure, but loose
legibility I think. e.g. - http://www.pasteall.org/pic/show.php?id=9873

http://codereview.appspot.com/4289041/diff/1/source/blender/editors/space_view3d/drawmesh.c
File source/blender/editors/space_view3d/drawmesh.c (right):

http://codereview.appspot.com/4289041/diff/1/source/blender/editors/space_view3d/drawmesh.c#newcode447
source/blender/editors/space_view3d/drawmesh.c:447: Material *ma=
give_current_material(Gtexdraw.ob, matnr+1);
I don't know why to use matnr+1, but it's like this all over the code.

http://codereview.appspot.com/4289041/diff/1/source/blender/editors/space_view3d/drawmesh.c#newcode453
source/blender/editors/space_view3d/drawmesh.c:453: if (tface &&
set_draw_settings_cached(0, Gtexdraw.istex, tface, lit, Gtexdraw.ob,
matnr, (ma?ma->gameopt.flag&GEMAT_TWOSIDED:0),
(ma?ma->gameopt.alpha_blend:0))) {
The call for set_draw_settings_cached grown with time. I wonder if
should pass the material flags and do the checks internally or not.

In case of not I could probably make the line bigger and the code
shorter with:
- lit
+ (ma && ma->mode & MA_SHLESS?0:Gtexdraw.islit)

http://codereview.appspot.com/4289041/diff/1/source/blender/gpu/GPU_material.h
File source/blender/gpu/GPU_material.h (right):

http://codereview.appspot.com/4289041/diff/1/source/blender/gpu/GPU_material.h#newcode96
source/blender/gpu/GPU_material.h:96: GPU_BLEND_ALPHA_SORT = 8
for the GPU the GPU_BLEND_ALPHA_SORT works like GPU_BLEND_ALPHA. I can
convert one into another it that's cleaner for Blender.

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

http://codereview.appspot.com/4289041/diff/1/source/blender/makesrna/intern/rna_material.c#newcode731
source/blender/makesrna/intern/rna_material.c:731: srna=
RNA_def_struct(brna, "MaterialGameOp", NULL);
On 2011/03/11 20:47:50, brechtvl wrote:
> No need to use abbreviation here, can be MaterialGameSettings.

Done.

http://codereview.appspot.com/4289041/diff/1/source/blender/makesrna/intern/rna_material.c#newcode1884
source/blender/makesrna/intern/rna_material.c:1884:
RNA_def_property_ui_text(prop, "Game Options", "Game material
settings");
On 2011/03/11 20:47:50, brechtvl wrote:
> Could "game options" be renamed to "game settings" here and in other
places?
> It's more consistent with existing RNA properties.

Done.

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

http://codereview.appspot.com/4289041/diff/1/source/gameengine/Converter/BL_BlenderDataConversion.cpp#newcode93
source/gameengine/Converter/BL_BlenderDataConversion.cpp:93: #include
"BKE_image.h"
On 2011/03/11 20:42:59, jesterKing wrote:
> On 2011/03/11 19:58:26, dfelinto wrote:
> > I have no idea when to include "includes" here or in the extern "C"
below.

> Depends, some of the headers have an extern "C" block themselves, so
you can
> check those before deciding. If they have one, put them here. Best
would be to
> have the extern "C" blocks in each header themselves, so you don't
have to think
> about it later.

Done.

http://codereview.appspot.com/4289041/diff/1/source/gameengine/Converter/BL_BlenderDataConversion.cpp#newcode149
source/gameengine/Converter/BL_BlenderDataConversion.cpp:149: //#include
"IMB_imbuf.h"
On 2011/03/11 20:42:59, jesterKing wrote:
> I'd say just remove these lines altogether instead of commenting them
out.

Done.

http://codereview.appspot.com/4289041/diff/1/source/gameengine/Converter/BL_BlenderDataConversion.cpp#newcode625
source/gameengine/Converter/BL_BlenderDataConversion.cpp:625: /** No
material, what to do? let's see what is in the UV and set the material
accordingly
On 2011/03/11 20:42:59, jesterKing wrote:
> no need to put a /** doxygen starting comment here :)

Done.

http://codereview.appspot.com/4289041/diff/1/source/gameengine/Converter/BL_BlenderDataConversion.cpp#newcode734
source/gameengine/Converter/BL_BlenderDataConversion.cpp:734:
material->tface		= tface; //XXXDF
On 2011/03/11 19:58:26, dfelinto wrote:
> we still need to pass on the tface because of the image. I forgot to
remove my
> comment here. sorry

Done.

http://codereview.appspot.com/4289041/diff/1/source/gameengine/Rasterizer/RAS_IRasterizer.h
File source/gameengine/Rasterizer/RAS_IRasterizer.h (left):

http://codereview.appspot.com/4289041/diff/1/source/gameengine/Rasterizer/RAS_IRasterizer.h#oldcode74
source/gameengine/Rasterizer/RAS_IRasterizer.h:74:
RAS_RENDER_3DPOLYGON_TEXT = 16384	/* TF_BMFONT */
The current BGE defines may lack a bit of criteria. I found a bit lost
on when to use RAS_ .. or KX_.. or bring the define all the way from
DNA_material_types.h.

http://codereview.appspot.com/4289041/