Remesh modifier (dual contouring)

This patch adds a new remeshing modifier. The algorithm is based on the paper "Dual Contouring of Hermite Data", and the implementation was contributed to Blender by Dr. Tao Ju. The contributed code is in intern/dualcon, and was modified to compile under gcc and work on 64-bit systems. A small C wrapper was also added in order to interface it with Blender. Other than that, the rest of the patch is just standard modifier stuff.

Documentation and examples on the Blender wiki:
http://wiki.blender.org/index.php/User:Nicholasbishop/RemeshModifier

[nicholasbishop, 2011-12-15 18:27:46]

Hmm, trying again, this time with the intern/dualcon directory included :)

[sergey.vfx, 2011-12-16 14:58:03]

Hi,

Made basic review (almost general design and code, not logic itself). There are
some small notes written in comments.

Most confusing for me were that face/edges/etc maps declarations in header
files. They're using the same variable name and not sure why there's no linking
errors :)

http://codereview.appspot.com/5491053/diff/2001/intern/dualcon/dualcon.h
File intern/dualcon/dualcon.h (right):

http://codereview.appspot.com/5491053/diff/2001/intern/dualcon/dualcon.h#newc...
intern/dualcon/dualcon.h:1: #ifndef DUALCON_H
Picky and simple todo: think having standardt gpl header with
copytight/contributors is nice to have.

http://codereview.appspot.com/5491053/diff/2001/intern/dualcon/intern/Integer...
File intern/dualcon/intern/IntegerGrid.h (right):

http://codereview.appspot.com/5491053/diff/2001/intern/dualcon/intern/Integer...
intern/dualcon/intern/IntegerGrid.h:12: const int vertmap[8][3] =
{{0,0,0},{0,0,1},{0,1,0},{0,1,1},{1,0,0},{1,0,1},{1,1,0},{1,1,1}} ;
Not sure it's nice to have such kind of constants defined in header. This might
case issues if file is included from several places - most probably you'll have
linking error. Can see two ways:
- Make them static
- Move them inside Projections class (not currently sure if they're using
outside of this class). Maybe making them static also will help in this way.

http://codereview.appspot.com/5491053/diff/2001/intern/dualcon/intern/Integer...
intern/dualcon/intern/IntegerGrid.h:166: /*
Picky thing: commonly we don't store commented out code in svn. But if it makes
sense for debugging or so, then it's of course can be stored :)

http://codereview.appspot.com/5491053/diff/2001/intern/dualcon/intern/Integer...
intern/dualcon/intern/IntegerGrid.h:735: void crossProduct ( LONG a[3], LONG
b[3], LONG res[3] )
The same method can be seen in Projections class. Maybe it can be de-duplicated?

http://codereview.appspot.com/5491053/diff/2001/intern/dualcon/intern/MemoryA...
File intern/dualcon/intern/MemoryAllocator.h (right):

http://codereview.appspot.com/5491053/diff/2001/intern/dualcon/intern/MemoryA...
intern/dualcon/intern/MemoryAllocator.h:72: data = ( UCHAR ** )realloc( data,
sizeof ( UCHAR * ) * datablocknum ) ;
Picky: dualcon is in intern/ and in theory it might use guarded allocator. Don't
have strong opinion here, but maybe it might help debugging memory leaks and so?

http://codereview.appspot.com/5491053/diff/2001/intern/dualcon/intern/Project...
File intern/dualcon/intern/Projections.h (right):

http://codereview.appspot.com/5491053/diff/2001/intern/dualcon/intern/Project...
intern/dualcon/intern/Projections.h:21: const int vertmap[8][3] =
{{0,0,0},{0,0,1},{0,1,0},{0,1,1},{1,0,0},{1,0,1},{1,1,0},{1,1,1}} ;
Hmm.. i've seen very familiar declarations in IntegerGrid.h. Can't they run into
conflict?

Also some methods like crossProduct form classes declared here seems to be
totally the same. Maybe some utility module might be created?

http://codereview.appspot.com/5491053/diff/2001/intern/dualcon/intern/eigen.cpp
File intern/dualcon/intern/eigen.cpp (right):

http://codereview.appspot.com/5491053/diff/2001/intern/dualcon/intern/eigen.c...
intern/dualcon/intern/eigen.cpp:1: #include "eigen.h"
Name of file confuses a bit. Does it have something familiar with Eigen library?
And if so maybe it might be used?

http://codereview.appspot.com/5491053/diff/2001/release/scripts/startup/bl_ui...
File release/scripts/startup/bl_ui/properties_data_modifier.py (right):

http://codereview.appspot.com/5491053/diff/2001/release/scripts/startup/bl_ui...
release/scripts/startup/bl_ui/properties_data_modifier.py:821: row =
layout.row()
UI block's active flag is commonly setting before placing properties. Same few
lined below.

http://codereview.appspot.com/5491053/diff/2001/source/blender/makesdna/DNA_m...
File source/blender/makesdna/DNA_modifier_types.h (right):

http://codereview.appspot.com/5491053/diff/2001/source/blender/makesdna/DNA_m...
source/blender/makesdna/DNA_modifier_types.h:1042: typedef enum
RemeshModifierMode {
Maybe it might worth adding values here? So modes might be added/removed in safe
way?

http://codereview.appspot.com/5491053/diff/2001/source/blender/makesrna/inter...
File source/blender/makesrna/intern/rna_modifier.c (right):

http://codereview.appspot.com/5491053/diff/2001/source/blender/makesrna/inter...
source/blender/makesrna/intern/rna_modifier.c:72: {eModifierType_Remesh,
"REMESH", 0 /* TODO */, "Remesh", ""},
Think we might ask Ton (or artist directly) for icons.

[nicholasbishop, 2011-12-18 00:12:25]

Updated with fixes from first review

[nicholasbishop, 2011-12-18 00:25:05]

Thanks for reviewing :)

Uploaded a new patch with some of the comments addressed. Also now includes
MOD_remesh.c, which I accidentally left out before.

Some notes:
You're right about the duplicate data in Projections.h and IntegerGrid.h; turns
out IntegerGrid.h isn't used anywhere in this code, so removed it.

Changed the constant arrays defined in Projections.h to extern, and moved their
initialization into a new Projection.cpp file.

Trimmed down the memory allocator code a bit, only two classes there are
actually used. Didn't change the alloc/free calls to guardedalloc, but it's a
smaller change now if we prefer to do that.

As suggested, gave RemeshModifierMode explicit values and swapped the
'row.active' order in the UI file.

Regarding GPL headers and the eigen files: I added GPL headers to the dualcon.h
and dualcon_c_api.cpp files with myself as contributor; we should also be safe
adding GPL header with Tao Ju as contributor for most of the dualcon intern
code. However, the eigen files have a different author, and we may not have
permission to distribute it. (Went back to old emails and found this concern was
discussed then.) I'll look into replacing this code with existing matrix code
(possibly using the Eigen3 lib.)

[nicholasbishop, 2011-12-19 08:14:51]

Replace eigen.h and eigen.cpp

[nicholasbishop, 2011-12-19 08:16:24]

On 2011/12/19 08:14:51, nicholasbishop wrote:
> Replace eigen.h and eigen.cpp

Uploaded new patch that uses the Eigen3 lib for some matrix calculations rather
than the eigen.h and eigen.cpp files.

[nicholasbishop, 2011-12-19 16:04:30]

Compilation fix and GPL headers

[sergey.vfx, 2011-12-20 15:00:02]

Hi,

Just two minor tihngs noted incomments. And one thing i'm not sure about atm:
it's related on usage of int64_t. Depending on order of includes, it might give
issues when building with msvc. Will compile this patch tomorrow to doublecheck,
so you might not worry about thi atm.

http://codereview.appspot.com/5491053/diff/19/intern/dualcon/CMakeLists.txt
File intern/dualcon/CMakeLists.txt (right):

http://codereview.appspot.com/5491053/diff/19/intern/dualcon/CMakeLists.txt#n...
intern/dualcon/CMakeLists.txt:1: # $Id$
Picky: we decided not to use $$Id keyword anymore

http://codereview.appspot.com/5491053/diff/19/intern/dualcon/intern/octree.h
File intern/dualcon/intern/octree.h (right):

http://codereview.appspot.com/5491053/diff/19/intern/dualcon/intern/octree.h#...
intern/dualcon/intern/octree.h:101: const int edgevmap[12][2] =
{{0,4},{1,5},{2,6},{3,7},{0,2},{1,3},{4,6},{5,7},{0,1},{2,3},{4,5},{6,7}};
Still onfuses a bit with constants from Pojections.cpp

[nicholasbishop, 2011-12-21 19:36:23]

Remove $Id$ and clean up more static arrays

[nicholasbishop, 2011-12-21 19:39:29]

>
http://codereview.appspot.com/5491053/diff/19/intern/dualcon/intern/octree.h#...
> intern/dualcon/intern/octree.h:101: const int edgevmap[12][2] =
> {{0,4},{1,5},{2,6},{3,7},{0,2},{1,3},{4,6},{5,7},{0,1},{2,3},{4,5},{6,7}};
> Still onfuses a bit with constants from Pojections.cpp

Cleaned this up a bit. As with the projections arrays, made these arrays extern
and moved definitions into octree.cpp. Two of them (vert and edge maps) had
identical data to the ones in Projections.h/cpp, so removed them. For some
reason, the face map array has quite different data. I don't know enough about
the internal workings of the algorithm to explain this difference, so just left
that array the same for now.

[sergey.vfx, 2011-12-21 20:14:53]

Looks good to me.

[nicholasbishop, 2011-12-21 21:18:09]

Updated with build fixes from Sergey