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

[jason.hays22 at gmail.com]

I also added hidden verts support to a function in drawobject.c--I
forgot that in the SVN list of changes.


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);
On 2011/09/15 07:57:10, nazgul wrote:
> 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?

Done.

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

http://codereview.appspot.com/5021044/diff/1/source/blender/blenkernel/intern/DerivedMesh.c#newcode1729
source/blender/blenkernel/intern/DerivedMesh.c:1729: input = -1;
On 2011/09/15 03:00:08, ideasman42 wrote:
> a bit odd logic, could just set the color to black and return here?

Done.

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

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/armature/editarmature.c#newcode4312
source/blender/editors/armature/editarmature.c:4312: /* Jason was here,
I'm doing a select for multibone painting */
On 2011/09/15 03:00:08, ideasman42 wrote:
> Not sure about changing bone selection logic (from user POV), at the
very least
> I think it should work the same as it does now, unless multipaint is
enabled -
> if the functionality is really needed.

Done.

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#newcode708
source/blender/editors/object/object_vgroup.c:708: MVert *mv =
me->mvert;
On 2011/09/15 03:00:08, ideasman42 wrote:
> picky comment: 'mv' is mainly used for single verts, would rename to
'mvert'  to
> avoid confusion.

Done.

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) {
On 2011/09/15 07:57:10, nazgul wrote:
> Looks like count wouldn't be set in several cases (if found is zero to
the end
> of this funciton)

How?  Unless it is a single point in space, it'll have vertices
surrounding it..  Do you have an example?

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) {
On 2011/09/15 07:57:10, nazgul wrote:
> Why points are passed as reference to an array?

Done.

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;
On 2011/09/15 07:57:10, nazgul wrote:
> 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.

Done.

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");
On 2011/09/15 07:57:10, nazgul wrote:
> 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.

Done.

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;
On 2011/09/15 07:57:10, nazgul wrote:
> 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.

Done.

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");
On 2011/09/15 07:57:10, nazgul wrote:
> Again, why not allocate in stack?

Done.

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");
On 2011/09/15 07:57:10, nazgul wrote:
> 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.

Done.

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;
On 2011/09/15 07:57:10, nazgul wrote:
> Isn't it CTX_data_active_object? If it is, i've seen more usages of
> CTX_data_pointer_get_type() to obtain object.

Done.

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

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/sculpt_paint/paint_ops.c#newcode622
source/blender/editors/sculpt_paint/paint_ops.c:622:
On 2011/09/15 03:00:08, ideasman42 wrote:
> Notice CTRL+I for invert selection is missing here.

Done.

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

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/space_view3d/drawobject.c#newcode6531
source/blender/editors/space_view3d/drawobject.c:6531: // TODO: support
hidden vertices
On 2011/09/15 03:00:08, ideasman42 wrote:
> This is fairly easy to add, just need to pass the vertex array in user
data.

Done.

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