Page MenuHome

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

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

Details

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

Folkert de Vries (flokkievids) updated this revision to Unknown Object (????).Jul 5 2014, 1:33 PM

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

Tamito Kajiyama (kjym3) requested changes to this revision.Jul 9 2014, 7:18 AM

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

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

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

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.

Folkert de Vries (flokkievids) updated this revision to Diff 2186.

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.

Tamito Kajiyama (kjym3) accepted this revision.

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
203–206

Should be self.position = position.

503

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
503

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.

Folkert de Vries (flokkievids) updated this revision to Diff 2199.

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.