[Bf-codereview] Pepper to trunk merge (issue 4934047)

[dfelinto at gmail.com]

Started reviewing Mitchell's code. I need to finish later, I stopped in
the last file I commented.

Mitchell, Campbell already sent an email to him to take a look here. He
will step in once he find time. He is likely very busy.



http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Converter/BL_ActionActuator.cpp
File source/gameengine/Converter/BL_ActionActuator.cpp (right):

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Converter/BL_ActionActuator.cpp#newcode94
source/gameengine/Converter/BL_ActionActuator.cpp:94:
m_layer_weight(layer_weight),
i think it's better to stick to one name convention. either use _ as
separator or not.

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Converter/BL_ActionActuator.cpp#newcode142
source/gameengine/Converter/BL_ActionActuator.cpp:142: bool use_continue
= false;
I wonder if we should follow the convention pre-stablished in the
functin (i.e. bUseContinue) or rename old variables (which is probably a
bad idea since they are named like this in other files). imho it's
really bad to look at a code and feel it is a patched work.

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Converter/BL_ActionActuator.cpp#newcode144
source/gameengine/Converter/BL_ActionActuator.cpp:144: short play_mode =
BL_Action::ACT_MODE_PLAY;
why do we have play_mode and m_playtype? can't they follow the same
name? (m_playtype and playtime)?

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Converter/BL_ActionActuator.cpp#newcode145
source/gameengine/Converter/BL_ActionActuator.cpp:145: float start =
m_startframe, end = m_endframe;
i would avoid those double declarations with values. I don't recall this
being used anywhere in blender code before.

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Converter/BL_ActionActuator.cpp#newcode151
source/gameengine/Converter/BL_ActionActuator.cpp:151: // Convert
playmode
I would use switch instead of if/else if/else if/.../

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Converter/BL_ActionActuator.cpp#newcode158
source/gameengine/Converter/BL_ActionActuator.cpp:158: // We handle ping
pong ourselves to increase compabitility with the pre-Pepper actuator
this kind of comment will be hard to understand in 3 years from now. I
would say:
"We handle pingpong ourselves to increase backward compatibility with
old 3 non-unified animation actuators (Ipo/action/shapekeys)."

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Converter/BL_ActionActuator.cpp#newcode166
source/gameengine/Converter/BL_ActionActuator.cpp:166: m_localtime =
end;
no need for a new variable (although I suspect blender has a macro for
variable swapping).

Do instead:
m_localtime = start;
// swapping start with end time
start = end;
end=m_localtime;

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Converter/BL_ActionActuator.cpp#newcode179
source/gameengine/Converter/BL_ActionActuator.cpp:179: (m_playtype ==
ACT_ACTION_LOOP_STOP ||
macros: (you may need to include a BK_ header (fileutils iirc)
ELEM(m_playtime, ACT_ACTION_LOOP_STOP, ACT_ACTION_FLIPPER)

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Converter/BL_ActionActuator.cpp#newcode192
source/gameengine/Converter/BL_ActionActuator.cpp:192: if (use_continue
&& m_flag & ACT_FLAG_ACTIVE)
- if (use_continue && m_flag & ACT_FLAG_ACTIVE)
+ if (use_continue && (m_flag & ACT_FLAG_ACTIVE))

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Converter/BL_ActionActuator.cpp#newcode203
source/gameengine/Converter/BL_ActionActuator.cpp:203: if (m_playtype ==
ACT_ACTION_PLAY)
I don't see why you need to set this everyframe.
an option is to have it set in the python mode function (and in the
flipper after it's converted to a play actuator) -- let's hope no one is
checking the action type from python during this time ;)

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Converter/BL_ActionActuator.cpp#newcode236
source/gameengine/Converter/BL_ActionActuator.cpp:236: else if
(m_playtype == ACT_ACTION_LOOP_END || m_playtype == ACT_ACTION_PINGPONG)
again, use switch here. and use:
case ACT_ACTION_PINGPONG:
     m_flag ^= ACT_FLAG_REVERSE;
[no break here so pingpong can use the following code]

case ACT_ACTION_LOOP_END:
     // Convert into a play and let it finish
     obj->SetPlayMode(m_layer, BL_Action::ACT_MODE_PLAY);
     m_flag |= ACT_FLAG_PLAY_END;
     break;
case ACT_ACTION_FLIPPER: ...)

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Converter/BL_ActionActuator.h
File source/gameengine/Converter/BL_ActionActuator.h (right):

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Converter/BL_ActionActuator.h#newcode152
source/gameengine/Converter/BL_ActionActuator.h:152: ACT_FLAG_PLAY_END	=
1<<5
I'm not sure C++ allows to, but if you can put a comma in the end of
this line you make it easier for new elements to be added later (a patch
can come in without changing the previous line, making the svn blame
more useful ;)

-ACT_FLAG_PLAY_END»      = 1<<5
+ACT_FLAG_PLAY_END»      = 1<<5,

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Converter/BL_ArmatureObject.cpp
File source/gameengine/Converter/BL_ArmatureObject.cpp (right):

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Converter/BL_ArmatureObject.cpp#newcode141
source/gameengine/Converter/BL_ArmatureObject.cpp:141: {
detail but, remember to meld/svn base the files before commit. this is a
classic example of unnecessary change

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Converter/BL_ArmatureObject.cpp#newcode147
source/gameengine/Converter/BL_ArmatureObject.cpp:147: short mode =
ACTSTRIPMODE_BLEND;
? seriously, why bother setting up a "mode" variable if the old code is
removed anyways?
If you think it's relevant to keep the old switch structure for further
reference,implementations why don't you comment it out completely, add
some notes, and add the actual code (dstweight = 1.0F - srcweight;)
outside the switch?

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Converter/BL_ArmatureObject.h
File source/gameengine/Converter/BL_ArmatureObject.h (right):

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Converter/BL_ArmatureObject.h#newcode142
source/gameengine/Converter/BL_ArmatureObject.h:142:
int		m_vert_deform_type;
I guess we do need some code guidelines for Blender. otherwise using tab
for alignment of variables is senseless.

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

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Converter/BL_BlenderDataConversion.cpp#newcode1812
source/gameengine/Converter/BL_BlenderDataConversion.cpp:1812:
arm->gevertdeformer
I would keep the blender scene as the last argument of the function and
squeze in your new variable before it.

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Converter/BL_DeformableGameObject.cpp
File source/gameengine/Converter/BL_DeformableGameObject.cpp (right):

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Converter/BL_DeformableGameObject.cpp#newcode94
source/gameengine/Converter/BL_DeformableGameObject.cpp:94: // has
relative keys
you are running shape_deformer->GetKey() 2 + len(kb) times here.

I don't think compilers optimize that (sure it's C++ and super fast, but
still).
I would create a 'key' variable in the begin to use along the code.

http://codereview.appspot.com/4934047/