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

[brechtvanlommel at gmail.com]

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


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

http://codereview.appspot.com/5189041/diff/7001/release/scripts/startup/bl_ui/properties_physics_dynamicpaint.py#newcode49
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/properties_physics_dynamicpaint.py#newcode143
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/properties_physics_dynamicpaint.py#newcode147
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/intern/dynamicpaint.c
File source/blender/blenkernel/intern/dynamicpaint.c (right):

http://codereview.appspot.com/5189041/diff/7001/source/blender/blenkernel/intern/dynamicpaint.c#newcode33
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/intern/dynamicpaint.c#newcode74
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/BLI_kdtree.c
File source/blender/blenlib/intern/BLI_kdtree.c (right):

http://codereview.appspot.com/5189041/diff/7001/source/blender/blenlib/intern/BLI_kdtree.c#newcode469
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/physics/dynamicpaint_ops.c
File source/blender/editors/physics/dynamicpaint_ops.c (right):

http://codereview.appspot.com/5189041/diff/7001/source/blender/editors/physics/dynamicpaint_ops.c#newcode405
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/physics/dynamicpaint_ops.c#newcode428
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/intern/MOD_dynamicpaint.c
File source/blender/modifiers/intern/MOD_dynamicpaint.c (right):

http://codereview.appspot.com/5189041/diff/7001/source/blender/modifiers/intern/MOD_dynamicpaint.c#newcode15
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/intern/MOD_dynamicpaint.c#newcode61
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/intern/MOD_dynamicpaint.c#newcode88
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/intern/MOD_dynamicpaint.c#newcode126
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/nodes/node_shader_geom.c
File source/blender/nodes/shader/nodes/node_shader_geom.c (right):

http://codereview.appspot.com/5189041/diff/7001/source/blender/nodes/shader/nodes/node_shader_geom.c#newcode50
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/include/texture.h
File source/blender/render/intern/include/texture.h (right):

http://codereview.appspot.com/5189041/diff/7001/source/blender/render/intern/include/texture.h#newcode71
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/source/shadeinput.c
File source/blender/render/intern/source/shadeinput.c (right):

http://codereview.appspot.com/5189041/diff/7001/source/blender/render/intern/source/shadeinput.c#newcode1117
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/source/shadeoutput.c
File source/blender/render/intern/source/shadeoutput.c (right):

http://codereview.appspot.com/5189041/diff/7001/source/blender/render/intern/source/shadeoutput.c#newcode877
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.

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