[Bf-codereview] Freestyle r54826 branch review (issue 7416049)

[tamito.kajiyama at gmail.com]

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_operators/freestyle.py
File release/scripts/startup/bl_operators/freestyle.py (right):

https://codereview.appspot.com/7416049/diff/1/release/scripts/startup/bl_operators/freestyle.py#newcode62
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_operators/freestyle.py#newcode76
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_operators/freestyle.py#newcode107
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_class.h#newcode250
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/application/AppConfig.cpp
File source/blender/freestyle/intern/application/AppConfig.cpp (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern/application/AppConfig.cpp#newcode68
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/application/AppView.h
File source/blender/freestyle/intern/application/AppView.h (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern/application/AppView.h#newcode38
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/blender_interface/BlenderStrokeRenderer.cpp
File
source/blender/freestyle/intern/blender_interface/BlenderStrokeRenderer.cpp
(right):

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern/blender_interface/BlenderStrokeRenderer.cpp#newcode85
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/blender_interface/BlenderStrokeRenderer.cpp#newcode179
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/blender_interface/BlenderStyleModule.h
File
source/blender/freestyle/intern/blender_interface/BlenderStyleModule.h
(right):

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern/blender_interface/BlenderStyleModule.h#newcode54
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/blender_interface/FRS_freestyle.cpp
File source/blender/freestyle/intern/blender_interface/FRS_freestyle.cpp
(right):

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern/blender_interface/FRS_freestyle.cpp#newcode197
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/python/BPy_Convert.cpp
File source/blender/freestyle/intern/python/BPy_Convert.cpp (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern/python/BPy_Convert.cpp#newcode624
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/python/Director.cpp
File source/blender/freestyle/intern/python/Director.cpp (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern/python/Director.cpp#newcode240
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/python/UnaryPredicate1D/BPy_ShapeUP1D.cpp
File
source/blender/freestyle/intern/python/UnaryPredicate1D/BPy_ShapeUP1D.cpp
(right):

https://codereview.appspot.com/7416049/diff/1/source/blender/freestyle/intern/python/UnaryPredicate1D/BPy_ShapeUP1D.cpp#newcode72
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/rna_scene.c
File source/blender/makesrna/intern/rna_scene.c (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/makesrna/intern/rna_scene.c#newcode2504
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.

https://codereview.appspot.com/7416049/