This is looking very nice. I would like to add that this is an initial patch whose main goal is to create a stable basis for further development of freestyle. With these proposed changes we hope to solve one of the problems style module writing currently has: The immense unstructured list of imports. Some minor style improvements have been incorporated on the fly (use Vector instead of mathutils.Vector, use bool instead of int where appropriate, ect.)

some comments:

• There is a small mistake on line 1098: it says Vecor instead of Vector.
• Maybe the docstrings need some more standardization? in chainingiterators.py I used the docstring style that bpy_extras has, because some arguments needed some more detailed information, in shaders.py that wasn't necesary because most argument names speak for themselves. would it be worth the effort to make all docstrings comply to this 'official' standard, or does this not have too high a priority?

In general, +1 for the reorganization.

knit-picks
freestyle.chainingiterators - this name is a bit odd (too verbose), Python for example has itertools module.

Maybe:

• freestyle.iterators (you could have other iterators there too one day?)
• freestyle.itertools_chain (suggests you have other iterator tools in this module too, so maybe misleading).
• freestyle.chain_iterators
• freestyle.iter_chain (Extends nicely if you want to add iter_foo, iter_bar, and autocompletes nice and stays brief)

Note: freestyle.chainingiterators.py has names such as pySketchyChainingIterator, Suggest to remove the py prefix.

Weather an iterator is implimented in Python or CPython-API is an internal detail and shouldn't determine naming.

... unless its for internal use only (like we have for _cycles and _freestyle), but thats a different issue being a private API.

Thank you all for the prompt responses.

@Folkert de Vries (flokkievids)

The typo has been fixed in the local repo on my side. Docstrings could be further improved over time, possibly in a separate patch or task (as in T37452).

@Campbell Barton (campbellbarton)

The positive evaluation is very much appreciated.

freestyle.chainingiterators - this name is a bit odd (too verbose), Python for example has itertools module.

There are many different iterators in the Freestyle Python API, but only chaining iterators can be user-defined by subclassing freestyle.types.ChainingIterator. The freestyle.chainingiterators submodule is meant to be a collection of predefined chaining iterators (both those written in C++ and in Python). Other iterator classes are stored in the freestyle.types submodule instead.

I also thought the module name 'freestyle.chainingiterators' is a bit too verbose, but I did not come up with a better naming idea. The suggested names are much appreciated. I would keep this naming issue open and wait for other reviewers' comments.

Note: freestyle.chainingiterators.py has names such as pySketchyChainingIterator, Suggest to remove the py prefix.

Sergey also gave me the same suggestion during the code review toward the trunk merger (link). My response was the following:

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.

These redundant Python classes have been kept as coding examples for style module writers. I would keep the 'py' prefix for the rationale described above unless this is considered a merge stopper.

It might also be an idea to put the most trivial Python-defined freestyle types into a template (like those that already exist for bpy and osl/cycles). the "py-" prefix could then be removed for types that aren't also defined in C. For types defined in C and Python it might be an idea to only expose the C-defined one.

this would clean up the files the api uses and the ammount of imports, and the 'educational' types would all be grouped and more easy to find.

Thanks @Campbell Barton (campbellbarton) for the acceptance.

Just pushed 6498b96, 54e9016, 8ab3cf1, e847328, 0b22827, and 4683ac3 (separately instead of a single commit, for record of patch revisions by @Folkert de Vries (flokkievids) and me through T37565).