BGE: Material replacement for texface options

[patch originally submitted to: http://projects.blender.org/tracker/?func=detail&aid=26461&group_id=9&atid=127 ]

This patch follows (with small changes) the proposal presented here - http://wiki.blender.org/index.php/User:Dfelinto/TexFace
Not implemented: Backward Compatibility. Please refer to the bf-committers for discussions on that.
Other technical discussions, bugs, reviews, can happen here. 

Summary:
========
The idea here is to move the texface options into the material panel.
See the wip.jpg and ui.jpg images for the final UI (the one in the proposal had to be changed).

1 - Some of the legacy problems 2.49 and 2.5x has with the texface system:
========================================================
1.1) Shadow, Bilboard and Halo are mutual exclusive (in the code), yet you can select a face to be more than one mode.
1.2) Sort only works for blend Alpha yet it's an option regardless of the Transparency Blend you pick.
1.3) Shared doesn't affect anything in BGE.
1.4) ObColor only works for Text objects (old bitmap texts) when using Texture Face Materials. (not address yet, I so far ignored obcolor)

2 - Code that needs review:
===================
2.1) set_draw_settings_cached
this function was changed considerably. I think I could pass the material instead of flags and do the guessing game inside the function. Thoughts?

2.2) defines, defines, defines
They are a few defines with the same value. We had more (and even some hardcoded values :/). What I did was to avoid bringing the DNA_material_types.h defines all the way to the Rasterizer. So Rasterizer has its own defines. The code is commented to make it easy for grep them.
I'm not sure, however, if this is a good idea - or if the best way to do it is to use the GEMAT_ defines everywhere.

2.3) As a user I can't understand how ZSorting would affect Add Alpha. I tried indeed and nothing happens. The BGE code, however, consider valid the combination Add + Sort. Standing to the perception that Add shouldn't have sorting, I brought the sort option as part of the blending menu, so we have opaque, add, clip, blend alpha and blend alpha sort. In the code I could, after setting the material to use zsort, to convert GEMAT_ ALPHA_SORT to GEMAT_ALPHA. It works fine indeed. However I prefered to create the GPU_BLEND_ equivalent of the GEMAT_ALPHA_SORT and deal with that in the gpu as if it's alpha.

3 - Random Notes:
============
3.1) Now "Use Face Textures" in material Option panel will work in Multitexture even if there is no texture channel.

3.2) In FaceTexture mode it will use TexFace all the time, even if you don't check the "Use Texture Face" option in the UI. It's a matter of decision, since the code for either way is there. I decided by the solution that makes the creation of a material fast - in this mode the user doesn't need to mess with textures or this "Use Texture Face" option at all. I'm not strong in my opinion here. But I think if we don't have this then what is the point of the Texture Face mode?

3.3) I kept references for tface only when we need the image, UV or the tiling setting. It should help later when/if we split the Image and UV layers from the tface struct  (Campbell and Brecht proposal).

3.4) Attached also two of the sample files I used for tests - shadeless.blend and everything.blend. [ actually I don't see where to add more files here, so you can get them in the original report - 
http://projects.blender.org/tracker/?func=detail&aid=26461&group_id=9&atid=127 ]

3.5) I don't expect anyone to understand the whole patch. It involves a lot of areas that I myself wasn't familiar before doing this code. But a review in the general structure and one or other specific area would be nice.

Thanks,
Dalai

[dfelinto, 2011-03-11 19:58:26]

Some of my own comments in the code.
(ps !! this tool is really nice)

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

http://codereview.appspot.com/4289041/diff/1/source/blender/makesrna/intern/r...
source/blender/makesrna/intern/rna_actuator.c:1176:
RNA_def_property_ui_text(prop, "Distance", "Keep this distance to target");
Hm the changes on this file have absolute nothing to do with the patch. I was
working on that in parallel and commit it.

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

http://codereview.appspot.com/4289041/diff/1/source/gameengine/Converter/BL_B...
source/gameengine/Converter/BL_BlenderDataConversion.cpp:93: #include
"BKE_image.h"
I have no idea when to include "includes" here or in the extern "C" below.

http://codereview.appspot.com/4289041/diff/1/source/gameengine/Converter/BL_B...
source/gameengine/Converter/BL_BlenderDataConversion.cpp:659: material->ras_mode
|= (mat && (mat->gameopt.alpha_blend & GEMAT_ALPHA_SORT))? ZSORT: 0;
unlike in the original code I'm only allowing ZSort for Alpha Blend.

http://codereview.appspot.com/4289041/diff/1/source/gameengine/Converter/BL_B...
source/gameengine/Converter/BL_BlenderDataConversion.cpp:734: material->tface		=
tface; //XXXDF
we still need to pass on the tface because of the image. I forgot to remove my
comment here. sorry

http://codereview.appspot.com/4289041/diff/1/source/gameengine/Converter/BL_B...
source/gameengine/Converter/BL_BlenderDataConversion.cpp:905: /**
If I uncomment this line you need to check "use texture face" even in Texture
Face mode. I think it would be good to require that for consistency sake.
However users may find more handy to not have to? maybe gray'ing out this option
when in TextureFace would be enough to communicate this.

http://codereview.appspot.com/4289041/diff/1/source/gameengine/Rasterizer/RAS...
File source/gameengine/Rasterizer/RAS_IPolygonMaterial.cpp (right):

http://codereview.appspot.com/4289041/diff/1/source/gameengine/Rasterizer/RAS...
source/gameengine/Rasterizer/RAS_IPolygonMaterial.cpp:120: 
ok that was something I forgot to revert. There is no reason to not use the
handy C++ m_prop(prop) syntax.

[jesterKing, 2011-03-11 20:42:59]

Some small comments, nothing big.

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

http://codereview.appspot.com/4289041/diff/1/source/gameengine/Converter/BL_B...
source/gameengine/Converter/BL_BlenderDataConversion.cpp:93: #include
"BKE_image.h"
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.

http://codereview.appspot.com/4289041/diff/1/source/gameengine/Converter/BL_B...
source/gameengine/Converter/BL_BlenderDataConversion.cpp:149: //#include
"IMB_imbuf.h"
I'd say just remove these lines altogether instead of commenting them out.

http://codereview.appspot.com/4289041/diff/1/source/gameengine/Converter/BL_B...
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
no need to put a /** doxygen starting comment here :)

http://codereview.appspot.com/4289041/diff/1/source/gameengine/Converter/BL_B...
source/gameengine/Converter/BL_BlenderDataConversion.cpp:1014: /** set render
flags */
Again as earlier, no doxygen comment here

[brechtvl, 2011-03-11 20:47:50]

I only had a quick glance, will do proper review later, mainly testing out the
system.

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

http://codereview.appspot.com/4289041/diff/1/release/scripts/ui/properties_ma...
release/scripts/ui/properties_material.py:609: class
MATERIAL_PT_gameop(MaterialButtonsPanel, bpy.types.Panel):
Don't think we need this abbreviation, can be MATERIAL_PT_game_settings

http://codereview.appspot.com/4289041/diff/1/release/scripts/ui/properties_ma...
release/scripts/ui/properties_material.py:632:
row.prop(game,"face_orientation",text="")
It should not be necessary to add labels manually, if you add a split with two
columns, the resulting layout should be the same, but without manually
specifying the label text.

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

http://codereview.appspot.com/4289041/diff/1/source/blender/makesrna/intern/r...
source/blender/makesrna/intern/rna_material.c:731: srna= RNA_def_struct(brna,
"MaterialGameOp", NULL);
No need to use abbreviation here, can be MaterialGameSettings.

http://codereview.appspot.com/4289041/diff/1/source/blender/makesrna/intern/r...
source/blender/makesrna/intern/rna_material.c:1884:
RNA_def_property_ui_text(prop, "Game Options", "Game material settings");
Could "game options" be renamed to "game settings" here and in other places?
It's more consistent with existing RNA properties.

[dingto, 2011-03-11 23:08:27]

No need to use split

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

http://codereview.appspot.com/4289041/diff/1/release/scripts/ui/properties_ma...
release/scripts/ui/properties_material.py:621: split = layout.split()
No need to use split here. 
row = layout.row() will do the same job.

[dfelinto, 2011-03-11 23:25:15]

Update with suggestions from 1st reviews. Also test to see if new patch
integrates well.

[dfelinto, 2011-03-11 23:25:53]

"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_ma...
File release/scripts/ui/properties_material.py (right):

http://codereview.appspot.com/4289041/diff/1/release/scripts/ui/properties_ma...
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_ma...
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_vie...
File source/blender/editors/space_view3d/drawmesh.c (right):

http://codereview.appspot.com/4289041/diff/1/source/blender/editors/space_vie...
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_vie...
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....
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/r...
File source/blender/makesrna/intern/rna_material.c (right):

http://codereview.appspot.com/4289041/diff/1/source/blender/makesrna/intern/r...
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/r...
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_B...
File source/gameengine/Converter/BL_BlenderDataConversion.cpp (right):

http://codereview.appspot.com/4289041/diff/1/source/gameengine/Converter/BL_B...
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_B...
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_B...
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_B...
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...
File source/gameengine/Rasterizer/RAS_IRasterizer.h (left):

http://codereview.appspot.com/4289041/diff/1/source/gameengine/Rasterizer/RAS...
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.

[brechtvl, 2011-03-12 12:14:29]

Some more naming nits.

Overall: could we use a single name for "alpha mode", "blend mode", "alpha
blend" and "transp" throughout the code, RNA api and UI?

http://codereview.appspot.com/4289041/diff/11002/source/blender/makesdna/DNA_...
File source/blender/makesdna/DNA_material_types.h (right):

http://codereview.appspot.com/4289041/diff/11002/source/blender/makesdna/DNA_...
source/blender/makesdna/DNA_material_types.h:81: int face_ori;
Just name it int face_orientation;, not need for abbrevation.

http://codereview.appspot.com/4289041/diff/11002/source/blender/makesdna/DNA_...
source/blender/makesdna/DNA_material_types.h:101: struct GameSettings gameopt;
Can this just be named "game", opt postfix is not common in blender.

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

http://codereview.appspot.com/4289041/diff/11002/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_material.c:721: {GEMAT_ALPHA_SORT,
"ALPHASORT", 0, "Alpha Sort", "Sort faces for correct alpha drawing (slow, use
Alpha Clip instead when possible)"},
Change "ALPHASORT" to "ALPHA_SORT".

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

http://codereview.appspot.com/4289041/diff/11002/source/gameengine/Converter/...
source/gameengine/Converter/BL_BlenderDataConversion.cpp:234: static void
SetDefaultFaceType(Scene* scene)
Change to SetDefaultLightMode.

http://codereview.appspot.com/4289041/diff/11002/source/gameengine/Rasterizer...
File source/gameengine/Rasterizer/RAS_IPolygonMaterial.h (right):

http://codereview.appspot.com/4289041/diff/11002/source/gameengine/Rasterizer...
source/gameengine/Rasterizer/RAS_IPolygonMaterial.h:52: struct GameOptions;
Change to GameSettings.

[brechtvl, 2011-03-12 12:19:51]

> 2.1) set_draw_settings_cached
> this function was changed considerably. I think I could pass the material
> instead of flags and do the guessing game inside the function. Thoughts?

Definitely. Also things may be simplified by checking if the material is the
same, rather than comparing individual properties.

> 2.2) defines, defines, defines

I think it's ok to use separate defines as you did, seems consistent with
existing code.

> 2.3) As a user I can't understand how ZSorting would affect Add Alpha. I tried
> indeed and nothing happens. The BGE code, however, consider valid the
> combination Add + Sort. Standing to the perception that Add shouldn't have
> sorting, I brought the sort option as part of the blending menu.

I think this is a good change. Also agree with further other points.

[brechtvl, 2011-03-12 12:34:37]

Some more comments. Beyond that, it looks fine to me and I also found no bugs in
testing, main issue still is backwards compatibility.

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

http://codereview.appspot.com/4289041/diff/11002/source/blender/editors/space...
source/blender/editors/space_view3d/drawmesh.c:274: static int
set_draw_settings_cached(int clearcache, int textured, MTFace *texface, int lit,
Object *litob, int litmatnr, int doublesided, int transp)
I get the impression we could get away with just 3 parameters here: clearcache,
MTFace*, and Material*, so the code can probably be simplified quite a bit.

http://codereview.appspot.com/4289041/diff/11002/source/blender/editors/space...
source/blender/editors/space_view3d/drawmesh.c:581: (ma && (ma->gameopt.flag &
GEMAT_INVISIBLE))
I don't think we need to take into account the visible flag in weight paint
mode?

[dfelinto, 2011-03-15 02:34:08]

Update with alphablend rename + fix for a bug in GLSL 3dview (derived mesh was
still using tface)

[dfelinto, 2011-03-15 02:51:30]

Comments over the last patch.
1)renamed all blend modes, alpha modes, ... to alphablend (using alpha_blend for
DNA but alphablend for the code variables).

2)bug fixed - in GLSL mode tface alpha mode was being used in 3dview

3) there is still a last known bug. See comments in DerivedMesh.c

4) as commented before I couldn't make the UI without the labels.

http://codereview.appspot.com/4289041/diff/29/source/blender/blenkernel/inter...
File source/blender/blenkernel/intern/DerivedMesh.c (right):

http://codereview.appspot.com/4289041/diff/29/source/blender/blenkernel/inter...
source/blender/blenkernel/intern/DerivedMesh.c:965: dodraw = setMaterial(matnr =
new_matnr, &gattribs);
~remaining BUG~
In GLSL and edit mode AlphaBlend is always SOLID.

When Blender goes to edit mode the GMS.alphapass is 0. That happens because
v3d->transp=FALSE since we are not in the "view3d_draw_transp" pass.
The old workaround was to set the alphablend after setting the material. I see 2
options:
(1) set alphapass TRUE when in edit mode; OR
(2)get material from GPU and set alpha based on the material (on the real
Material, not GMS.alphablend since this is solid when we are not in the alpha
pass).

http://codereview.appspot.com/4289041/diff/29/source/blender/blenkernel/inter...
File source/blender/blenkernel/intern/cdderivedmesh.c (right):

http://codereview.appspot.com/4289041/diff/29/source/blender/blenkernel/inter...
source/blender/blenkernel/intern/cdderivedmesh.c:1021: continue;
Unlike in DerivedMesh.c we have the right alpha when we get here. So no need to
set it again based on the face.

http://codereview.appspot.com/4289041/diff/29/source/blender/blenkernel/inter...
source/blender/blenkernel/intern/cdderivedmesh.c:1177: }
I'm not so sure of what it's going on here, I probably over deleted code. I will
leave it for later, in a final round of adjustments.

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

http://codereview.appspot.com/4289041/diff/29/source/blender/editors/space_vi...
source/blender/editors/space_view3d/drawmesh.c:274: /* Icky globals, fix with
userdata parameter */
moved this to above the set_draw_settings_cached to avoid the need of a
prototype.

[brechtvl, 2011-03-15 11:49:08]

Latest changes look good.

On 2011/03/15 02:51:30, dfelinto wrote:
> 4) as commented before I couldn't make the UI without the labels.

Yes, just checked this myself, I though this was working but it's not.

>
http://codereview.appspot.com/4289041/diff/29/source/blender/blenkernel/inter...
> File source/blender/blenkernel/intern/DerivedMesh.c (right):
> 
>
http://codereview.appspot.com/4289041/diff/29/source/blender/blenkernel/inter...
> source/blender/blenkernel/intern/DerivedMesh.c:965: dodraw = setMaterial(matnr
=
> new_matnr, &gattribs);
> ~remaining BUG~
> In GLSL and edit mode AlphaBlend is always SOLID.
> 
> When Blender goes to edit mode the GMS.alphapass is 0. That happens because
> v3d->transp=FALSE since we are not in the "view3d_draw_transp" pass.
> The old workaround was to set the alphablend after setting the material. I see
2
> options:
> (1) set alphapass TRUE when in edit mode; OR
> (2)get material from GPU and set alpha based on the material (on the real
> Material, not GMS.alphablend since this is solid when we are not in the alpha
> pass).

Perhaps we can do a third alpha pass mode for edit mode, 0 for solid, 1 for
transp, 2 for solid + transp.

[dfelinto, 2011-03-16 08:48:53]

Backward compatibility for non disputant materials - (Brecht this is the msg I
can't go over limit).

[dfelinto, 2011-03-16 09:24:19]

the last patch set seemed broken. it was failing. let me try again

[dfelinto, 2011-03-16 09:28:53]

A new attempt of sending the 4th patch. this time I created it in windows. (it
was in OSX before)

[dfelinto, 2011-03-16 09:37:31]

It's no longer working.  Maybe there is an incompatibility between by latest
patch - http://dl.dropbox.com/u/3292898/texface/texface_online_4c.diff and the
third  one?

The only file I'm changing is readfile.c, I don't understand what the problem
is.
Following are my comments to my code. Once we sort that out I put them back in
the "tracker".

###############
function to handle backward compatibility for what I'm calling "non-disputed"
materials. In other words, Materials that are only associated with faces that
share the same texface flags.

Do you think it should be a separated function, or it's ok to have it inside one
of the do_version checks?

code copied/adapted from zsort doversion. It works in my test, but I'm still not
so familiar with the way we are accessing the data here. It's like the zsort so
it must be right.

transp can be 0, 1 or 3. doing <<15 I make sure it goes after the last value
used by mode (1 << 15).

[re: TC_LIGHT]
This will affect render, actually. So it's probably better if rwe don't do this
conversion no?

[dfelinto, 2011-03-16 16:34:51]

readfile fixed (it was an EOL problem) - backward compatibility partially
implemented

[brechtvl, 2011-03-17 09:55:00]

I would put the version patch in a separate function as you mentioned.

The error message when it fails conversion could be clearer, now it's as if
there is a bug, could maybe print that it skipped conversion rather than failed,
and give a hint about how to fix it?

We may indeed better skip TF_LIGHT, but if we keep it, it seems the test should
be if(!(flag & TF_LIGHT))?

I didn't expect this would break backwards compatibility that much, so to me
it's not so obvious that we should commit this now, but I think it's up to game
engine devs to decide.

[dfelinto, 2011-06-19 15:07:07]

hello all, some update (with no patch):

the reason I put this patch on hold (for 3 months) was because it was not so
obvious that it was a clear commit (due to backcomp.) so I moved on to priorize
other projects (time has being a big constraint lately, sorry).

Things I'm implementing (not in tracker):
- extend doversion to take care of material conflicts (material
splitting/creation) when the file was previously opened/saved with Blender2.5xx
and BGE was set as the RenderEngine
- create an entry in the Help menu to force conversion of conflicted materials
for 2.49 files or 2.5x files saved without BGE as RenderEngine

Thing I could use a hand with (and could be committed separately?)
- implement a 3rd pass to the 3dview drawing. right now we have solid and
transp. Edit mode needs a solid + transp mode though (see Brecht notes above).
* I tried to do that, but it was really hard to get back to my original line of
thought on this after leaving for so long. After the topics above are finished I
will give it another go if anyone else.

[dfelinto, 2011-07-05 08:04:29]

New patch, doversion + backward compatibility (Help Menu) working

[dfelinto, 2011-07-05 08:11:29]

On 2011/07/05 08:04:29, dfelinto wrote:
> New patch, doversion + backward compatibility (Help Menu) working

Note: you can find the new changes in material.c and readfile.c. I consider the
patch now ready and complete. For the conversion you can use the TexFace
conversion in the Help menu. Also I updated the wiki page with the final UI plus
some info in the backcompatibility.

[benoit.bolsee, 2011-07-25 22:32:27]

http://codereview.appspot.com/4289041/diff/38001/source/blender/blenkernel/in...
File source/blender/blenkernel/intern/material.c (right):

http://codereview.appspot.com/4289041/diff/38001/source/blender/blenkernel/in...
source/blender/blenkernel/intern/material.c:1478: flag ^= TF_LIGHT;
^= is the in-place X-or operation, it means that the TF_LIGHT bit is toggled.
Doesn't sound like the intended operation.
Shouldn't this be "flag &= ~TF_LIGHT" instead (clear TF_LIGHT bit)?
Same comment for all the other ^= operation

http://codereview.appspot.com/4289041/diff/38001/source/blender/blenkernel/in...
source/blender/blenkernel/intern/material.c:1494: void decode_tfaceflag(Material
*ma, int convertall)
This function is tricky because it takes ma->game->flag and converts it in-place
into something else. It means that ma->game->flag must contain initially a valid
that requires conversion. It would be much cleaner if the value to be converted
was passed in argument instead.

http://codereview.appspot.com/4289041/diff/38001/source/blender/blenkernel/in...
source/blender/blenkernel/intern/material.c:1579: sprintf(newname, expr,
newname, flag);
This code is messy and unsafe. Here is a better way to achieve the desired
result in a single line:
sprintf(newname, "%.*s.TF.%0*d", 32-(digits+5), matname, digits, flag)

32 is a magic number, are you sure it's correct? I see the ID name is limited to
24 (MAX_ID_NAME) and should be the case for material as well. Better use a
constant than a numeric value in any case.

[dfelinto, 2011-07-26 07:18:50]

fixed Benoit's comments + problem with meshes with no material.

[benoit.bolsee, 2011-07-26 21:07:50]

more comments. I haven't checked the do_version() function yet.

http://codereview.appspot.com/4289041/diff/50001/source/blender/blenkernel/in...
File source/blender/blenkernel/intern/material.c (right):

http://codereview.appspot.com/4289041/diff/50001/source/blender/blenkernel/in...
source/blender/blenkernel/intern/material.c:560: memmove((*matar), (*matar) + 1,
sizeof(void *) * ((*totcol) - (index + 1)));
Although this code is not part of the patch, it looks enormously like a bug.
Correct code should be
memmove((*matar)+index, (*matar)+(index+1), sizeof(void
*)*((*totcol)-(index+1)));

http://codereview.appspot.com/4289041/diff/50001/source/blender/blenkernel/in...
source/blender/blenkernel/intern/material.c:1542: flag &= ~TF_ALPHA << 15;
The correct expression is ~(TF_ALPHA << 15)
Always put paranthesis around shift operator as the precedence order is often
misleading.

http://codereview.appspot.com/4289041/diff/50001/source/blender/blenkernel/in...
source/blender/blenkernel/intern/material.c:1686: 
an optimization is possible here: if the MTFace mode don't change the material
settings, no need to create a new one.

http://codereview.appspot.com/4289041/diff/50001/source/blender/blenkernel/in...
source/blender/blenkernel/intern/material.c:1716: //			if(me->mat[a] == ma)
material_pop_id(&me->id, a);
Two problem:
1. You must break after material_pop_id (a material can only be present once
afaik)
2. Doesn't this screw up the material number for the mface?

[dfelinto, 2011-07-27 15:18:38]

reply to Benoit's second batch of review comments.

http://codereview.appspot.com/4289041/diff/50001/source/blender/blenkernel/in...
File source/blender/blenkernel/intern/material.c (right):

http://codereview.appspot.com/4289041/diff/50001/source/blender/blenkernel/in...
source/blender/blenkernel/intern/material.c:560: memmove((*matar), (*matar) + 1,
sizeof(void *) * ((*totcol) - (index + 1)));
I replied the original commit in bf-committers with your suggestion.
On 2011/07/26 21:07:50, benoit.bolsee wrote:
> Although this code is not part of the patch, it looks enormously like a bug.
> Correct code should be
> memmove((*matar)+index, (*matar)+(index+1), sizeof(void
> *)*((*totcol)-(index+1)));

http://codereview.appspot.com/4289041/diff/50001/source/blender/blenkernel/in...
source/blender/blenkernel/intern/material.c:1542: flag &= ~TF_ALPHA << 15;
ok, done. (will be present in the next patch)
On 2011/07/26 21:07:50, benoit.bolsee wrote:
> The correct expression is ~(TF_ALPHA << 15)
> Always put paranthesis around shift operator as the precedence order is often
> misleading.

http://codereview.appspot.com/4289041/diff/50001/source/blender/blenkernel/in...
source/blender/blenkernel/intern/material.c:1686: 
I do this for faces with no material (see check_tfaceneedmaterial()). The
problem with having this for every faces is that sometimes the material itself
has changes that would conflict with the tface options (even with the default
ones). E.g. the material is toggled as shadeless while the tface is with light.

On 2011/07/26 21:07:50, benoit.bolsee wrote:
> an optimization is possible here: if the MTFace mode don't change the material
> settings, no need to create a new one.

http://codereview.appspot.com/4289041/diff/50001/source/blender/blenkernel/in...
source/blender/blenkernel/intern/material.c:1716: //			if(me->mat[a] == ma)
material_pop_id(&me->id, a);
It does break things, that's why it's commented out.
But also because I couldn't decide on whether or not we want to remove the
original material (which is no longer being used) from the mesh so I left it
aside.
Any thoughts? (it's more a user POV question)

On 2011/07/26 21:07:50, benoit.bolsee wrote:
> Two problem:
> 1. You must break after material_pop_id (a material can only be present once
> afaik)
> 2. Doesn't this screw up the material number for the mface?

[dfelinto, 2011-07-27 16:30:43]

fix for alpha (from Benoit's comment) + fix for faces w no material (the mat was
being decoded 2x)

[dfelinto, 2011-07-27 21:58:24]

materialpop fix was committed, so now I uncommented the material pop function

[dfelinto, 2011-07-27 22:07:22]

http://codereview.appspot.com/4289041/diff/50001/source/blender/blenkernel/in...
File source/blender/blenkernel/intern/material.c (right):

http://codereview.appspot.com/4289041/diff/50001/source/blender/blenkernel/in...
source/blender/blenkernel/intern/material.c:1716: //			if(me->mat[a] == ma)
material_pop_id(&me->id, a);
uncommented the code (now that material_pop_id is working again).
replying to original problems:
1. we should NOT have a break if there is a match, because a mesh can indeed
have the same material in more than one mat. slot.

2. that might screw up with material number for mface ... although imo it should
be addressed inside material_pop_id if it's not.

*. what I'm wondering now is if once the material is popped out it doesn't mess
up with the for loop itself. It probably does. I'll run some tests and look at
the code more carefully.

On 2011/07/27 15:18:39, dfelinto wrote:
> It does break things, that's why it's commented out.
> But also because I couldn't decide on whether or not we want to remove the
> original material (which is no longer being used) from the mesh so I left it
> aside.
> Any thoughts? (it's more a user POV question)
> 
> On 2011/07/26 21:07:50, benoit.bolsee wrote:
> > Two problem:
> > 1. You must break after material_pop_id (a material can only be present once
> > afaik)
> > 2. Doesn't this screw up the material number for the mface?
>

[dfelinto, 2011-07-29 09:14:10]

fix for Benoit's suggestions (over email). waiting bug/patch #28111 to be
committed

[dfelinto, 2011-08-01 23:18:36]

update after mat_nr fix + convert mesh (UV) only once (using TF_CONVERTED): it's
all good now

[benoit.bolsee, 2011-08-08 15:49:23]

Hi Dalai,

I didn't have much time anymore to look into the patch but I looked at
some random files outside the material conversion and didn't find any
problem. That part of the code seems more systematic and less prone to
errors. I trust you have also tested that part more fully.
I see that SVN is now frozen for 2.59. Did you have a chance to push the
patch into trunk?

Regards,
Benoit


> -----Original Message-----
> From: dfelinto@gmail.com [mailto:dfelinto@gmail.com] 
> Sent: mardi 2 août 2011 1:19
> To: bf-codereview@blender.org; nathan.letwory@gmail.com; 
> brechtvanlommel@gmail.com; dingto@googlemail.com; 
> benoit.bolsee@online.be
> Cc: reply@codereview.appspotmail.com
> Subject: Re: BGE: Material replacement for texface options 
> (issue4289041)
> 
> 
> update after mat_nr fix + convert mesh (UV) only once (using
> TF_CONVERTED): it's all good now
> 
http://codereview.appspot.com/4289041/

[dfelinto, 2011-08-16 21:11:05]

create materials when no materials existent (Ton's suggestion) + mode doversion
inside liblink_mesh

[dfelinto, 2011-08-16 21:21:06]

I talked with Ton and he suggested me to do most of the conversion at load time
(in oppose to rely in the help menu).

After a big hack (setting G.main manually due to material.c rely heavily in
G.main :/) it's working now.

Final things missing:
- doversion/convert "Face Texture" to "Multitexture Material"
- remove texture face as a material mode
- code cleanup (now the doversion code is almost the same as in material.c, it
could probably be unified)
- add alert in Info Editor telling user to read the console.
- transp problem (old problem -- se above Brecht comments), edit mode need solid
+ alpha pass

I'm uploading new builds, I will run more tests before tackling the final
points. My goal is to have this in for the 1st 2.60 release).

Again, the only changes are in readfile.c and material.c

[dfelinto, 2011-09-02 05:41:18]

No change, only updating the patch to 2.60

[dfelinto, 2011-09-04 00:19:21]

doversion unified with material code. using BKE_report for error. next: alpha
fix and I'm finished.

[dfelinto, 2011-09-04 00:40:09]

Benoit,
I didn't commit it to 2.59. I trusted the code was good but didn't want to rush.
I consider 2.5x not bge ready anyways (not without pepper and cucumber fixes).

2.6 on the other hand will be a really good bge release imho. So I'm planning to
commit this right after cucumber gets it (or before, but I want to spare Daniel
from fixing eventual conflicts ;). I asked Brecht to give a final look at the
do_version code since I now moved the conversion code to inside the
lib_link_mesh (Janne's suggestion).

Talking with Mitchell I decided to skip conversion of the light flag as well.
The reason is: the light flag is not used by GLSL shade mode. So it would be
impossible to handle this without assuming the file was designed to work in the
mode it's on.

Finally, I think that there is actually no need to remove "Texture Face" from
the glsl/mt enum. After this patch if we want we can completely remove this
mode. But for now I prefer to leave this patch without addressing this.

I'll now look at the alpha problem (in edit mode). This should be the only thing
left. Although could even be fixed after the patch gets in.

* something I couldn't implement: I'm using BKE_reports to popup an error, but
it's printing the error in the console instead  :/ So I don't know if I should
use it instead of printf for the warnings as well.

[dfelinto, 2011-09-04 02:38:21]

"Final" patch - alpha working (Brecht pls see drawobject.c). couldn't get
BKE_report to popup error)

[dfelinto, 2011-09-12 08:24:00]

fixing bugs I found with my test files. one bug left: http://goo.gl/jExRs shows
solid in 3d view

[dfelinto, 2011-09-14 09:05:16]

fixed a bunch of corner cases, glsl and multitexture should be good now. need to
test more files

[dfelinto, 2011-09-15 09:16:26]

Rename TwoSided to Back Culling + set it on by default + fixes for multitexture
- final builds tmr

[dfelinto, 2011-09-15 23:27:47]

reverted twosided by default - now back cull is off bu default

[dfelinto, 2011-09-18 09:23:55]

final functional patch. doversion could be faster if I flag the material as
converted. but it work.

[dfelinto, 2011-09-19 07:17:40]

Final patch - to be committed tomorrow morning

[dfelinto, 2011-09-19 20:03:24]

Patch committed revision: 40372

I included in this commit a rename from the Shading Mode "Texture Face" to
"Singletexture". This can be discussed in parallel and rolled back if people
disagree. The reason for including this is to make the commit consistent on its
own, and now the only real difference between Multitexture and the former
Texture Face is the support for multiple textures in the later.