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

[sergey.vfx at gmail.com]

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/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
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_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 #####
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_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
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_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 *****
Guess this is merge resolved incorrect?

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.
Is it also a merge issue?

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):
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/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):
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#newcode143
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_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
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_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
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_linestyle.h#newcode48
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_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
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/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']:
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/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
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/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
Ok, same as above. And repeated for any further idef check around data
specification.

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) {
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/intern/linestyle.c#newcode232
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/intern/linestyle.c#newcode875
source/blender/blenkernel/intern/linestyle.c:875: switch (m->type) {
What this is about?

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? */
Well, issue is already mentioned.

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
Would prefer initialize all the properties anyway.

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)
Shouldn't be needed. All freestyle-specific stuff is on higher level
than blenlib.

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)
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/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"
Seems to be some changes were revetred in this file.

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 */
ifdef isn't needed here.

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)
Shall not be checked in DNA

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 */
Shall be 64 and commented MAX_NAME

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];
Guess shall be 1024 which matches MAX_FILE?

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
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

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