[Bf-codereview] Blender Dynamic Paint (soc-2011-carrot) (issue 5189041)

[g.ulairi at gmail.com]

Hi,

Made really quick review, so just some of issues reported.

General remarks:
- You're checking for result of MEM_calloc function which i'm not sure
really useful. Blender needs more general mechanism to survive memory
shortage and that checks only made code a bit less easy to read and
sometimes returning form middle of function lead to memory leaks.
- I'm not sure making some structures from BVH public also necessary. I
didn't see dynamic-paint structures used in that callbacks (probably
just overview them).
- Not sure about check for dynamic paint modifiers existance when adding
new modifier. If there's no such check, some part of code can fail due
to they're using first modifier only.

Didn't make design check yet, so most probably more motes would be
posted soon.


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

http://codereview.appspot.com/5189041/diff/1/source/blender/blenkernel/intern/dynamicpaint.c#newcode65
source/blender/blenkernel/intern/dynamicpaint.c:65: #include
"../editors/include/ED_screen.h"
Editors shouldn't be included from kernel

http://codereview.appspot.com/5189041/diff/1/source/blender/blenkernel/intern/dynamicpaint.c#newcode74
source/blender/blenkernel/intern/dynamicpaint.c:74: #include
"intern/openexr/openexr_api.h"
Can't see why this include is necessary, IMB should be able to handle
everythin you want from exr files to be handled.

http://codereview.appspot.com/5189041/diff/1/source/blender/blenkernel/intern/dynamicpaint.c#newcode78
source/blender/blenkernel/intern/dynamicpaint.c:78: #include
"intern/MOD_util.h"
Also bad-level call. If some function which is used by modifiers only is
needed ,it better be moved to kernel and modifiers should use it.

http://codereview.appspot.com/5189041/diff/1/source/blender/blenkernel/intern/dynamicpaint.c#newcode85
source/blender/blenkernel/intern/dynamicpaint.c:85: #include
"../render/intern/include/texture.h"
Maybe there's more "straight" method of reading material color. Maybe
some code could be re-shuffled and moved to kernel.. Better ask Brecht
about this.

http://codereview.appspot.com/5189041/diff/1/source/blender/blenkernel/intern/dynamicpaint.c#newcode99
source/blender/blenkernel/intern/dynamicpaint.c:99: struct DerivedMesh;
Why this is needed here? IMO, it'll be safer to include DNA_{ovject,
scene}_types.h/BKE_DerivedMesh.h to have this structures defined here,

http://codereview.appspot.com/5189041/diff/1/source/blender/blenkernel/intern/dynamicpaint.c#newcode113
source/blender/blenkernel/intern/dynamicpaint.c:113: int neighY[8] =
{0,1,1, 1, 0,-1,-1,-1};
This line and several lines upper: global variables should be declared
as "static". Otherwise it'll be really easy to run into symbol conflicts
on linking.

http://codereview.appspot.com/5189041/diff/1/source/blender/blenkernel/intern/dynamicpaint.c#newcode142
source/blender/blenkernel/intern/dynamicpaint.c:142: } Vec3f;
Isn't it a big overhead to define struct for vector?

http://codereview.appspot.com/5189041/diff/1/source/blender/blenkernel/intern/dynamicpaint.c#newcode479
source/blender/blenkernel/intern/dynamicpaint.c:479:
BKE_animsys_evaluate_animdata(scene, &cu->id, cu->adt, frame,
ADT_RECALC_ANIM);
Not so clear why it's needed and why it's needed only for curves.

http://codereview.appspot.com/5189041/diff/1/source/blender/blenkernel/intern/dynamicpaint.c#newcode499
source/blender/blenkernel/intern/dynamicpaint.c:499: ob->recalc =
oflags;
It's also not so clear for me why such kind of tricks is needed. You're
saving flags, forcing animation re-calculation, restoring flags. Not
sure why such kind of flag restoring can really help -- o.e. object
would be in another position and wouldn't be re-calculated to be on
correct position after this tricks..

http://codereview.appspot.com/5189041/diff/1/source/blender/blenkernel/intern/dynamicpaint.c#newcode509
source/blender/blenkernel/intern/dynamicpaint.c:509: #define
BRUSH_USES_VELOCITY (1<<0)
This and some other defines -- personally i prefer all flags be defined
in header file on in the beginning of implementation file. It's easier
to track them.

http://codereview.appspot.com/5189041/diff/1/source/blender/blenkernel/intern/dynamicpaint.c#newcode659
source/blender/blenkernel/intern/dynamicpaint.c:659: #ifdef _OPENMP
AFAIK, it's not necessary to check for _OPENMP before #pagma omp -- if
openmp is disabled in build rules this pragma would be ignored.

http://codereview.appspot.com/5189041/diff/1/source/blender/blenkernel/intern/dynamicpaint.c#newcode679
source/blender/blenkernel/intern/dynamicpaint.c:679: dim[2] =
grid->grid_bounds.max[2]-grid->grid_bounds.min[2];
On the first glance it can be replaced with sub_v3_v3v3. But few lines
downer you're assigning vector's components to scalar variables.
Probably vector or scalar values can be avoided.

http://codereview.appspot.com/5189041/diff/1/source/blender/blenkernel/intern/dynamicpaint.c#newcode692
source/blender/blenkernel/intern/dynamicpaint.c:692: return;
You'll have memory leak here.

http://codereview.appspot.com/5189041/diff/1/source/blender/blenkernel/intern/dynamicpaint.c#newcode721
source/blender/blenkernel/intern/dynamicpaint.c:721: if (!grid->bounds
|| !grid->s_pos || !grid->s_num || !grid->t_index || !temp_s_num ||
!temp_t_index)
Not sure when this can happen. Only when total_points/grid_cells are
zero? IMO, this cases should be handled by checking element count, not
result of malloc functions.

http://codereview.appspot.com/5189041/diff/1/source/blender/blenkernel/intern/dynamicpaint.c#newcode959
source/blender/blenkernel/intern/dynamicpaint.c:959: if (!surface)
return NULL;
Such kind of checks aren't really necessary/helpful. Blender needs more
general system to handle out-of-memory errors. And the worst thing is
that even if malloc returned non-zero, it doesn't mean you'll be able to
use all memory you requested..

http://codereview.appspot.com/5189041/diff/1/source/blender/blenkernel/intern/dynamicpaint.c#newcode990
source/blender/blenkernel/intern/dynamicpaint.c:990: if (scene) {
How dynamic paint can be started without scene?

http://codereview.appspot.com/5189041/diff/1/source/blender/blenkernel/intern/dynamicpaint.c#newcode1171
source/blender/blenkernel/intern/dynamicpaint.c:1171: if (surface->type
== MOD_DPAINT_SURFACE_T_PAINT) {
Maybe switch would be more readable?

http://codereview.appspot.com/5189041/diff/1/source/blender/blenlib/BLI_kdtree.h
File source/blender/blenlib/BLI_kdtree.h (right):

http://codereview.appspot.com/5189041/diff/1/source/blender/blenlib/BLI_kdtree.h#newcode55
source/blender/blenlib/BLI_kdtree.h:55: } KDTreeNode;
Is this structure really have to become public (maybe overviewed usage
of this structure somewhere else)?

http://codereview.appspot.com/5189041/