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

[miikah88 at gmail.com]

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/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)
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/properties_physics_dynamicpaint.py#newcode147
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/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	*/
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/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)
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/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	*/
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/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#newcode61
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/intern/MOD_dynamicpaint.c#newcode88
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/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"},
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/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);
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/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 */
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)

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