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

[ideasman42 at gmail.com]

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/intern/anim_sys.c
File source/blender/blenkernel/intern/anim_sys.c (right):

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

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

http://codereview.appspot.com/4934047/diff/6043/source/blender/blenkernel/intern/sound.c#newcode663
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/animation/drivers.c
File source/blender/editors/animation/drivers.c (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/editors/animation/drivers.c#newcode386
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/armature/poseobject.c
File source/blender/editors/armature/poseobject.c (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/editors/armature/poseobject.c#newcode1191
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/sound_ops.c
File source/blender/editors/sound/sound_ops.c (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/editors/sound/sound_ops.c#newcode649
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_nla/nla_draw.c
File source/blender/editors/space_nla/nla_draw.c (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/editors/space_nla/nla_draw.c#newcode474
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_sequence_types.h
File source/blender/makesdna/DNA_sequence_types.h (right):

http://codereview.appspot.com/4934047/diff/6043/source/blender/makesdna/DNA_sequence_types.h#newcode230
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/intern/rna_access.c
File source/blender/makesrna/intern/rna_access.c (right):

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

http://codereview.appspot.com/4934047/diff/6043/source/blender/makesrna/intern/rna_userdef.c#newcode2954
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.

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