Freestyle Python API: Updates and speedups for the parameter editor
ClosedPublic
Actions

Authored by Folkert de Vries (flokkievids) on Jun 28 2014, 4:44 PM.

Details

Reviewers

Tamito Kajiyama (kjym3)

Summary

In addition to D319, this patch updates the parameter editor, the UI of Freestyle.

Using new API functionality and experience gained in making D319, This patch provides a quite noticable speedup for commonly-used Freestyle linestyle modifiers.

As this patch touches a lot of code (and mainly the foundations) It is likely that mistakes are made.
I've made a regression suite for the Freestyle Python API, but testing with scenes used in production is very much appreciated.

Diff Detail

Event Timeline

Big Update

This is a bit of an experiment: I've used namedtuples to store multi-component information (like a range, which has a minimum and maximum value, and a difference between max and min). I quite like the way it looks and works, and it isn't any slower (I tested this thoroughly)
I've removed the 'dunder' before names (like self.__func); it is quite ugly and serves no purpose.
removed the empty BaseColorShader
in the C iterator code, swapped RuntimeError for the more descriptive StopIteration
implemented CurvePoint.fedge. A small optimization (15%, but that's 1/100 of a second), but it also looks much nicer and seems more logical/practical.
I went over the code with a pep8 checker, fixing some small things (whitespace, mainly)

Again some pretty nice speed improvements were made, mainly by optimizing functions in utils.py

Many thanks for the continued effort in updating and optimizing the Freestyle Python API implementation. Please find inlined my comments to the code changes.

release/scripts/freestyle/modules/freestyle/functions.py
105 ↗	(On Diff #2046)	Is there any reason to comment this assertion?
release/scripts/freestyle/modules/freestyle/predicates.py
163	I guess you meant `len(predicates)` at the end of the error message.
255	Concerning the comment, I guess `sum(1 for ... if ...)` is faster because summation only takes place for those items that hold the condition (i.e., the smaller the number of items for which the condition is true is, the faster the new expression is).
release/scripts/freestyle/modules/freestyle/utils.py
129	Is there any advantage in using .incremented() ? If not, always using tee() seems fine.
283	The slower version can be removed.
release/scripts/freestyle/modules/parameter_editor.py
114	I guess we could unify these named tuples by a more general name, e.g. 'Bound' or 'Limit'.
169	I believe `self.mapping_type` is not necessary.
228	You can just remove the old `blend_thickness()` (and optionally rename `blend_set_thickness()` to `blend_thickness()`). There is no point to keep the "blend only" one.
447	I am not comfortable with the changes here. I would keep `iter_material_color()` (or `get_material_color()` in line with `get_material_value()` as you proposed) for reusability and self-documentation. If speed really matters here, we still have an option to rewrite Python iterators in C/C++.
514	We might ask Campbell to improve Vector.orthogonal() to accept a 2D vector by reminding him that BLI_mathutils has `ortho_v2_v2()` in C.
515	It looks like normalization is missing.
537	We need to store normals because we modify vertex locations which in turn alter normals.
601	I would add a line like `normals = tuple(stroke_normal(stroke))` before the `for` loop and leave comments highlighting the need of storing copies of normals.
707	The original expressions look easier to read and still I don't expect performance loss with them.
751	This `for` loop does not seem correct. Please double-check visual results. The use of `cycle()` may still be relevant, e.g.: pattern_cycle = cycle(self._pattern) pattern = next(pattern_cycle) visible = True for svert in it: pos = it.t # curvilinear abscissa if pos - start + sampling > pattern: start = pos pattern = next(pattern_cycle) visible = not visible
855	Using `.is_begin` and `.is_end` would better document boundary conditions. Please consider revising the code here.

Hi TK, thanks for the review

I was away for a couple of days, so I'm sorry for the somewhat delayed response. I've added some comments. an updated revision against master will probably be ready tomorrow.

btw, did you ask campbell to look into the orthogonal method for 2d-vectors? (I may have missed that)

release/scripts/freestyle/modules/freestyle/functions.py
105 ↗	(On Diff #2046)	well, is this assertion needed? I've added a comment to describe the input it expects, and if an incorrect object is given, line 106 will raise a correct error (AttributeError, I think) The thing here is that every object is checked, and this function is called many times in loops. (isinstance also needs to find out that StrokeVertex is a subclass of Curvepoint for every call), so removing this assertion has a, albeit small, advantage speedwise.
release/scripts/freestyle/modules/freestyle/utils.py
129	as I understand it, tee stores (at least parts of) the iterators objects in memory. tee itself has some overhead as well. Incremented() gives a new iterator, no copying is done. Tests also indicated that it is slightly faster. if the above is true, it is also more efficient memory-wise. (this is also in agreement with Alex Gaynor's Fast python, slow python talk, in which he states that the best tool for the job is almost always the most specific one.)
release/scripts/freestyle/modules/parameter_editor.py
515	it isn't, the division by l (the Vector's length) takes care of that.
707	use of indices and range in this manner is generally discouraged and considered unpythonic. the problem here is that pairwise doesn't consider the step value. I'll look into it further (maybe itertools has something for this)
751	You're right.

Hi flokkievids,

Added a few replies to some of your comments. I have just submitted a patch for extending Vector.orthogonal() to accept a 2D vector (D668). Let's see.

release/scripts/freestyle/modules/freestyle/functions.py
105 ↗	(On Diff #2046)	Ok, the rationale sounds fine to me.
release/scripts/freestyle/modules/freestyle/utils.py
129	How about using `hasattr(itrable, "incremented")` then instead of listing class names? Another option is just to assume that `iterable` has the incremented() method and catch AttributeError.
release/scripts/freestyle/modules/parameter_editor.py
515	You are right.

updates:

optimized the pairwise() function. tests with the timeit module show that this is the fastest
added get_test_stroke() to freestyle.utils. I found that it is often quite handy to have a constant stroke object for testing

(It seems unnecessarily cumbersome to create a stroke object, are there other ways?)

use a BoundedProperty namedtuple primitive (is this a good name?)
accounted for the new freestyle line materials

I used default function arguments in a few places to optimize (like def(a={type1, type2}): ... , this causes the object to be created only once at compile time instead of on every call at runtime)

still quite a few comments (and commented out bits). if you are OK with the functionallity/style I'll clean it up.

Thanks for the code updates! The patch looks fine and almost ready to land. A few remarks are mostly for your interest in performance.

release/scripts/freestyle/modules/freestyle/utils.py
166	Just for curiosity, do you have a performance difference if division by `normfac` is replaced by a multiplication by `1/normfac`? normfac = 1.0 / normfac for svert in stroke: ... yield (svert, (distance - range_min) * normfac)
release/scripts/freestyle/modules/parameter_editor.py
209	Should be `self.position = position`.
519	It might worth for speed to compute the coefficient `1 / self.wavelength * 2 * pi` outside the `for` loop.

This revision is now accepted and ready to land.Jul 23 2014, 3:01 AM

thanks for the input, cleaning up now.

release/scripts/freestyle/modules/freestyle/utils.py
166	with 100K iterations, division is .005 seconds slower than multiplication. Not worth it in my opinion.
release/scripts/freestyle/modules/parameter_editor.py
519	speedwise the difference is minimal (0.03 seconds on 10K iterations). it seems that the main speed advantage comes from interning the value (cutting out the self.* lookup in the loop) but because the loop itself gets a little bit more readable this way as well I think it's good to incorporate this change.

some final improvements:

use Vector.orthogonal for 2d vectors
use the new CurvePoint.fedge method in the python code
moved BoundedProperty to freestyle.utils
some more cleanup

most shaders have a speed increase of about 10 to 30% (we're talking tenths of seconds here, though). Furthermore I think the code is in pretty good shape now, ready for future expansion.

closed with commit rBb408d8af31c9

Tamito Kajiyama (kjym3) mentioned this in T42508: Freestyle method join_upred_predicates missing from rel. 2.71 to 2.72b.Nov 7 2014, 3:03 PM

Tamito Kajiyama (kjym3) mentioned this in rBS1819fa2b5aab: A follow-up to Patch D623: minor code updates and style cleanup..Nov 20 2014, 9:53 AM