Blender Dynamic Paint (soc-2011-carrot)

[nazgul, 2011-10-06 13:35:33]

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...
File source/blender/blenkernel/intern/dynamicpaint.c (right):

http://codereview.appspot.com/5189041/diff/1/source/blender/blenkernel/intern...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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_kdtre...
source/blender/blenlib/BLI_kdtree.h:55: } KDTreeNode;
Is this structure really have to become public (maybe overviewed usage of this
structure somewhere else)?

[MiikaH, 2011-10-06 16:11:29]

6.10.2011 16:35, g.ulairi@gmail.com wrote:
> 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.
Yes, I'm not sure if this is good practice at all. It certainly makes 
code harder to read and write...

Otherwise however, it would crash and I'm sure that's not good either. :p

> - 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).
This is clearly an issue in my code. I suppose there was need for that 
struct when I was experimenting with particle brush code, but I never 
noticed it was no more used when I finished.

[MiikaH, 2011-10-06 17:08:54]

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

http://codereview.appspot.com/5189041/diff/1/source/blender/blenkernel/intern...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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...
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_kdtre...
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.

[nazgul, 2011-10-11 12:13:12]

Pardon, didn't have enough time yet to make deeper review. Answered some
comments.

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

http://codereview.appspot.com/5189041/diff/1/source/blender/blenkernel/intern...
source/blender/blenkernel/intern/dynamicpaint.c:74: #include
"intern/openexr/openexr_api.h"
It should be something like ibuf->ftype= OPENEXR and then IMB_saveiff should
save this ibuf as exr image.

http://codereview.appspot.com/5189041/diff/1/source/blender/blenkernel/intern...
source/blender/blenkernel/intern/dynamicpaint.c:78: #include "intern/MOD_util.h"
Didn't use it before but sounds like it's custom data layer related. So probably
it should be in customdata.c

http://codereview.appspot.com/5189041/diff/1/source/blender/blenkernel/intern...
source/blender/blenkernel/intern/dynamicpaint.c:85: #include
"../render/intern/include/texture.h"
Yes, understand this. I asked Brecht to help you here.

http://codereview.appspot.com/5189041/diff/1/source/blender/blenkernel/intern...
source/blender/blenkernel/intern/dynamicpaint.c:499: ob->recalc = oflags;
Problem that you aren't restoring all states, you're restoring just flags.
Probably just tagging object to be re-calculated after your work is nicer -- all
dependencies would be updated automatically due to depsgraph.

http://codereview.appspot.com/5189041/diff/1/source/blender/blenkernel/intern...
source/blender/blenkernel/intern/dynamicpaint.c:659: #ifdef _OPENMP
It's not really hurts, just other places doesn't use this check. In fact, think
can be kept unchanged here.

http://codereview.appspot.com/5189041/diff/1/source/blender/blenkernel/intern...
source/blender/blenkernel/intern/dynamicpaint.c:990: if (scene) {
I see. And yes, comments for non-obvious cases helps a lot :)

[MiikaH, 2011-10-13 22:07:31]

Updated issue with latest carrot revision.

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

http://codereview.appspot.com/5189041/diff/7001/source/blender/blenkernel/int...
source/blender/blenkernel/intern/dynamicpaint.c:67: #include "intern/MOD_util.h"
Oh, forgot to remove this line even though it's no longer needed. Will update in
next commit. :p

[nazgul, 2011-10-19 16:55:34]

Didn't notice something fatal. But as i told, a asked Brecht to check
render-related stuff. And probably he'll suggest something nicer for that
subframe updated.. Apart this thins, don't see any real issues.

http://codereview.appspot.com/5189041/diff/7001/source/blender/blenkernel/BKE...
File source/blender/blenkernel/BKE_dynamicpaint.h (right):

http://codereview.appspot.com/5189041/diff/7001/source/blender/blenkernel/BKE...
source/blender/blenkernel/BKE_dynamicpaint.h:28: struct PaintAdjData *adj_data;
/* adjacency data for current surface */
Not sure, but probably some compilers can generate warning/error here. Better to
add "struct PaintAdjData;" to the beginning of this file (like struct bContext
and so)

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

http://codereview.appspot.com/5189041/diff/7001/source/blender/blenkernel/int...
source/blender/blenkernel/intern/dynamicpaint.c:2537: #if 0
In fact, if it's useful for debuggning this can be replaced with if (G.f &
G_DEBUG). Just a note :)

http://codereview.appspot.com/5189041/diff/7001/source/blender/blenkernel/int...
source/blender/blenkernel/intern/dynamicpaint.c:5121: if(md && md->mode &
(eModifierMode_Realtime | eModifierMode_Render))
Not sure, but does it mean modifier would be applied during viewport playback
when realtime apply is disabled but apply during render is allowed?

http://codereview.appspot.com/5189041/diff/7001/source/blender/blenloader/int...
File source/blender/blenloader/intern/readfile.c (right):

http://codereview.appspot.com/5189041/diff/7001/source/blender/blenloader/int...
source/blender/blenloader/intern/readfile.c:4132: if (pmd->brush) {
Isn't it a duplicated check?

[MiikaH, 2011-10-19 17:13:13]

Some replies to comments.

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

http://codereview.appspot.com/5189041/diff/7001/source/blender/blenkernel/int...
source/blender/blenkernel/intern/dynamicpaint.c:2537: #if 0
On 2011/10/19 16:55:34, nazgul wrote:
> In fact, if it's useful for debuggning this can be replaced with if (G.f &
> G_DEBUG). Just a note :)
This is only useful when debugging UV surface generation code, so no point
enabling it for general debug. However it's useful enough to keep here as #if 0
imo. :)

http://codereview.appspot.com/5189041/diff/7001/source/blender/blenkernel/int...
source/blender/blenkernel/intern/dynamicpaint.c:5121: if(md && md->mode &
(eModifierMode_Realtime | eModifierMode_Render))
On 2011/10/19 16:55:34, nazgul wrote:
> Not sure, but does it mean modifier would be applied during viewport playback
> when realtime apply is disabled but apply during render is allowed?
You tell me. I'm not experienced enough in Blender's way of handling
simultaneous rendering/viewport usage to judge that. :x

Anyway, this part is quite much copied from smoke simulator (smoke.c) so I
assume it's ok.

http://codereview.appspot.com/5189041/diff/7001/source/blender/blenloader/int...
File source/blender/blenloader/intern/readfile.c (right):

http://codereview.appspot.com/5189041/diff/7001/source/blender/blenloader/int...
source/blender/blenloader/intern/readfile.c:4132: if (pmd->brush) {
On 2011/10/19 16:55:34, nazgul wrote:
> Isn't it a duplicated check?
Haha, dunno how I have missed that!

[brechtvl, 2011-10-20 14:03:28]

Comments from a quick read over the diffs, more later.

http://codereview.appspot.com/5189041/diff/7001/release/scripts/startup/bl_ui...
File release/scripts/startup/bl_ui/properties_physics_dynamicpaint.py (right):

http://codereview.appspot.com/5189041/diff/7001/release/scripts/startup/bl_ui...
release/scripts/startup/bl_ui/properties_physics_dynamicpaint.py:49:
layout.prop(md, "ui_type", expand=True)
There's various enums used to define UI context, but this is confusing, for
example in this case it seems like you have to make an object either a canvas or
a brush, but this isn't the case. I'd make these separate panels with a checkbox
next to the title to enable/disable.

http://codereview.appspot.com/5189041/diff/7001/release/scripts/startup/bl_ui...
release/scripts/startup/bl_ui/properties_physics_dynamicpaint.py:143:
layout.label(text="-WIP-")
Should this still be here?

http://codereview.appspot.com/5189041/diff/7001/release/scripts/startup/bl_ui...
release/scripts/startup/bl_ui/properties_physics_dynamicpaint.py:147: bl_label =
"Dynamic Paint: Advanced"
Don't use ":" in labels for consistency with other panels.

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

http://codereview.appspot.com/5189041/diff/7001/source/blender/blenkernel/int...
source/blender/blenkernel/intern/dynamicpaint.c:33: #include
"DNA_userdef_types.h"	/* to get temp file path	*/
I don't see user preferences used here, but in any case it's not where you
should get the temp file path from.

http://codereview.appspot.com/5189041/diff/7001/source/blender/blenkernel/int...
source/blender/blenkernel/intern/dynamicpaint.c:74: #include
"../render/intern/include/texture.h"
This internal includes really shouldn't be done here. Make an api to the render
module functions instead, keeping as much hidden as possible.

http://codereview.appspot.com/5189041/diff/7001/source/blender/blenlib/intern...
File source/blender/blenlib/intern/BLI_kdtree.c (right):

http://codereview.appspot.com/5189041/diff/7001/source/blender/blenlib/intern...
source/blender/blenlib/intern/BLI_kdtree.c:469: int
BLI_kdtree_range_search_thread_safe(KDTree *tree, float range, float *co, float
*nor, KDTreeNearest *nearest, int limit)
I don't understand what makes this function thread safe and the other not? The
only difference I see is that this one has a fixed limit for the stack size and
number of points, while the other doesn't.

The reason for a stack size limit is unclear to me, it's just going to result in
incorrect lookups. And for a limited number of points there is
BLI_kdtree_find_n_nearest?

http://codereview.appspot.com/5189041/diff/7001/source/blender/editors/physic...
File source/blender/editors/physics/dynamicpaint_ops.c (right):

http://codereview.appspot.com/5189041/diff/7001/source/blender/editors/physic...
source/blender/editors/physics/dynamicpaint_ops.c:405: /* Format time string	*/
There's a utility function for this: BLI_timestr. If this doesn't do exactly
what you need, either modify it or move this code into a new utility function.

http://codereview.appspot.com/5189041/diff/7001/source/blender/editors/physic...
source/blender/editors/physics/dynamicpaint_ops.c:428:
sprintf(pmd->canvas->ui_info, "Bake Complete! (Time: %s)", timestr);
Use BLI_snprintf instead.

http://codereview.appspot.com/5189041/diff/7001/source/blender/modifiers/inte...
File source/blender/modifiers/intern/MOD_dynamicpaint.c (right):

http://codereview.appspot.com/5189041/diff/7001/source/blender/modifiers/inte...
source/blender/modifiers/intern/MOD_dynamicpaint.c:15: #include "stddef.h"
Use #include <stddef.h>

http://codereview.appspot.com/5189041/diff/7001/source/blender/modifiers/inte...
source/blender/modifiers/intern/MOD_dynamicpaint.c:61: dataMask |= (1 <<
CD_MDEFORMVERT);
Are all these layers always needed? Is it possible to check which ones are
needed beforehand?

http://codereview.appspot.com/5189041/diff/7001/source/blender/modifiers/inte...
source/blender/modifiers/intern/MOD_dynamicpaint.c:88: for(; base; base =
base->next) {
It seems brush_group is not taken into account here.

http://codereview.appspot.com/5189041/diff/7001/source/blender/modifiers/inte...
source/blender/modifiers/intern/MOD_dynamicpaint.c:126: walk(userData, ob, md,
""); /* property name isn't used */
It isn't used now, but will be soon. It should contain the rna path to each
texture pointer, will be used to list all textures on an object.

http://codereview.appspot.com/5189041/diff/7001/source/blender/nodes/shader/n...
File source/blender/nodes/shader/nodes/node_shader_geom.c (right):

http://codereview.appspot.com/5189041/diff/7001/source/blender/nodes/shader/n...
source/blender/nodes/shader/nodes/node_shader_geom.c:50: {	SOCK_FLOAT,   0,
"Vertex Alpha"},
I don't think there should be a separate output for this, let users extract the
alpha channel if they want it?

http://codereview.appspot.com/5189041/diff/7001/source/blender/render/intern/...
File source/blender/render/intern/include/texture.h (right):

http://codereview.appspot.com/5189041/diff/7001/source/blender/render/intern/...
source/blender/render/intern/include/texture.h:71: void do_volume_tex(struct
ShadeInput *shi, const float *xyz, int mapto_flag, float *col, float *val,
struct Render *re);
Why change xyz[3] to *xyz? The former is more descriptive.

http://codereview.appspot.com/5189041/diff/7001/source/blender/render/intern/...
File source/blender/render/intern/source/shadeinput.c (right):

http://codereview.appspot.com/5189041/diff/7001/source/blender/render/intern/...
source/blender/render/intern/source/shadeinput.c:1117: /* try to prevent invalid
color sampling of zero alpha points */
I don't understand what this code is supposed to do? I get the impression this
is perhaps compensating for the alpha not being premultiplied.

http://codereview.appspot.com/5189041/diff/7001/source/blender/render/intern/...
File source/blender/render/intern/source/shadeoutput.c (right):

http://codereview.appspot.com/5189041/diff/7001/source/blender/render/intern/...
source/blender/render/intern/source/shadeoutput.c:877: float inv_alpha = 1.0f -
shi->vcol[3];
inv_alpha => neg_alpha. It's negated, not inverted.

[MiikaH, 2011-10-20 18:09:57]

Replied to some comments. I'll work tomorrow on some material/rendering changes
discussed in irc.

http://codereview.appspot.com/5189041/diff/7001/release/scripts/startup/bl_ui...
File release/scripts/startup/bl_ui/properties_physics_dynamicpaint.py (right):

http://codereview.appspot.com/5189041/diff/7001/release/scripts/startup/bl_ui...
release/scripts/startup/bl_ui/properties_physics_dynamicpaint.py:49:
layout.prop(md, "ui_type", expand=True)
On 2011/10/20 14:03:28, brechtvl wrote:
> There's various enums used to define UI context, but this is confusing, for
> example in this case it seems like you have to make an object either a canvas
or
> a brush, but this isn't the case. I'd make these separate panels with a
checkbox
> next to the title to enable/disable.
Personally, I definitely wouldn't like to display any brush settings/panels when
only canvas is used vice versa. There are already too many panels visible. And
having similar looking panels for color adjustment etc. in both canvas and brush
panels it could be quite messy to show everything.

Also even when both are enabled I really prefer being able to toggle
brush/canvas panels visibility to only see settings I'm interested in (or are
related to each other).

There are also those huge "Add/Remove Canvas/Brush" buttons below so it should
be quite clear that you can enable both at once.

http://codereview.appspot.com/5189041/diff/7001/release/scripts/startup/bl_ui...
release/scripts/startup/bl_ui/properties_physics_dynamicpaint.py:147: bl_label =
"Dynamic Paint: Advanced"
On 2011/10/20 14:03:28, brechtvl wrote:
> Don't use ":" in labels for consistency with other panels.

Ok, changed.

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

http://codereview.appspot.com/5189041/diff/7001/source/blender/blenkernel/int...
source/blender/blenkernel/intern/dynamicpaint.c:33: #include
"DNA_userdef_types.h"	/* to get temp file path	*/
On 2011/10/20 14:03:28, brechtvl wrote:
> I don't see user preferences used here, but in any case it's not where you
> should get the temp file path from.
Oh, right. That one is no longer used.

http://codereview.appspot.com/5189041/diff/7001/source/blender/blenlib/intern...
File source/blender/blenlib/intern/BLI_kdtree.c (right):

http://codereview.appspot.com/5189041/diff/7001/source/blender/blenlib/intern...
source/blender/blenlib/intern/BLI_kdtree.c:469: int
BLI_kdtree_range_search_thread_safe(KDTree *tree, float range, float *co, float
*nor, KDTreeNearest *nearest, int limit)
On 2011/10/20 14:03:28, brechtvl wrote:
> I don't understand what makes this function thread safe and the other not? The
> only difference I see is that this one has a fixed limit for the stack size
and
> number of points, while the other doesn't.
> 
> The reason for a stack size limit is unclear to me, it's just going to result
in
> incorrect lookups. And for a limited number of points there is
> BLI_kdtree_find_n_nearest?

Original function allocates memory which apparently doesn't work well with
OpenMP threads. It was constantly crashing.

http://codereview.appspot.com/5189041/diff/7001/source/blender/editors/physic...
File source/blender/editors/physics/dynamicpaint_ops.c (right):

http://codereview.appspot.com/5189041/diff/7001/source/blender/editors/physic...
source/blender/editors/physics/dynamicpaint_ops.c:405: /* Format time string	*/
On 2011/10/20 14:03:28, brechtvl wrote:
> There's a utility function for this: BLI_timestr. If this doesn't do exactly
> what you need, either modify it or move this code into a new utility function.
Oh great! That should be fine, certainly will clear up the code alot.

http://codereview.appspot.com/5189041/diff/7001/source/blender/modifiers/inte...
File source/blender/modifiers/intern/MOD_dynamicpaint.c (right):

http://codereview.appspot.com/5189041/diff/7001/source/blender/modifiers/inte...
source/blender/modifiers/intern/MOD_dynamicpaint.c:61: dataMask |= (1 <<
CD_MDEFORMVERT);
On 2011/10/20 14:03:28, brechtvl wrote:
> Are all these layers always needed? Is it possible to check which ones are
> needed beforehand?
Yes, it's possible. Changing.

http://codereview.appspot.com/5189041/diff/7001/source/blender/modifiers/inte...
source/blender/modifiers/intern/MOD_dynamicpaint.c:88: for(; base; base =
base->next) {
On 2011/10/20 14:03:28, brechtvl wrote:
> It seems brush_group is not taken into account here.
Yeah, forgot this one when I added groups...

http://codereview.appspot.com/5189041/diff/7001/source/blender/nodes/shader/n...
File source/blender/nodes/shader/nodes/node_shader_geom.c (right):

http://codereview.appspot.com/5189041/diff/7001/source/blender/nodes/shader/n...
source/blender/nodes/shader/nodes/node_shader_geom.c:50: {	SOCK_FLOAT,   0,
"Vertex Alpha"},
On 2011/10/20 14:03:28, brechtvl wrote:
> I don't think there should be a separate output for this, let users extract
the
> alpha channel if they want it?
I didn't know this was possible? When looking at any material nodes there is
always a separate alpha output/input and I don't see alpha value in
"separate/combine RGB" node either.

http://codereview.appspot.com/5189041/diff/7001/source/blender/render/intern/...
File source/blender/render/intern/include/texture.h (right):

http://codereview.appspot.com/5189041/diff/7001/source/blender/render/intern/...
source/blender/render/intern/include/texture.h:71: void do_volume_tex(struct
ShadeInput *shi, const float *xyz, int mapto_flag, float *col, float *val,
struct Render *re);
On 2011/10/20 14:03:28, brechtvl wrote:
> Why change xyz[3] to *xyz? The former is more descriptive.
Oh, this is a merge mistake. This was changed to [3] in trunk after I added the
render struct. -> Apparently I missed that change while merging.

http://codereview.appspot.com/5189041/diff/7001/source/blender/render/intern/...
File source/blender/render/intern/source/shadeinput.c (right):

http://codereview.appspot.com/5189041/diff/7001/source/blender/render/intern/...
source/blender/render/intern/source/shadeinput.c:1117: /* try to prevent invalid
color sampling of zero alpha points */
On 2011/10/20 14:03:28, brechtvl wrote:
> I don't understand what this code is supposed to do? I get the impression this
> is perhaps compensating for the alpha not being premultiplied.
Yeah, I'm not sure if internal color data should be premultiplied or not. E.g.
would it work with mix nodes properly? (current non-multiplied code seems to
work quite well :s)

[brechtvl, 2011-10-26 16:34:51]

More comments.

http://codereview.appspot.com/5189041/diff/34001/source/blender/blenkernel/BK...
File source/blender/blenkernel/BKE_dynamicpaint.h (right):

http://codereview.appspot.com/5189041/diff/34001/source/blender/blenkernel/BK...
source/blender/blenkernel/BKE_dynamicpaint.h:17: #include
"DNA_dynamicpaint_types.h"
Convention is to avoid including headers in headers, and used struct
declarations instead.

http://codereview.appspot.com/5189041/diff/34001/source/blender/blenkernel/BK...
source/blender/blenkernel/BKE_dynamicpaint.h:41: short state;	/* -1 = doesn't
exist (On UV mapped image
Some #defines for this might be good, rather than numbers.

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

http://codereview.appspot.com/5189041/diff/34001/source/blender/blenkernel/in...
source/blender/blenkernel/intern/dynamicpaint.c:2754: static void
mesh_faces_spherecast_dp(void *userdata, int index, const BVHTreeRay *ray,
BVHTreeRayHit *hit)
I'd prefer these to be in bvhutils.c. Or at least add a prefix to the function
ray_tri_intersection to disambiguate it from generic math functions.

http://codereview.appspot.com/5189041/diff/34001/source/blender/blenlib/BLI_m...
File source/blender/blenlib/BLI_math_geom.h (right):

http://codereview.appspot.com/5189041/diff/34001/source/blender/blenlib/BLI_m...
source/blender/blenlib/BLI_math_geom.h:63: void closest_to_line_segment_v2(float
*closest, float p[2], float l1[2], float l2[2]);
const qualifier is missing, other functions here have this.

http://codereview.appspot.com/5189041/diff/34001/source/blender/editors/physi...
File source/blender/editors/physics/dynamicpaint_ops.c (right):

http://codereview.appspot.com/5189041/diff/34001/source/blender/editors/physi...
source/blender/editors/physics/dynamicpaint_ops.c:74: pmd->canvas->active_sur++;
Double indentation here and the 3 following exec functions.

http://codereview.appspot.com/5189041/diff/34001/source/blender/editors/physi...
source/blender/editors/physics/dynamicpaint_ops.c:207: if (pmd->canvas) {
Nitpick: perhaps also OPERATOR_CANCELLED if(!pmd->canvas)

http://codereview.appspot.com/5189041/diff/34001/source/blender/editors/physi...
source/blender/editors/physics/dynamicpaint_ops.c:278: if (frames <= 0)
{sprintf(canvas->error, "No frames to bake.");printf("DynamicPaint bake failed:
%s", canvas->error);return 0;}
I'd rather not have printfs in addition to BKE_report here. All information
should be communicated via the user interface anyway, if all operators started
doing printf as well it becomes messy.

Also better use BLI_strncpy instead of printf here an in other places.

http://codereview.appspot.com/5189041/diff/34001/source/blender/editors/physi...
source/blender/editors/physics/dynamicpaint_ops.c:314: char filename[250];
Use FILE_MAX for filepath sizes.

http://codereview.appspot.com/5189041/diff/34001/source/blender/editors/physi...
source/blender/editors/physics/dynamicpaint_ops.c:320: /* Add frame number
padding	*/
Most of this code can be replaced by BKE_makepicstring.

http://codereview.appspot.com/5189041/diff/34001/source/blender/editors/physi...
source/blender/editors/physics/dynamicpaint_ops.c:405: *  Report for ended bake
and how long it took */
I don't think this ui info is necessary, would remove the label in the UI.
Better to just communicate everything through the reports system. If each tool
did this, it would get out of hand a bit.

http://codereview.appspot.com/5189041/diff/34001/source/blender/editors/physi...
source/blender/editors/physics/dynamicpaint_ops.c:443: void
DPAINT_OT_bake(wmOperatorType *ot)
This should ideally be a modal operator that uses the progress bar mechanism.
The timercursor is no longer used in other places.

http://codereview.appspot.com/5189041/diff/34001/source/blender/editors/space...
File source/blender/editors/space_view3d/drawobject.c (right):

http://codereview.appspot.com/5189041/diff/34001/source/blender/editors/space...
source/blender/editors/space_view3d/drawobject.c:2698: if(md && md->mode &
(eModifierMode_Realtime | eModifierMode_Render))
Should this only check for Realtime and not for render?

http://codereview.appspot.com/5189041/diff/34001/source/blender/makesrna/inte...
File source/blender/makesrna/intern/rna_dynamicpaint.c (right):

http://codereview.appspot.com/5189041/diff/34001/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_dynamicpaint.c:319:
{MOD_DPAINT_INITIAL_VERTEXCOLOR, "VERTEXCOLOR", ICON_GROUP_VCOL, "Vertex Color",
""},
VERTEXCOLOR => VERTEX_COLOR

http://codereview.appspot.com/5189041/diff/34001/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_dynamicpaint.c:422: prop=
RNA_def_property(srna, "start_frame", PROP_INT, PROP_NONE);
Use frame_start, frame_end and frame_substeps for consistency with other
properties.

http://codereview.appspot.com/5189041/diff/34001/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_dynamicpaint.c:441: prop=
RNA_def_property(srna, "use_anti_aliasing", PROP_BOOLEAN, PROP_NONE);
Consistency with scene render property: use_anti_aliasing => use_antialiasing

http://codereview.appspot.com/5189041/diff/34001/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_dynamicpaint.c:595: prop=
RNA_def_property(srna, "disp_factor", PROP_FLOAT, PROP_NONE);
Don't abbreviate displace to disp.

http://codereview.appspot.com/5189041/diff/34001/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_dynamicpaint.c:612: prop=
RNA_def_property(srna, "incremental_disp", PROP_BOOLEAN, PROP_NONE);
Add use_ prefix.

http://codereview.appspot.com/5189041/diff/34001/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_dynamicpaint.c:688: {MOD_DPAINT_COL_PSYS,
"PSYS", ICON_PARTICLES, "Particle System", ""},
PSYS => PARTICLE_SYSTEM

http://codereview.appspot.com/5189041/diff/34001/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_dynamicpaint.c:691: {MOD_DPAINT_COL_VOLDIST,
"VOLDIST", ICON_META_CUBE, "Mesh Volume + Proximity", ""},
VOLDIST => VOLUME_DISTANCE

http://codereview.appspot.com/5189041/diff/34001/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_dynamicpaint.c:710: {MOD_DPAINT_RAY_ZPLUS,
"ZPLUS", 0, "Z-Axis", ""},
ZPLUS => Z_AXIS

http://codereview.appspot.com/5189041/diff/34001/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_dynamicpaint.c:812: prop=
RNA_def_property(srna, "prox_ramp_alpha", PROP_BOOLEAN, PROP_NONE);
prox => proximity

http://codereview.appspot.com/5189041/diff/34001/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_dynamicpaint.c:826: prop=
RNA_def_property(srna, "ray_dir", PROP_ENUM, PROP_NONE);
dir => direction

http://codereview.appspot.com/5189041/diff/34001/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_dynamicpaint.c:829:
RNA_def_property_ui_text(prop, "Ray Dir", "Defines ray direction to use for
projection. If brush object is located in that direction it's painted");
Dir => Direction

http://codereview.appspot.com/5189041/diff/34001/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_dynamicpaint.c:839: prop=
RNA_def_property(srna, "psys", PROP_POINTER, PROP_NONE);
psys => particle_system

http://codereview.appspot.com/5189041/diff/34001/source/blender/makesrna/inte...
source/blender/makesrna/intern/rna_dynamicpaint.c:847: prop=
RNA_def_property(srna, "use_part_radius", PROP_BOOLEAN, PROP_NONE);
part => use_particle_radius

[brechtvl, 2011-10-26 18:12:15]

Default Brush color could be something nicer than (0, 0, 1), e.g. (0.2, 0.5,
0.8).

Further, regarding the layout in panels, in general buttons spanning the entire
width should be avoided, unless for e.g. a file path that needs this much space.
Here's some suggestions to make it more co
nsistent & compact.

Dynamic Paint Velocity:

Remove "Brush Velocity Settings:" and "Smudge:" labels, they are unneeded.
Smudge settings could be on a single row:
[Do Smudge] [Strength]

Dynamic Paint Advanced: 

[Wetmap Drying] [Time] [Slow]
[Dissolve] [Time] [Slow]
[Brush Group:     ]  

Dynamic Paint Output:

[Bake Image Sequence] (at the top, like Bake panel)
[UV Layer:        ]
----------------------
[              ][Folder icon] (over entire width, like render output panel)
[PNG] [Premultiply Alpha]
----------------------
[Paintmaps: paintmap ]
[Wetmaps:   wetmap   ]

[brechtvl, 2011-10-26 18:43:44]

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

http://codereview.appspot.com/5189041/diff/34001/source/blender/blenkernel/in...
source/blender/blenkernel/intern/dynamicpaint.c:103: #define VECMULVAL(v,val)
	{*(v)*=(val); *(v+1)*=(val); *(v+2)*=(val);}
mul_v3_fl can be used for this, and add_v3_fl could be added. Also general
comment, we're trying to switch over from the macro's to math functions, e.g.
VECSUB to sub_v3_v3v3, would be nice if these were used where possible.

http://codereview.appspot.com/5189041/diff/34001/source/blender/blenkernel/in...
source/blender/blenkernel/intern/dynamicpaint.c:331: strncpy(name, basename,
62); /* in case basename is surface->name use a copy */
Use BLI_strncpy instead of strncpy, the latter will not null terminate strings
that are too long.

http://codereview.appspot.com/5189041/diff/34001/source/blender/blenkernel/in...
source/blender/blenkernel/intern/dynamicpaint.c:419: t_color[2] =
t_color[2]*invFact + s_color[2]*factor;
interp_v3_v3v3  could be used here.

http://codereview.appspot.com/5189041/diff/34001/source/blender/blenkernel/in...
source/blender/blenkernel/intern/dynamicpaint.c:975:
sprintf(surface->image_output_path, "%sdynamicpaint/", "/tmp/");
Rather than /tmp/, this could use BLI_temporary_dir(), since /tmp/ does not
exist on windows.

http://codereview.appspot.com/5189041/diff/34001/source/blender/blenkernel/in...
source/blender/blenkernel/intern/dynamicpaint.c:1289: #ifdef _OPENMP
Such #ifdefs aren't needed around #pragma omp, it will simply get ignored.

http://codereview.appspot.com/5189041/diff/34001/source/blender/blenkernel/in...
source/blender/blenkernel/intern/dynamicpaint.c:1602: col[i*4+j].r = (unsigned
char)(material->b*255);
For this conversion here and further in the function, use the FTOCHAR() /
F3TOCHAR3() macro, which does proper clamping and rounding.

http://codereview.appspot.com/5189041/diff/34001/source/blender/blenkernel/in...
source/blender/blenkernel/intern/dynamicpaint.c:1724: /* check if this vertex
has a weight */
defvert_find_index and defvert_verify_index functions could be used here.

[brechtvl, 2011-11-01 19:22:55]

Latest changes look good, will take a closer look and do some more testing
tomorrow, but I guess this is getting close to being ready for merge.