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

brechtvanlommel at gmail.com brechtvanlommel at gmail.com
Wed Aug 24 16:28:30 CEST 2011


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_constraints.py
File release/scripts/modules/mocap_constraints.py (right):

http://codereview.appspot.com/4934047/diff/6043/release/scripts/modules/mocap_constraints.py#newcode1
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/properties_data_armature.py
File release/scripts/startup/bl_ui/properties_data_armature.py (right):

http://codereview.appspot.com/4934047/diff/6043/release/scripts/startup/bl_ui/properties_data_armature.py#newcode103
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/intern/depsgraph.c
File source/blender/blenkernel/intern/depsgraph.c (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/blenkernel/intern/depsgraph.c#newcode2068
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/intern/fmodifier.c
File source/blender/blenkernel/intern/fmodifier.c (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/blenkernel/intern/fmodifier.c#newcode1212
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/intern/readfile.c
File source/blender/blenloader/intern/readfile.c (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/blenloader/intern/readfile.c#newcode11752
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/intern/rna_armature.c
File source/blender/makesrna/intern/rna_armature.c (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/makesrna/intern/rna_armature.c#newcode870
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_ActionManager.cpp
File source/gameengine/Ketsji/BL_ActionManager.cpp (right):

http://codereview.appspot.com/4934047/diff/6043/source/gameengine/Ketsji/BL_ActionManager.cpp#newcode46
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.

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


More information about the Bf-codereview mailing list