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

[ideasman42 at gmail.com]

Reviewers: bf-codereview_blender.org,

Message:
Some issues from my review raised a few issues..

- Should pose bone select behavior be changed?
- Should we accept vgroup weight fix tool (moves verts about and checks
their new locations by recalculating the entire modifier stack).


There are some other todo's but Im not so worried about these. - vertex
theme color, hidden verts, they are easy to do.


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;
a bit odd logic, could just set the color to black and return here?

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

http://codereview.appspot.com/5021044/diff/1/source/blender/blenkernel/intern/cdderivedmesh.c#newcode284
source/blender/blenkernel/intern/cdderivedmesh.c:284: // TODO define
selected color
This is fairly easy to add, can do this once in trunk.

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 */
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.

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;
picky comment: 'mv' is mainly used for single verts, would rename to
'mvert'  to avoid confusion.

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/object/object_vgroup.c#newcode2365
source/blender/editors/object/object_vgroup.c:2365: void
OBJECT_OT_vertex_group_fix(wmOperatorType *ot)
I'm worried about this operator, it moves verts 1 by 1 and rebuilds all
modifiers, this just seems like its going to be really inefficient.

On the other hand if a user has to do this manually its going to take
them a time anyway.

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:
Notice CTRL+I for invert selection is missing here.

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#newcode1077
source/blender/editors/sculpt_paint/paint_vertex.c:1077: #if 0 /* UNUSED
*/
left in commented code, once merged into trunk we should remove them.

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/sculpt_paint/paint_vertex.c#newcode1512
source/blender/editors/sculpt_paint/paint_vertex.c:1512: static void
do_weight_paint_vertex(VPaint *wp, Object *ob, int index,
I'd like to see if do_weight_paint_vertex() can be split up better so
the simple behavior can he kept easy to follow and the complex behavior
kept in its own else {} clause.

I'd need to look into this further in exactly how to do it, but think
its worth trying.

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/sculpt_paint/paint_vertex.c#newcode1936
source/blender/editors/sculpt_paint/paint_vertex.c:1936: // Ugly hack,
to avoid drawing vertex index when getting the face index buffer -
campbell
I had to add this, but should have made more comprehensive comment,
vertex selection is not needed here. we could make it not conflict but
even then it still wouldnt be needed.

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
This is fairly easy to add, just need to pass the vertex array in user
data.

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

http://codereview.appspot.com/5021044/diff/1/source/blender/editors/space_view3d/view3d_select.c#newcode206
source/blender/editors/space_view3d/view3d_select.c:206: static void
EM_backbuf_checkAndSelectTVerts(Mesh *me, int select)
This functions confusingly prefixed EM_ which stands for EditMesh, but
this actually runs in object mode.



Please review this at http://codereview.appspot.com/5021044/

Affected files:
   M     release/scripts/startup/bl_ui/properties_data_mesh.py
   M     release/scripts/startup/bl_ui/space_view3d.py
   M     release/scripts/startup/bl_ui/space_view3d_toolbar.py
   M     source/blender/blenkernel/BKE_DerivedMesh.h
   M     source/blender/blenkernel/BKE_armature.h
   M     source/blender/blenkernel/BKE_paint.h
   M     source/blender/blenkernel/intern/DerivedMesh.c
   M     source/blender/blenkernel/intern/armature.c
   M     source/blender/blenkernel/intern/cdderivedmesh.c
   M     source/blender/blenkernel/intern/paint.c
   M     source/blender/editors/armature/editarmature.c
   M     source/blender/editors/armature/meshlaplacian.c
   M     source/blender/editors/include/ED_mesh.h
   M     source/blender/editors/include/ED_view3d.h
   M     source/blender/editors/interface/interface_templates.c
   M     source/blender/editors/mesh/editmesh.c
   M     source/blender/editors/mesh/editmesh_mods.c
   M     source/blender/editors/object/object_intern.h
   M     source/blender/editors/object/object_ops.c
   M     source/blender/editors/object/object_vgroup.c
   M     source/blender/editors/sculpt_paint/paint_image.c
   M     source/blender/editors/sculpt_paint/paint_intern.h
   M     source/blender/editors/sculpt_paint/paint_ops.c
   M     source/blender/editors/sculpt_paint/paint_utils.c
   M     source/blender/editors/sculpt_paint/paint_vertex.c
   M     source/blender/editors/space_view3d/drawmesh.c
   M     source/blender/editors/space_view3d/drawobject.c
   M     source/blender/editors/space_view3d/space_view3d.c
   M     source/blender/editors/space_view3d/view3d_header.c
   M     source/blender/editors/space_view3d/view3d_select.c
   M     source/blender/makesdna/DNA_mesh_types.h
   M     source/blender/makesdna/DNA_object_types.h
   M     source/blender/makesdna/DNA_scene_types.h
   M     source/blender/makesrna/intern/rna_internal.h
   M     source/blender/makesrna/intern/rna_mesh.c
   M     source/blender/makesrna/intern/rna_object.c
   M     source/blender/makesrna/intern/rna_scene.c