Issue 12966044: Data transfer review
|
Created:
10 years, 9 months ago by ideasman42 Modified:
10 years, 9 months ago Reviewers:
eng.walidshouman CC:
ideasman42 Base URL:
https://svn.blender.org/svnroot/bf-blender/branches/soc-2013-meshdata_transfer/ Visibility:
Public. |
DescriptionDat Tx Review
Patch Set 1 #
Total comments: 12
MessagesTotal messages: 4
Checked over BM_mesh_data_copy and BM_mesh_multi_layer_copy and don't think its ready for trunk, too much unnecessary complexity. https://codereview.appspot.com/12966044/diff/1/source/blender/bmesh/tools/bme... File source/blender/bmesh/tools/bmesh_data_transfer.c (left): https://codereview.appspot.com/12966044/diff/1/source/blender/bmesh/tools/bme... source/blender/bmesh/tools/bmesh_data_transfer.c:2102: bool BM_mesh_multi_layer_copy(BMesh* bm_src, BMesh *bm_dst, const struct ReplaceLayerInfo replace_info, int type, void *weights, *picky*, better pass a const pointer to the struct rather then a copy. https://codereview.appspot.com/12966044/diff/1/source/blender/bmesh/tools/bme... source/blender/bmesh/tools/bmesh_data_transfer.c:2156: lcol_out.a += (lcol2->a * weight); This is likely to cause float precision issues. Best accumulate as floats and then convert back to unsigbed char. https://codereview.appspot.com/12966044/diff/1/source/blender/bmesh/tools/bme... source/blender/bmesh/tools/bmesh_data_transfer.c:2192: BM_ITER_ELEM_INDEX (f_dst, &fiter, v, BM_FACES_OF_VERT, a) {} Looks like this can be replaced by: a = f_dst->len; https://codereview.appspot.com/12966044/diff/1/source/blender/bmesh/tools/bme... source/blender/bmesh/tools/bmesh_data_transfer.c:2422: //loop over the src faces This block is far too complex, apart from nested loops being problematic, there just has to be a simpler way to achieve this. https://codereview.appspot.com/12966044/diff/1/source/blender/bmesh/tools/bme... source/blender/bmesh/tools/bmesh_data_transfer.c:2583: bool BM_mesh_data_copy(BMesh *bm_src, BMesh* bm_dst, int type, const struct ReplaceLayerInfo replace_info, bool relative_to_target, This function generalizes data transfer, Whether to split each layer type into its own function or not is disputable (with good helper functions and api calls it could be made to work well I believe), but at the moment this function mixed logic in a confusing way that really makes it hard to follow. https://codereview.appspot.com/12966044/diff/1/source/blender/bmesh/tools/bme... source/blender/bmesh/tools/bmesh_data_transfer.c:2606: const int exp_vert_per_face = 10; Using an arbitrary size isn't needed and is a bit sloppy, the total number of face corners is known (bm->totloop), and the length of each face is known. You can allocate a single array matching the length of the total number of loops in the bmesh. Then assign a segment of that array to each face, no guessing needed. https://codereview.appspot.com/12966044/diff/1/source/blender/bmesh/tools/bme... source/blender/bmesh/tools/bmesh_data_transfer.c:2632: const int cd_dvert_dst_offset = CustomData_get_offset(&bm_dst->vdata, CD_MDEFORMVERT); *picky*. The way data for multiple types of copying is collected is a bit disorganized. If this is needed I would group vars assosiated with each type then initialize each in a switch (possibly use a struct for each type). https://codereview.appspot.com/12966044/diff/1/source/blender/bmesh/tools/bme... source/blender/bmesh/tools/bmesh_data_transfer.c:2657: int const exp_dst_loops_per_src_face = dst_src_faces_ratio * exp_loop_per_face * exp_tolerance; This seems to be doing some fairly confusing guess work, I dont see this as being justified, Think of the big picture, You lookup a part of the mesh on the source which is near the destination, interpolate its data and copy to destination... This should be possible without guessing or doing approximations. https://codereview.appspot.com/12966044/diff/1/source/blender/bmesh/tools/bme... source/blender/bmesh/tools/bmesh_data_transfer.c:2888: else if (src_lay_start == src_lay_end) { I'm not convinced there needs to be a separate branch of code here, it should be possible to have a single loop that handles data transfer.
Sign in to reply to this message.
correct, correction :) https://codereview.appspot.com/12966044/diff/1/source/blender/bmesh/tools/bme... File source/blender/bmesh/tools/bmesh_data_transfer.c (left): https://codereview.appspot.com/12966044/diff/1/source/blender/bmesh/tools/bme... source/blender/bmesh/tools/bmesh_data_transfer.c:2192: BM_ITER_ELEM_INDEX (f_dst, &fiter, v, BM_FACES_OF_VERT, a) {} On 2013/08/20 07:02:00, ideasman42 wrote: > Looks like this can be replaced by: > > a = f_dst->len; My mistake, a = BM_vert_face_count(v)
Sign in to reply to this message.
https://codereview.appspot.com/12966044/diff/1/source/blender/bmesh/tools/bme... File source/blender/bmesh/tools/bmesh_data_transfer.c (left): https://codereview.appspot.com/12966044/diff/1/source/blender/bmesh/tools/bme... source/blender/bmesh/tools/bmesh_data_transfer.c:2422: //loop over the src faces On 2013/08/20 07:02:00, ideasman42 wrote: > This block is far too complex, apart from nested loops being problematic, there > just has to be a simpler way to achieve this. /brief: this block searches for the connected faces in the source mesh, then according to the destination loops that are connected to each source face it makes the decision -by looking at the source face and its neighbors- whether to weld the destination loops or not welding is done by adding the loops to l_grp's to be later averaged at once getting rid of this part would make us not able to connect the faces of the same island, the only thing I could think of for this part is to make the steps a bit meaningful/modular https://codereview.appspot.com/12966044/diff/1/source/blender/bmesh/tools/bme... source/blender/bmesh/tools/bmesh_data_transfer.c:2606: const int exp_vert_per_face = 10; On 2013/08/20 07:02:00, ideasman42 wrote: > Using an arbitrary size isn't needed and is a bit sloppy, the total number of > face corners is known (bm->totloop), and the length of each face is known. You > can allocate a single array matching the length of the total number of loops in > the bmesh. Then assign a segment of that array to each face, no guessing needed. the main usage of the exp variables -as used in line 2720- is based on not having any clue about the variable in advance and we may need to use malloc/realloc several times within some loops to make an online update for the allocated memory, with the aid of the expected variables we could make a single offline malloc and later expand by using realloc only if needed within the code so its use is for speeding up the code by avoiding nested reallocations ... in that case is there any better way to approach the speed optimisation?
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||
