[Bf-codereview] Remesh modifier (dual contouring) (issue 5491053)

[sergey.vfx at gmail.com]

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#newcode1
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/IntegerGrid.h
File intern/dualcon/intern/IntegerGrid.h (right):

http://codereview.appspot.com/5491053/diff/2001/intern/dualcon/intern/IntegerGrid.h#newcode12
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/IntegerGrid.h#newcode166
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/IntegerGrid.h#newcode735
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/MemoryAllocator.h
File intern/dualcon/intern/MemoryAllocator.h (right):

http://codereview.appspot.com/5491053/diff/2001/intern/dualcon/intern/MemoryAllocator.h#newcode72
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/Projections.h
File intern/dualcon/intern/Projections.h (right):

http://codereview.appspot.com/5491053/diff/2001/intern/dualcon/intern/Projections.h#newcode21
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.cpp#newcode1
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/properties_data_modifier.py
File release/scripts/startup/bl_ui/properties_data_modifier.py (right):

http://codereview.appspot.com/5491053/diff/2001/release/scripts/startup/bl_ui/properties_data_modifier.py#newcode821
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_modifier_types.h
File source/blender/makesdna/DNA_modifier_types.h (right):

http://codereview.appspot.com/5491053/diff/2001/source/blender/makesdna/DNA_modifier_types.h#newcode1042
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/intern/rna_modifier.c
File source/blender/makesrna/intern/rna_modifier.c (right):

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

http://codereview.appspot.com/5491053/