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

[brechtvanlommel at gmail.com]

More comments.


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

http://codereview.appspot.com/5189041/diff/34001/source/blender/blenkernel/BKE_dynamicpaint.h#newcode17
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/BKE_dynamicpaint.h#newcode41
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/intern/dynamicpaint.c
File source/blender/blenkernel/intern/dynamicpaint.c (right):

http://codereview.appspot.com/5189041/diff/34001/source/blender/blenkernel/intern/dynamicpaint.c#newcode2754
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_math_geom.h
File source/blender/blenlib/BLI_math_geom.h (right):

http://codereview.appspot.com/5189041/diff/34001/source/blender/blenlib/BLI_math_geom.h#newcode63
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/physics/dynamicpaint_ops.c
File source/blender/editors/physics/dynamicpaint_ops.c (right):

http://codereview.appspot.com/5189041/diff/34001/source/blender/editors/physics/dynamicpaint_ops.c#newcode74
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/physics/dynamicpaint_ops.c#newcode207
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/physics/dynamicpaint_ops.c#newcode278
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/physics/dynamicpaint_ops.c#newcode314
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/physics/dynamicpaint_ops.c#newcode320
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/physics/dynamicpaint_ops.c#newcode405
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/physics/dynamicpaint_ops.c#newcode443
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_view3d/drawobject.c
File source/blender/editors/space_view3d/drawobject.c (right):

http://codereview.appspot.com/5189041/diff/34001/source/blender/editors/space_view3d/drawobject.c#newcode2698
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/intern/rna_dynamicpaint.c
File source/blender/makesrna/intern/rna_dynamicpaint.c (right):

http://codereview.appspot.com/5189041/diff/34001/source/blender/makesrna/intern/rna_dynamicpaint.c#newcode319
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/intern/rna_dynamicpaint.c#newcode422
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/intern/rna_dynamicpaint.c#newcode441
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/intern/rna_dynamicpaint.c#newcode595
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/intern/rna_dynamicpaint.c#newcode612
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/intern/rna_dynamicpaint.c#newcode688
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/intern/rna_dynamicpaint.c#newcode691
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/intern/rna_dynamicpaint.c#newcode710
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/intern/rna_dynamicpaint.c#newcode812
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/intern/rna_dynamicpaint.c#newcode826
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/intern/rna_dynamicpaint.c#newcode829
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/intern/rna_dynamicpaint.c#newcode839
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/intern/rna_dynamicpaint.c#newcode847
source/blender/makesrna/intern/rna_dynamicpaint.c:847: prop=
RNA_def_property(srna, "use_part_radius", PROP_BOOLEAN, PROP_NONE);
part => use_particle_radius

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