[Bf-codereview] Skin modifier (issue 6220055)

[ideasman42 at gmail.com]

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

http://codereview.appspot.com/6220055/diff/1/source/blender/blenkernel/intern/customdata.c#newcode1003
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/editderivedmesh.c
File source/blender/blenkernel/intern/editderivedmesh.c (right):

http://codereview.appspot.com/6220055/diff/1/source/blender/blenkernel/intern/editderivedmesh.c#newcode1698
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/object.c
File source/blender/blenkernel/intern/object.c (right):

http://codereview.appspot.com/6220055/diff/1/source/blender/blenkernel/intern/object.c#newcode1089
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/bmo_extrude.c
File source/blender/bmesh/operators/bmo_extrude.c (right):

http://codereview.appspot.com/6220055/diff/1/source/blender/bmesh/operators/bmo_extrude.c#newcode190
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/bmo_extrude.c#newcode244
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/bmo_extrude.c#newcode345
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/object_modifier.c
File source/blender/editors/object/object_modifier.c (right):

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?

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/object_modifier.c#newcode1387
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/object_modifier.c#newcode1418
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/object_modifier.c#newcode1556
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/object_modifier.c#newcode1571
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/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.

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

http://codereview.appspot.com/6220055/diff/1/source/blender/editors/space_view3d/drawobject.c#newcode2406
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_view3d/drawobject.c#newcode2410
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_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.

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

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?

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

http://codereview.appspot.com/6220055/diff/1/source/blender/editors/transform/transform_conversions.c#newcode1925
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/transform_conversions.c#newcode1926
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/transform_conversions.c#newcode1928
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/transform_conversions.c#newcode2048
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_modifier_types.h
File source/blender/makesdna/DNA_modifier_types.h (right):

http://codereview.appspot.com/6220055/diff/1/source/blender/makesdna/DNA_modifier_types.h#newcode1080
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/rna_mesh.c
File source/blender/makesrna/intern/rna_mesh.c (right):

http://codereview.appspot.com/6220055/diff/1/source/blender/makesrna/intern/rna_mesh.c#newcode2604
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/rna_mesh.c#newcode2608
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/rna_modifier.c
File source/blender/makesrna/intern/rna_modifier.c (right):

http://codereview.appspot.com/6220055/diff/1/source/blender/makesrna/intern/rna_modifier.c#newcode3222
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/MOD_skin.c
File source/blender/modifiers/intern/MOD_skin.c (right):

http://codereview.appspot.com/6220055/diff/1/source/blender/modifiers/intern/MOD_skin.c#newcode137
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/MOD_skin.c#newcode215
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/MOD_skin.c#newcode294
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/MOD_skin.c#newcode316
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/MOD_skin.c#newcode319
source/blender/modifiers/intern/MOD_skin.c:319: wire = FALSE;
could break; here?

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.

http://codereview.appspot.com/6220055/diff/1/source/blender/modifiers/intern/MOD_skin.c#newcode1035
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/MOD_skin.c#newcode1053
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/MOD_skin.c#newcode1072
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/MOD_skin.c#newcode1080
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/MOD_skin.c#newcode1226
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/MOD_skin.c#newcode1334
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/MOD_skin.c#newcode1346
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/MOD_skin.c#newcode1394
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/MOD_skin.c#newcode1397
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/MOD_skin.c#newcode1439
source/blender/modifiers/intern/MOD_skin.c:1439: if
(BM_edge_face_count(e) == 2) {
again: if (BM_edge_face_pair(...)) { is better here.

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