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

ideasman42 at gmail.com ideasman42 at gmail.com
Sat Mar 2 11:56:47 CET 2013 

Reviewers: bf-codereview_blender.org,

Message:
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_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')
suggest to use sys.float_info.min/max

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'''
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_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'''
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_class.h#newcode250
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/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";
Why does freestyle need a web browser?

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))
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/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);
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/blender_interface/BlenderStrokeRenderer.cpp#newcode179
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/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);
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/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++) {
copy_m4m4 would do here.

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));
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/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)) {
Is the type of 'result' checked here? looks like for PyFloat_AsDouble
not.

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);
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/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);
*picky* - should be "show_render"



Please review this at https://codereview.appspot.com/7416049/

Affected files:
   M     CMakeLists.txt
   A     CMakeLists.txt.user
   M     SConstruct
   A     blender.kdev4
   M     build_files/build_environment/install_deps.sh
   M     build_files/scons/config/darwin-config.py
   M     build_files/scons/config/freebsd7-config.py
   M     build_files/scons/config/freebsd8-config.py
   M     build_files/scons/config/freebsd9-config.py
   M     build_files/scons/config/linux-config.py
   M     build_files/scons/config/linuxcross-config.py
   M     build_files/scons/config/win32-mingw-config.py
   M     build_files/scons/config/win32-vc-config.py
   M     build_files/scons/config/win64-mingw-config.py
   M     build_files/scons/config/win64-vc-config.py
   M     build_files/scons/tools/Blender.py
   M     build_files/scons/tools/btools.py
   M     intern/cycles/blender/addon/ui.py
   A     release/scripts/freestyle/style_modules/ChainingIterators.py
   A     release/scripts/freestyle/style_modules/Functions0D.py
   A     release/scripts/freestyle/style_modules/Functions1D.py
   A     release/scripts/freestyle/style_modules/PredicatesB1D.py
   A     release/scripts/freestyle/style_modules/PredicatesU0D.py
   A     release/scripts/freestyle/style_modules/PredicatesU1D.py
   A     release/scripts/freestyle/style_modules/anisotropic_diffusion.py
   A      
release/scripts/freestyle/style_modules/apriori_and_causal_density.py
   A     release/scripts/freestyle/style_modules/apriori_density.py
   A     release/scripts/freestyle/style_modules/backbone_stretcher.py
   A     release/scripts/freestyle/style_modules/blueprint_circles.py
   A     release/scripts/freestyle/style_modules/blueprint_ellipses.py
   A     release/scripts/freestyle/style_modules/blueprint_squares.py
   A     release/scripts/freestyle/style_modules/cartoon.py
   A     release/scripts/freestyle/style_modules/contour.py
   A     release/scripts/freestyle/style_modules/curvature2d.py
   A     release/scripts/freestyle/style_modules/external_contour.py
   release/scripts/freestyle/style_modules/external_contour_sketchy.py
   A     release/scripts/freestyle/style_modules/external_contour_smooth.py
   A     release/scripts/freestyle/style_modules/haloing.py
   A     release/scripts/freestyle/style_modules/ignore_small_occlusions.py
   A     release/scripts/freestyle/style_modules/invisible_lines.py
   A     release/scripts/freestyle/style_modules/japanese_bigbrush.py
   A     release/scripts/freestyle/style_modules/logical_operators.py
   A      
release/scripts/freestyle/style_modules/long_anisotropically_dense.py
   A     release/scripts/freestyle/style_modules/multiple_parameterization.py
   A     release/scripts/freestyle/style_modules/nature.py
   A     release/scripts/freestyle/style_modules/near_lines.py
   A      
release/scripts/freestyle/style_modules/occluded_by_specific_object.py
   A     release/scripts/freestyle/style_modules/parameter_editor.py
   A     release/scripts/freestyle/style_modules/polygonalize.py
   release/scripts/freestyle/style_modules/qi0.py
   A     release/scripts/freestyle/style_modules/qi0_not_external_contour.py
   A     release/scripts/freestyle/style_modules/qi1.py
   A     release/scripts/freestyle/style_modules/qi2.py
   A     release/scripts/freestyle/style_modules/sequentialsplit_sketchy.py
   A     release/scripts/freestyle/style_modules/shaders.py
   A      
release/scripts/freestyle/style_modules/sketchy_multiple_parameterization.py
   A     release/scripts/freestyle/style_modules/sketchy_topology_broken.py
   A      
release/scripts/freestyle/style_modules/sketchy_topology_preserved.py
   A      
release/scripts/freestyle/style_modules/split_at_highest_2d_curvatures.py
   A     release/scripts/freestyle/style_modules/split_at_tvertices.py
   A     release/scripts/freestyle/style_modules/stroke_texture.py
   A     release/scripts/freestyle/style_modules/suggestive.py
   A      
release/scripts/freestyle/style_modules/thickness_fof_depth_discontinuity.py
   A     release/scripts/freestyle/style_modules/tipremover.py
   A     release/scripts/freestyle/style_modules/tvertex_remover.py
   A     release/scripts/freestyle/style_modules/uniformpruning_zsort.py
   D     release/scripts/modules/bl_i18n_utils/bl_extract_messages.py
   A     release/scripts/modules/bl_i18n_utils/bl_process_msg.py
   D     release/scripts/modules/bl_i18n_utils/languages_menu_utils.py
   A     release/scripts/modules/bl_i18n_utils/rtl_preprocess.py
   D     release/scripts/modules/bl_i18n_utils/rtl_utils.py
   M     release/scripts/modules/bl_i18n_utils/settings.py
   M     release/scripts/modules/bl_i18n_utils/spell_check_utils.py
   A     release/scripts/modules/bl_i18n_utils/update_languages_menu.py
   M     release/scripts/modules/bl_i18n_utils/utils.py
   M     release/scripts/modules/bpy/utils.py
   M     release/scripts/modules/console/intellisense.py
   M     release/scripts/startup/bl_operators/__init__.py
   A     release/scripts/startup/bl_operators/freestyle.py
   M     release/scripts/startup/bl_operators/rigidbody.py
   M     release/scripts/startup/bl_ui/__init__.py
   M     release/scripts/startup/bl_ui/properties_render.py
   A     release/scripts/startup/bl_ui/properties_render_layer.py
   M     release/scripts/startup/bl_ui/space_view3d.py
   M     source/blender/CMakeLists.txt
   M     source/blender/SConscript
   M     source/blender/blenfont/BLF_translation.h
   M     source/blender/blenfont/CMakeLists.txt
   M     source/blender/blenfont/SConscript
   M     source/blender/blenkernel/BKE_global.h
   A     source/blender/blenkernel/BKE_linestyle.h
   M     source/blender/blenkernel/BKE_main.h
   M     source/blender/blenkernel/CMakeLists.txt
   M     source/blender/blenkernel/SConscript
   M     source/blender/blenkernel/intern/anim_sys.c
   M     source/blender/blenkernel/intern/bpath.c
   M     source/blender/blenkernel/intern/group.c
   M     source/blender/blenkernel/intern/idcode.c
   M     source/blender/blenkernel/intern/library.c
   A     source/blender/blenkernel/intern/linestyle.c
   M     source/blender/blenkernel/intern/material.c
   M     source/blender/blenkernel/intern/object.c
   M     source/blender/blenkernel/intern/scene.c
   M     source/blender/blenkernel/intern/subsurf_ccg.c
   M     source/blender/blenlib/CMakeLists.txt
   M     source/blender/blenlib/SConscript
   M     source/blender/blenloader/CMakeLists.txt
   M     source/blender/blenloader/SConscript
   M     source/blender/blenloader/intern/readfile.c
   M     source/blender/blenloader/intern/writefile.c
   M     source/blender/bmesh/CMakeLists.txt
   M     source/blender/bmesh/SConscript
   M     source/blender/bmesh/bmesh_class.h
   M     source/blender/bmesh/intern/bmesh_construct.c
   M     source/blender/bmesh/intern/bmesh_operators.h
   M     source/blender/bmesh/operators/bmo_similar.c
   M     source/blender/editors/animation/CMakeLists.txt
   M     source/blender/editors/animation/SConscript
   M     source/blender/editors/animation/anim_channels_defines.c
   M     source/blender/editors/animation/anim_channels_edit.c
   M     source/blender/editors/animation/anim_filter.c
   M     source/blender/editors/animation/fmodifier_ui.c
   M     source/blender/editors/armature/editarmature.c
   M     source/blender/editors/curve/CMakeLists.txt
   M     source/blender/editors/curve/SConscript
   M     source/blender/editors/curve/editcurve.c
   M     source/blender/editors/include/ED_anim_api.h
   M     source/blender/editors/include/UI_resources.h
   M     source/blender/editors/interface/CMakeLists.txt
   M     source/blender/editors/interface/SConscript
   M     source/blender/editors/interface/interface.c
   M     source/blender/editors/interface/interface_templates.c
   M     source/blender/editors/interface/resources.c
   M     source/blender/editors/io/CMakeLists.txt
   M     source/blender/editors/io/SConscript
   M     source/blender/editors/mesh/CMakeLists.txt
   M     source/blender/editors/mesh/SConscript
   M     source/blender/editors/mesh/editmesh_select.c
   M     source/blender/editors/mesh/editmesh_tools.c
   M     source/blender/editors/mesh/mesh_intern.h
   M     source/blender/editors/mesh/mesh_ops.c
   M     source/blender/editors/object/object_add.c
   M     source/blender/editors/object/object_constraint.c
   M     source/blender/editors/object/object_intern.h
   M     source/blender/editors/object/object_ops.c
   source/blender/editors/object/object_relations.c
   M     source/blender/editors/object/object_select.c
   M     source/blender/editors/physics/physics_pointcache.c
   M     source/blender/editors/render/CMakeLists.txt
   M     source/blender/editors/render/SConscript
   M     source/blender/editors/render/render_intern.h
   M     source/blender/editors/render/render_ops.c
   M     source/blender/editors/render/render_shading.c
   M     source/blender/editors/screen/area.c
   M     source/blender/editors/space_buttons/CMakeLists.txt
   M     source/blender/editors/space_buttons/SConscript
   M     source/blender/editors/space_buttons/buttons_context.c
   M     source/blender/editors/space_buttons/buttons_header.c
   M     source/blender/editors/space_buttons/buttons_texture.c
   M     source/blender/editors/space_buttons/space_buttons.c
   M     source/blender/editors/space_file/CMakeLists.txt
   M     source/blender/editors/space_file/SConscript
   M     source/blender/editors/space_file/filelist.c
   M     source/blender/editors/space_image/image_ops.c
   M     source/blender/editors/space_info/info_ops.c
   M     source/blender/editors/space_info/info_stats.c
   M     source/blender/editors/space_nla/CMakeLists.txt
   M     source/blender/editors/space_nla/SConscript
   M     source/blender/editors/space_nla/nla_buttons.c
   M     source/blender/editors/space_nla/nla_channels.c
   M     source/blender/editors/space_nla/nla_edit.c
   M     source/blender/editors/space_node/node_group.c
   M     source/blender/editors/space_text/text_header.c
   M     source/blender/editors/space_text/text_ops.c
   M     source/blender/editors/space_view3d/CMakeLists.txt
   M     source/blender/editors/space_view3d/SConscript
   M     source/blender/editors/space_view3d/drawobject.c
   M     source/blender/editors/space_view3d/space_view3d.c
   source/blender/editors/space_view3d/view3d_header.c
   M     source/blender/editors/transform/transform_ops.c
   M     source/blender/editors/util/ed_util.c
   M     source/blender/editors/uvedit/uvedit_ops.c
   A     source/blender/freestyle/CMakeLists.txt
   A     source/blender/freestyle/FRS_freestyle.h
   A     source/blender/freestyle/FRS_freestyle_config.h
   A     source/blender/freestyle/SConscript
   A     source/blender/freestyle/intern/application/AppCanvas.cpp
   A     source/blender/freestyle/intern/application/AppCanvas.h
   A     source/blender/freestyle/intern/application/AppConfig.cpp
   A     source/blender/freestyle/intern/application/AppConfig.h
   A     source/blender/freestyle/intern/application/AppView.cpp
   A     source/blender/freestyle/intern/application/AppView.h
   A     source/blender/freestyle/intern/application/Controller.cpp
   A     source/blender/freestyle/intern/application/Controller.h
   A      
source/blender/freestyle/intern/blender_interface/BlenderFileLoader.cpp
   A      
source/blender/freestyle/intern/blender_interface/BlenderFileLoader.h
   A      
source/blender/freestyle/intern/blender_interface/BlenderStrokeRenderer.cpp
   A      
source/blender/freestyle/intern/blender_interface/BlenderStrokeRenderer.h
   A      
source/blender/freestyle/intern/blender_interface/BlenderStyleModule.h
   A      
source/blender/freestyle/intern/blender_interface/BlenderTextureManager.cpp
   [[ 570 additional files ]]

More information about the Bf-codereview mailing list