Page MenuHome
Differential D545

Freestyle Python API: new methods for Stroke and StrokeVertexIterator
ClosedPublic
Actions

Authored by Folkert de Vries (flokkievids) on May 23 2014, 7:52 PM.

Details

Reviewers
Tamito Kajiyama (kjym3)
Maniphest Tasks
T37839: Iterator incremented() and decremented() methods
Commits
rBS840891e22a12: D545: Freestyle Python API: new methods for Stroke and StrokeVertexIterator.
rB840891e22a12: D545: Freestyle Python API: new methods for Stroke and StrokeVertexIterator.
Summary

This patch adds some new functionality to Stroke and StrokeVertexIterator objects.

For strokes, the reversed method is implemented. it works the same as with other python sequence objects (returns an iterator starting from the end). this in in effect the same as Stroke.stroke_vertices_end().

For StrokeVertexIterator an incremented, decremented and reversed method has been added.

incremented and decremented return a new StrokeVertexIterator object that has been in- or decremented,
reversed returns a new StrokeVertexIterator object that will traverse in the opposite direction.

Diff Detail

Event Timeline

Folkert de Vries (flokkievids) updated this revision to Unknown Object (????).May 24 2014, 12:04 PM

The new BPy_StrokeVertexIterator(*self) used self->sv_it for copy->sv_it. incrementing one would increment the other, which is undesired.
This has now been fixed by copying the StrokeVertexIterator object itself.

Additionally, with the first version of this patch a for-loop and a reversed iterator object would be out of sync. this has now been fixed.
I also added some further comments to explain what is going on in iter_next.

Folkert de Vries (flokkievids) updated this revision to Unknown Object (????).May 24 2014, 9:03 PM

Added construction of iterators from Interface1D elements.

This provides a better solution for:
it = Interface0DIterator(iter(stroke))

the new method allows:
it = Interface0DIterator(stroke)
it = StrokeVertexIterator(stroke)

This means a cleaner syntax and prevents the creation of an extra iterator object.

Folkert de Vries (flokkievids) updated this revision to Unknown Object (????).May 24 2014, 9:05 PM
Tamito Kajiyama (kjym3) requested changes to this revision.Jun 2 2014, 4:44 AM

The patch looks mostly okay. The added constructor of iterators from Stroke instance is a good idea.

When you make a patch revision, please keep the original white space (indentation) to make code review easier.

source/blender/freestyle/intern/python/Interface1D/BPy_Stroke.cpp
313

I would suggest adding a docstring, e.g.:

PyDoc_STRVAR(Stroke_reversed_doc,
	".. method:: __reversed__()\n"
	"\n"
	"   Returns a StrokeVertexIterator iterating over the vertices of the Stroke\n"
	"   in the reversed order (from the last to the first).\n"
	"\n"
	"   :return: A StrokeVertexIterator pointing after the last StrokeVertex.\n"
	"   :rtype: :class:`StrokeVertexIterator`");
source/blender/freestyle/intern/python/Iterator/BPy_Interface0DIterator.cpp
74 ↗(On Diff #1757)

The keyword argument name could be inter. Not really an issue but the camel case name seems unusual.

source/blender/freestyle/intern/python/Iterator/BPy_Interface0DIterator.h
29 ↗(On Diff #1757)

This #include should be done in BPy_Interface0DIterator.cpp because it is only necessary for the implementation.

source/blender/freestyle/intern/python/Iterator/BPy_StrokeVertexIterator.cpp
70

This first call of PyErr_Clear() is unnecessary.

source/blender/freestyle/intern/python/Iterator/BPy_StrokeVertexIterator.h
29 ↗(On Diff #1757)

This #include should be done in BPy_StrokeVertexIterator.cpp because it is only necessary for the implementation.

source/blender/freestyle/intern/view_map/Interface0D.h
203 ↗(On Diff #1757)

The added copy() method seems an over-generalization. For instance calling this method on a StrokeVertexIterator instance may result in an Interface0DIterator rather than a copy of the StrokeVertexIterator.

I suggest removing this method and using the usual new Interface0DIterator(it) syntax.

Folkert de Vries (flokkievids) updated this revision to Unknown Object (????).Jun 3 2014, 12:00 AM

Thanks TK for the review.

In line with your comments I have:

  • added your docstring to reversed
  • moved includes to the .cpp files
  • gave some proper variable names
  • removed the first PyErr_Clear in BPy_StrokeVertexIterator.cpp; I had just copied that blindly, which is obviously stupid.
  • used correct object duplication method
  • fixed problem with whitespace

I think this patch is complete now. I've implemented the new features from this patch into the python scripts locally already, and it all works nicely.

Tamito Kajiyama (kjym3) accepted this revision.Jun 3 2014, 4:43 PM

Thanks for the update of the patch. Now everything looks okay to me.
I will merge the patch to Git master for inclusion in the 2.72 release and close T37839.

source/blender/freestyle/intern/python/Iterator/BPy_Interface0DIterator.cpp
79 ↗(On Diff #1829)

Looks like I forgot to point out this redundant call of PyErr_Clear().

Folkert de Vries (flokkievids) updated this revision to Unknown Object (????).Jun 14 2014, 10:45 PM

remove redundant call to PyErr_Clear()

Tamito Kajiyama (kjym3) closed this revision.Jun 16 2014, 3:14 AM

Closed by commit rB840891e22a12 (authored by @Tamito Kajiyama (kjym3)).

Tamito Kajiyama (kjym3) mentioned this in rBS840891e22a12: D545: Freestyle Python API: new methods for Stroke and StrokeVertexIterator..Nov 20 2014, 9:45 AM

Revision Contents
Changeset List

PathSize
source/
blender/
freestyle/
intern/
python/
Interface1D/
7 lines
Iterator/
76 lines

Diff 1751

View Options

source/blender/freestyle/intern/python/Interface1D/BPy_Stroke.cpp

Loading...
View Options

source/blender/freestyle/intern/python/Iterator/BPy_StrokeVertexIterator.cpp

Loading...