Skin modifier

This patch adds a new modifier for creating a polygon skin based on
mesh vertices and edges. It's based in part on the paper "B-Mesh: A Fast
Modeling System for Base Meshes of 3D Articulated Shapes" (Zhongping
Ji, Ligang Liu, Yigang Wang).

User documentation:
http://wiki.blender.org/index.php/User:Nicholasbishop/SkinModifier

The code is also accessible as a git repository:
https://github.com/nicholasbishop/blender/tree/skin-modifier

Note: The modifier has an icon created by Julio Iglesias, but the blender_icons.png.c update has been removed, as it makes the patch too large for the code review tool.

The full log follows.

Author: Nicholas Bishop <nicholasbishop@gmail.com>
Date:   Fri Mar 23 01:58:56 2012 -0400

    Add vertex skin radii scaling as a transform operator.
    
    Add a new transform operator, "Skin Resize", which scales the X and Y
    axes of the radius field in MVertSkin. It's bound to CTRL+AKEY.
    
Author: Nicholas Bishop <nicholasbishop@gmail.com>
Date:   Wed Apr 18 00:39:29 2012 -0400

    Clear skin root flag on new vertices created by extruding.
    
Author: Nicholas Bishop <nicholasbishop@gmail.com>
Date:   Sun Apr 15 13:03:46 2012 -0400

    Add operator to extract armature and vertex groups from skin.
    
    * The operator creates bones for each input edge (does not subdivide
      them like the skin operator does), adds a fake root bone for skin
      roots with multiple children.
    
    * The operator adds vertex weight groups to the original mesh.
    
    * Make copy_object_transform() public, used to match the armature
      object to the mesh object.
    
Author: Nicholas Bishop <nicholasbishop@gmail.com>
Date:   Fri Mar 23 04:11:00 2012 -0400

    Add skin vertex operators.
    
    * Add operator to mark selected vertices as skin roots.
    * Add operator to mark/clear selected vertices as loose.
    * Add operator to equalize skin radii.
    
Author: Nicholas Bishop <nicholasbishop@gmail.com>
Date:   Tue Mar 27 13:02:26 2012 -0400

    Ensure skin nodes get created when adding modifier.
    
Author: Nicholas Bishop <nicholasbishop@gmail.com>
Date:   Fri Mar 23 04:48:38 2012 -0400

    Draw skin vertex roots in edit mode.
    
    Roots are drawn with a view-oriented dashed red circle around the
    vertex location. The circle's radius is the average if the skin
    vertex's X and Y radii.
    
Author: Nicholas Bishop <nicholasbishop@gmail.com>
Date:   Wed Feb 29 22:39:09 2012 -0500

    Add skin modifier: DNA, RNA, UI, and MOD_skin.c implementation.
    
    Skin modifier documentation:
    http://wiki.blender.org/index.php/User:Nicholasbishop/SkinModifier
    
    Implementation based in part off the paper "B-Mesh: A Fast Modeling
    System for Base Meshes of 3D Articulated Shapes" (Zhongping Ji,
    Ligang Liu, Yigang Wang)
    
    Note that to avoid confusion with Blender's BMesh data structure,
    this tool is renamed as the Skin modifier.
    
    The B-Mesh paper is current available here:
    http://www.math.zju.edu.cn/ligangliu/CAGD/Projects/BMesh/
    
    The main missing features in this code compared to the paper are:
    
    * No mesh evolution. The paper suggests iteratively subsurfing the
      skin output and adapting the output to better conform with the
      spheres of influence surrounding each vertex.
    
    * No mesh fairing. The paper suggests re-aligning output edges to
      follow principal mesh curvatures.
    
    * No auxiliary balls. These would serve to influence mesh
      evolution, which as noted above is not implemented.
    
    The code also adds some features not present in the paper:
    
    * Loops in the input edge graph.
    
    * Concave surfaces around branch nodes. The paper does not discuss
      how to handle non-convex regions; this code adds a number of
      cleanup operations to handle many (though not all) of these
      cases.

Author: Nicholas Bishop <nicholasbishop@gmail.com>
Date:   Tue Apr 24 14:20:55 2012 -0400

    Add skin modifier icon created by Julio Iglesias.
    
Author: Nicholas Bishop <nicholasbishop@gmail.com>
Date:   Mon Apr 9 12:26:51 2012 -0400

    Add MVertSkin DNA/RNA and customdata (CD_MVERT_SKIN).
    
    The MVertSkin currently just stores local skin radii and skin
    flags (MVertSkinFlag).
    
    Skin modifier documentation:
    http://wiki.blender.org/index.php/User:Nicholasbishop/SkinModifier

[ideasman42, 2012-05-20 17:53:48]

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

http://codereview.appspot.com/6220055/diff/1/source/blender/blenkernel/intern...
source/blender/blenkernel/intern/customdata.c:1003: vs[i].radius[2] = 0.25f;
*picky* - copy_v3_fl() saves some pointer lookups.

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

http://codereview.appspot.com/6220055/diff/1/source/blender/blenkernel/intern...
source/blender/blenkernel/intern/editderivedmesh.c:1698: eve =
BM_iter_new(&iter, bmdm->tc->bm, BM_VERTS_OF_MESH, NULL);
may as well use BM_ITER_MESH_INDEX macro

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

http://codereview.appspot.com/6220055/diff/1/source/blender/blenkernel/intern...
source/blender/blenkernel/intern/object.c:1089: void
copy_object_transform(Object *ob_tar, const Object *ob_src)
for exposed api calls - BKE_object_transform_copy

http://codereview.appspot.com/6220055/diff/1/source/blender/bmesh/operators/b...
File source/blender/bmesh/operators/bmo_extrude.c (right):

http://codereview.appspot.com/6220055/diff/1/source/blender/bmesh/operators/b...
source/blender/bmesh/operators/bmo_extrude.c:190: 
prefer this has CustomData_has_layer(&bm->vdata, CD_MVERT_SKIN) check to avoid
looping when none are set.

http://codereview.appspot.com/6220055/diff/1/source/blender/bmesh/operators/b...
source/blender/bmesh/operators/bmo_extrude.c:244: vs =
CustomData_bmesh_get(&bm->vdata, dupev->head.data, CD_MVERT_SKIN);
same again, best do:
const int has_vskin = CustomData_has_layer(&bm->vdata, CD_MVERT_SKIN);

... then check this within the loop.

http://codereview.appspot.com/6220055/diff/1/source/blender/bmesh/operators/b...
source/blender/bmesh/operators/bmo_extrude.c:345: BMO_ITER(v, &siter, bm,
&dupeop, "newout", BM_VERT) {
again, check `has_vskin`

http://codereview.appspot.com/6220055/diff/1/source/blender/editors/object/ob...
File source/blender/editors/object/object_modifier.c (right):

http://codereview.appspot.com/6220055/diff/1/source/blender/editors/object/ob...
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?

think this would be better incase some other part of the code adds skin layers.

http://codereview.appspot.com/6220055/diff/1/source/blender/editors/object/ob...
source/blender/editors/object/object_modifier.c:1387: BMVert *v2 = bm_edge->v1
== bm_vert ? bm_edge->v2 : bm_edge->v1;
this is a function BM_edge_other_vert()

http://codereview.appspot.com/6220055/diff/1/source/blender/editors/object/ob...
source/blender/editors/object/object_modifier.c:1418: bm_vert->head.hflag &
BM_ELEM_SELECT) {
*picky* - brace on new line

http://codereview.appspot.com/6220055/diff/1/source/blender/editors/object/ob...
source/blender/editors/object/object_modifier.c:1556: Mesh *me = skin_ob->data;
*suggestion* - could use derived deform instead - to get shape key data.

http://codereview.appspot.com/6220055/diff/1/source/blender/editors/object/ob...
source/blender/editors/object/object_modifier.c:1571: v = (e->v1 == parent_v ?
e->v2 : e->v1);
use BM_edge_other_vert

http://codereview.appspot.com/6220055/diff/1/source/blender/editors/object/ob...
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.

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

http://codereview.appspot.com/6220055/diff/1/source/blender/editors/space_vie...
source/blender/editors/space_view3d/drawobject.c:2406: const MVertSkin *vs =
CustomData_bmesh_get(&data->em->bm->vdata, eve->head.data, CD_MVERT_SKIN);
if this mesh has verts should be stored in the drawDMVerts_userData, then
data->has_vskin can be checked each call.

http://codereview.appspot.com/6220055/diff/1/source/blender/editors/space_vie...
source/blender/editors/space_view3d/drawobject.c:2410: if(vs && (vs->flag &
MVERT_SKIN_ROOT)) {
*picky* if( should have a space between.

http://codereview.appspot.com/6220055/diff/1/source/blender/editors/space_vie...
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.

http://codereview.appspot.com/6220055/diff/1/source/blender/editors/space_vie...
File source/blender/editors/space_view3d/view3d_draw.c (right):

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

http://codereview.appspot.com/6220055/diff/1/source/blender/editors/transform...
File source/blender/editors/transform/transform_conversions.c (right):

http://codereview.appspot.com/6220055/diff/1/source/blender/editors/transform...
source/blender/editors/transform/transform_conversions.c:1925: else if((t->mode
== TFM_SKIN_RESIZE) &&
*picky* - would nest 2 if statements here, so other modes added below wont get
checked if this fails because of NULL pointer... or this could be made into a
switch.

http://codereview.appspot.com/6220055/diff/1/source/blender/editors/transform...
source/blender/editors/transform/transform_conversions.c:1926: (vs =
CustomData_bmesh_get(&em->bm->vdata,
note - since _poll() checks for the data is the check for NULL even needed here?

http://codereview.appspot.com/6220055/diff/1/source/blender/editors/transform...
source/blender/editors/transform/transform_conversions.c:1928: CD_MVERT_SKIN)))
{
*picky*, brace on newline

http://codereview.appspot.com/6220055/diff/1/source/blender/editors/transform...
source/blender/editors/transform/transform_conversions.c:2048:
CustomData_has_layer(&bm->vdata, CD_MVERT_SKIN)) {
*picky* brace on newline.

http://codereview.appspot.com/6220055/diff/1/source/blender/makesdna/DNA_modi...
File source/blender/makesdna/DNA_modifier_types.h (right):

http://codereview.appspot.com/6220055/diff/1/source/blender/makesdna/DNA_modi...
source/blender/makesdna/DNA_modifier_types.h:1080: 
not sure its useful to typedef enums here (the types cant be used in DNA and
dont have a common prefix so clog namespace a bit), think anonymous enums would
be better here.

http://codereview.appspot.com/6220055/diff/1/source/blender/makesrna/intern/r...
File source/blender/makesrna/intern/rna_mesh.c (right):

http://codereview.appspot.com/6220055/diff/1/source/blender/makesrna/intern/r...
source/blender/makesrna/intern/rna_mesh.c:2604:
RNA_def_property_boolean_sdna(prop, NULL, "flag", MVERT_SKIN_ROOT);
as boolean should be use_root

http://codereview.appspot.com/6220055/diff/1/source/blender/makesrna/intern/r...
source/blender/makesrna/intern/rna_mesh.c:2608: prop = RNA_def_property(srna,
"loose", PROP_BOOLEAN, PROP_NONE);
as boolean should be use_loose

http://codereview.appspot.com/6220055/diff/1/source/blender/makesrna/intern/r...
File source/blender/makesrna/intern/rna_modifier.c (right):

http://codereview.appspot.com/6220055/diff/1/source/blender/makesrna/intern/r...
source/blender/makesrna/intern/rna_modifier.c:3222: prop =
RNA_def_property(srna, "x_symmetry", PROP_BOOLEAN, PROP_NONE);
For sculpt we have:
prop = RNA_def_property(srna, "use_symmetry_x", PROP_BOOLEAN, PROP_NONE);

ok to use same name?

http://codereview.appspot.com/6220055/diff/1/source/blender/modifiers/intern/...
File source/blender/modifiers/intern/MOD_skin.c (right):

http://codereview.appspot.com/6220055/diff/1/source/blender/modifiers/intern/...
source/blender/modifiers/intern/MOD_skin.c:137: static int edge_other(const
MEdge *e, int v)
this exists BM_edge_other_vert

http://codereview.appspot.com/6220055/diff/1/source/blender/modifiers/intern/...
source/blender/modifiers/intern/MOD_skin.c:215: return BM_iter_as_array(bm,
BM_FACES_OF_EDGE, diag,
this isnt optimal - use BM_edge_face_pair(...)

http://codereview.appspot.com/6220055/diff/1/source/blender/modifiers/intern/...
source/blender/modifiers/intern/MOD_skin.c:294:
BM_mesh_elem_hflag_disable_all(bm, BM_ALL, BM_ELEM_TAG, 0);
*picky*, use FALSE as last arg - all other callers do.

http://codereview.appspot.com/6220055/diff/1/source/blender/modifiers/intern/...
source/blender/modifiers/intern/MOD_skin.c:316: int wire = TRUE;
*picky* would call `is_wire`

http://codereview.appspot.com/6220055/diff/1/source/blender/modifiers/intern/...
source/blender/modifiers/intern/MOD_skin.c:319: wire = FALSE;
could break; here?

http://codereview.appspot.com/6220055/diff/1/source/blender/modifiers/intern/...
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.

http://codereview.appspot.com/6220055/diff/1/source/blender/modifiers/intern/...
source/blender/modifiers/intern/MOD_skin.c:1035: static BMEdge
*get_longest_face_edge(BMFace *f)
should be made into api function - bmesh_queries.c -- BM_face_find_long_edge ?
(transform code could use this when selecting a face orientation to use with an
active face)

http://codereview.appspot.com/6220055/diff/1/source/blender/modifiers/intern/...
source/blender/modifiers/intern/MOD_skin.c:1053: static BMEdge
*get_shortest_face_edge(BMFace *f)
same as above

http://codereview.appspot.com/6220055/diff/1/source/blender/modifiers/intern/...
source/blender/modifiers/intern/MOD_skin.c:1072: static BMVert
**get_face_verts(BMFace *f)
*suggestion*
alloc's per face like this can be avoided by having BLI_array_ defined in the
outer loop and always ensuring its at least as bit as the face length, then the
array is passed along with the face - this avoids a lot of re-allocs.

http://codereview.appspot.com/6220055/diff/1/source/blender/modifiers/intern/...
source/blender/modifiers/intern/MOD_skin.c:1080: BM_ITER_ELEM(v, &iter, f,
BM_VERTS_OF_FACE)
could use BM_iter_as_array here

http://codereview.appspot.com/6220055/diff/1/source/blender/modifiers/intern/...
source/blender/modifiers/intern/MOD_skin.c:1226: len += len_v3v3(a[j]->co,
b[orders[i][j]]->co);
for length comparison use len_squared_v3v3()

http://codereview.appspot.com/6220055/diff/1/source/blender/modifiers/intern/...
source/blender/modifiers/intern/MOD_skin.c:1334: tri[1][i] != tri[0][2]) {
*picky* brace on second line.

http://codereview.appspot.com/6220055/diff/1/source/blender/modifiers/intern/...
source/blender/modifiers/intern/MOD_skin.c:1346: (tri[0][(i + 1) % 3] == e->v1
|| tri[0][(i + 1) % 3] == e->v2)) {
*picky* brace on second line.

http://codereview.appspot.com/6220055/diff/1/source/blender/modifiers/intern/...
source/blender/modifiers/intern/MOD_skin.c:1394: if (BM_edge_face_count(e) == 2)
{
better to call BM_edge_is_manifold()

http://codereview.appspot.com/6220055/diff/1/source/blender/modifiers/intern/...
source/blender/modifiers/intern/MOD_skin.c:1397: BM_iter_as_array(so->bm,
BM_FACES_OF_EDGE, e, (void **)adj, 2);
infact you can remove the manifold check above and this to by calling:

if (BM_edge_face_pair(...)) {

http://codereview.appspot.com/6220055/diff/1/source/blender/modifiers/intern/...
source/blender/modifiers/intern/MOD_skin.c:1439: if (BM_edge_face_count(e) == 2)
{
again: if (BM_edge_face_pair(...)) { is better here.

[nicholasbishop, 2012-05-20 22:47:03]

Thanks for reviewing :)

I'm working my way through the changes now, couple quick questions:

1) BM_edge_face_pair() seems to return true for edges with just one face. The
function doc says it "returns TRUE when only 2 faces are found". Guessing it
needs an extra check?

2) Is there an equivalent to BM_edge_other_vert for MEdge? Some of the marked
places aren't BMesh.

[ideasman42, 2012-05-21 06:46:47]

1) --- thats bad!, fixed in svn.
2) --- no, but may as well add one.

On 2012/05/20 22:47:03, nicholasbishop wrote:
> Thanks for reviewing :)
> 
> I'm working my way through the changes now, couple quick questions:
> 
> 1) BM_edge_face_pair() seems to return true for edges with just one face. The
> function doc says it "returns TRUE when only 2 faces are found". Guessing it
> needs an extra check?
> 
> 2) Is there an equivalent to BM_edge_other_vert for MEdge? Some of the marked
> places aren't BMesh.

[nicholasbishop, 2012-05-21 23:13:31]

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/ob...
> 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/ob...
> 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_vie...
> 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_vie...
> 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/...
> 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?

[ideasman42, 2012-05-22 06:25:45]

LGTM (noted one small suggestion)

http://codereview.appspot.com/6220055/diff/4003/source/blender/modifiers/inte...
File source/blender/modifiers/intern/MOD_skin.c (right):

http://codereview.appspot.com/6220055/diff/4003/source/blender/modifiers/inte...
source/blender/modifiers/intern/MOD_skin.c:960: dv->totweight =
input_dvert->totweight;
prefer to use defvert_copy() here

[nicholasbishop, 2012-05-22 15:35:00]

Committed as revisions 46885-46886, 46888-46897.

Had a glitch with git-svn dcommit, so most of the commits lost their "reviewed
by Campbell Barton" line, but thanks again for reviewing :)

On 2012/05/22 06:25:45, ideasman42 wrote:
> LGTM (noted one small suggestion)
> 
>
http://codereview.appspot.com/6220055/diff/4003/source/blender/modifiers/inte...
> File source/blender/modifiers/intern/MOD_skin.c (right):
> 
>
http://codereview.appspot.com/6220055/diff/4003/source/blender/modifiers/inte...
> source/blender/modifiers/intern/MOD_skin.c:960: dv->totweight =
> input_dvert->totweight;
> prefer to use defvert_copy() here