Powerstroke LPE renders with spurious circles (rev >= 14226)

[Parcly Taxel]

[Trisquel 7.0, Inkscape trunk r14236] The powerstroke LPE now draws spurious (large arcs of) circles everywhere along a path, where there were none before. In the attached file, these circles appear and disappear every time the path is moved. Result of the 2geom update in r14226.

[su_v (suv-lp)]

Reproduced with Inkscape 0.91+devel r14236 on OS X 10.7.5.

Based on tests with archived builds:
- not reproduced with rev <= 14224,
- reproduced with rev >= r14228;
likely a regression introduced with the 2geom update in r14226:
http://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/revision/14226

[su_v (suv-lp)]

AFAICT also affects extrapolated arc joins in 'Taper Stroke' and 'Join Type' LPE:

[jazzynico (jazzynico)]

Also reproduced on Windows XP (32 bit) with Inkscape trunk rev. 14237.

[Alvin Penner (apenner)]

as far as I can tell, the source of the problem is the two following lines, lines 436 and 437 in file lpe-powerstroke.cpp

Geom::EllipticalArc *arc0 = circle1.arc(B[prev_i].at1(), 0.5*(B[prev_i].at1()+sol), sol);
Geom::EllipticalArc *arc1 = circle2.arc(sol, 0.5*(sol+B[i].at0()), B[i].at0());

the problem with these calls is that the three points in the call appear to be collinear. This should never be allowed to happen. Attached is some documentation from the file ellipse.cpp, routine Ellipse::arc, line 205:

    // Determination of large arc flag:
    // The arc is larger than half of the ellipse if the inner point
    // is on the same side of the line going from the initial
    // to the final point as the center of the ellipse

According to this documentation, the curvature of the three points in the call will be used to determine the flags, and the flags will be indeterminate if the three points are collinear. (Excellent documentation, by the way!)

[Parcly Taxel (parclytaxel-deactivatedaccount)]

I tried my hand at translating the code found in src/helper/geom-pathstroke.cpp by Liam White (https://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/view/head:/src/helper/geom-pathstroke.cpp#L178) into the main powerstroke LPE; the result is attached. Let me know what you think of it.

(Seriously, why hasn't the requisite amount of refactoring been done?)

[Liam P. White (liampwhite)]

Well, I didn't mean to transcribe the code, I mean to expose the path_outline API to other things.

[Liam P. White (liampwhite)]

The new arc generation code causes this particular regression.

Reverting it fixes it, but likely causes a new regression elsewhere. Will need 2geom developers' opinions on how to properly fix this.

[Parcly Taxel (parclytaxel-deactivatedaccount)]

Oh come on, not all cases have been handled! Look at the attached file; there should be a mitre at the corner but it isn't there.

[Liam P. White (liampwhite)]

That is a different bug; please avoid cluttering the comments of this report with that at least until the arcing bug Alvin and I mentioned is solved.

[Alvin Penner (apenner)]

attached is a proposed patch for this bug, for discussion purposes. This has been tested successfully on rev 14237.
The patch will refuse to change the large_arc_flag if the three incoming points are too close to being collinear, and will instead use the default value of false. The test for collinearity is dimensionless and is equivalent to measuring the angle in radians.

In practice there are two distinct cases that can occur. The first case is the case where the incoming three points are actually on the circle and are collinear, meaning the radius is infinite. In this case, setting this flag to be false is reasonable. The second case is the case where the user has, for convenience, set the three incoming points to be collinear even though they are not. This is equivalent to arbitrarily forcing the flag to be false, if the current proposal is implemented. The file live_effects\lpe-powerstroke.cpp contains two instances of this type of usage, and the file helper\geom-pathstroke.cpp contains about seven instances of this type of call. In all of these cases, as far as I can tell, the intent of the function call was to finish a join of two segments, so the decision to force the join to use the small arc option seems like a perfectly justifiable choice. In any event, this is not a 2geom issue, it is a decision the original programmer must make outside 2geom.

comments?

[Alvin Penner (apenner)]

 attached is an alternative proposal, which could be used instead of comment 11. It is understood that there may be some reluctance to use comment 11 because 2geom typically does not use numerical tolerances to resolve round-off issues. Also it is quite clear that the tolerance used there, EPSILON, is much larger than necessary to resolve the issue, since the typical round-off error in these calculations is about 1E-15.
 so the alternative proposal is to modify lpe-powerstroke.cpp instead of modifying 2geom. The attached patch contains calls from lpe-powerstroke to Ellipse.arc which solves the bug by forcing the large arc flag to always be false. The syntax of these calls is identical to that proposed in comment 6 above. The call is done in such a way that the variable sdist_inner = -sdist_c in the file ellipse.cpp. In effect, the inner point is made to be a reflection of the center point, reflected about the versor. This type of call should only be made in those cases where it has been determined apriori that the large flag must be false, no matter what. If this proposal is implemented then it would likely be necessary to also change all instances of this type of call in the file geom-pathstroke.cpp as well.
 personally, I think this is a better solution than comment 11.

[Parcly Taxel (parclytaxel-deactivatedaccount)]

I actually thought of the exact same solution as that in comment #12, based on a geometric interpretation that places the middle point at the fourth vertex of a rhombus. It performs very fast and is open to refactoring.

[Krzysztof Kosinski (tweenk)]

The intent of the new 2Geom implementation or arc() is to avoid the use of trigonometric functions. The differences between the old and new implementations are shown in the attached picture - for given two endpoints and a circle, it shows where the middle must be in order for the given arc to be chosen.

[Krzysztof Kosinski (tweenk)]

I think the solution in comment #12 is better, though on the other hand calling arc() with these three parameters should give the expected result. I think I found a way to preserve the old behavior while using only subtractions and divisions, will report soon.

[Krzysztof Kosinski (tweenk)]

Okay, I have the old behavior working correctly without the use of trig functions and while evaluating at most 1 more cross product than before - committed to lib2geom as r2420. Will do a sync to Inkscape now.

[Krzysztof Kosinski (tweenk)]

Try Inkscape r14247.

[Krzysztof Kosinski (tweenk)]

The joins are now stable, but the effect seems really off. For example, the round joins are nowhere near correct. :/

[Alvin Penner (apenner)]

as far as I can tell, the problem with the rounded join also existed previously, before rev 14247

[Alvin Penner (apenner)]

reported as a separate Bug 1475097

[Alvin Penner (apenner)]

thanks, Krzysztof, original bug report confirmed fixed in rev 14248