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

[dfelinto at gmail.com]

reply to Benoit's second batch of review comments.


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

http://codereview.appspot.com/4289041/diff/50001/source/blender/blenkernel/intern/material.c#newcode560
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/intern/material.c#newcode1542
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/intern/material.c#newcode1686
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/intern/material.c#newcode1716
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?

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