Details

Reviewers

Maniphest Tasks

T41464: Material Boundary bug in Freestyle

Summary

commit rBeb8964fb7f19: Fix T41464: Material Boundary bug in Freestyle exposed a problem in the current freestyle iterator code.

to illustrate, the following code would previously raise a StopIteration during the final iteration:

stroke = get_test_stroke()
it = Interface0DIterator(stroke)

while not it.is_end:
    next(it)

really, the above code should work as expected, end normally and leave an iterator object for which (it.is_end == True). To this end an isLast method has been implemented for Interface0DIterator, which is true if and only if the iterator points to the final valid element of the iterator.

The is_end method has been overloaded to use isLast for BPy_StrokeVertex- and BPy_Interface0DIterator. This should fix the current problems and make iterators safer for future development.

additionally, isLast is now used in the next() method, which makes for some nicer code there.

Diff Detail

Event Timeline

Folkert de Vries (flokkievids) updated this revision to Diff 2469.Aug 21 2014, 10:29 PM

Folkert de Vries (flokkievids) retitled this revision from to Freestyle: further fixes for iterators.

Folkert de Vries (flokkievids) updated this object.

Folkert de Vries (flokkievids) added a reviewer: Tamito Kajiyama (kjym3).

Folkert de Vries (flokkievids) added a project: BF Blender.

Folkert de Vries (flokkievids) added a task: T41464: Material Boundary bug in Freestyle.

The .is_end property cannot be an alias of .is_last. If .is_end is an alias of .is_last, then:

while not it.is_end:
    v = it.object
    it.increment()

will mean

while not it.is_last:
    v = it.object
    it.increment()

hence the last element will be ignored. That is, the proposed changes are backward incompatible.

The problem comes from the fact that next(it) first increments the iterator and then looks up the pointed object. The conventional iteration in Freestyle, on the other hand, first looks up the object and then increments the iterator. The .is_end property works fine with the first-lookup-then-increment fashion, but does not work well with next(it).

A solution could be just to introduce .is_last and do not change .is_end.

yea, well that was stupid...

fixed it now. StrokeVertex- and Interface0DIterator now have a seperate isLast attribute. I've also 'upgraded' MaterialBoundaryUP0D to utilize it.

I've not yet found any other places where .is_last is useful. I've updated my regression testing script (should be up on github in a few days) and found no more errors.

To prevent (email) spam: Thanks as well for the comments on the cpp patch, will probably fix those tomorrow

Just an idea and not a request for code revision: how about naming the new property as .at_last instead of .is_last? This name appears more in line with the name of the internal flag at_first. It might be also useful to expose that flag as a property .at_first (just to keep the API symmetric, as I don't have immediate examples of use cases).

Tamito Kajiyama (kjym3) requested changes to this revision.Aug 24 2014, 4:03 PM

Tamito Kajiyama (kjym3) edited edge metadata.

Tamito Kajiyama (kjym3) added inline comments.

release/scripts/freestyle/modules/parameter_editor.py
821	This revision is not correct. Checking of .is_last must be done after the first call of `next()`. if it.is_begin: return False it.decrement() prev, v = it.object, next(it) if it.is_last: return False succ = next(it)
source/blender/freestyle/intern/python/Iterator/BPy_Interface0DIterator.cpp
123	I would swap the two conditions: if (self->if0D_it->isLast() \|\| self->if0D_it->isEnd()) {
source/blender/freestyle/intern/python/Iterator/BPy_StrokeVertexIterator.cpp
121	Same comment with the previous one: if (self->sv_it->isLast() \|\| self->sv_it->isEnd()) {

This revision now requires changes to proceed.Aug 24 2014, 4:03 PM

Thanks for the comments.

I think renaming to .at_last is a good idea. it.at_first indeed has no real purpose, since it is exactly the same as it.is_begin.
So, it.at_first really has no purpose in Python. not sure whether it'd be a good idea to include a useless attribute for symmetrical.

release/scripts/freestyle/modules/parameter_editor.py
821	well, say it points to element 6, then it.is_begin is false it.decrement() -> it now points to element 5 prev = it.object -> prev points to element 5 v = next(it) -> v points to element 6; it points to element 6 (remember here that next() increments first, and then does the lookup) so if it.is_last is true now, it would have been true at the top (and vice versa). So I believe my solution is correct (tests suggest this too).

It's true that .at_first is the same with .is_begin, and there's no point to have it. I leave the final decision to you.

release/scripts/freestyle/modules/parameter_editor.py
821	Yes, you're right. Thanks for the clarification!

fixes after code review.

I've decided not to include at_first. Instead I've added a comment to it.at_last saying that it.is_begin should be used instead.
swapped conditions in the iter_next function so their order is more logical.

I think this patch is finished now.

Tamito Kajiyama (kjym3) requested changes to this revision.Aug 29 2014, 3:48 AM

Tamito Kajiyama (kjym3) edited edge metadata.

Tamito Kajiyama (kjym3) added inline comments.

source/blender/freestyle/intern/view_map/Interface0D.h
312	`_iterator->copy()` returns a pointer to newly allocated memory. The copy needs to be deleted before exiting from the isLast() method.

This revision now requires changes to proceed.Aug 29 2014, 3:48 AM

some updates

completed the renaming
use a different method for atLast

not copying the Iterator object turns out to be twice as fast (from python), and is also shorter/cleaner/more logical than allocating a whole new object for a single check. The proposed method increments the iterator, does the check and then decrements it.

Due to the implementation StrokeVertexIterator its atLast method can't be const any more, but I see no problem here because the iterator is always returned in the same position, and its contents are not touched.

The patch looks okay now. Thanks for the contribution!

This revision is now accepted and ready to land.Aug 31 2014, 2:14 PM

Noticed that the test .blend file provided in T41464 do not render as expected.
The patch looks fine, so there is a related bug (I guess somewhere in CurvePointIterator).
I will keep the patch on hold until the regression is addressed.

The file seems expired, could you mail it to me/use pasteall?

flokkievids,
I have just uploaded the test file and attached it to T41464.

flokkievids,
I just identified the cause of the regression, that is, it.object cannot be mixed with next(it) at the moment since the former does not account for the internal at_first flag of the iterator. The bug fix is shown in an inline comment below.

Another solution could be to modify the getter function for it.object to always set the at_first flag to false. This way next(it) won't retrieve the first element when that element has already been retrieved by it.object.

I personally don't like the alternative solution because this kind of implicit side effects behind the dereference operation is difficult to manage from users' view point.

release/scripts/freestyle/modules/parameter_editor.py
821	This line of code has to be: prev, v, succ = next(it), next(it), next(it)

TK,

Thanks for the bug hunting. I guess it.object could use

if (self.py_it.isBegin())
    self.at_start = false;
...

But this indeed leads to unclear code. It might be better to use either it.increment() or next(it), and make sure they work on their own.

actually, Freestyle's iterators are already quite complex and this kind of corner cases and unexpected behavior should in my opinion be documented and explained.

We could add another internal flag to prevent increment/decrement and next() from being mixed. Let us keep this open for now, and I will proceed with the merge of the patch with the bug fix.

fix bug the python code (next(it) and it.object aren't always the same)

Tamito Kajiyama (kjym3) commandeered this revision.Sep 5 2014, 2:29 AM

Tamito Kajiyama (kjym3) edited reviewers, added: Folkert de Vries (flokkievids); removed: Tamito Kajiyama (kjym3).

This revision now requires review to proceed.Sep 5 2014, 2:29 AM

Closed by commit rBf6b4b1296125d43e94968709bc52d5f7a062734e.

Freestyle: further fixes for iterators
AbandonedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 2521

release/scripts/freestyle/modules/parameter_editor.py

source/blender/freestyle/intern/python/Iterator/BPy_Interface0DIterator.cpp

source/blender/freestyle/intern/python/Iterator/BPy_StrokeVertexIterator.cpp

source/blender/freestyle/intern/stroke/StrokeIterators.h

source/blender/freestyle/intern/view_map/Interface0D.h

Freestyle: further fixes for iteratorsAbandonedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 2521

release/scripts/freestyle/modules/parameter_editor.py

source/blender/freestyle/intern/python/Iterator/BPy_Interface0DIterator.cpp

source/blender/freestyle/intern/python/Iterator/BPy_StrokeVertexIterator.cpp

source/blender/freestyle/intern/stroke/StrokeIterators.h

source/blender/freestyle/intern/view_map/Interface0D.h

Freestyle: further fixes for iterators
AbandonedPublic
Actions

Revision Contents
Changeset List