Same question as in line 360.

Same comment here as in AndUP1D.

Same comment here as in OrUP1D.

Please, consider keeping the original order of classes to make code review easier.

Casting to float is not necessary in Python 3. An integer divided by another gives a float value.

Just a thought: This shader relies on vertex index to determine new stroke thickness. That would work fine if the vertex intervals are more and less the same; otherwise nonlinear thickness variation is expected. We might want to rely on the u parameter of the stroke instead, to assure smooth thickness changes.

Redundant casting to float: c should already be a float.

The constant is 0.01 in the original code.

How about using the notation stroke[0], stroke[-1] and so on here? That makes the creation of iterators unnecessary.

Using Iterface1D.vertices_begin() may further simplify the updated code (not tested):

it = stroke.vertices_begin()
for svert in it:
    svert.point += self._normal(it) * self._lambda * self._angle(it)

It is noted that the .object property of Interface0DIterator gives a StrokeVertex instance if the iterator is created from Stroke. Python does not have a dynamic_cast operator available in C++, so casting to StrokeVertex is automatically done when the .object property value is returned.

No return value is required.

Using stroke[0] and stroke[-1] would be easier.

An explicit int comparison qi == 0 is preferable to me.

A shorter form tuple(it) could be used here instead of (svert for svert in it), but maybe the former appears too cryptic. I prefer the latter.

Is this strokeAsBezierShader class meant to be here?

These 3 lines look like testing stuff. Please, remove them if they are unnecessary for a release.

Originally these iter_ helper functions were returning an iterator and extra object(s), because in the caller side, one might need to call 0D/1D functions which take an Interface0DIterator. It is true that returning a stroke vertex instead of an iterator saves one line of code (i.e., sv = it.object), but at the cost of creating an iterator in parallel to the iter_ loop on the caller side if it is necessary to call 0D/1D functions. To me returning an iterator instead of a stroke vertex seems a way to go since it will help make user code simple and straightforward although the extra line sv = it.object might appear clumsy.

The function name could be rgb_to_intensity or rgb_to_grayscale.

There are many named RGB-to-grayscale conversion methods (see http://en.wikipedia.org/wiki/Grayscale for more info), but the formula used here seems not any of them. I don't remember from where I took it...

This helper function seems not to work in the way the name suggests, since prev and svert are not being updated in the for loop.

How about a further simplification:

v1 = min(v1, fac * v2)

The call of the .copy() method seems redundant:

point = -sv.point_3d.normalized()

The whole updates of for iterations below can be revised after the iterator API revisions in D545, so I leave them for another round of review in the feature.

This added condition seems wrong. If self._length_min is set to a number, then the length checking should be performed even if self._length_max is unset (and vice versa).

Same comment as in line 794.

I prefer either if len(pattern) > 0 or if pattern.

In what way? I didn't really test this but it seems to me that any() made for situations exactly like this. I would assume that it has also been optimized (memory usage ect.)

It shouldn't be slower and the code shows exactly what you want to achieve.

tests with imagemagick over the past two months have not once given an error here. I will however look into it.

We spoke about this earlier. there is a chance the iterator will turn back on itself, creating an infinite loop. This eliminates the chance of that happening.

it is not correct. probably forgot that while testing. using self._func(inter) will however result in an infinite loop printing "this viewmap does not exist", alike most, if not all, ...Viewmap...-methods.

because of this i've ommited it from my tests.

Again, unable to test, executing this function enters an infinite loop printing "this steerable viewmap level does not exist"

thought about this aswell. speedwise, it should barely matter (may be faster even, albeit mininally)

It would however make regression testing inaccurate. I propose to stick to the old approach for now, verify that all works well. Then we can implement the use of the u parameter.

in python, explicit is said to be better than implicit.
I like this better than the empty 'return'. it is explicit about what it returns.

actually, verticesToRemove is a generator object here. altough the effect in this case may be minimal, I think it is a good practice to use generators when possible.

No. this was an experiment with your similar shader. we can look into inclusion of such a shader after this patch gets applied.

You are right, it looks like I was confused with all().

Thanks for the reminder. The code revision looks okay.

I agree that explicit is better than implicit in Python. In this case, however, the shade() method is not supposed to return a value. Explicitly returning None appears weird to me.

I agree with you that using generators when possible is a good practice.

Another concern I got is whether using a generator here is safe or not. The generator (svert for svert in it) keeps a pointer to a StrokeVertex in a sequence of objects, and does not keep all references of the StrokeVertex objects to be removed. This situation is similar to a common coding error in Python where items are removed from a list within a loop over the list item:

for x in somelist:
    somelist.remove(x)

Since Python programmers are familiar with this common mistake, it would appear straightforward that verticesToRemove is a genuine list or tuple.

I liked the expression sum(1 for ve in it if func(it) > self._a) in a previous revision. Is there any reason the new form is preferable?

These nested for loops are difficult to read... Why not iterate self._nbIter-1 times and creating a new Interface0DIterator instance inside the loop?

for i in range(1, self._nbIter):
    it = Interface0DIterator(stroke)
    for svert in it: # this works in Python
        svert.point += self._normal(it) * self._lambda * self._angle(it)

This assignment seems redundant.

Please, remove this debug print.

Just a thought: We could define a local function (predicate) and use filter built-in function to make this generator-based expression:

mate_id = vertex.get_mate(self.current_edge).id

def is_mate(ve):
    return ve.id == mate_id

return next(filter(is_mate, it), None)

The predicate can be an unnamed lambda function in this case, there are similar cases in other chaining iterators that could employ the same idiom. Regular use of the same idiom is preferable.

Minor comment: not result and bpy.app.debug_freestyle would a bit faster.

Should be TVertex. Please, double-check all chaining iterators for this typo.

Should be if not self._predicates:.

Same as the comment in line 482.

This comment is not always true, since pyNatureBP1D is mostly meant to be used as an argument of an API function that takes a 1D binary predicate.

c = i / n

c = i / n

Now this can be it = Interface0DIterator(stroke).

c = 1.0 - 2.0 * abs(i / n - 0.5)

Please, consider revising these nested for loops as suggested in https://developer.blender.org/D319?id=1881#inline-1886.

it2 = it.incremented()

I believe verticesToRemove = tuple(svert for svert in it) is safer (see https://developer.blender.org/D319?id=1502#inline-1884 ).

I prefer using a generator expression over an anonymous function and a barely-used built-in.
I'll look into a broader usage of the idiom.

Just interested here: would it really, significantly matter?
Adjusted in rev. 7

I hadn't noticed yet. a change of test scene revealed this problem. fixed it now

No, not really. My comment was mostly for consistency (in other cases, we have put bpy.app.debug_freestyle at the end of conditions).

It doesn't, sadly. it raises:
AttributeError: Vector addition: (int + Vector) invalid type for this operation
including the empty Vector as a starting point makes this work.

I'm still unsure about the name. do you have a prefference?

Ok, sum() takes two arguments and the second is the initial value, which in this case should be Vector((0, 0)). No problem then.

I don't really have much preference: rgb_to_grayscale, rgb_to_intensity, chromatize, or whatever makes sense. One option is rgb_to_bw() since the same formula appears in source/blender/blenlib/intern/math_color_inline.c.

I saw this helper function also in your regression test suite. Do you actually want to include this in the freestyle.utils module?

took that one

I think it's not needed. (pretty nice recipe though)