Issue 15580049: Laplacian Deform Modifier
|
Created:
10 years, 6 months ago by howardt Modified:
10 years, 6 months ago Base URL:
https://svn.blender.org/svnroot/bf-blender/trunk/blender/ Visibility:
Public. |
DescriptionLaplacian Deform Modifier
Patch Set 1 #
Total comments: 37
Patch Set 2 : Laplacian Deform update 1 #
Total comments: 47
MessagesTotal messages: 10
https://codereview.appspot.com/15580049/diff/1/release/scripts/startup/bl_ui/... File release/scripts/startup/bl_ui/properties_data_modifier.py (right): https://codereview.appspot.com/15580049/diff/1/release/scripts/startup/bl_ui/... release/scripts/startup/bl_ui/properties_data_modifier.py:377: layout.row().prop(md, "iterations") Your code had just layout.pro(md, "iterations") but the property didn't show up for me; changing it to this made it show up. https://codereview.appspot.com/15580049/diff/1/source/blender/modifiers/inter... File source/blender/modifiers/intern/MOD_laplaciandeform.c (right): https://codereview.appspot.com/15580049/diff/1/source/blender/modifiers/inter... source/blender/modifiers/intern/MOD_laplaciandeform.c:67: static LaplacianSystem *newLaplacianSystem() use *newLaplacianSystem(void) to avoid a compiler warning. https://codereview.appspot.com/15580049/diff/1/source/blender/modifiers/inter... source/blender/modifiers/intern/MOD_laplaciandeform.c:92: static LaplacianSystem *initLaplacianSystem(int totalVerts, int totalEdges, int totalFaces, int totalAnchors, totalFaces is unused. delete it from argument list and from the call, below. https://codereview.appspot.com/15580049/diff/1/source/blender/modifiers/inter... source/blender/modifiers/intern/MOD_laplaciandeform.c:106: sys->co = (float (*)[3])MEM_callocN(sizeof(float)*(totalVerts * 3), "DeformCoordinates"); Put spaces around * in sizeof(float) * (totalVerts * 3) And do the same in the following lines. https://codereview.appspot.com/15580049/diff/1/source/blender/modifiers/inter... source/blender/modifiers/intern/MOD_laplaciandeform.c:153: if (clen < FLT_EPSILON){ space between ) and { https://codereview.appspot.com/15580049/diff/1/source/blender/modifiers/inter... source/blender/modifiers/intern/MOD_laplaciandeform.c:160: static void initLaplacianMatrix( LaplacianSystem *sys) For future maintainers of this code, some kind of documentation of how this system is set up would be important. A pointer to the paper you got it from, and/or maybe a Blender developer wiki page that explains how the system is constructed. https://codereview.appspot.com/15580049/diff/1/source/blender/modifiers/inter... source/blender/modifiers/intern/MOD_laplaciandeform.c:182: idv4 = has_4_vert ? vidf[3] : 0; Comment that case of faces with > n verts are excluded by test in LaplacianDeformModifier_do below. Though would be nicer if you could figure out how to gracefully fail in such cases. For example, could you just ignore ngons if all vertices in it are also in adjacent faces that are not ngons? https://codereview.appspot.com/15580049/diff/1/source/blender/modifiers/inter... source/blender/modifiers/intern/MOD_laplaciandeform.c:325: beta = dot_v3v3(uij, di); proper indentation using tabs of this and following line. https://codereview.appspot.com/15580049/diff/1/source/blender/modifiers/inter... source/blender/modifiers/intern/MOD_laplaciandeform.c:362: fni[0] = alpha*ni[0] + beta * uij[0] + gamma * e2[0]; spaces around * in alpha*ni[0] similar in following two lines. https://codereview.appspot.com/15580049/diff/1/source/blender/modifiers/inter... source/blender/modifiers/intern/MOD_laplaciandeform.c:381: struct BMesh *bm; bm is unsed in this function. delete it and the assignment to it two lines below this. https://codereview.appspot.com/15580049/diff/1/source/blender/modifiers/inter... source/blender/modifiers/intern/MOD_laplaciandeform.c:537: MDeformVert *dv = NULL; dv is unused. delete this line. https://codereview.appspot.com/15580049/diff/1/source/blender/modifiers/inter... source/blender/modifiers/intern/MOD_laplaciandeform.c:593: smd->vertexco = (float *)MEM_callocN(sizeof(float)*(numVerts*3), "ModDeformCoordinates"); space around multiplication * https://codereview.appspot.com/15580049/diff/1/source/blender/modifiers/inter... source/blender/modifiers/intern/MOD_laplaciandeform.c:643: static bool onlyChangueAnchors(LaplacianDeformModifierData *smd, Object *ob, DerivedMesh *dm, int numVerts) use onlyChangeAnchors as name instead (Change instead of Changue is the English word). https://codereview.appspot.com/15580049/diff/1/source/blender/modifiers/inter... source/blender/modifiers/intern/MOD_laplaciandeform.c:687: static bool onlyChangueGroup(LaplacianDeformModifierData *smd, Object *ob, DerivedMesh *dm, int numVerts) call it onlyChangeGroup instead. Also, parameter ob is unused, so remove from parameter list and calls of this function. https://codereview.appspot.com/15580049/diff/1/source/blender/modifiers/inter... source/blender/modifiers/intern/MOD_laplaciandeform.c:725: BKE_report(NULL, RPT_ERROR, "Ngons not being supported"); I know I asked for this earlier, but this does have the problem that the message will show on the console every time this function is called -- maybe a huge number of times. But I don't have a better idea except to make the deformer do something reasonable when faces > 4 verts are present. https://codereview.appspot.com/15580049/diff/1/source/blender/modifiers/inter... source/blender/modifiers/intern/MOD_laplaciandeform.c:802: LaplacianDeformModifierData *smd = (LaplacianDeformModifierData *) md; Comment out this line too, or remove both this and following line. smd is unused otherwise. But what is this function supposed to do? Is there logic that needs to go here that you need to still work on? https://codereview.appspot.com/15580049/diff/1/source/blender/modifiers/inter... source/blender/modifiers/intern/MOD_laplaciandeform.c:809: LaplacianDeformModifierData *smd = (LaplacianDeformModifierData *) md; smd is unused, so remove this.
Sign in to reply to this message.
I uploaded Alexander's changes in response to my first comments as a second patch set. Only one more comment from me in the new patch set. https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... File source/blender/modifiers/intern/MOD_laplaciandeform.c (right): https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:712: static bool onlyChangeGroup(LaplacianDeformModifierData *smd, Object *ob, DerivedMesh *dm, int numVerts) The parameter ob is unused. Please remove it.
Sign in to reply to this message.
https://codereview.appspot.com/15580049/diff/1/release/scripts/startup/bl_ui/... File release/scripts/startup/bl_ui/properties_data_modifier.py (right): https://codereview.appspot.com/15580049/diff/1/release/scripts/startup/bl_ui/... release/scripts/startup/bl_ui/properties_data_modifier.py:377: layout.row().prop(md, "iterations") On 2013/10/22 14:14:16, howardt wrote: > Your code had just layout.pro(md, "iterations") but the property didn't show up > for me; changing it to this made it show up. Done. https://codereview.appspot.com/15580049/diff/1/source/blender/modifiers/inter... File source/blender/modifiers/intern/MOD_laplaciandeform.c (right): https://codereview.appspot.com/15580049/diff/1/source/blender/modifiers/inter... source/blender/modifiers/intern/MOD_laplaciandeform.c:67: static LaplacianSystem *newLaplacianSystem() On 2013/10/22 14:14:16, howardt wrote: > use *newLaplacianSystem(void) to avoid a compiler warning. Done. https://codereview.appspot.com/15580049/diff/1/source/blender/modifiers/inter... source/blender/modifiers/intern/MOD_laplaciandeform.c:92: static LaplacianSystem *initLaplacianSystem(int totalVerts, int totalEdges, int totalFaces, int totalAnchors, On 2013/10/22 14:14:16, howardt wrote: > totalFaces is unused. delete it from argument list and from the call, below. Done. https://codereview.appspot.com/15580049/diff/1/source/blender/modifiers/inter... source/blender/modifiers/intern/MOD_laplaciandeform.c:106: sys->co = (float (*)[3])MEM_callocN(sizeof(float)*(totalVerts * 3), "DeformCoordinates"); On 2013/10/22 14:14:16, howardt wrote: > Put spaces around * in sizeof(float) * (totalVerts * 3) > And do the same in the following lines. Done. https://codereview.appspot.com/15580049/diff/1/source/blender/modifiers/inter... source/blender/modifiers/intern/MOD_laplaciandeform.c:153: if (clen < FLT_EPSILON){ On 2013/10/22 14:14:16, howardt wrote: > space between ) and { Done. https://codereview.appspot.com/15580049/diff/1/source/blender/modifiers/inter... source/blender/modifiers/intern/MOD_laplaciandeform.c:153: if (clen < FLT_EPSILON){ On 2013/10/22 14:14:16, howardt wrote: > space between ) and { Done. https://codereview.appspot.com/15580049/diff/1/source/blender/modifiers/inter... source/blender/modifiers/intern/MOD_laplaciandeform.c:160: static void initLaplacianMatrix( LaplacianSystem *sys) /** * This method computes the Laplacian Matrix and Differential Coordinates for all vertex in the mesh. * The Linear system is LV = d * Where L is Laplacian Matrix, V as the vertexes in Mesh, d is the differential coordinates * The Laplacian Matrix is computes as a * Lij = sum(Wij) (if i == j) * Lij = Wij (if i != j) * Wij is weight between vertex Vi and vertex Vj, we use cotangent weight * * The Differential Coordinate is computes as a * di = Vi * sum(Wij) - sum(Wij * Vj) * Where : * di is the Differential Coordinate i * sum (Wij) is the sum of all weights between vertex Vi and its vertexes neighbors (Vj) * sum (Wij * Vj) is the sum of the product between vertex neighbor Vj and weight Wij for all neighborhood. * * This Laplacian Matrix is described in the paper: * Desbrun M. et.al, Implicit fairing of irregular meshes using diffusion and curvature flow, SIGGRAPH '99, pag 317-324, * New York, USA * * The computation of Laplace Beltrami operator on Hybrid Triangle/Quad Meshes is described in the paper: * Pinzon A., Romero E., Shape Inflation With an Adapted Laplacian Operator For Hybrid Quad/Triangle Meshes, * Conference on Graphics Patterns and Images, SIBGRAPI, 2013 * * The computation of Differential Coordinates is described in the paper: * Sorkine, O. Laplacian Surface Editing. Proceedings of the EUROGRAPHICS/ACM SIGGRAPH Symposium on Geometry Processing, * 2004. p. 179-188. */ https://codereview.appspot.com/15580049/diff/1/source/blender/modifiers/inter... source/blender/modifiers/intern/MOD_laplaciandeform.c:182: idv4 = has_4_vert ? vidf[3] : 0; The calculation of the curvature flow on polygon meshes depend on the relationship of the vertices in the whole equation, for this reason it is estimated Laplacian operators on manifold meshes. And operators to discretize the Laplace Beltrami operator working only with triangles and quadrangles https://codereview.appspot.com/15580049/diff/1/source/blender/modifiers/inter... source/blender/modifiers/intern/MOD_laplaciandeform.c:325: beta = dot_v3v3(uij, di); On 2013/10/22 14:14:16, howardt wrote: > proper indentation using tabs of this and following line. Done. https://codereview.appspot.com/15580049/diff/1/source/blender/modifiers/inter... source/blender/modifiers/intern/MOD_laplaciandeform.c:362: fni[0] = alpha*ni[0] + beta * uij[0] + gamma * e2[0]; On 2013/10/22 14:14:16, howardt wrote: > spaces around * in alpha*ni[0] > similar in following two lines. Done. https://codereview.appspot.com/15580049/diff/1/source/blender/modifiers/inter... source/blender/modifiers/intern/MOD_laplaciandeform.c:381: struct BMesh *bm; On 2013/10/22 14:14:16, howardt wrote: > bm is unsed in this function. delete it and the assignment to it two lines > below this. Done. https://codereview.appspot.com/15580049/diff/1/source/blender/modifiers/inter... source/blender/modifiers/intern/MOD_laplaciandeform.c:537: MDeformVert *dv = NULL; On 2013/10/22 14:14:16, howardt wrote: > dv is unused. delete this line. Done. https://codereview.appspot.com/15580049/diff/1/source/blender/modifiers/inter... source/blender/modifiers/intern/MOD_laplaciandeform.c:537: MDeformVert *dv = NULL; On 2013/10/22 14:14:16, howardt wrote: > dv is unused. delete this line. Done. https://codereview.appspot.com/15580049/diff/1/source/blender/modifiers/inter... source/blender/modifiers/intern/MOD_laplaciandeform.c:593: smd->vertexco = (float *)MEM_callocN(sizeof(float)*(numVerts*3), "ModDeformCoordinates"); On 2013/10/22 14:14:16, howardt wrote: > space around multiplication * Done. https://codereview.appspot.com/15580049/diff/1/source/blender/modifiers/inter... source/blender/modifiers/intern/MOD_laplaciandeform.c:643: static bool onlyChangueAnchors(LaplacianDeformModifierData *smd, Object *ob, DerivedMesh *dm, int numVerts) On 2013/10/22 14:14:16, howardt wrote: > use onlyChangeAnchors as name instead (Change instead of Changue is the English > word). Done. https://codereview.appspot.com/15580049/diff/1/source/blender/modifiers/inter... source/blender/modifiers/intern/MOD_laplaciandeform.c:687: static bool onlyChangueGroup(LaplacianDeformModifierData *smd, Object *ob, DerivedMesh *dm, int numVerts) On 2013/10/22 14:14:16, howardt wrote: > call it onlyChangeGroup instead. > Also, parameter ob is unused, so remove from parameter list and calls of this > function. Done. https://codereview.appspot.com/15580049/diff/1/source/blender/modifiers/inter... source/blender/modifiers/intern/MOD_laplaciandeform.c:687: static bool onlyChangueGroup(LaplacianDeformModifierData *smd, Object *ob, DerivedMesh *dm, int numVerts) On 2013/10/22 14:14:16, howardt wrote: > call it onlyChangeGroup instead. > Also, parameter ob is unused, so remove from parameter list and calls of this > function. Done. https://codereview.appspot.com/15580049/diff/1/source/blender/modifiers/inter... source/blender/modifiers/intern/MOD_laplaciandeform.c:725: BKE_report(NULL, RPT_ERROR, "Ngons not being supported"); On 2013/10/22 14:14:16, howardt wrote: > I know I asked for this earlier, but this does have the problem that the message > will show on the console every time this function is called -- maybe a huge > number of times. But I don't have a better idea except to make the deformer do > something reasonable when faces > 4 verts are present. Done. https://codereview.appspot.com/15580049/diff/1/source/blender/modifiers/inter... source/blender/modifiers/intern/MOD_laplaciandeform.c:802: LaplacianDeformModifierData *smd = (LaplacianDeformModifierData *) md; On 2013/10/22 14:14:16, howardt wrote: > Comment out this line too, or remove both this and following line. smd is > unused otherwise. But what is this function supposed to do? Is there logic that > needs to go here that you need to still work on? Done. https://codereview.appspot.com/15580049/diff/1/source/blender/modifiers/inter... source/blender/modifiers/intern/MOD_laplaciandeform.c:809: LaplacianDeformModifierData *smd = (LaplacianDeformModifierData *) md; On 2013/10/22 14:14:16, howardt wrote: > smd is unused, so remove this. Done. https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... File source/blender/modifiers/intern/MOD_laplaciandeform.c (right): https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:712: static bool onlyChangeGroup(LaplacianDeformModifierData *smd, Object *ob, DerivedMesh *dm, int numVerts) On 2013/10/23 10:58:55, howardt wrote: > The parameter ob is unused. Please remove it. Done. https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:712: static bool onlyChangeGroup(LaplacianDeformModifierData *smd, Object *ob, DerivedMesh *dm, int numVerts) On 2013/10/23 10:58:55, howardt wrote: > The parameter ob is unused. Please remove it. Done.
Sign in to reply to this message.
Lots of small comments - main thing Im concerned about is converting to BMesh which seems unnecessary and could be replaced with duplicating DerivedMesh arrays. https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... File source/blender/modifiers/intern/MOD_laplaciandeform.c (right): https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:112: memset(sys->no, 0.0, sizeof(float) * totalVerts * 3); This is incorrect and probably works by accident. second memset() arg is a `char`, but the array is already calloced (which happens to zero the floats anyway). in some cases there is a difference, but in practice - you can assume a 0'd array gives 0.0f values. These memset lines can be removed. in fact - there is no need to calloc OR memset, just malloc, caller has to fill in anyway. https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:117: static void deleteVoidPointer(void *data) This function is _not_ nulling the pointer from the caller. (only the argument). Replace this function with MEM_SAFE_FREE() macro instead. https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:130: deleteVoidPointer(sys->co); I can see that in some cases a safer memory freeing function is nice (one that NULL's the value), however, since this just frees `sys` at the end, theres really no point --- just call MEM_freeN() directly. https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:322: BLI_array_free(vidn); Calling free for every vertex isn't efficient, better call once at the end, and empty on each iteration. https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:341: i = BM_elem_index_get(v); rather then getting the index like this you can use BM_ITER_MESH_INDEX() macro. https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:583: BLI_array_declare(index_anchors); BLI_array_*** gives quite a lot of overhead (heavy macros). Suggest to simply over-alloc. Then realloc a smaller size once the total is known. As mentioned in previous comments - this part could/should be refactored into its own function. https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:592: bm = DM_to_bmesh(dm, false); Why is BMesh being used here? - checked code and can't really see a good reason for this. Why not just duplicate DM poly/vert arrays? https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:594: if (dv) { again - an incremented, valid pointer will never become NULL (unless it wraps around, lets assume that wont happen :) ) https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:606: for (i = 0; i < total_anchors; i++) { both these for() loops can be replaced with memcpy() https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:613: BM_ITER_MESH (v, &viter, bm, BM_VERTS_OF_MESH) { This loop can be replaced with BM_iter_as_arrayN() call. https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:618: smd->vertexco = (float *) MEM_callocN(sizeof(float) * (numVerts * 3), "ModDeformCoordinates"); No need to calloc if the data is immediately overwritten. https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:618: smd->vertexco = (float *) MEM_callocN(sizeof(float) * (numVerts * 3), "ModDeformCoordinates"); again, no need to calloc. just malloc. https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:634: BLI_array_declare(index_anchors); This array is allocated, filled, then never used, however I noticed this is also duplicate code from onlyChangeAnchors() and initSystem(), which also contain 'BLI_array_append(index_anchors, i);'. Suggest to de-duplate all 3 into one function, with the option to count and not allocate. https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:642: if(BLI_strcasecmp(smd->anchor_grp_name, sys->anchor_grp_name) != 0) { Why is this using BLI_strcasecmp? - vertex group names are case sensitive. (goes for other uses of BLI_strcasecmp in this file) https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:651: if (dv) { This check is redundant and can be removed, 'dv' will always be non-NULL. https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:668: static bool onlyChangeAnchors(LaplacianDeformModifierData *smd, Object *ob, DerivedMesh *dm, int numVerts) almost exact duplicate of isSystemDifferent() with flipped return value. Except for 'dvert' test. Suggest to de-duplicate, perhaps move 'modifier_get_vgroup' outside the function and handle missing dvert case outside. https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:678: BLI_array_declare(index_anchors); again - allocs and doesnt use. see comment for isSystemDifferent https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:680: if (sys->total_verts != numVerts) { Looks like this could call, onlyChangeGroup()? https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:757: filevertexCos = (float (*)[3]) MEM_callocN(sizeof(float) * (numVerts * 3), "TempModDeformCoordinates"); *picky*, my personal preference. replace: sizeof(float) * (numVerts * 3) with: sizeof(float[3]) * numVerts ... same for other cases in this patch. https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:757: filevertexCos = (float (*)[3]) MEM_callocN(sizeof(float) * (numVerts * 3), "TempModDeformCoordinates"); No need to calloc if the data is immediately overwritten. https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:784: filevertexCos = (float (*)[3]) MEM_callocN(sizeof(float) * (numVerts * 3), "TempDeformCoordinates"); No need to calloc if the data is immediately overwritten. https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:857: DerivedMesh *dm = get_dm(ob, editData, derivedData, NULL, false, false); Note that this converts the BMesh to a DerivedMesh, then back to a BMesh, seems a bit unnecessary. --- but better check on if BMesh could be used internally in the first-place.
Sign in to reply to this message.
Added minor note, also - tselem_draw_icon() misses the new modifier in 'switch
((ModifierType)md->type) {' statement.
https://codereview.appspot.com/15580049/diff/60001/source/blender/makesrna/in...
File source/blender/makesrna/intern/rna_modifier.c (right):
https://codereview.appspot.com/15580049/diff/60001/source/blender/makesrna/in...
source/blender/makesrna/intern/rna_modifier.c:3686: prop =
RNA_def_property(srna, "repetitions", PROP_INT, PROP_NONE);
Smooth modifier calls this 'iterations', best use consistent names (same goes
for struct member name).
Sign in to reply to this message.
https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... File source/blender/modifiers/intern/MOD_laplaciandeform.c (right): https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:112: memset(sys->no, 0.0, sizeof(float) * totalVerts * 3); ok, Done, thanks for the clarification. This line was removed. https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:117: static void deleteVoidPointer(void *data) Done, The method was removed and replaced by MEM_SAFE_FREE https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:130: deleteVoidPointer(sys->co); On 2013/10/23 20:39:20, ideasman42 wrote: > I can see that in some cases a safer memory freeing function is nice (one that > NULL's the value), however, since this just frees `sys` at the end, theres > really no point --- just call MEM_freeN() directly. Done. https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:322: BLI_array_free(vidn); Done, I put BLI_array_free(vidn); out the for loop https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:341: i = BM_elem_index_get(v); On 2013/10/23 20:39:20, ideasman42 wrote: > rather then getting the index like this you can use BM_ITER_MESH_INDEX() macro. Done. https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:583: BLI_array_declare(index_anchors); This function is only used one time, then the system uses the cache to recalculate the solution. https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:592: bm = DM_to_bmesh(dm, false); I use Bmesh for visit the neighbors of every vertex, you can see this in rotateDifferentialCoordinates line 348, 349 https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:594: if (dv) { I made this only for prevents any problem. https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:606: for (i = 0; i < total_anchors; i++) { On 2013/10/23 20:39:20, ideasman42 wrote: > both these for() loops can be replaced with memcpy() Done. https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:613: BM_ITER_MESH (v, &viter, bm, BM_VERTS_OF_MESH) { On 2013/10/23 20:39:20, ideasman42 wrote: > This loop can be replaced with BM_iter_as_arrayN() call. Done. https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:618: smd->vertexco = (float *) MEM_callocN(sizeof(float) * (numVerts * 3), "ModDeformCoordinates"); On 2013/10/23 20:39:20, ideasman42 wrote: > No need to calloc if the data is immediately overwritten. Done. https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:618: smd->vertexco = (float *) MEM_callocN(sizeof(float) * (numVerts * 3), "ModDeformCoordinates"); On 2013/10/23 20:39:20, ideasman42 wrote: > again, no need to calloc. just malloc. Done. https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:642: if(BLI_strcasecmp(smd->anchor_grp_name, sys->anchor_grp_name) != 0) { Done, replaced by strcmp https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:651: if (dv) { On 2013/10/23 20:39:20, ideasman42 wrote: > This check is redundant and can be removed, 'dv' will always be non-NULL. Done. https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:651: if (dv) { On 2013/10/23 20:39:20, ideasman42 wrote: > This check is redundant and can be removed, 'dv' will always be non-NULL. Done. https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:757: filevertexCos = (float (*)[3]) MEM_callocN(sizeof(float) * (numVerts * 3), "TempModDeformCoordinates"); On 2013/10/23 20:39:20, ideasman42 wrote: > No need to calloc if the data is immediately overwritten. Done. https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:757: filevertexCos = (float (*)[3]) MEM_callocN(sizeof(float) * (numVerts * 3), "TempModDeformCoordinates"); On 2013/10/23 20:39:20, ideasman42 wrote: > *picky*, my personal preference. > > replace: sizeof(float) * (numVerts * 3) > with: sizeof(float[3]) * numVerts > > ... same for other cases in this patch. Done. https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:757: filevertexCos = (float (*)[3]) MEM_callocN(sizeof(float) * (numVerts * 3), "TempModDeformCoordinates"); On 2013/10/23 20:39:20, ideasman42 wrote: > No need to calloc if the data is immediately overwritten. Done. https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:784: filevertexCos = (float (*)[3]) MEM_callocN(sizeof(float) * (numVerts * 3), "TempDeformCoordinates"); On 2013/10/23 20:39:20, ideasman42 wrote: > No need to calloc if the data is immediately overwritten. Done.
Sign in to reply to this message.
https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... File source/blender/modifiers/intern/MOD_laplaciandeform.c (right): https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:634: BLI_array_declare(index_anchors); On 2013/10/23 20:39:20, ideasman42 wrote: > This array is allocated, filled, then never used, however I noticed this is also > duplicate code from onlyChangeAnchors() and initSystem(), which also contain > 'BLI_array_append(index_anchors, i);'. > > Suggest to de-duplate all 3 into one function, with the option to count and not > allocate. Done. https://codereview.appspot.com/15580049/diff/60001/source/blender/modifiers/i... source/blender/modifiers/intern/MOD_laplaciandeform.c:678: BLI_array_declare(index_anchors); On 2013/10/23 20:39:20, ideasman42 wrote: > again - allocs and doesnt use. see comment for isSystemDifferent Done.
Sign in to reply to this message.
Regarding NOT using BMesh. First - perhaps get everything else finalized first, then evaluate removing bmesh use. 2 reasons to do this - - memory use, bmesh are very heavy on memory use. - conversion speed (probably less of an issue). BM_EDGES_OF_VERT and BM_FACES_OF_VERT can be replaced with - BKE_mesh_vert_poly_map_create() and BKE_mesh_vert_edge_map_create() I know its less flexible to use fixes arrays from DerivedMesh, but if this isnt changing a lot, IMHO its a worthwhile tradeoff.
Sign in to reply to this message.
Hi Campbell, Howard I need your help I know you have much experience with Blender, and your help would save me much time to search into the code of Blender, if you cannot help me right now so busy that I will continue looking. About the bmesh use: I can delete the use of bmesh, if I can retrieve topology information from other methods, this not represent a problem. If the problem is the computation of Bmesh, Bmesh is computing only one time in the initialization of the Laplacian system, because the LaplacianDeform Modifier, use the cache information for calculate de solution of the system. LaplacianDeform requirement - The system captures the vertex positions at the moment of configure the name of anchors vertex group. - The system only works with triangles and quads, because the Laplace Beltrami Operators have known discretization on triangles and quads. - I need the information of vertex positions - I need the information of faces vertex (only triangles and quads). About Ngons. I look the code in source\blender\editors\armature\meshlaplacian.c, I saw in this file the author use Mesh struct, and this struct only have quads, and triangles. Do you know some structure that allows me to get a tessellated mesh? (I can store the tessellation information for save time), and how I can convert (ModifierData *md, Object *ob, DerivedMesh *derivedData) to this struct. Att. Alexander Pinzon Fernandez 2013/10/23 <ideasman42@gmail.com> > Regarding NOT using BMesh. > First - perhaps get everything else finalized first, then evaluate > removing bmesh use. > > > 2 reasons to do this - > - memory use, bmesh are very heavy on memory use. > - conversion speed (probably less of an issue). > > > BM_EDGES_OF_VERT and BM_FACES_OF_VERT can be replaced with - > BKE_mesh_vert_poly_map_create(**) and BKE_mesh_vert_edge_map_create(**) > > I know its less flexible to use fixes arrays from DerivedMesh, but if > this isnt changing a lot, IMHO its a worthwhile tradeoff. > > https://codereview.appspot.**com/15580049/<https://codereview.appspot.com/155... >
Sign in to reply to this message.
On Tue, Oct 29, 2013 at 1:04 PM, Alexander Pinzon Fernandez < apinzonf@gmail.com> wrote: > Hi Campbell, Howard > > I need your help > > I know you have much experience with Blender, and your help would save me > much time to search into the code of Blender, if you cannot help me right > now so busy that I will continue looking. > > About the bmesh use: > I can delete the use of bmesh, if I can retrieve topology information from > other methods, this not represent a problem. > If the problem is the computation of Bmesh, Bmesh is computing only one > time in the initialization of the Laplacian system, because the > LaplacianDeform Modifier, use the cache information for calculate de > solution of the system. > > Did you see Campbell's previous message where he suggested BKE_mesh_vert_poly_map_create() and BKE_mesh_vert_edge_map_create()? My opinion is that it is less essential to remove the use of Bmesh, and I think Campbell might be willing to do it later if he wants to. He points out that the issue is more the space usage of Bmesh than the time to convert. > LaplacianDeform requirement > - The system captures the vertex positions at the moment of configure the > name of anchors vertex group. > - The system only works with triangles and quads, because the Laplace > Beltrami Operators have known discretization on triangles and quads. > - I need the information of vertex positions > - I need the information of faces vertex (only triangles and quads). > > About Ngons. > I look the code in source\blender\editors\armature\meshlaplacian.c, I saw > in this file the author use Mesh struct, and this struct only have quads, > and triangles. > > Do you know some structure that allows me to get a tessellated mesh? (I > can store the tessellation information for save time), and how I can > convert (ModifierData *md, Object *ob, DerivedMesh *derivedData) to this > struct. > > I sent you mail last night explaining how to use scanfill to get the triangulation of a polygon. Are you looking for something simpler? Or do you need more explanation of how to use it? Another alternative would be to duplicate an ngon face in Bmesh and then use the Bmesh operator that triangulates that face, and then delete the face afterwards. Or you could triangulate the ngon using the Bmesh operator, and then after you have built your Laplacian system, merge all of the triangles back into an ngon. Both of these solutions seem more work to me than just using scanfill. Maybe Campbell knows of an internal function that just triangulates a polygon, given coordinates, but I couldn't find one. > Att. > Alexander Pinzon Fernandez > > > > 2013/10/23 <ideasman42@gmail.com> > > Regarding NOT using BMesh. >> First - perhaps get everything else finalized first, then evaluate >> removing bmesh use. >> >> >> 2 reasons to do this - >> - memory use, bmesh are very heavy on memory use. >> - conversion speed (probably less of an issue). >> >> >> BM_EDGES_OF_VERT and BM_FACES_OF_VERT can be replaced with - >> BKE_mesh_vert_poly_map_create(**) and BKE_mesh_vert_edge_map_create(**) >> >> I know its less flexible to use fixes arrays from DerivedMesh, but if >> this isnt changing a lot, IMHO its a worthwhile tradeoff. >> >> https://codereview.appspot.**com/15580049/<https://codereview.appspot.com/155... >> > >
Sign in to reply to this message.
|
