Page MenuHome

Freestyle Python API reorganization in line with the bpy modules
ClosedPublic

Authored by Tamito Kajiyama (kjym3) on Jan 1 2014, 9:29 AM.

Details

Summary

This patch set reorganizes the modules of the Freestyle Python API in line with the bpy module and its submodules, now that some breakage of backwards compatibility is allowed with the upcoming 2.70 release. The proposed changes aim to address the following issues observed in the present Freestyle implementation:

  1. Freestyle API constructs for style module writing in Python are not logically organized. The API modules are partly organized in terms of implementation languages (C and Python) and partly based on implemented roles (predicates, chaining iterators, and so on).
  2. All Python scripts for Freestyle, i.e., both API modules and application programs (style modules), are stored in the same 'release/scripts/freestyle/style_modules' directory. This makes it difficult for new users to find style modules.

The new Python module directory structure is proposed as follows:

  • release/scripts/freestyle/modules - Freestyle API modules
  • release/scripts/freestyle/styles - predefined style modules

In Python, the Freestyle API modules are reorganized as follows:

  • freestyle - top-level package just containing the following submodules.
  • freestyle.types - classes for core data structures, and base classes for user-defined stylization rules (i.e., chaining iterators, predicates, functions, and shaders).
  • freestyle.chainingiterators - predefined chaining iterators.
  • freestyle.predicates - predefined 0D and 1D predicates.
  • freestyle.functions - predefined 0D and 1D functions.
  • freestyle.shaders - predefined stroke shaders.
  • freestyle.utils - utility functions for style module writing.

The effects of the proposed changes are illustrated below by taking the japanese_bigbrush.py style module as example. With the present API organization (before the proposed changes), the import statements in the beginning of the style module have been written as follows:

from freestyle import BezierCurveShader, ChainSilhouetteIterator, ConstantColorShader, \
    ConstantThicknessShader, IntegrationType, Operators, QuantitativeInvisibilityUP1D, \
    SamplingShader, TextureAssignerShader, TipRemoverShader
from Functions0D import pyInverseCurvature2DAngleF0D
from PredicatesB1D import pyLengthBP1D
from PredicatesU0D import pyParameterUP0D
from PredicatesU1D import pyDensityUP1D, pyHigherLengthUP1D, pyHigherNumberOfTurnsUP1D
from logical_operators import NotUP1D
from shaders import pyNonLinearVaryingThicknessShader, pySamplingShader

With the proposed API changes, these import statements will be written as follows:

from freestyle.chainingiterators import ChainSilhouetteIterator
from freestyle.functions import pyInverseCurvature2DAngleF0D
from freestyle.predicates import (
    NotUP1D,
    QuantitativeInvisibilityUP1D,
    pyDensityUP1D,
    pyHigherLengthUP1D,
    pyHigherNumberOfTurnsUP1D,
    pyLengthBP1D,
    pyParameterUP0D,
    )
from freestyle.shaders import (
    BezierCurveShader,
    ConstantColorShader,
    ConstantThicknessShader,
    SamplingShader,
    TextureAssignerShader,
    TipRemoverShader,
    pyNonLinearVaryingThicknessShader,
    pySamplingShader,
    )
from freestyle.types import IntegrationType, Operators

From the applicative perspective, the new organization will be much easier for style module writers to find relevant Python constructs from the freestyle.* submodules. It is remarked that there is no functional difference (no visual effects in rendering results) between the present and new module organizations.

Acknowledgement: flokkievids (Folkert Vries) for careful discussions, code review and patches through the task T37565.

Diff Detail

Event Timeline

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