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

[miikah88 at gmail.com]

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"
On 2011/10/06 13:35:33, nazgul wrote:
> Editors shouldn't be included from kernel
Image sequence baking uses that to update scene frame.
-> I'm solving this by moving "dynamicPaint_bakeImageSequence()" and
"dynamicPaint_initBake()" to editors.

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"
On 2011/10/06 13:35:33, nazgul wrote:
> Can't see why this include is necessary, IMB should be able to handle
everythin
> you want from exr files to be handled.
I'm not sure how. "imb_save_openexr()" is only defined at openexr_api.h
and I don't see any other way to directly save ImBuf as exr.

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"
On 2011/10/06 13:35:33, nazgul wrote:
> 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.
This is included for "validate_layer_name()" function. So I should move
it to the blenkernel? Any suggestions where?

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"
On 2011/10/06 13:35:33, nazgul wrote:
> 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.
Yes, I asked brecht about this a couple of months ago and he didn't have
any other ideas of how to do this.

The issue is reading not only material's base color but color of all
textures linked to the material properly. Only way I can think of is
through rendering code. Of course a api of some kind could be written to
access outside render module?

http://codereview.appspot.com/5189041/diff/1/source/blender/blenkernel/intern/dynamicpaint.c#newcode99
source/blender/blenkernel/intern/dynamicpaint.c:99: struct DerivedMesh;
On 2011/10/06 13:35:33, nazgul wrote:
> 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,
Good point, all of those are already included... Removed.

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};
On 2011/10/06 13:35:33, nazgul wrote:
> 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.

Done.

http://codereview.appspot.com/5189041/diff/1/source/blender/blenkernel/intern/dynamicpaint.c#newcode142
source/blender/blenkernel/intern/dynamicpaint.c:142: } Vec3f;
On 2011/10/06 13:35:33, nazgul wrote:
> Isn't it a big overhead to define struct for vector?
I don't see how. It's used for huge arrays storing vector for each
canvas "point". Thus is allocated in a single call.

Anyway, it makes code cleaner and perhaps faster when accessing that
array. E.g. instead of vec[index*3], a single vec[index] is needed.

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);
On 2011/10/06 13:35:33, nazgul wrote:
> Not so clear why it's needed and why it's needed only for curves.
Curve has to be updated for curve guided/following objects.

Updating other properties isn't imo necessary because this is just for
substep calculation. No one notices if material color etc. doesn't get
updated every substep, while for object movement it's a must. :p

http://codereview.appspot.com/5189041/diff/1/source/blender/blenkernel/intern/dynamicpaint.c#newcode499
source/blender/blenkernel/intern/dynamicpaint.c:499: ob->recalc =
oflags;
On 2011/10/06 13:35:33, nazgul wrote:
> 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..
Don't know enough about Blender animation system for this. I figured
it's always better to restore objects to their original state.

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)
On 2011/10/06 13:35:33, nazgul wrote:
> 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.
Okay moved them on top.

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

Some compilers still show pragma warnings and I'm not sure what happens
if someone compiles Blender with openmp flag manually set on compiler
settins.

So originally added these to make sure absolutely no OpenMP is
used/visible if the flag is not defiend. But if you think it makes code
messy and should be removed I can do that. :p

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 2011/10/06 13:35:33, nazgul wrote:
> 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.
Good point. Changing that.

http://codereview.appspot.com/5189041/diff/1/source/blender/blenkernel/intern/dynamicpaint.c#newcode692
source/blender/blenkernel/intern/dynamicpaint.c:692: return;
On 2011/10/06 13:35:33, nazgul wrote:
> You'll have memory leak here.
Indeed, fixed.

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)
On 2011/10/06 13:35:33, nazgul wrote:
> 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.
If memory runs out during any of those allocations. Without returning,
it would crash. So how should I handle this?

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;
On 2011/10/06 13:35:33, nazgul wrote:
> 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..
I have a bad habit of making sure every pointer is valid... :p

What do you mean by malloc returning non-zero on error? By definition it
returns null on allocation error, right?

http://codereview.appspot.com/5189041/diff/1/source/blender/blenkernel/intern/dynamicpaint.c#newcode990
source/blender/blenkernel/intern/dynamicpaint.c:990: if (scene) {
On 2011/10/06 13:35:33, nazgul wrote:
> How dynamic paint can be started without scene?

Currently "dynamicPaint_createType()" can be called when scene is set to
null. This happens for example at "dynamicPaint_Modifier_copy()", where
scene isn't available but it only initializes the modifier and copies
variables later.

I suppose I should add a comment mentioning about this? :D

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) {
On 2011/10/06 13:35:33, nazgul wrote:
> Maybe switch would be more readable?
Yeah, perhaps. Changing it for now.

However I have been planning to write a better way to handle different
surface types than massive if loops. However for now I don't have enough
time from school to focus on it...

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;
On 2011/10/06 13:35:33, nazgul wrote:
> Is this structure really have to become public (maybe overviewed usage
of this
> structure somewhere else)?

Reverted this part.

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