[Bf-codereview] Dyntopo (issue 6947064)

[sergey.vfx at gmail.com]

Did initial review.

In general seems to be fine, but would need to compile patch first and
see how it works (mainly not sure from code if CD layers like UV will be
preserved by BMLog).

Do have note about code style -- we're not actually using style where
function arguments are split onto different lines, one per line. See
http://wiki.blender.org/index.php/Dev:Doc/CodeStyle#Indentation

Also seems pbvh changes weren't fully ported onto new trunk -- functions
are still using BLI_ prefix, however functions are in blenkernel
already.

Also, why range tree is intern? Unless it's a library you're gonna to
maintain, think extern/ is better place?

Will report back when will compile patch, do have some places not sure
about atm. Stay tuned :)


https://codereview.appspot.com/6947064/diff/1/release/scripts/startup/bl_ui/space_view3d_toolbar.py
File release/scripts/startup/bl_ui/space_view3d_toolbar.py (right):

https://codereview.appspot.com/6947064/diff/1/release/scripts/startup/bl_ui/space_view3d_toolbar.py#newcode884
release/scripts/startup/bl_ui/space_view3d_toolbar.py:884: col.active =
context.sculpt_object.use_dynamic_topology_sculpting
Pedantic: active flag is usually set before filling layout with a
properties.

https://codereview.appspot.com/6947064/diff/1/source/blender/blenkernel/intern/pbvh.c
File source/blender/blenkernel/intern/pbvh.c (right):

https://codereview.appspot.com/6947064/diff/1/source/blender/blenkernel/intern/pbvh.c#newcode1531
source/blender/blenkernel/intern/pbvh.c:1531: void
BLI_pbvh_node_draw(PBVHNode *node, void *data_v)
This is actually should be BKE_. Some other functions are also seems not
be using proper prefix after recent changes in trunk.

https://codereview.appspot.com/6947064/diff/1/source/blender/blenkernel/intern/pbvh.c#newcode1674
source/blender/blenkernel/intern/pbvh.c:1674: node->layer_disp =
MEM_callocN(sizeof(float) * totvert, "layer disp");
Not sure yet if this name conflicts with do_layer_brush. If so, better
to use more unique name here.

https://codereview.appspot.com/6947064/diff/1/source/blender/blenkernel/intern/pbvh_intern.h
File source/blender/blenkernel/intern/pbvh_intern.h (right):

https://codereview.appspot.com/6947064/diff/1/source/blender/blenkernel/intern/pbvh_intern.h#newcode27
source/blender/blenkernel/intern/pbvh_intern.h:27: } BB;
Since it became more or less public, i would call structures in more
verbose manner and use more verbose prefix for functions. To avoid
possible shadow declarations or other possible confusion in the future.

https://codereview.appspot.com/6947064/diff/1/source/blender/blenlib/BLI_math_geom.h
File source/blender/blenlib/BLI_math_geom.h (right):

https://codereview.appspot.com/6947064/diff/1/source/blender/blenlib/BLI_math_geom.h#newcode76
source/blender/blenlib/BLI_math_geom.h:76: void closest_to_tri_v3(float
r[3], const float p[3], const float t1[3], const float t2[3], const
float t3[3]);
Don't think name actually corresponds what this functions is doing: name
says it searches point closest to triangle, not point on triangle.
Better naming could be:
- closest_on_tri_v3
- closest_on_tri_to_point_v3

https://codereview.appspot.com/6947064/diff/1/source/blender/blenlib/intern/buffer.c
File source/blender/blenlib/intern/buffer.c (right):

https://codereview.appspot.com/6947064/diff/1/source/blender/blenlib/intern/buffer.c#newcode53
source/blender/blenlib/intern/buffer.c:53: void
BLI_buffer_free(BLI_Buffer *buffer)
Should this function reset allocated/used counters?

https://codereview.appspot.com/6947064/diff/1/source/blender/bmesh/intern/bmesh_log.c
File source/blender/bmesh/intern/bmesh_log.c (right):

https://codereview.appspot.com/6947064/diff/1/source/blender/bmesh/intern/bmesh_log.c#newcode131
source/blender/bmesh/intern/bmesh_log.c:131:
assert(BLI_ghash_haskey(log->id_to_elem, key));
Would use BLI_assert, so it'll be possible to avoid aborts even in debug
builds.

https://codereview.appspot.com/6947064/diff/1/source/blender/editors/sculpt_paint/sculpt.c
File source/blender/editors/sculpt_paint/sculpt.c (right):

https://codereview.appspot.com/6947064/diff/1/source/blender/editors/sculpt_paint/sculpt.c#newcode4056
source/blender/editors/sculpt_paint/sculpt.c:4056: origco = (void*)0x1;
Hrm, sounds a bit nasty =\

https://codereview.appspot.com/6947064/diff/1/source/blender/editors/sculpt_paint/sculpt.c#newcode4665
source/blender/editors/sculpt_paint/sculpt.c:4665: static void
SCULPT_OT_optimize(wmOperatorType *ot)
Are there plans of avoiding this operator? :)

https://codereview.appspot.com/6947064/diff/1/source/blender/makesrna/intern/rna_sculpt_paint.c
File source/blender/makesrna/intern/rna_sculpt_paint.c (right):

https://codereview.appspot.com/6947064/diff/1/source/blender/makesrna/intern/rna_sculpt_paint.c#newcode44
source/blender/makesrna/intern/rna_sculpt_paint.c:44: #include "bmesh.h"
Not sure why bmesh.h is needed here?

https://codereview.appspot.com/6947064/