Freestyle r54826 branch review

[ideasman42, 2013-03-02 10:56:47]

First pass review of freestyle branch review.

Summery

* Having C++ new/delete manage blender library data isn't great practice though
I dont yet have the big-picture on freestyle integration in my head yet.

* Would prefer mesh flags be named generically rather then adding freestyle
flags to each edge/faces.

* Looks like python-api is missing a lot of type checking, maybe memory leaks?

https://codereview.appspot.com/7416049/diff/1/release/scripts/startup/bl_oper...
File release/scripts/startup/bl_operators/freestyle.py (right):

https://codereview.appspot.com/7416049/diff/1/release/scripts/startup/bl_oper...
release/scripts/startup/bl_operators/freestyle.py:62: min_dist = float('inf')
suggest to use sys.float_info.min/max

https://codereview.appspot.com/7416049/diff/1/release/scripts/startup/bl_oper...
release/scripts/startup/bl_operators/freestyle.py:76: '''Add the data paths to
the Freestyle Edge Mark property of selected edges to the active keying set'''
Im really not sure we should be encouraging users to have fcurves with
vert/edge/face data in it. While its possible its not very good practice.

https://codereview.appspot.com/7416049/diff/1/release/scripts/startup/bl_oper...
release/scripts/startup/bl_operators/freestyle.py:107: '''Add the data paths to
the Freestyle Face Mark property of selected polygons to the active keying
set'''
same as above...

https://codereview.appspot.com/7416049/diff/1/source/blender/bmesh/bmesh_class.h
File source/blender/bmesh/bmesh_class.h (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/bmesh/bmesh_clas...
source/blender/bmesh/bmesh_class.h:250: BM_ELEM_FREESTYLE = (1 << 6), /* used
for Freestyle faces and edges */
I've seen this referenced in the code, would prefer this be more generic named
so it can be reused in more places, how is this used for egded/faces?

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
File source/blender/freestyle/intern/application/AppConfig.cpp (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
source/blender/freestyle/intern/application/AppConfig.cpp:68: _BrowserCmd =
"C:\\Program Files\\Internet Explorer\\iexplore.exe %s";
Why does freestyle need a web browser?

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
File source/blender/freestyle/intern/application/AppView.h (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
source/blender/freestyle/intern/application/AppView.h:38: #  define __max(x,y)
(max(x,y))
are these really needed? We have C++ code which uses min/max in other parts of
blender.

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
File source/blender/freestyle/intern/blender_interface/BlenderStrokeRenderer.cpp
(right):

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
source/blender/freestyle/intern/blender_interface/BlenderStrokeRenderer.cpp:85:
freestyle_scene = BKE_scene_add(G.main, name);
Am not so happy with duplicating a scene, then removing it and all objects on
free (below), would like to know more about why this is needed.

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
source/blender/freestyle/intern/blender_interface/BlenderStrokeRenderer.cpp:179:
strcpy(name, ob->id.name);
*Bug* - 24 is nolonger bit enough - better use sizeof(ob->id.name)

Also looks like copying the name isnt even needed?

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
File source/blender/freestyle/intern/blender_interface/BlenderStyleModule.h
(right):

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
source/blender/freestyle/intern/blender_interface/BlenderStyleModule.h:54:
BKE_text_unlink(G.main, _text);
not really happy about C++ classes destructor managing blend file data, seems
like asking for troubles.

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
File source/blender/freestyle/intern/blender_interface/FRS_freestyle.cpp
(right):

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
source/blender/freestyle/intern/blender_interface/FRS_freestyle.cpp:197: for
(int i = 0; i < 4; i++) {
copy_m4m4 would do here.

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
File source/blender/freestyle/intern/python/BPy_Convert.cpp (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
source/blender/freestyle/intern/python/BPy_Convert.cpp:624: float x =
PyFloat_AsDouble(PyList_GetItem(obj, 0));
If one of the items isnt a number, looks like the exception won't be handled.
Goes for other uses of PyFloat_AsDouble here.

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
File source/blender/freestyle/intern/python/Director.cpp (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
source/blender/freestyle/intern/python/Director.cpp:240: if
(BPy_UnaryFunction0DDouble_Check(obj)) {
Is the type of 'result' checked here? looks like for PyFloat_AsDouble not.

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
File source/blender/freestyle/intern/python/UnaryPredicate1D/BPy_ShapeUP1D.cpp
(right):

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
source/blender/freestyle/intern/python/UnaryPredicate1D/BPy_ShapeUP1D.cpp:72:
self->py_up1D.up1D = new Predicates1D::ShapeUP1D(u1, u2);
Is this freed? - looks like it could be a memory leak.

Note: I didnt check all py classes for this, but if this is a leak. all Py
classes should be checked.

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

https://codereview.appspot.com/7416049/diff/1/source/blender/makesrna/intern/...
source/blender/makesrna/intern/rna_scene.c:2504: prop = RNA_def_property(srna,
"use", PROP_BOOLEAN, PROP_NONE);
*picky* - should be "show_render"

[kjym3, 2013-03-03 01:58:03]

Many thanks for the thorough code review.
All review comments were carefully addressed and replies have been attached to
each comment on a point-by-point basis.
All code changes based on the review comments are available in the latest
revision 54984.
Any further code review is duly acknowledged.

With best regards,
Tamito

https://codereview.appspot.com/7416049/diff/1/release/scripts/startup/bl_oper...
File release/scripts/startup/bl_operators/freestyle.py (right):

https://codereview.appspot.com/7416049/diff/1/release/scripts/startup/bl_oper...
release/scripts/startup/bl_operators/freestyle.py:62: min_dist = float('inf')
On 2013/03/02 10:56:47, ideasman42 wrote:
> suggest to use sys.float_info.min/max

Done.

https://codereview.appspot.com/7416049/diff/1/release/scripts/startup/bl_oper...
release/scripts/startup/bl_operators/freestyle.py:76: '''Add the data paths to
the Freestyle Edge Mark property of selected edges to the active keying set'''
On 2013/03/02 10:56:47, ideasman42 wrote:
> Im really not sure we should be encouraging users to have fcurves with
> vert/edge/face data in it. While its possible its not very good practice.

Edge marks are used by users to directly control the detection of feature edges
upon which stylized strokes are drawn.  Basically, all marked edges will be
detected as feature edges.
This operator is intended to allow for keyframing edge marks.  In animation this
operator is handy since users can fine control the detection of feature edges on
a per-frame basis.
This operator was introduced based on a feature request from branch users, so I
assume there is at least one use case.

https://codereview.appspot.com/7416049/diff/1/release/scripts/startup/bl_oper...
release/scripts/startup/bl_operators/freestyle.py:107: '''Add the data paths to
the Freestyle Face Mark property of selected polygons to the active keying
set'''
On 2013/03/02 10:56:47, ideasman42 wrote:
> same as above...

The same rationale with the edge mark operator applies to this one for
keyframing face marks.

https://codereview.appspot.com/7416049/diff/1/source/blender/bmesh/bmesh_class.h
File source/blender/bmesh/bmesh_class.h (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/bmesh/bmesh_clas...
source/blender/bmesh/bmesh_class.h:250: BM_ELEM_FREESTYLE = (1 << 6), /* used
for Freestyle faces and edges */
On 2013/03/02 10:56:47, ideasman42 wrote:
> I've seen this referenced in the code, would prefer this be more generic named
> so it can be reused in more places, how is this used for egded/faces?

This symbol is used for marking edges and faces to allow users to fine control
the detection of feature edges upon which stylized strokes are drawn.  This
marker is used in the same way with seam and sharp marks.  The symbols for these
markers have specific objectives in the code and likely to appear quite
confusing if used for other purposes.  By the same token, BM_ELEM_FREESTYLE
should be specifically used for tuning Freestyle stroke rendering.

I think it is okay to rename this symbol to a more general name, but it is a bit
difficult for me to imagine any other generalized use of the symbol.

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
File source/blender/freestyle/intern/application/AppConfig.cpp (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
source/blender/freestyle/intern/application/AppConfig.cpp:68: _BrowserCmd =
"C:\\Program Files\\Internet Explorer\\iexplore.exe %s";
On 2013/03/02 10:56:47, ideasman42 wrote:
> Why does freestyle need a web browser?

Not really necessary any more.  This and the help index in the next lines are a
leftover since Freestyle was a standalone program.  I simply removed them.

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
File source/blender/freestyle/intern/application/AppView.h (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
source/blender/freestyle/intern/application/AppView.h:38: #  define __max(x,y)
(max(x,y))
On 2013/03/02 10:56:47, ideasman42 wrote:
> are these really needed? We have C++ code which uses min/max in other parts of
> blender.

Removed redundant definitions of __min and __max macros by replacing them with
std::min() and std::max(), respectively.

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
File source/blender/freestyle/intern/blender_interface/BlenderStrokeRenderer.cpp
(right):

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
source/blender/freestyle/intern/blender_interface/BlenderStrokeRenderer.cpp:85:
freestyle_scene = BKE_scene_add(G.main, name);
On 2013/03/02 10:56:47, ideasman42 wrote:
> Am not so happy with duplicating a scene, then removing it and all objects on
> free (below), would like to know more about why this is needed.

Freestyle draws stylized strokes on the basis of feature edges detected from a
given 3D scene.  In most cases feature edges are part of mesh edges present in
the 3D scene.

Stylized strokes are created through geometry manipulation (e.g., concatenation
and splitting of feature edges) and style manipulation (e.g., specification of
stroke color, alpha transparency and stroke thickness).  Strokes may have
variable color and thickness along its extent.

The strokes need to be rendered in some way, and in the present Freestyle
implementation the rendering is done by the Blender Internal (BI) renderer. 
Since the stroke geometry data are not available in the original 3D scene, a new
temporary 3D scene populated with meshes representing the stylized strokes is
built on the fly.  It is noted that this temporary scene is not a duplication of
the original 3D scene.  The two scenes are completely different in terms of mesh
data populating the individual scenes.  It is also remarked that stroke geometry
data are very unlikely to be static in animation rendering, so there is no point
to keep the temporary scene data for rendering another frame.

Obviously, another implementation option is to rely on an external 2D graphics
rendering engine (e.g., Cairo and Anti-Grain Geometry), but at the cost of
maintaining (possibly many) external dependencies.  For now the renderer of
choice is the BI and this implementation has been quite well tested for years. 
I am open to other implementation options, and any development direction is a
matter of discussion.

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
source/blender/freestyle/intern/blender_interface/BlenderStrokeRenderer.cpp:179:
strcpy(name, ob->id.name);
On 2013/03/02 10:56:47, ideasman42 wrote:
> *Bug* - 24 is nolonger bit enough - better use sizeof(ob->id.name)
> 
> Also looks like copying the name isnt even needed?

Done.

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
File source/blender/freestyle/intern/blender_interface/BlenderStyleModule.h
(right):

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
source/blender/freestyle/intern/blender_interface/BlenderStyleModule.h:54:
BKE_text_unlink(G.main, _text);
On 2013/03/02 10:56:47, ideasman42 wrote:
> not really happy about C++ classes destructor managing blend file data, seems
> like asking for troubles. 

Moved from C++ class destructor to a specific method for releasing resources.

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
File source/blender/freestyle/intern/blender_interface/FRS_freestyle.cpp
(right):

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
source/blender/freestyle/intern/blender_interface/FRS_freestyle.cpp:197: for
(int i = 0; i < 4; i++) {
On 2013/03/02 10:56:47, ideasman42 wrote:
> copy_m4m4 would do here.

Done.  Also used unit_m4() for setting freestyle_mv to unit matrix.

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
File source/blender/freestyle/intern/python/BPy_Convert.cpp (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
source/blender/freestyle/intern/python/BPy_Convert.cpp:624: float x =
PyFloat_AsDouble(PyList_GetItem(obj, 0));
On 2013/03/02 10:56:47, ideasman42 wrote:
> If one of the items isnt a number, looks like the exception won't be handled.
> Goes for other uses of PyFloat_AsDouble here.

Added a proper check of each item and made sure the exception will be forwarded
to the caller.

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
File source/blender/freestyle/intern/python/Director.cpp (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
source/blender/freestyle/intern/python/Director.cpp:240: if
(BPy_UnaryFunction0DDouble_Check(obj)) {
On 2013/03/02 10:56:47, ideasman42 wrote:
> Is the type of 'result' checked here? looks like for PyFloat_AsDouble not.

Yes, it is guaranteed that the 'result' here is a Python float object.  When the
'obj' is a BPy_UnaryFunction0DDouble instance, the 'result' value is returned
from UnaryFunction0DDouble___call__() (defined in
source/blender/freestyle/intern/python/UnaryFunction0D/BPy_UnaryFunction0DDouble.cpp).
 Since the returned Python object is created from a C++ double variable, there
is no risk of a type error here.  The same applies to the other cases of
UnaryFunction0D subclasses.

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
File source/blender/freestyle/intern/python/UnaryPredicate1D/BPy_ShapeUP1D.cpp
(right):

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
source/blender/freestyle/intern/python/UnaryPredicate1D/BPy_ShapeUP1D.cpp:72:
self->py_up1D.up1D = new Predicates1D::ShapeUP1D(u1, u2);
On 2013/03/02 10:56:47, ideasman42 wrote:
> Is this freed? - looks like it could be a memory leak.
> 
> Note: I didnt check all py classes for this, but if this is a leak. all Py
> classes should be checked.

Yes, self->py_up1D.up1D is freed in the destructor of the UnaryPredicate1D class
that is the base class of ShapeUP1D defined here.  Many other Python classes are
implemented in this way.

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

https://codereview.appspot.com/7416049/diff/1/source/blender/makesrna/intern/...
source/blender/makesrna/intern/rna_scene.c:2504: prop = RNA_def_property(srna,
"use", PROP_BOOLEAN, PROP_NONE);
On 2013/03/02 10:56:47, ideasman42 wrote:
> *picky* - should be "show_render"

Done.

[ideasman42, 2013-03-06 04:34:18]

Hi thanks for addressing most of these issues.

Freestyle code managing blender scene data (copying+freeing). I can accept as
being convenient even though this is something to be avoided where possible.

But there is one area of this branch I can't really accept - and this is the
addition of flags in edges/faces, only for freestyle's use.

I just think it sets a bad precedent that integrating new systems adds arbitrary
data to mesh elements - when we may want to use these later for more general
purposes.
Further - this is what mesh customdata was added for.

So I'd propose freestyle have its own customdata layers for edges/faces.
This could be an int layer which simply uses flags, or a struct which has
arbitrary data for freestyle.

In the long run this is probably better since edges could have their own
non-uniform thickness.
You could have edges blend in/out, currently from what I can see they either
draw or not - which I assume could flicker in some cases.

[kjym3, 2013-03-13 07:07:14]

Many thanks for the review comments and suggestions.

> So I'd propose freestyle have its own customdata layers for edges/faces.
> This could be an int layer which simply uses flags, or a struct which has
> arbitrary data for freestyle.

This proposal is much appreciated, and I made an attempt to implement
edge/face marks based on Mesh CustomData in the branch revision 55228.
Now Mesh, BMesh and DerivedMesh (CDDM) have CustomData layers of
type CD_FREESTYLE_EDGE/FACE for keeping Freestyle edge/face attributes.

Instead of having only a boolean variable for edge/face marks, the new
CustomData layers store a struct (FreestyleEdge and FreestyleFace) in view
of future additions of new edge/face attributes as you suggested.

Most references of old extra flags were removed from the code base.  The
only exceptions are those in source/blender/blenloader/intern/readfile.c and
source/blender/makesrna/intern/rna_mesh.c.  The former file has additional
backward compatibility code for converting old flag-based edge/marks to
the CustomData-based implementation, to allow branch users to migrate
existing .blend files to the new system.  All references of the extra flags can
be removed when the trunk merger is done (as indicated by code comments
in the form "TO BE REMOVED").

Any further review comments and suggestions are duly acknowledged.

[sergey.vfx, 2013-03-18 16:38:04]

First set of comments. Didn't check on source/blender/freestyle yet. Mainly:

- Either diff is made against wrong revision or some merge issues happened.
There're lots of non-freestyle changes which are actually reverting changes from
trunk. Please check on this.

- Not so much fan of lots ifdef's all over. Especially around data structures
which are file specification. I would say there shall be no file specification
(DNA) and RNA for it depending on build flag. Just always assume this data
structures are allowed. Otherwise code becomes much more cluttered and i could
foresee some further reports related on data loss when opening+saving file in
build without freestyle.

- Don't think adding new contest to buttons space is so much nice idea.

- Interface things and python operators shall be localized into own files.

- Exposing freestyle-specific flags to modifiers and bmesh are not making me
happy as well. Is it possible to generalize flags?

- It's not so much clear who's copyright holder of most of python files. Would
be nice to mention it in comment explicitly.

Will review rest of the files later.

https://codereview.appspot.com/7416049/diff/1/CMakeLists.txt.user
File CMakeLists.txt.user (right):

https://codereview.appspot.com/7416049/diff/1/CMakeLists.txt.user#newcode1
CMakeLists.txt.user:1: <?xml version="1.0" encoding="UTF-8"?>
Guess this file is added by accident? :)

https://codereview.appspot.com/7416049/diff/1/blender.kdev4
File blender.kdev4 (right):

https://codereview.appspot.com/7416049/diff/1/blender.kdev4#newcode1
blender.kdev4:1: [Project]
Guess this file is added by accident as well.

https://codereview.appspot.com/7416049/diff/1/build_files/build_environment/i...
File build_files/build_environment/install_deps.sh (left):

https://codereview.appspot.com/7416049/diff/1/build_files/build_environment/i...
build_files/build_environment/install_deps.sh:800: # Optional tests and cmd
tools
Unless this is a merge conflict which wasn't resolved 100% correct, why would
you need to touch this?

https://codereview.appspot.com/7416049/diff/1/release/scripts/freestyle/style...
File release/scripts/freestyle/style_modules/ChainingIterators.py (right):

https://codereview.appspot.com/7416049/diff/1/release/scripts/freestyle/style...
release/scripts/freestyle/style_modules/ChainingIterators.py:1: # ##### BEGIN
GPL LICENSE BLOCK #####
Pedantic (not stoppers for merge):
* Seems not pep-120 complaint
* Not sure you actually need py prefix for all the classes

https://codereview.appspot.com/7416049/diff/1/release/scripts/freestyle/style...
File release/scripts/freestyle/style_modules/PredicatesU1D.py (right):

https://codereview.appspot.com/7416049/diff/1/release/scripts/freestyle/style...
release/scripts/freestyle/style_modules/PredicatesU1D.py:24: count = 0
Really need to be a global? If so, is it surviving loading new files, doing
material previews, setting sequencer to rendered preview and so works fine with
this?

https://codereview.appspot.com/7416049/diff/1/release/scripts/modules/bl_i18n...
File release/scripts/modules/bl_i18n_utils/bl_extract_messages.py (left):

https://codereview.appspot.com/7416049/diff/1/release/scripts/modules/bl_i18n...
release/scripts/modules/bl_i18n_utils/bl_extract_messages.py:1: # ***** BEGIN
GPL LICENSE BLOCK *****
Guess this is merge resolved incorrect?

https://codereview.appspot.com/7416049/diff/1/release/scripts/modules/bpy/uti...
File release/scripts/modules/bpy/utils.py (left):

https://codereview.appspot.com/7416049/diff/1/release/scripts/modules/bpy/uti...
release/scripts/modules/bpy/utils.py:646: # Build an RNA path from
struct/property/enum names.
Is it also a merge issue?

https://codereview.appspot.com/7416049/diff/1/release/scripts/startup/bl_ui/p...
File release/scripts/startup/bl_ui/properties_render.py (right):

https://codereview.appspot.com/7416049/diff/1/release/scripts/startup/bl_ui/p...
release/scripts/startup/bl_ui/properties_render.py:311: class
RENDER_PT_freestyle(RenderFreestyleButtonsPanel, Panel):
If freestyle is considered to be optional, it doesn't make sense to have this
panel defined here.

https://codereview.appspot.com/7416049/diff/1/release/scripts/startup/bl_ui/p...
File release/scripts/startup/bl_ui/properties_render_layer.py (right):

https://codereview.appspot.com/7416049/diff/1/release/scripts/startup/bl_ui/p...
release/scripts/startup/bl_ui/properties_render_layer.py:230: class
RENDERLAYER_PT_freestyle(RenderLayerFreestyleButtonsPanel, Panel):
Same as above. And guess there could be more freestyle-specific panels defined
in general UI files. Better to move them all to own files.

https://codereview.appspot.com/7416049/diff/1/source/blender/CMakeLists.txt
File source/blender/CMakeLists.txt (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/CMakeLists.txt#n...
source/blender/CMakeLists.txt:143: list(APPEND SRC_DNA_INC
Not sure why do you need this. Even if freestule is optional, DNA shall never
depend on this.

https://codereview.appspot.com/7416049/diff/1/source/blender/blenfont/BLF_tra...
File source/blender/blenfont/BLF_translation.h (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/blenfont/BLF_tra...
source/blender/blenfont/BLF_translation.h:133: #ifdef WITH_FREESTYLE
Personally, would make I18N aware of enabled feature set. Just assume all the
features are here.

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/BKE_l...
File source/blender/blenkernel/BKE_linestyle.h (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/BKE_l...
source/blender/blenkernel/BKE_linestyle.h:36: #ifdef WITH_FREESTYLE
Personally wouldn't add extra ifdefs, this file is used by freestyle only
anyway.

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/BKE_l...
source/blender/blenkernel/BKE_linestyle.h:48: FreestyleLineStyle
*FRS_new_linestyle(const char *name, struct Main *main);
If it's blender kernel, prefix shall be BKE_. If other blenkernel files doesn't
use this header, perhaps it could be moved to ../freestyle.

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/BKE_m...
File source/blender/blenkernel/BKE_main.h (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/BKE_m...
source/blender/blenkernel/BKE_main.h:91: #ifdef WITH_FREESTYLE
Don't think it's good idea. All the datablocks are independent from feature
implementation. DNA, fileio shouldn't take care of whether some feature enabled
or not.

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/SCons...
File source/blender/blenkernel/SConscript (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/SCons...
source/blender/blenkernel/SConscript:151: if env['WITH_BF_FREESTYLE']:
Same as above. If linestyle is not used in other BKE files, easier to move it to
../freestyle

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/inter...
File source/blender/blenkernel/intern/anim_sys.c (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/inter...
source/blender/blenkernel/intern/anim_sys.c:89: #ifdef WITH_FREESTYLE
Data specification shall not depends on feature sets. Same goes to other changes
in this file.

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/inter...
File source/blender/blenkernel/intern/bpath.c (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/inter...
source/blender/blenkernel/intern/bpath.c:73: #ifdef WITH_FREESTYLE
Ok, same as above. And repeated for any further idef check around data
specification.

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/inter...
File source/blender/blenkernel/intern/linestyle.c (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/inter...
source/blender/blenkernel/intern/linestyle.c:178: if (m) {
Personally, i'm not so big fan of checking malloc/calloc result. It's not
reliable and lots of other places in blender will crash in case not enough
memory.

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/inter...
source/blender/blenkernel/intern/linestyle.c:232:
((LineStyleColorModifier_DistanceFromCamera *)m)->color_ramp = add_colorband(1);
Think it'll be more readable to assign temporary variable rather than doing
multiple casts.

Same goes to some other cases in this file.

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/inter...
source/blender/blenkernel/intern/linestyle.c:875: switch (m->type) {
What this is about?

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/inter...
source/blender/blenkernel/intern/linestyle.c:965: /* XXX Do we want to keep that
goto? Or use a boolean var? */
Well, issue is already mentioned.

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/inter...
File source/blender/blenkernel/intern/material.c (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/inter...
source/blender/blenkernel/intern/material.c:156: #ifdef WITH_FREESTYLE
Would prefer initialize all the properties anyway.

https://codereview.appspot.com/7416049/diff/1/source/blender/blenlib/CMakeLis...
File source/blender/blenlib/CMakeLists.txt (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/blenlib/CMakeLis...
source/blender/blenlib/CMakeLists.txt:168: if(WITH_FREESTYLE)
Shouldn't be needed. All freestyle-specific stuff is on higher level than
blenlib.

https://codereview.appspot.com/7416049/diff/1/source/blender/blenloader/CMake...
File source/blender/blenloader/CMakeLists.txt (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/blenloader/CMake...
source/blender/blenloader/CMakeLists.txt:69: if(WITH_FREESTYLE)
Don't think it's needed. Opening .blend with freestyle data in freestyle-less
build and saving the file shouldn't changing data. It shall be possible.

https://codereview.appspot.com/7416049/diff/1/source/blender/editors/animatio...
File source/blender/editors/animation/anim_channels_defines.c (left):

https://codereview.appspot.com/7416049/diff/1/source/blender/editors/animatio...
source/blender/editors/animation/anim_channels_defines.c:39: #include
"BLF_translation.h"
Seems to be some changes were revetred in this file.

https://codereview.appspot.com/7416049/diff/1/source/blender/editors/include/...
File source/blender/editors/include/UI_resources.h (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/editors/include/...
source/blender/editors/include/UI_resources.h:209: /* #ifdef WITH_FREESTYLE */
ifdef isn't needed here.

https://codereview.appspot.com/7416049/diff/1/source/blender/makesdna/CMakeLi...
File source/blender/makesdna/CMakeLists.txt (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/makesdna/CMakeLi...
source/blender/makesdna/CMakeLists.txt:26: if(WITH_FREESTYLE)
Shall not be checked in DNA

https://codereview.appspot.com/7416049/diff/1/source/blender/makesdna/DNA_fre...
File source/blender/makesdna/DNA_freestyle_types.h (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/makesdna/DNA_fre...
source/blender/makesdna/DNA_freestyle_types.h:93: char name[32]; /* line set
name */
Shall be 64 and commented MAX_NAME

https://codereview.appspot.com/7416049/diff/1/source/blender/makesdna/DNA_fre...
source/blender/makesdna/DNA_freestyle_types.h:110: char module_path[256];
Guess shall be 1024 which matches MAX_FILE?

https://codereview.appspot.com/7416049/diff/1/source/blender/windowmanager/in...
File source/blender/windowmanager/intern/wm_files.c (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/windowmanager/in...
source/blender/windowmanager/intern/wm_files.c:441: #ifdef WITH_FREESTYLE
This call looks a bit nasty from here.

https://codereview.appspot.com/7416049/diff/1/user-config.py
File user-config.py (right):

https://codereview.appspot.com/7416049/diff/1/user-config.py#newcode1
user-config.py:1: WITH_BF_3DMOUSE = False
This shall not be in svn

[ideasman42, 2013-03-19 00:41:57]

Noticed 2 points...
- When using a script line style it defaults to contour.py I get this error:
---
Traceback (most recent call last):
  File "/contour.py", line 29, in <module>
    Operators.select(AndUP1D(QuantitativeInvisibilityUP1D(0), ContourUP1D()))
NameError: name 'AndUP1D' is not defined
---


Currently the absolute filepath to the script is used:

/src/blender_freestyle/release/scripts/freestyle/style_modules/contour.py

Storing absolute paths to a file thats apart of the installation isnt good
practice.

Possibilities for module import:

- Select the name of the module only. "contour" which is imported from
freestyle/style_modules

- name the package as a string "freestyle.style_modules.contour", use a script
to popup a menu and list these module so you dont have to lookup the path
externally to find the module name.

[irieshinsuke, 2013-03-21 12:39:57]

Added comments about two things:

1. UI glitch in property panel of 3d view
2. Inconsistency between factory settings and default values

https://codereview.appspot.com/7416049/diff/1/release/scripts/startup/bl_ui/s...
File release/scripts/startup/bl_ui/space_view3d.py (right):

https://codereview.appspot.com/7416049/diff/1/release/scripts/startup/bl_ui/s...
release/scripts/startup/bl_ui/space_view3d.py:2508: if context.scene and
bpy.app.build_options.freestyle:
Horizontally split row is too narrow to display labels "Freestyle Edge Marks"
and "Freestyle Face Marks", so both of them are truncated to "Freestyle".

I think "col = layout.column()" should be inserted here to avoid this glitch.

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
File source/blender/freestyle/intern/blender_interface/FRS_freestyle.cpp
(right):

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
source/blender/freestyle/intern/blender_interface/FRS_freestyle.cpp:649: {
Why these default values are different from the factory settings
(mode=FREESTYLE_CONTROL_EDITOR_MODE, crease_angle=134.43, etc.)?

[kjym3, 2013-03-23 22:46:07]

Dear Reviewers,

Thank you all for the careful code review.  Please, find my
replies attached to individual review comments on a
point-by-point basis.  Most suggestions have already been
addressed in recent commits in the branch.  Any further code
review is duly acknowledged.


@Sergey,

> - Either diff is made against wrong revision or some merge
> issues happened.  There're lots of non-freestyle changes which
> are actually reverting changes from trunk. Please check on
> this.

I confirm that all the indicated issues were not present in the
branch, so no worry about potential merge issues.

> - Not so much fan of lots ifdef's all over. Especially around
> data structures which are file specification.

Freestyle-related DNA and RNA stuff was refactored in the branch
revision 55525.  Your suggestions are much appreciated.

> - Exposing freestyle-specific flags to modifiers and bmesh are
> not making me happy as well. Is it possible to generalize
> flags?

In reply to recent review comments from Campbell on the same
matter, Freestyle-specific flags were removed in revision 55228
and now mesh CustomData is used instead to keep edge/face marks.
I would appreciate it if you could take a look at the revision
and check if further improvements are required.

> - It's not so much clear who's copyright holder of most of
> python files. Would be nice to mention it in comment
> explicitly.

I will check the copyright holders of the Python scripts and duly
update header comments.


@Campbell,

The error in contour.py was fixed in revision 55425.

For what concerns absolute paths to style modules, Brecht also
gave me some comments in his review last October.  Here is a
quote from my reply to the comments:

http://lists.blender.org/pipermail/bf-committers/2012-October/037945.html
>
> > * Python style module scripts are specified with their full paths, to
> > the scripts folder that comes with the blender executable. This will
> > not work when trying to use the .blend file on another computer with
> > Blender installed in a different location. User written scripts should
> > get relative paths, the built in ones I'm not sure about. Maybe they
> > should be referred to only by their name, or work more as presets for
> > users to write their own and place them next to the .blend file or in
> > a text datablock.
> 
> These points are absolutely reasonable.  In fact, using text blocks instead
> of external Python script files was a to-do item, but then the development
> focus was shifted to the Parameter Editor mode.  I agree that text blocks
> should be used to store Python style modules in .blend files.

Style module files are meant to be manually modified by users,
since stylization parameters such as line color, transparency,
and line thickness are coded in Python.  All the style modules in
the scripts/freestyle/style_modules directory are just examples
or templates from where users can start writing their own style
modules.  Many stylization parameter values in the bundled style
modules are indicative and subject to frequent changes by users.
For this reason, maybe it is a good idea to expose the bundled
style module scripts to users as something final and unchanged.
It is likely that users have many different style modules next to
.blend files, since that is the standard way to organize external
files.  To this end, relative paths (starting with //) are
applicable to Python style modules.

Using text blocks to store Python style modules is definitely an
option.  If you think this is really a merger stopper, I would
take this approach. That said, having relative paths to external
scripts placed next .blend files works quite nicely in my
opinion.  Let me know what you think.

https://codereview.appspot.com/7416049/diff/1/CMakeLists.txt.user
File CMakeLists.txt.user (right):

https://codereview.appspot.com/7416049/diff/1/CMakeLists.txt.user#newcode1
CMakeLists.txt.user:1: <?xml version="1.0" encoding="UTF-8"?>
Looks like so.  There are no changes to this file in the Freestyle branch.

https://codereview.appspot.com/7416049/diff/1/blender.kdev4
File blender.kdev4 (right):

https://codereview.appspot.com/7416049/diff/1/blender.kdev4#newcode1
blender.kdev4:1: [Project]
Yes, it looks like that.

https://codereview.appspot.com/7416049/diff/1/build_files/build_environment/i...
File build_files/build_environment/install_deps.sh (left):

https://codereview.appspot.com/7416049/diff/1/build_files/build_environment/i...
build_files/build_environment/install_deps.sh:800: # Optional tests and cmd
tools
These changes are not from the branch.  Looks like included here by accident.

https://codereview.appspot.com/7416049/diff/1/release/scripts/freestyle/style...
File release/scripts/freestyle/style_modules/ChainingIterators.py (right):

https://codereview.appspot.com/7416049/diff/1/release/scripts/freestyle/style...
release/scripts/freestyle/style_modules/ChainingIterators.py:1: # ##### BEGIN
GPL LICENSE BLOCK #####
I am not sure what pep-120 refers to.  Some information about it would be much
appreciated.

The py prefix is a naming convention originally used in the stand-alone
Freestyle program.  There are pairs of same-functionality classes (predicates,
functions, and so on) written in both C/C++ and Python, with the py prefix in
the latter.  It seems that the duplication is for a historical reason
(implementation in Python first, and translation to C/C++ afterwards for speed).

Redundant Python implementations can be removed without loss of functionality,
but I personally prefer to keep them because they are easy to read and serve as
a staring point helping branch users to write their own style modules in Python.
 If we agree to keep the redundant Python implementations for educational
purposes, then it would make sense to keep the py prefix as well to distinguish
them from the corresponding C/C++ implementations.

https://codereview.appspot.com/7416049/diff/1/release/scripts/freestyle/style...
File release/scripts/freestyle/style_modules/PredicatesU1D.py (right):

https://codereview.appspot.com/7416049/diff/1/release/scripts/freestyle/style...
release/scripts/freestyle/style_modules/PredicatesU1D.py:24: count = 0
The variable 'count' does not have to be global.  Fixed in revision 55427.

https://codereview.appspot.com/7416049/diff/1/release/scripts/modules/bl_i18n...
File release/scripts/modules/bl_i18n_utils/bl_extract_messages.py (left):

https://codereview.appspot.com/7416049/diff/1/release/scripts/modules/bl_i18n...
release/scripts/modules/bl_i18n_utils/bl_extract_messages.py:1: # ***** BEGIN
GPL LICENSE BLOCK *****
Looks like included here by accident.  There is no change to this file in the
branch.

https://codereview.appspot.com/7416049/diff/1/release/scripts/modules/bpy/uti...
File release/scripts/modules/bpy/utils.py (left):

https://codereview.appspot.com/7416049/diff/1/release/scripts/modules/bpy/uti...
release/scripts/modules/bpy/utils.py:646: # Build an RNA path from
struct/property/enum names.
Yes, looks like included by accident.

https://codereview.appspot.com/7416049/diff/1/release/scripts/startup/bl_ui/p...
File release/scripts/startup/bl_ui/properties_render.py (right):

https://codereview.appspot.com/7416049/diff/1/release/scripts/startup/bl_ui/p...
release/scripts/startup/bl_ui/properties_render.py:311: class
RENDER_PT_freestyle(RenderFreestyleButtonsPanel, Panel):
Moved the panel definition to new module "bl_ui.properties_freestyle" in
revision 55488.

https://codereview.appspot.com/7416049/diff/1/release/scripts/startup/bl_ui/p...
File release/scripts/startup/bl_ui/properties_render_layer.py (right):

https://codereview.appspot.com/7416049/diff/1/release/scripts/startup/bl_ui/p...
release/scripts/startup/bl_ui/properties_render_layer.py:230: class
RENDERLAYER_PT_freestyle(RenderLayerFreestyleButtonsPanel, Panel):
Moved all Freestyle-specific panels to new module "bl_ui.properties_freestyle"
in revision 55488.

https://codereview.appspot.com/7416049/diff/1/release/scripts/startup/bl_ui/s...
File release/scripts/startup/bl_ui/space_view3d.py (right):

https://codereview.appspot.com/7416049/diff/1/release/scripts/startup/bl_ui/s...
release/scripts/startup/bl_ui/space_view3d.py:2508: if context.scene and
bpy.app.build_options.freestyle:
I am not fully sure if this change should be made.  Any UI label will be
truncated when the panel width is narrower than the label width.  The Freestyle
Edge/Face Mark labels in question can be completely shown if the panel is widen.
 Brecht told me that UI controls should be organized in two columns in general,
and the Overlays section exactly follows this rule.  I tend to be reluctant to
violate it here.

https://codereview.appspot.com/7416049/diff/1/source/blender/CMakeLists.txt
File source/blender/CMakeLists.txt (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/CMakeLists.txt#n...
source/blender/CMakeLists.txt:143: list(APPEND SRC_DNA_INC
Made changes to include DNA_linestyle_types.h even if Freestyle is disabled.

https://codereview.appspot.com/7416049/diff/1/source/blender/blenfont/BLF_tra...
File source/blender/blenfont/BLF_translation.h (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/blenfont/BLF_tra...
source/blender/blenfont/BLF_translation.h:133: #ifdef WITH_FREESTYLE
On 2013/03/18 16:38:04, sergey.vfx wrote:
> Personally, would make I18N aware of enabled feature set. Just assume all the
> features are here.

Done.

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/BKE_l...
File source/blender/blenkernel/BKE_linestyle.h (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/BKE_l...
source/blender/blenkernel/BKE_linestyle.h:36: #ifdef WITH_FREESTYLE
On 2013/03/18 16:38:04, sergey.vfx wrote:
> Personally wouldn't add extra ifdefs, this file is used by freestyle only
> anyway.

Done.

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/BKE_l...
source/blender/blenkernel/BKE_linestyle.h:48: FreestyleLineStyle
*FRS_new_linestyle(const char *name, struct Main *main);
The prefix was changed to BKE_.  The header file has been kept here for two
reasons: 1) ../freestyle is compiled only when Freestyle is enabled; and 2)
other Blender components (e.g., blenkernel and makesrna) rely on it anyway.

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/BKE_m...
File source/blender/blenkernel/BKE_main.h (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/BKE_m...
source/blender/blenkernel/BKE_main.h:91: #ifdef WITH_FREESTYLE
On 2013/03/18 16:38:04, sergey.vfx wrote:
> Don't think it's good idea. All the datablocks are independent from feature
> implementation. DNA, fileio shouldn't take care of whether some feature
enabled
> or not.

Done.

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/SCons...
File source/blender/blenkernel/SConscript (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/SCons...
source/blender/blenkernel/SConscript:151: if env['WITH_BF_FREESTYLE']:
Made changes so as to always build intern/linestyle.c.  The conditional
definition of the symbol WITH_FREESTYLE is kept, because intern/subsurf_ccg.c
contains Freestyle-specific operations that are necessary only when Freestyle is
enabled.

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/inter...
File source/blender/blenkernel/intern/anim_sys.c (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/inter...
source/blender/blenkernel/intern/anim_sys.c:89: #ifdef WITH_FREESTYLE
On 2013/03/18 16:38:04, sergey.vfx wrote:
> Data specification shall not depends on feature sets. Same goes to other
changes
> in this file.

Done.

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/inter...
File source/blender/blenkernel/intern/bpath.c (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/inter...
source/blender/blenkernel/intern/bpath.c:73: #ifdef WITH_FREESTYLE
On 2013/03/18 16:38:04, sergey.vfx wrote:
> Ok, same as above. And repeated for any further idef check around data
> specification.

Done.

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/inter...
File source/blender/blenkernel/intern/linestyle.c (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/inter...
source/blender/blenkernel/intern/linestyle.c:178: if (m) {
Removed NULL checks as suggested.

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/inter...
source/blender/blenkernel/intern/linestyle.c:232:
((LineStyleColorModifier_DistanceFromCamera *)m)->color_ramp = add_colorband(1);
Agreed and updated the code as suggested.

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/inter...
source/blender/blenkernel/intern/linestyle.c:875: switch (m->type) {
This switch statement was meant to be a place to add modifier-specific resource
management stuff, but not used so far.  Just removed in revision 55430.

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/inter...
source/blender/blenkernel/intern/linestyle.c:965: /* XXX Do we want to keep that
goto? Or use a boolean var? */
Fixed in revision 55431.

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/inter...
File source/blender/blenkernel/intern/material.c (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/inter...
source/blender/blenkernel/intern/material.c:156: #ifdef WITH_FREESTYLE
On 2013/03/18 16:38:04, sergey.vfx wrote:
> Would prefer initialize all the properties anyway.

Done.

https://codereview.appspot.com/7416049/diff/1/source/blender/blenlib/CMakeLis...
File source/blender/blenlib/CMakeLists.txt (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/blenlib/CMakeLis...
source/blender/blenlib/CMakeLists.txt:168: if(WITH_FREESTYLE)
Just removed the conditional.

https://codereview.appspot.com/7416049/diff/1/source/blender/blenloader/CMake...
File source/blender/blenloader/CMakeLists.txt (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/blenloader/CMake...
source/blender/blenloader/CMakeLists.txt:69: if(WITH_FREESTYLE)
On 2013/03/18 16:38:04, sergey.vfx wrote:
> Don't think it's needed. Opening .blend with freestyle data in freestyle-less
> build and saving the file shouldn't changing data. It shall be possible.

Done.

https://codereview.appspot.com/7416049/diff/1/source/blender/editors/animatio...
File source/blender/editors/animation/anim_channels_defines.c (left):

https://codereview.appspot.com/7416049/diff/1/source/blender/editors/animatio...
source/blender/editors/animation/anim_channels_defines.c:39: #include
"BLF_translation.h"
Looks like an issue introduced when the diff was created.  The reverting changes
are not from the Freestyle branch.  Only Freestyle-specific additions have been
made in the branch SVN.

https://codereview.appspot.com/7416049/diff/1/source/blender/editors/include/...
File source/blender/editors/include/UI_resources.h (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/editors/include/...
source/blender/editors/include/UI_resources.h:209: /* #ifdef WITH_FREESTYLE */
On 2013/03/18 16:38:04, sergey.vfx wrote:
> ifdef isn't needed here.

Done.

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
File source/blender/freestyle/intern/blender_interface/FRS_freestyle.cpp
(right):

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
source/blender/freestyle/intern/blender_interface/FRS_freestyle.cpp:649: {
Fixed in revision 55484.  Also fixed the default values of line sets and line
styles.  It is noted that the factory settings in the branch are quite
error-prone, since branch-specific changes are often lost when trunk changes are
merged.  That will no longer happen after the trunk merger :)

https://codereview.appspot.com/7416049/diff/1/source/blender/makesdna/CMakeLi...
File source/blender/makesdna/CMakeLists.txt (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/makesdna/CMakeLi...
source/blender/makesdna/CMakeLists.txt:26: if(WITH_FREESTYLE)
On 2013/03/18 16:38:04, sergey.vfx wrote:
> Shall not be checked in DNA

Done.

https://codereview.appspot.com/7416049/diff/1/source/blender/makesdna/DNA_fre...
File source/blender/makesdna/DNA_freestyle_types.h (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/makesdna/DNA_fre...
source/blender/makesdna/DNA_freestyle_types.h:93: char name[32]; /* line set
name */
On 2013/03/18 16:38:04, sergey.vfx wrote:
> Shall be 64 and commented MAX_NAME

Done.

https://codereview.appspot.com/7416049/diff/1/source/blender/makesdna/DNA_fre...
source/blender/makesdna/DNA_freestyle_types.h:110: char module_path[256];
On 2013/03/18 16:38:04, sergey.vfx wrote:
> Guess shall be 1024 which matches MAX_FILE?

Done.

https://codereview.appspot.com/7416049/diff/1/source/blender/windowmanager/in...
File source/blender/windowmanager/intern/wm_files.c (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/windowmanager/in...
source/blender/windowmanager/intern/wm_files.c:441: #ifdef WITH_FREESTYLE
Removed the Freestyle-specific call in revision 55542.  Now the post-load stuff
is done through a callback based on BLI_callbacks.

https://codereview.appspot.com/7416049/diff/1/user-config.py
File user-config.py (right):

https://codereview.appspot.com/7416049/diff/1/user-config.py#newcode1
user-config.py:1: WITH_BF_3DMOUSE = False
Looks like included by accident when the diff was created.  This file is not
part of the branch.

[ideasman42, 2013-03-27 22:56:53]

Hi, checking customdata changes.

Firstly, I dont think ME_CDFLAG_FREESTYLE_EDGE, ME_CDFLAG_FREESTYLE_FACE should
be defined at all, Once the conversion from existing, older blend files is done
- the CustomData layers are in the mesh and there is no need to store this
setting in me->cd_flag.

The purpose of me->cd_flag is for setting which are apart of
DNA_meshdata_types.h but not customdata when converting into BMEditMesh (BMesh),
before this, customdata for edge crease for example was always being added to
BMesh data which was often not needed.

So I think these flags can simply be removed.


Next...
Notice that getCCGDerivedMesh is managing FreestyleEdge, FreestyleFace
specifically.
The purpose of customdata is to avoid having to have each modifier do special
checks for each data-type.

I think it would be better not to have this kind of check in the code. Instead
checks in convertblender.c can do lookups on the tessface's ORIGINDEX, Check for
CD_ORIGINDEX for similar examples.

[ideasman42, 2013-03-27 23:08:07]

Note regarding licensing,
Noticed that the headers include this:

 * The Original Code is Copyright (C) 2010 Blender Foundation.
 * All rights reserved.

This implies that the original authors have assigned their copyright to the
blender foundation (as opposed to releasing under a GPL2+ compatible license but
keeping their own copyright)

Authors Listed under '\author' in freestyle files:
- Bruno Levy
- Emmanuel Turquin
- Fredo Durand
- Stephane Grabli
- Stephane Popinet
- Sylvain Paris

Did these developers accept to assign copyright to the Blender Foundation?

[irieshinsuke, 2013-03-30 09:55:14]

Added a comment about the default line color.

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
File source/blender/freestyle/intern/blender_interface/FRS_freestyle.cpp
(right):

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
source/blender/freestyle/intern/blender_interface/FRS_freestyle.cpp:649: {
I think the default line color should remain black (the factory setting also
should be black), because we usually use dark line color for cartoon pictures.
After the default line color was changed to white, I have to change the line
color every time I add a line set...

[kjym3, 2013-04-03 00:25:10]

Dear Reviewers,

Thanks again for all the review comments.  Based on suggestions
through the comments as well as personal communications in the IRC, a
number of changes have been made to address all the required updates.
Here is a quick summary of changes:

- The me->cd_flag values ME_CDFLAG_FREESTYLE_EDGE and
ME_CDFLAG_FREESTYLE_FACE were simply removed [r55705].

- Freestyle-specific code was removed from getCCGDerivedMesh and
edge/face marks were reimplemented using CD_ORIGINDEX edge/poly layers
in convertblender.c [r55683].  Revision 55683 contains fixes for
modifiers not properly setting CD_ORIGINDEX layer entries.  These
fixes will be proposed as a patch set for the trunk and removed from
the branch.

- Copyright information in file headers was updated [r55680] (many
thanks to Campbell for the updates).

- Default line style parameters were updated as suggested [r55677].

- Absolute paths to external style module files were eliminated by
means of text datablocks.  Now style modules are stored within .blend
files. [r55741]

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
File source/blender/freestyle/intern/blender_interface/FRS_freestyle.cpp
(right):

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern...
source/blender/freestyle/intern/blender_interface/FRS_freestyle.cpp:649: {
On 2013/03/30 09:55:14, irieshinsuke wrote:
> I think the default line color should remain black (the factory setting also
> should be black), because we usually use dark line color for cartoon pictures.
> After the default line color was changed to white, I have to change the line
> color every time I add a line set...

Fixed in revision 55677.

[kjym3, 2013-04-03 01:02:08]

Dear Reviewers,

On 2013/04/03 00:25:10, kjym3 wrote:
> 
> - Freestyle-specific code was removed from getCCGDerivedMesh and
> edge/face marks were reimplemented using CD_ORIGINDEX edge/poly layers
> in convertblender.c [r55683].  Revision 55683 contains fixes for
> modifiers not properly setting CD_ORIGINDEX layer entries.  These
> fixes will be proposed as a patch set for the trunk and removed from
> the branch.

Here is a diff for the trunk:

http://www.pasteall.org/41011/diff

Upon inclusion of these fixes in the trunk, the corresponding changes
in the Freestyle branch will be deleted.  Any comments and suggestions
about the proposed fixes are duly acknowledged.

[irieshinsuke, 2013-04-03 06:27:18]

Hi,

On 2013/04/03 00:25:10, kjym3 wrote:
> - Freestyle-specific code was removed from getCCGDerivedMesh and
> edge/face marks were reimplemented using CD_ORIGINDEX edge/poly layers
> in convertblender.c [r55683].  Revision 55683 contains fixes for
> modifiers not properly setting CD_ORIGINDEX layer entries.  These
> fixes will be proposed as a patch set for the trunk and removed from
> the branch.

Since r55683, mirror modifier doesn't mirror the face marks.
Is the implementation still WIP?

[kjym3, 2013-04-03 18:28:02]

On 2013/04/03 06:27:18, irieshinsuke wrote:
> 
> Since r55683, mirror modifier doesn't mirror the face marks.
> Is the implementation still WIP?

No, the issue is a bug in the proposed changes to the Mirror modifier.
Fixed in revision 55764.

[kjym3, 2013-04-03 20:24:38]

Hi Campbell,

I just filed an updated patch set in the Patch Tracker.
http://projects.blender.org/tracker/index.php?func=detail&aid=34859&group_id=...

Could you please take a look at the patch and apply it to the trunk if the
changes are considered okay?

Thank you,
T.K.


On 2013/04/03 01:02:08, kjym3 wrote:
> Dear Reviewers,
> 
> On 2013/04/03 00:25:10, kjym3 wrote:
> > 
> > - Freestyle-specific code was removed from getCCGDerivedMesh and
> > edge/face marks were reimplemented using CD_ORIGINDEX edge/poly layers
> > in convertblender.c [r55683].  Revision 55683 contains fixes for
> > modifiers not properly setting CD_ORIGINDEX layer entries.  These
> > fixes will be proposed as a patch set for the trunk and removed from
> > the branch.
> 
> Here is a diff for the trunk:
> 
> http://www.pasteall.org/41011/diff
> 
> Upon inclusion of these fixes in the trunk, the corresponding changes
> in the Freestyle branch will be deleted.  Any comments and suggestions
> about the proposed fixes are duly acknowledged.

[ideasman42, 2013-04-04 17:32:53]

Hi Tamito, how are your plans for merging into trunk?

As far as I can see, the branch can be merged now.

I compiled the latest freestyle branch and noticed some issues but no blocking
issues...

- Commented on your patch, think we can resolve the issue after merge into
trunk.

- Noticed bpath.c still references python module path which is now marked as
deprecated, Could you remove deprecated DNA before the merge? (generally remove
any DNA data hanging around which isnt used anymore).

[kjym3, 2013-04-04 23:36:42]

Hi Campbell,

Many thanks for the approval as well as for the careful code review so far!

I am available this weekend and likely able to work on the trunk merger.
I would just proceed with the merger as soon as possible, to gain time for
solving potential issues during the merger.  Some help concerning the final
merging procedure through IRC conversations would be much appreciated.

On 2013/04/04 17:32:53, ideasman42 wrote:
> 
> - Commented on your patch, think we can resolve the issue after merge into
> trunk.

Agreed.

> - Noticed bpath.c still references python module path which is now marked as
> deprecated, Could you remove deprecated DNA before the merge? (generally
remove
> any DNA data hanging around which isnt used anymore).

Thanks for pointing this out.  I will remove it and all the other deprecated
DNA elements when the merger is carried out.