Pepper to trunk merge

[brechtvl, 2011-08-23 12:36:28]

Reply to test bf-codereview mail.

[ideasman42, 2011-08-23 18:39:45]

Went over parts of this patch.

First Pass Code Review:
... this means reading over the patch, checking code but not functionality
testing yet.

- Speaker
- Sound
- Animsys
- ... most files except for the ones listed below as having not looked over.


Did NOT Review:

- Audaspace (since neXyon is the author)
- Collada (best Nathan review)
- Game Engine Animation Code

http://codereview.appspot.com/4934047/diff/6043/source/blender/blenkernel/int...
File source/blender/blenkernel/intern/anim_sys.c (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/blenkernel/int...
source/blender/blenkernel/intern/anim_sys.c:1171: if
(RNA_struct_is_a(new_ptr.type, &RNA_PoseBone)) {
for speed in this case I think you'd be better just to use a comparison
(new_ptr.type == &RNA_PoseBone) since we know PoseBone's aren't subclassed. 

Since for non posebone types it will search up their type's bases.

http://codereview.appspot.com/4934047/diff/6043/source/blender/blenkernel/int...
File source/blender/blenkernel/intern/sequencer.c (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/blenkernel/int...
source/blender/blenkernel/intern/sequencer.c:3149: void
seq_update_sound_bounds_all(Scene *scene)
Being picky, but, would be good to use the style used in the rest of the file.

http://codereview.appspot.com/4934047/diff/6043/source/blender/blenkernel/int...
File source/blender/blenkernel/intern/sound.c (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/blenkernel/int...
source/blender/blenkernel/intern/sound.c:663: for(ob = bmain->object.first; ob;
ob = ob->id.next)
This looks wrong, its playing all objects sounds in the blend file, not just
ones in the scene.

Would be nice to loop over objects in `set` scenes too.

http://codereview.appspot.com/4934047/diff/6043/source/blender/editors/animat...
File source/blender/editors/animation/drivers.c (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/editors/animat...
source/blender/editors/animation/drivers.c:386: /* flags - on a
per-relevant-flag basis */
shouldn't this comment be removed?

http://codereview.appspot.com/4934047/diff/6043/source/blender/editors/armatu...
File source/blender/editors/armature/poseobject.c (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/editors/armatu...
source/blender/editors/armature/poseobject.c:1191: /* if selOnly option is
enabled, if user hasn't selected any bones,
don't think this is a good convention for tools in blender, would prefer to see
a warning report so the user knows they need to select.

http://codereview.appspot.com/4934047/diff/6043/source/blender/editors/sound/...
File source/blender/editors/sound/sound_ops.c (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/editors/sound/...
source/blender/editors/sound/sound_ops.c:649: void
SOUND_OT_update_animation_flags(wmOperatorType *ot)
Why is this operator needed?
This looks like the kind of function that blender should handle and not be
exposed to the user.

Assume its there for a reason so would be good to include as a comment.

http://codereview.appspot.com/4934047/diff/6043/source/blender/editors/space_...
File source/blender/editors/space_nla/nla_draw.c (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/editors/space_...
source/blender/editors/space_nla/nla_draw.c:474: if (strip->flag &
NLASTRIP_FLAG_REVERSE)
should be removed/commented or made into different strings.

http://codereview.appspot.com/4934047/diff/6043/source/blender/makesdna/DNA_s...
File source/blender/makesdna/DNA_sequence_types.h (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/makesdna/DNA_s...
source/blender/makesdna/DNA_sequence_types.h:230: typedef struct TitleCardVars {
prefer this be split off as a separate patch, or at least approved by peter
before merging.

http://codereview.appspot.com/4934047/diff/6043/source/blender/makesrna/inter...
File source/blender/makesrna/intern/rna_access.c (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/makesrna/inter...
source/blender/makesrna/intern/rna_access.c:1424: void
RNA_property_update_cache_add(PointerRNA *ptr, PropertyRNA *prop)
IMHO this should eventually cache based on unique pointer triplets:
  (ptr.id.data, ptr.data, update_func)

http://codereview.appspot.com/4934047/diff/6043/source/blender/makesrna/inter...
File source/blender/makesrna/intern/rna_userdef.c (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/makesrna/inter...
source/blender/makesrna/intern/rna_userdef.c:2954: prop= RNA_def_property(srna,
"use_update_recent_files_on_load", PROP_BOOLEAN, PROP_NONE);
While its not a big deal, -1 for making this a preference.

also rather changes like this are not made in a branch.

[brechtvl, 2011-08-24 14:28:30]

Quickly read over the code once, no proper review yet, but here are some
comments.

http://codereview.appspot.com/4934047/diff/6043/release/scripts/modules/mocap...
File release/scripts/modules/mocap_constraints.py (right):

http://codereview.appspot.com/4934047/diff/6043/release/scripts/modules/mocap...
release/scripts/modules/mocap_constraints.py:1: # ##### BEGIN GPL LICENSE BLOCK
#####
The mocap tools are already merged as an addon in bf-extensions, so don't forget
to leave these out of the merge (they could be removed in the pepper branch).

http://codereview.appspot.com/4934047/diff/6043/release/scripts/startup/bl_ui...
File release/scripts/startup/bl_ui/properties_data_armature.py (right):

http://codereview.appspot.com/4934047/diff/6043/release/scripts/startup/bl_ui...
release/scripts/startup/bl_ui/properties_data_armature.py:103: class
DATA_PT_bone_group_specials(Menu):
Change _PT_ to _MT_ since it's a menu, not a panel.

http://codereview.appspot.com/4934047/diff/6043/source/blender/blenkernel/int...
File source/blender/blenkernel/intern/depsgraph.c (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/blenkernel/int...
source/blender/blenkernel/intern/depsgraph.c:2068: if (adt->drivers.first)
This seems like a workaround for missing dependency graph relations? The driver
should add a relation and then the update flags should get flushed. They will be
missing if the object is being driven by a non-object datablock, but if that's
what this is intended to work around, perhaps that should be clarified in the
comment.

http://codereview.appspot.com/4934047/diff/6043/source/blender/blenkernel/int...
File source/blender/blenkernel/intern/fmodifier.c (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/blenkernel/int...
source/blender/blenkernel/intern/fmodifier.c:1212: * NOTE: this is really just a
hack so that we don't need to version patch old files ;)
I think this should just be version patched, and the use_influence property
removed.

http://codereview.appspot.com/4934047/diff/6043/source/blender/blenloader/int...
File source/blender/blenloader/intern/readfile.c (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/blenloader/int...
source/blender/blenloader/intern/readfile.c:11752: /* put compatibility code
here until next subversion bump */
Some of these changes need a subversion bump to avoid overwriting changes made
by the user, just noting so that whoever does the merge doesn't forget this.

http://codereview.appspot.com/4934047/diff/6043/source/blender/makesrna/inter...
File source/blender/makesrna/intern/rna_armature.c (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/makesrna/inter...
source/blender/makesrna/intern/rna_armature.c:870: prop= RNA_def_property(srna,
"vert_deformer", PROP_ENUM, PROP_NONE);
Convention is to avoid such abbreviations in rna property names. It might also
be good to give a hint in the comment about which features are (not) supported
in the optimized code.

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Ketsji/BL_A...
File source/gameengine/Ketsji/BL_ActionManager.cpp (right):

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Ketsji/BL_A...
source/gameengine/Ketsji/BL_ActionManager.cpp:46: if (m_layers[layer])
The "if (m_layers[layer])" checks seem a bit inconsistent, they are done in some
functions and not in others, but it seems that these pointers are never null.

[brechtvl, 2011-08-25 14:43:57]

Did a superficial review of collada code, found some small issues, will leave
more detailed review to Nathan.

http://codereview.appspot.com/4934047/diff/6043/source/blender/collada/Animat...
File source/blender/collada/AnimationExporter.cpp (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/collada/Animat...
source/blender/collada/AnimationExporter.cpp:36: 
This function doesn't take into account the export_selected setting, is that
intentional? Also e.g. forEachMeshObjectInScene checks the scene layers, but
e.g. for lights it's not done, it's not clear to me that these functions are
consistent.

http://codereview.appspot.com/4934047/diff/6043/source/blender/collada/Animat...
source/blender/collada/AnimationExporter.cpp:44: {
There's some strange indenting going on here, everything below this line seems
indented for no particular reason.

http://codereview.appspot.com/4934047/diff/6043/source/blender/collada/Animat...
source/blender/collada/AnimationExporter.cpp:71: if (!ob->data) return;
This test is unnecessary.

http://codereview.appspot.com/4934047/diff/6043/source/blender/collada/Animat...
source/blender/collada/AnimationExporter.cpp:170: float *quat =
(float*)MEM_callocN(sizeof(float) * fcu->totvert * 4, "quat output source
values");
Memory leak: quat is never freed.

http://codereview.appspot.com/4934047/diff/6043/source/blender/collada/Animat...
source/blender/collada/AnimationExporter.cpp:286: float * eul_axis =
(float*)MEM_callocN(sizeof(float) * fcu->totvert, "quat output source values");
Memory leak: eul and eul_axis are never freed.

http://codereview.appspot.com/4934047/diff/6043/source/blender/collada/Animat...
File source/blender/collada/AnimationImporter.cpp (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/collada/Animat...
source/blender/collada/AnimationImporter.cpp:182: cu->bezt[i].vec[1][0];
This statement does nothing, remove.

http://codereview.appspot.com/4934047/diff/6043/source/blender/collada/Animat...
source/blender/collada/AnimationImporter.cpp:192: else act = ob->adt->action;
These two lines and others further in this file can be simplified to just "act =
verify_adt_action((ID*)&ob->id, 1);"

http://codereview.appspot.com/4934047/diff/6043/source/blender/collada/Animat...
source/blender/collada/AnimationImporter.cpp:442: fcu->rna_path =
BLI_strdupn(rna_path, strlen(rna_path));
Can just use BLI_strdup(rna_path).

[brechtvl, 2011-08-26 13:00:20]

Did a basic review the game engine file changes. One thing I'm not sure about is
the Restrict Animation FPS feature. It can be faster of course, but it seems is
bound to give stuttering? It's not clear to me when this feature would be useful
to enable.

http://codereview.appspot.com/4934047/diff/6043/release/scripts/startup/bl_ui...
File release/scripts/startup/bl_ui/properties_game.py (right):

http://codereview.appspot.com/4934047/diff/6043/release/scripts/startup/bl_ui...
release/scripts/startup/bl_ui/properties_game.py:346: row.prop(gs,
"restrict_animation_updates")
These three buttons don't really fit one the same row, it doesn't show the full
text.

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Ketsji/CMak...
File source/gameengine/Ketsji/CMakeLists.txt (right):

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Ketsji/CMak...
source/gameengine/Ketsji/CMakeLists.txt:78: ../../../source/blender/gpu
Why were all these include paths added here, they mostly duplicate existing
ones?

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Ketsji/KX_K...
File source/gameengine/Ketsji/KX_KetsjiEngine.cpp (right):

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Ketsji/KX_K...
source/gameengine/Ketsji/KX_KetsjiEngine.cpp:1011: // nothing to do here,
everything relative now...
I guess this code can just be removed then?

[brechtvl, 2011-08-26 13:01:43]

Regarding the Audio panel for scenes, I get this python error when compiling
without ffmpeg:

/Users/brecht/dev/review/build/bin/blender.app/Contents/MacOS/2.59/scripts/startup/bl_ui/properties_scene.py:72
rna_uiItemR: property not found: RenderSettings.ffmpeg_audio_channels

[jesterKing, 2011-08-26 13:57:10]

Some small adds. Further: indentation is somewhat inconsistent over all files.
Just like naming of enums, functions and vars. I suggest this be fixed after
merge, esp. naming issues as these are planned for fixup in trunk at some point
anyway.

Armature animation baking should be ported from python still, but we agreed with
student he'd be doing that in the 3 months after GSoC.

http://codereview.appspot.com/4934047/diff/6043/source/blender/collada/Animat...
File source/blender/collada/AnimationExporter.cpp (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/collada/Animat...
source/blender/collada/AnimationExporter.cpp:36: 
On 2011/08/25 14:43:58, brechtvl wrote:
> This function doesn't take into account the export_selected setting, is that
> intentional? Also e.g. forEachMeshObjectInScene checks the scene layers, but
> e.g. for lights it's not done, it's not clear to me that these functions are
> consistent.

Indeed this is somewhat inconsistent. It's a problem mostly coming from trunk,
which should be addressed there. I propose to do that after code has been
merged.

http://codereview.appspot.com/4934047/diff/6043/source/blender/collada/Animat...
source/blender/collada/AnimationExporter.cpp:142: /* Old System
This commented-out code can be removed

[Moguri, 2011-08-26 19:49:55]

Here are my responses to the game engine comments so far. Also, would there be
any objections to me adding Benoit as a "reviewer" for the BGE animations part
of the patch?

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Ketsji/BL_A...
File source/gameengine/Ketsji/BL_ActionManager.cpp (right):

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Ketsji/BL_A...
source/gameengine/Ketsji/BL_ActionManager.cpp:46: if (m_layers[layer])
Initially not every layer was filled like it is now. The NULL checks came from
then and weren't removed when the code changed. They can be removed.

On 2011/08/24 14:28:30, brechtvl wrote:
> The "if (m_layers[layer])" checks seem a bit inconsistent, they are done in
some
> functions and not in others, but it seems that these pointers are never null.

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Ketsji/CMak...
File source/gameengine/Ketsji/CMakeLists.txt (right):

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Ketsji/CMak...
source/gameengine/Ketsji/CMakeLists.txt:78: ../../../source/blender/gpu
If you do an SVN blame, it looks like the result of a messed up merge. I think
the paths were cleaned up a bit in trunk, and the "new" paths shown are the old
versions.

On 2011/08/26 13:00:20, brechtvl wrote:
> Why were all these include paths added here, they mostly duplicate existing
> ones?

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Ketsji/KX_K...
File source/gameengine/Ketsji/KX_KetsjiEngine.cpp (right):

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Ketsji/KX_K...
source/gameengine/Ketsji/KX_KetsjiEngine.cpp:1011: // nothing to do here,
everything relative now...
This is a change from neXyon, but I'm going to guess it can be removed now.
Should DoSound() also be removed from KX_KetsjiEngine then?

On 2011/08/26 13:00:20, brechtvl wrote:
> I guess this code can just be removed then?

[dfelinto, 2011-08-27 09:19:57]

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/B...
File source/gameengine/Converter/BL_ActionActuator.cpp (right):

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Converter/B...
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/B...
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/B...
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/B...
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/B...
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/B...
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/B...
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/B...
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/B...
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/B...
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/B...
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/B...
File source/gameengine/Converter/BL_ActionActuator.h (right):

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Converter/B...
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/B...
File source/gameengine/Converter/BL_ArmatureObject.cpp (right):

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Converter/B...
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/B...
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/B...
File source/gameengine/Converter/BL_ArmatureObject.h (right):

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Converter/B...
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/B...
File source/gameengine/Converter/BL_BlenderDataConversion.cpp (right):

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Converter/B...
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/B...
File source/gameengine/Converter/BL_DeformableGameObject.cpp (right):

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Converter/B...
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.

[Moguri, 2011-08-28 08:39:37]

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

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Converter/B...
source/gameengine/Converter/BL_ActionActuator.cpp:203: if (m_playtype ==
ACT_ACTION_PLAY)
This isn't set every frame, just every time a new action is played.

On 2011/08/27 09:19:57, dfelinto wrote:
> 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/B...
File source/gameengine/Converter/BL_ActionActuator.h (right):

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Converter/B...
source/gameengine/Converter/BL_ActionActuator.h:152: ACT_FLAG_PLAY_END	= 1<<5
I thought I had tested this and C++ didn't allow it. However, after running
another test, it seems fine with it (at least msvc). I know I leave commas like
you suggest all the time in Python.

On 2011/08/27 09:19:57, dfelinto wrote:
> 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/B...
File source/gameengine/Converter/BL_ArmatureObject.cpp (right):

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Converter/B...
source/gameengine/Converter/BL_ArmatureObject.cpp:147: short mode =
ACTSTRIPMODE_BLEND;
If you look, the mode variable was already there, it was just moved. The changes
to this function shouldn't have gotten committed as they were bits of code I was
playing with earlier but decided not to use.

On 2011/08/27 09:19:57, dfelinto wrote:
> ? 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?

[theeth, 2011-08-28 16:23:18]

First pass review

http://codereview.appspot.com/4934047/diff/6043/intern/audaspace/FX/AUD_BaseI...
File intern/audaspace/FX/AUD_BaseIIRFilterReader.cpp (right):

http://codereview.appspot.com/4934047/diff/6043/intern/audaspace/FX/AUD_BaseI...
intern/audaspace/FX/AUD_BaseIIRFilterReader.cpp:36: #define CC m_specs.channels
+ m_channel
Should be undefed at the end of the file to be safe.

http://codereview.appspot.com/4934047/diff/6043/intern/audaspace/intern/AUD_3...
File intern/audaspace/intern/AUD_3DMath.h (right):

http://codereview.appspot.com/4934047/diff/6043/intern/audaspace/intern/AUD_3...
intern/audaspace/intern/AUD_3DMath.h:160: inline AUD_Vector3 operator*(const
float& op) const
Why use a reference to the float?

http://codereview.appspot.com/4934047/diff/6043/intern/audaspace/intern/AUD_R...
File intern/audaspace/intern/AUD_Reference.h (right):

http://codereview.appspot.com/4934047/diff/6043/intern/audaspace/intern/AUD_R...
intern/audaspace/intern/AUD_Reference.h:123: m_original = 0;
I'd prefer NULL instead of 0 for clarity sake.

Same for the other occurrences through the file.

http://codereview.appspot.com/4934047/diff/6043/intern/audaspace/intern/AUD_R...
File intern/audaspace/intern/AUD_ReferenceHandler.cpp (right):

http://codereview.appspot.com/4934047/diff/6043/intern/audaspace/intern/AUD_R...
intern/audaspace/intern/AUD_ReferenceHandler.cpp:33: std::map<void*, int>
AUD_ReferenceHandler::m_references;
unsigned int would be good too.

http://codereview.appspot.com/4934047/diff/6043/release/scripts/startup/bl_ui...
File release/scripts/startup/bl_ui/properties_data_speaker.py (right):

http://codereview.appspot.com/4934047/diff/6043/release/scripts/startup/bl_ui...
release/scripts/startup/bl_ui/properties_data_speaker.py:32: return
context.speaker and (engine in cls.COMPAT_ENGINES)
It would be nice to have a decorator for engine compatibility, that code is
everywhere way often.

http://codereview.appspot.com/4934047/diff/6043/source/blender/editors/transf...
File source/blender/editors/transform/transform_generics.c (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/editors/transf...
source/blender/editors/transform/transform_generics.c:846:
recalcData_actedit(t);
Spring cleaning, great.

http://codereview.appspot.com/4934047/diff/6043/source/blender/makesdna/DNA_s...
File source/blender/makesdna/DNA_speaker_types.h (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/makesdna/DNA_s...
source/blender/makesdna/DNA_speaker_types.h:42: short flag;
I'd suggest moving flag at the end of the struct, this way the alignment padding
is at the end.

http://codereview.appspot.com/4934047/diff/6043/source/blender/makesrna/inter...
File source/blender/makesrna/intern/rna_sound.c (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/makesrna/inter...
source/blender/makesrna/intern/rna_sound.c:99: RNA_def_property_ui_text(prop,
"Mono", "If the file contains multiple audio channels they are mixdown to a
signle one.");
signle -> single

Also, mixdown in this context sounds like bad grammar, but I'm not sure what
would be better: 'mixed down' is weird and 'reduced' might be a bit too generic.

http://codereview.appspot.com/4934047/diff/6043/source/blender/makesrna/inter...
File source/blender/makesrna/intern/rna_userdef.c (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/makesrna/inter...
source/blender/makesrna/intern/rna_userdef.c:2954: prop= RNA_def_property(srna,
"use_update_recent_files_on_load", PROP_BOOLEAN, PROP_NONE);
Agreed.

If needed, I think the proper solution would be to keep two separate lists
(recent opened, recent saved) and display the one that makes sense in context.

[aligorith, 2011-08-29 03:03:04]

http://codereview.appspot.com/4934047/diff/6043/source/blender/blenkernel/int...
File source/blender/blenkernel/intern/anim_sys.c (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/blenkernel/int...
source/blender/blenkernel/intern/anim_sys.c:1171: if
(RNA_struct_is_a(new_ptr.type, &RNA_PoseBone)) {
On 2011/08/23 18:39:45, ideasman42 wrote:
> for speed in this case I think you'd be better just to use a comparison
> (new_ptr.type == &RNA_PoseBone) since we know PoseBone's aren't subclassed. 
> 
> Since for non posebone types it will search up their type's bases.
Good point. Will do

http://codereview.appspot.com/4934047/diff/6043/source/blender/blenkernel/int...
File source/blender/blenkernel/intern/depsgraph.c (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/blenkernel/int...
source/blender/blenkernel/intern/depsgraph.c:2068: if (adt->drivers.first)
On 2011/08/24 14:28:30, brechtvl wrote:
> This seems like a workaround for missing dependency graph relations? The
driver
> should add a relation and then the update flags should get flushed. They will
be
> missing if the object is being driven by a non-object datablock, but if that's
> what this is intended to work around, perhaps that should be clarified in the
> comment.
Indeed, this is a workaround for missing relations from all manner of non-object
datablocks. In this particular case, it was just the current-frame in scene
which people often used, but as with many things like this, it extended to
everything else.

Will clarify the comment

http://codereview.appspot.com/4934047/diff/6043/source/blender/blenkernel/int...
File source/blender/blenkernel/intern/fmodifier.c (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/blenkernel/int...
source/blender/blenkernel/intern/fmodifier.c:1212: * NOTE: this is really just a
hack so that we don't need to version patch old files ;)
On 2011/08/24 14:28:30, brechtvl wrote:
> I think this should just be version patched, and the use_influence property
> removed.
I considered that, but to cover each of the places where these can live and
update them involves quite a bit of effort, which IMO isn't worth it.

Furthermore, this was assumed to not be frequently used enough that this setting
would be a hassle (or new FModifiers could always get created with this
enabled).

http://codereview.appspot.com/4934047/diff/6043/source/blender/editors/animat...
File source/blender/editors/animation/drivers.c (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/editors/animat...
source/blender/editors/animation/drivers.c:386: /* flags - on a
per-relevant-flag basis */
On 2011/08/23 18:39:45, ideasman42 wrote:
> shouldn't this comment be removed?

Yep. Will do

http://codereview.appspot.com/4934047/diff/6043/source/blender/editors/space_...
File source/blender/editors/space_nla/nla_draw.c (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/editors/space_...
source/blender/editors/space_nla/nla_draw.c:474: if (strip->flag &
NLASTRIP_FLAG_REVERSE)
On 2011/08/23 18:39:45, ideasman42 wrote:
> should be removed/commented or made into different strings.

Huh?

IIRC, I was still playing around with representations of reversed vs normal
order here and left this lying around. While not a biggie, I'll probably just
remove this.

http://codereview.appspot.com/4934047/diff/6043/source/blender/makesrna/inter...
File source/blender/makesrna/intern/rna_access.c (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/makesrna/inter...
source/blender/makesrna/intern/rna_access.c:1424: void
RNA_property_update_cache_add(PointerRNA *ptr, PropertyRNA *prop)
On 2011/08/23 18:39:45, ideasman42 wrote:
> IMHO this should eventually cache based on unique pointer triplets:
>   (ptr.id.data, ptr.data, update_func)
"Eventually", but only if we ever find ourselves with such many such update
functions to justify needing to store the different ptr.data's. 

But if and when this happens, we'd probably want to be able to flag the update
functions as requiring id only or both, so that we can optimise for all those
current cases where update only reads id pointer to tag depsgraph on that ID.

[nexyon, 2011-08-29 17:56:50]

Note: Merge is already scheduled for tomorrow between 8-10 am UTC in case there
are no real merge/show stoppers with Joshua, Brecht and Nathan ready to help in
case there are any problems.

Minor things can be fixed in trunk too and shouldn't unnecessarily delay
merging.

http://codereview.appspot.com/4934047/diff/6043/intern/audaspace/FX/AUD_BaseI...
File intern/audaspace/FX/AUD_BaseIIRFilterReader.cpp (right):

http://codereview.appspot.com/4934047/diff/6043/intern/audaspace/FX/AUD_BaseI...
intern/audaspace/FX/AUD_BaseIIRFilterReader.cpp:36: #define CC m_specs.channels
+ m_channel
Why exactly? I have never heard about such a define influencing anything else
than the file it is defined in and this is a .cpp that never gets included
anywhere.

http://codereview.appspot.com/4934047/diff/6043/intern/audaspace/intern/AUD_3...
File intern/audaspace/intern/AUD_3DMath.h (right):

http://codereview.appspot.com/4934047/diff/6043/intern/audaspace/intern/AUD_3...
intern/audaspace/intern/AUD_3DMath.h:160: inline AUD_Vector3 operator*(const
float& op) const
References are generally used for operator overloading when objects are used as
pointers are faster than copying a full object. In this case it's true that a
float might be faster than a pointer and smaller on 64 bit machines, but this
operator is inline, so it doesn't matter anyway, the reference might even make
it easier for the compiler to recognize it can use the original float.

http://codereview.appspot.com/4934047/diff/6043/intern/audaspace/intern/AUD_R...
File intern/audaspace/intern/AUD_Reference.h (right):

http://codereview.appspot.com/4934047/diff/6043/intern/audaspace/intern/AUD_R...
intern/audaspace/intern/AUD_Reference.h:123: m_original = 0;
The reason I use 0 in this file is that NULL is not defined and I don't really
like to include a standard header just to get NULL defined, but if you really
want to, just tell me which header I should include best and I'll change it.

http://codereview.appspot.com/4934047/diff/6043/release/scripts/startup/bl_ui...
File release/scripts/startup/bl_ui/properties_data_speaker.py (right):

http://codereview.appspot.com/4934047/diff/6043/release/scripts/startup/bl_ui...
release/scripts/startup/bl_ui/properties_data_speaker.py:32: return
context.speaker and (engine in cls.COMPAT_ENGINES)
I guess this is something general regarding the blender python UI source,
Campbell might be interested, but it's not really relevant for this specific
review, is it?

http://codereview.appspot.com/4934047/diff/6043/source/blender/blenkernel/int...
File source/blender/blenkernel/intern/sound.c (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/blenkernel/int...
source/blender/blenkernel/intern/sound.c:663: for(ob = bmain->object.first; ob;
ob = ob->id.next)
On 2011/08/23 18:39:45, ideasman42 wrote:
> This looks wrong, its playing all objects sounds in the blend file, not just
> ones in the scene.

Fixed this by iterating over scene's bases now.

> Would be nice to loop over objects in `set` scenes too.

Don't understand what you mean here.

http://codereview.appspot.com/4934047/diff/6043/source/blender/editors/sound/...
File source/blender/editors/sound/sound_ops.c (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/editors/sound/...
source/blender/editors/sound/sound_ops.c:649: void
SOUND_OT_update_animation_flags(wmOperatorType *ot)
The problem is: Where to do this in blender? Doing it every frame change for
example is quite a waste of time and according to joshua id_data_find_fcurve
isn't too fast.

http://codereview.appspot.com/4934047/diff/6043/source/blender/makesdna/DNA_s...
File source/blender/makesdna/DNA_speaker_types.h (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/makesdna/DNA_s...
source/blender/makesdna/DNA_speaker_types.h:42: short flag;
Hmm, I thought it would be better to keep flag near the "head" of the struct for
the reason it's there in most other of blender's structs. But can change it for
sure, if you disagree.

[ideasman42, 2011-08-29 21:07:54]

follow up on some comments from nexyon.

http://codereview.appspot.com/4934047/diff/6043/release/scripts/startup/bl_ui...
File release/scripts/startup/bl_ui/properties_data_speaker.py (right):

http://codereview.appspot.com/4934047/diff/6043/release/scripts/startup/bl_ui...
release/scripts/startup/bl_ui/properties_data_speaker.py:32: return
context.speaker and (engine in cls.COMPAT_ENGINES)
yep, dont worry about fixing this in the branch.

http://codereview.appspot.com/4934047/diff/6043/source/blender/blenkernel/int...
File source/blender/blenkernel/intern/sound.c (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/blenkernel/int...
source/blender/blenkernel/intern/sound.c:663: for(ob = bmain->object.first; ob;
ob = ob->id.next)
Scenes can show set scenes - a background set.

See SETLOOPER macro, but again, this can wait until after the merge.

http://codereview.appspot.com/4934047/diff/6043/source/blender/editors/sound/...
File source/blender/editors/sound/sound_ops.c (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/editors/sound/...
source/blender/editors/sound/sound_ops.c:649: void
SOUND_OT_update_animation_flags(wmOperatorType *ot)
when inserting and removing keyframes on seq data?

- the sequencer really doesn't work well with the animation system :S, but think
it would be good to add a comment here about why this is needed and why its not
simple to do automatically.

[theeth, 2011-08-29 22:14:05]

http://codereview.appspot.com/4934047/diff/6043/intern/audaspace/FX/AUD_BaseI...
File intern/audaspace/FX/AUD_BaseIIRFilterReader.cpp (right):

http://codereview.appspot.com/4934047/diff/6043/intern/audaspace/FX/AUD_BaseI...
intern/audaspace/FX/AUD_BaseIIRFilterReader.cpp:36: #define CC m_specs.channels
+ m_channel
It will actually affect the whole of the compilation unit, so it depends how the
build system is setup (if it calls the compiler on individual files or if it
lumps them together).

http://codereview.appspot.com/4934047/diff/6043/intern/audaspace/intern/AUD_3...
File intern/audaspace/intern/AUD_3DMath.h (right):

http://codereview.appspot.com/4934047/diff/6043/intern/audaspace/intern/AUD_3...
intern/audaspace/intern/AUD_3DMath.h:160: inline AUD_Vector3 operator*(const
float& op) const
The inline keyword is only a hint to the compiler, it's not forced to do it.

In any case, not a bit deal, I was just asking to see if there was a specific
reason (which could be the case with template specification but that's obviously
not the case here).

http://codereview.appspot.com/4934047/diff/6043/intern/audaspace/intern/AUD_R...
File intern/audaspace/intern/AUD_Reference.h (right):

http://codereview.appspot.com/4934047/diff/6043/intern/audaspace/intern/AUD_R...
intern/audaspace/intern/AUD_Reference.h:123: m_original = 0;
NULL is defined in stddef.h, you can just include that. I don't feel like
wanting to minimize external includes is a good enough reason in this case.
Especially since it affects code clarity a lot.

http://codereview.appspot.com/4934047/diff/6043/release/scripts/startup/bl_ui...
File release/scripts/startup/bl_ui/properties_data_speaker.py (right):

http://codereview.appspot.com/4934047/diff/6043/release/scripts/startup/bl_ui...
release/scripts/startup/bl_ui/properties_data_speaker.py:32: return
context.speaker and (engine in cls.COMPAT_ENGINES)
I meant that as a general remark. No need to change anything here as Campbell
said.

http://codereview.appspot.com/4934047/diff/6043/source/blender/makesdna/DNA_s...
File source/blender/makesdna/DNA_speaker_types.h (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/makesdna/DNA_s...
source/blender/makesdna/DNA_speaker_types.h:42: short flag;
It's not that big a deal, I just prefer padding at the end of a struct than in
the middle. When it's in the middle, it happens often that people add new
members at the end without reusing the padding space, growing the struct more
than they would need.