Code review - Issue 6220055: Skin modifierhttps://codereview.appspot.com/2012-05-22T15:35:00+00:00rietveld
Message from unknown
2012-05-19T20:10:05+00:00nicholasbishopurn:md5:47f557f4de2fc45dad6f1e208f4ccd2f
Message from NicholasBishop@gmail.com
2012-05-19T20:10:07+00:00nicholasbishopurn:md5:349b4f01bd117fac56e8b5e1cefa2bab
Message from ideasman42@gmail.com
2012-05-20T17:53:48+00:00ideasman42urn:md5:b918c004db37b96afba3b40c88d9725c
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.
Message from NicholasBishop@gmail.com
2012-05-20T22:47:03+00:00nicholasbishopurn:md5:8dd56e210ef07de64c495b9a764d334f
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.
Message from ideasman42@gmail.com
2012-05-21T06:46:47+00:00ideasman42urn:md5:7b60bf9794b85b03b6d9e078b0ef8f5e
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.
Message from unknown
2012-05-21T23:12:04+00:00nicholasbishopurn:md5:4a35e7cfedef0b4dad625d7b5daaeb09
Message from NicholasBishop@gmail.com
2012-05-21T23:13:31+00:00nicholasbishopurn:md5:124fa2dfcefcbb8e121a6d14da9e662c
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?
Message from ideasman42@gmail.com
2012-05-22T06:25:45+00:00ideasman42urn:md5:352d8b0dbb37a5e7271fbfc34434b98b
LGTM (noted one small suggestion)
http://codereview.appspot.com/6220055/diff/4003/source/blender/modifiers/intern/MOD_skin.c
File source/blender/modifiers/intern/MOD_skin.c (right):
http://codereview.appspot.com/6220055/diff/4003/source/blender/modifiers/intern/MOD_skin.c#newcode960
source/blender/modifiers/intern/MOD_skin.c:960: dv->totweight = input_dvert->totweight;
prefer to use defvert_copy() here
Message from NicholasBishop@gmail.com
2012-05-22T15:35:00+00:00nicholasbishopurn:md5:10ecb0621429501b9160f80deb9f242a
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/intern/MOD_skin.c
> File source/blender/modifiers/intern/MOD_skin.c (right):
>
> http://codereview.appspot.com/6220055/diff/4003/source/blender/modifiers/intern/MOD_skin.c#newcode960
> source/blender/modifiers/intern/MOD_skin.c:960: dv->totweight =
> input_dvert->totweight;
> prefer to use defvert_copy() here