[Bf-codereview] Skin modifier (issue 6220055)

[NicholasBishop at gmail.com]

I've updated the patch with most of the changes you suggested
applied.

New BKE/BM functions:
* BKE_mesh_edge_other_vert()
* BM_face_find_shortest_edge()
* BM_face_find_longest_edge()

Some additional notes and questions:


http://codereview.appspot.com/6220055/diff/1/source/blender/editors/object/object_modifier.c#newcode1353
> source/blender/editors/object/object_modifier.c:1353: vs->flag |=
> MVERT_SKIN_ROOT;
> rather then setting here, couldn't the customdata's set_default()
callback
> handle this?

Only one vertex in the array is supposed to be marked with the flag,
don't think that set_default() can handle that.


http://codereview.appspot.com/6220055/diff/1/source/blender/editors/object/object_modifier.c#newcode1692
> source/blender/editors/object/object_modifier.c:1692:
> BLI_insertlinkafter(&ob->modifiers, skin_md, arm_md);
> shouldnt this use api calls - ED_object_modifier_add or at least
modifier_new()
> ? - dont think its good to be adding modifiers directly.

It does use modifier_new(), few lines up. Didn't use
ED_object_modifier_add() because it doesn't allow to choose the
modifier's position in the stack.


http://codereview.appspot.com/6220055/diff/1/source/blender/editors/space_view3d/drawobject.c#newcode2414
> source/blender/editors/space_view3d/drawobject.c:2414: glColor4f(0.7,
0.3, 0.3,
> 1);
> colors should be stored in drawDMVerts_userData to avoid
UI_ThemeColor4 lookups
> per vertex, this is already done in other areas.

Done; the existing code didn't cache the values, but added caching for
all of the theme lookups. The skin root also didn't have a theme
setting, added TH_SKIN_ROOT and versioning code to set the default
value (file subversion bumped to 6.)


http://codereview.appspot.com/6220055/diff/1/source/blender/editors/space_view3d/view3d_draw.c#newcode2310
> source/blender/editors/space_view3d/view3d_draw.c:2310:
> is this really needed always - even in bounding box drawtype?

Moved this to ED_view3d_object_datamask(), adds the mvert_skin mask if
the object is in edit mode. That seem right?


http://codereview.appspot.com/6220055/diff/1/source/blender/modifiers/intern/MOD_skin.c#newcode862
> source/blender/modifiers/intern/MOD_skin.c:862:
> this should use defvert_verify_index() or defvert_add_index_notest()
to define a
> new api function, really dont think this should be done in modifiers.

Replaced the code there with defvert_add_index_notest(). It's pretty
small piece of code now, not sure what would go into a new API function?


http://codereview.appspot.com/6220055/