[Bf-codereview] Weight Paint (Radish Branch 2011) (issue 5021044)

[g.ulairi at gmail.com]

Made some review.

Some general notes (which aren't actually stoppers and can be made after
merge). I'm not fan of adding comments like "Jason was here" -- it's svn
and it stores that it's your code, so i prefer putting only comments
which actually helps to understand code. Also i've seen ambiguity in
using '//' vs. '/* */' comments. IMO better not to mix this comments.
Also, not sure why, but we didn't use FALSE/TRUE constants much before.


http://codereview.appspot.com/5021044/diff/1/source/blender/blenkernel/BKE_DerivedMesh.h
File source/blender/blenkernel/BKE_DerivedMesh.h (right):

http://codereview.appspot.com/5021044/diff/1/source/blender/blenkernel/BKE_DerivedMesh.h#newcode226
source/blender/blenkernel/BKE_DerivedMesh.h:226: void
(*drawSelectedVerts)(DerivedMesh *dm);
Not sure it's really cool change. If you add new callback to DerivedMesh
it should be implemented for all types of derived mesh, but I didn't see
changes to CCGDM. If this function is intended to work with CDDM only,
wouldn't it better to implement it outside of DerivedMesh structure?

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/mesh/editmesh.c
File source/blender/editors/mesh/editmesh.c (right):

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/mesh/editmesh.c#newcode1995
source/blender/editors/mesh/editmesh.c:1995: /* Jason note: caller needs
to run paintvert_flush_flags(ob) after this */
I can see paintvert_flush_flags() called in the end of this function. Is
this comment still relevant or this function will be called twice in
several cases?

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/object/object_vgroup.c
File source/blender/editors/object/object_vgroup.c (right):

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/object/object_vgroup.c#newcode789
source/blender/editors/object/object_vgroup.c:789: static int*
getSurroundingVerts(Mesh *me, int vert, int *count) {
Looks like count wouldn't be set in several cases (if found is zero to
the end of this funciton)

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/object/object_vgroup.c#newcode851
source/blender/editors/object/object_vgroup.c:851: static void
getSingleCoordinate(MVert **points, int count, float *coord) {
Why points are passed as reference to an array?

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/object/object_vgroup.c#newcode854
source/blender/editors/object/object_vgroup.c:854: coord[k] = 0;
This line and few next lines: why not use vector functions from math
module? I mean zero_v3(), add_v3_v3() and mul_v3_fl.

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/object/object_vgroup.c#newcode901
source/blender/editors/object/object_vgroup.c:901: projA =
MEM_callocN(sizeof(float)*3, "projectedA");
Why don't allocate vectors in stack? It'll be faster and you shouldn't
worry about free-ing memory in the end of this function.

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/object/object_vgroup.c#newcode920
source/blender/editors/object/object_vgroup.c:920: dm->needsFree = 1;
This doesn't look correct for me. Or readability can be improved here.
You're releasing dm which was passed as argument, and then set
ob->derivedDeform to NULL. If you're always passing derivedDeform here
as dm, then why not use ob->derivedDeform for this and avoid sending dm
as argument. And if dm is not always derivedDeform, then it'll be memory
leak here because you're setting derivedDeform to NULL without freeing
memory.
Also, i thought it's more convenient to free derivedFinal so all
familiar derived meshes are also re-calculated.

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/object/object_vgroup.c#newcode945
source/blender/editors/object/object_vgroup.c:945: float *oldPos =
MEM_callocN(sizeof(float)*3, "oldPosition");
Again, why not allocate in stack?

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/object/object_vgroup.c#newcode948
source/blender/editors/object/object_vgroup.c:948: float **changes =
MEM_mallocN(sizeof(float *)*totweight, "vertHorzChange");
IMO, this can be simplified by using float (*changes)[2] and allocating
memory for totweight 2-component vectors. Saves amount of allocations
and should give speedup.

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/object/object_vgroup.c#newcode2337
source/blender/editors/object/object_vgroup.c:2337: Object *ob=
CTX_data_pointer_get_type(C, "object", &RNA_Object).data;
Isn't it CTX_data_active_object? If it is, i've seen more usages of
CTX_data_pointer_get_type() to obtain object.

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/sculpt_paint/paint_vertex.c
File source/blender/editors/sculpt_paint/paint_vertex.c (right):

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/sculpt_paint/paint_vertex.c#newcode1192
source/blender/editors/sculpt_paint/paint_vertex.c:1192: #if 0 /* UNUSED
*/
IMO better to remove unused code. But it can be done after merge, so
it'll be in svn history.

http://codereview.appspot.com/5021044/diff/1/source/blender/makesrna/intern/rna_object.c
File source/blender/makesrna/intern/rna_object.c (right):

http://codereview.appspot.com/5021044/diff/1/source/blender/makesrna/intern/rna_object.c#newcode224
source/blender/makesrna/intern/rna_object.c:224: void
rna_update_active_object(Main *bmain, Scene *scene, PointerRNA *ptr)
I've seen this function is used only once in rna_scene. Is it really so
common so it should be public?

http://codereview.appspot.com/5021044/