Code review - Issue 7416049: Freestyle r54826 branch reviewhttps://codereview.appspot.com/2013-04-04T23:36:42+00:00rietveld
Message from unknown
2013-03-02T07:50:36+00:00ideasman42urn:md5:f7334483146f3d8b273de14ef3110e7f
Message from ideasman42@gmail.com
2013-03-02T10:56:47+00:00ideasman42urn:md5:938c35a1fe6686039968b052b172e373
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"
Message from tamito.kajiyama@gmail.com
2013-03-03T01:58:03+00:00kjym3urn:md5:0421375e0bceddd237d9887fa25cd10b
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.
Message from ideasman42@gmail.com
2013-03-06T04:34:18+00:00ideasman42urn:md5:62131b5fbf147bc2ac81381f2b66d213
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.
Message from tamito.kajiyama@gmail.com
2013-03-13T07:07:14+00:00kjym3urn:md5:81cb56abd8c3fd47b1efce6a60bfb9e1
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.
Message from sergey.vfx@gmail.com
2013-03-18T16:38:04+00:00sergey.vfxurn:md5:4069033fcace9ef16b91588bc8aaedf0
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
Message from ideasman42@gmail.com
2013-03-19T00:41:57+00:00ideasman42urn:md5:0c92d0afc9af95497d6b69998749896a
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.
Message from irieshinsuke@gmail.com
2013-03-21T12:39:57+00:00irieshinsukeurn:md5:04e3f8da86ed32b90b707e09025a3b78
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/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:
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/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: {
Why these default values are different from the factory settings (mode=FREESTYLE_CONTROL_EDITOR_MODE, crease_angle=134.43, etc.)?
Message from tamito.kajiyama@gmail.com
2013-03-23T22:46:07+00:00kjym3urn:md5:9b594ba032a225568e8cfd3311529c32
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.
Message from ideasman42@gmail.com
2013-03-27T22:56:53+00:00ideasman42urn:md5:a9f46b6f241b7d8ed38aff5eb3a9316f
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.
Message from ideasman42@gmail.com
2013-03-27T23:08:07+00:00ideasman42urn:md5:9daccdfacdbc3aec970b7c6ad90b8fa1
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?
Message from irieshinsuke@gmail.com
2013-03-30T09:55:14+00:00irieshinsukeurn:md5:9a295f5fe4daf0cdae23d186a5dbe4e8
Added a comment about the default line color.
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: {
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...
Message from tamito.kajiyama@gmail.com
2013-04-03T00:25:10+00:00kjym3urn:md5:6159d8c8d8f15ee06a1610a276f34000
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/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: {
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.
Message from tamito.kajiyama@gmail.com
2013-04-03T01:02:08+00:00kjym3urn:md5:f9b4fdf4b4a53776e5d53cd65a24ca1f
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.
Message from irieshinsuke@gmail.com
2013-04-03T06:27:18+00:00irieshinsukeurn:md5:f6a9ac441838c692e1f2343b7163d9aa
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?
Message from tamito.kajiyama@gmail.com
2013-04-03T18:28:02+00:00kjym3urn:md5:58a264daea91fb8023ae1a53d69feb5a
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.
Message from tamito.kajiyama@gmail.com
2013-04-03T20:24:38+00:00kjym3urn:md5:e911e5a763773ccce128d6fca00c7cde
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=9&atid=127
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.
Message from ideasman42@gmail.com
2013-04-04T17:32:53+00:00ideasman42urn:md5:08bc9c8edf6729bb9a954379f0736bda
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).
Message from tamito.kajiyama@gmail.com
2013-04-04T23:36:42+00:00kjym3urn:md5:10d57f9e2cc36cfa6ae3054491d554e7
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.