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

tamito.kajiyama at gmail.com tamito.kajiyama at gmail.com
Sat Mar 23 23:46:08 CET 2013 

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/install_deps.sh
File build_files/build_environment/install_deps.sh (left):

https://codereview.appspot.com/7416049/diff/1/build_files/build_environment/install_deps.sh#oldcode800
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_modules/ChainingIterators.py
File release/scripts/freestyle/style_modules/ChainingIterators.py
(right):

https://codereview.appspot.com/7416049/diff/1/release/scripts/freestyle/style_modules/ChainingIterators.py#newcode1
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_modules/PredicatesU1D.py
File release/scripts/freestyle/style_modules/PredicatesU1D.py (right):

https://codereview.appspot.com/7416049/diff/1/release/scripts/freestyle/style_modules/PredicatesU1D.py#newcode24
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_utils/bl_extract_messages.py
File release/scripts/modules/bl_i18n_utils/bl_extract_messages.py
(left):

https://codereview.appspot.com/7416049/diff/1/release/scripts/modules/bl_i18n_utils/bl_extract_messages.py#oldcode1
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/utils.py
File release/scripts/modules/bpy/utils.py (left):

https://codereview.appspot.com/7416049/diff/1/release/scripts/modules/bpy/utils.py#oldcode646
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/properties_render.py
File release/scripts/startup/bl_ui/properties_render.py (right):

https://codereview.appspot.com/7416049/diff/1/release/scripts/startup/bl_ui/properties_render.py#newcode311
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/properties_render_layer.py
File release/scripts/startup/bl_ui/properties_render_layer.py (right):

https://codereview.appspot.com/7416049/diff/1/release/scripts/startup/bl_ui/properties_render_layer.py#newcode230
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/space_view3d.py
File release/scripts/startup/bl_ui/space_view3d.py (right):

https://codereview.appspot.com/7416049/diff/1/release/scripts/startup/bl_ui/space_view3d.py#newcode2508
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#newcode143
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_translation.h
File source/blender/blenfont/BLF_translation.h (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/blenfont/BLF_translation.h#newcode133
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_linestyle.h
File source/blender/blenkernel/BKE_linestyle.h (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/BKE_linestyle.h#newcode36
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_linestyle.h#newcode48
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_main.h
File source/blender/blenkernel/BKE_main.h (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/BKE_main.h#newcode91
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/SConscript
File source/blender/blenkernel/SConscript (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/SConscript#newcode151
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/intern/anim_sys.c
File source/blender/blenkernel/intern/anim_sys.c (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/intern/anim_sys.c#newcode89
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/intern/bpath.c
File source/blender/blenkernel/intern/bpath.c (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/intern/bpath.c#newcode73
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/intern/linestyle.c
File source/blender/blenkernel/intern/linestyle.c (right):

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

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/intern/linestyle.c#newcode232
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/intern/linestyle.c#newcode875
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/intern/linestyle.c#newcode965
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/intern/material.c
File source/blender/blenkernel/intern/material.c (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/blenkernel/intern/material.c#newcode156
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/CMakeLists.txt
File source/blender/blenlib/CMakeLists.txt (right):

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

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

https://codereview.appspot.com/7416049/diff/1/source/blender/blenloader/CMakeLists.txt#newcode69
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/animation/anim_channels_defines.c
File source/blender/editors/animation/anim_channels_defines.c (left):

https://codereview.appspot.com/7416049/diff/1/source/blender/editors/animation/anim_channels_defines.c#oldcode39
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/UI_resources.h
File source/blender/editors/include/UI_resources.h (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/editors/include/UI_resources.h#newcode209
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/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#newcode649
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/CMakeLists.txt
File source/blender/makesdna/CMakeLists.txt (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/makesdna/CMakeLists.txt#newcode26
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_freestyle_types.h
File source/blender/makesdna/DNA_freestyle_types.h (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/makesdna/DNA_freestyle_types.h#newcode93
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_freestyle_types.h#newcode110
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/intern/wm_files.c
File source/blender/windowmanager/intern/wm_files.c (right):

https://codereview.appspot.com/7416049/diff/1/source/blender/windowmanager/intern/wm_files.c#newcode441
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.

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

More information about the Bf-codereview mailing list