trunk: ellipse tool flips segment to opposite half at angle 180° (rev >= 12594)

[su_v]

Steps to reproduce:
1) launch inkscape (new default prefs, default doc)
2) draw a circle
3) grab the rounded knot (arc handle) and drag it with 'Ctrl' pressed clock-wise, keeping the pointer well outside the circle (-> to draw a segment, not an arc)

Expected result:
The filled area of the segment correlates with the position of the arc handle.

Actual result:
If the arc spans 180° horizontally (or vertically), the filled area of the segment "flips" to the opposite half of the circle, and "flips" back as soon as the arc is ≠ 180° (see attached animated gif for a demonstration).

First encountered with r12602 on OS X 10.7.5, reproduced on Ubuntu 13.04 (VM, 64bit) with r12594 (PPA).

Based on tests with archived builds on OS X:
- not reproduced with r12593 and earlier revisions
- reproduced with r12594 and later,
this glitch seems to have been introduced in
Revision 12594: More refactoring, creation of member methods, ...
<http://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/revision/12594>

[Markus Engel (engelmarkus)]

Hm, look at 2geom/ray.h. The function "make_angle_bisector_ray" returns a ray with (0, 0) as its versor when you pass it two rays that start in (0, 0) and go in the opposite direction. I think this is not valid as this returned ray is not defined.
Try the attached patch; although that's not the "clean" solution to this. This should rather be fixed in 2geom directly.

[su_v (suv-lp)]

Patch tested successfully with r12602 on OS X 10.7.5 (from a user's perspective).

On 2013-09-27 19:04 +0200, Markus Engel wrote:
> Hm, look at 2geom/ray.h. The function "make_angle_bisector_ray"
> returns a ray with (0, 0) as its versor when you pass it two rays
> that start in (0, 0) and go in the opposite direction. I think this
> is not valid as this returned ray is not defined.
>
> Try the attached patch; although that's not the "clean" solution to
> this. This should rather be fixed in 2geom directly.

@Johan - maybe you can comment on this?

[Johan Engelen (johanengelen)]

Comment on patch: it contains floating point comparison! Use Geom::are_near for comparing floats.
If I understand the problem correctly, the code needs to check for versor being close to (0,0) ("middle_point.isZero()", in 2geom this does floating point comparison, but that's a bug I believe, so we can fix that too), not only the X-coord.

The bisector of two opposing rays is perhaps not so well defined (up or down?), so not sure if "make_angle_bisector_ray" is at fault there. But I don't think one should rely on the return value being (0,0) for opposing rays. You can go ahead and change it in 2geom, so that it has a defined orientation for opposing rays. (please document it for that function)

[Markus Engel (engelmarkus)]

I know that this patch is horrible, it's just there to show what the problem is ;) . I wouldn't use "==" in production code...
I think the angle bisector is defined, I mean, an angle between two points is defined as well by the order in which they are passed (usually ccw, except for 2geom, where this is cw for the inverted y axis).
So angle_bisector(start, end) means to me "from start, go in clockwise direction to end and cut that angle in two equally sized parts".

[Markus Engel (engelmarkus)]

Now this seems better, try this patch instead of the old one above.

@Johan: What do you think?

[Markus Engel (engelmarkus)]

I committed the changes to 2geom to fix this bug to the 2geom trunk. So with the next sync this will be fixed.
http://bazaar.launchpad.net/~lib2geom-hackers/lib2geom/trunk/revision/2128

[su_v (suv-lp)]

AFAICT this bug also breaks rendering of objects created with
- 'Extensions > Render > Wireframe Sphere'
(use '[x] Hide lines behind the sphere')

[su_v (suv-lp)]

@Markus - could you test e.g. the attached file? With r12661+ray.patch applied, there's still one ellipse/arc missing compared to revision <= 12593 and stable 0.48.x.

[Markus Engel (engelmarkus)]

The broken wireframe sphere has got to do with the big epsilon 2geom uses in its calculations. The attached patch would fix this. I'll ask on the mailing list why epsilon is so big.

[su_v (suv-lp)]

Patch tested unsuccessfully with r12712 on OS X 10.7.5: ellipses and circles are "randomly" broken (some do not render visibly at all and create a degenerated path when converted to path).

Steps to reproduce:
1) launch patched inkscape with fresh (new) preferences
2) apply 'Extensions > Render > Wireframe Sphere' with default values:
--> missing wireframes (AFAICT none of the closed ellipses and circles renders visibly)
3) draw an ellipse
--> works as expected
4) apply 'Extensions > Render > Wireframe Sphere' with '[x] Hide line behind the sphere'
--> only some of the arcs render (closed ellipses are missing)
5) select one of the degenerated ellipses of the wireframe and switch to the ellipse tool:
--> the ellipse handles are in the expected position
6) leave group, and draw a new ellipse:
--> renders invisible (and converts to a 2-node path ('Shift+Ctrl+C'))
7) after a click on 'Make the shape a whole ellipse', new ellipse/circles are drawn again as expected.

[Markus Engel (engelmarkus)]

This seems to be a problem now of finding the right value, 1e-9 works for me. Could you try this as well?

[su_v (suv-lp)]

> 1e-9 works for me. Could you try this as well?

Confirmed - I also get better results with 1e-9. Any other objects/actions I should test which could be affected (as side-effect) by this value?

[Markus Engel (engelmarkus)]

Well, this epsilon is used several times in 2geom, are_near, are_same, are_parallel, Point::is_normalized, Rect::hasZeroArea, ...
Everywhere floating-point comparisons are done. These were hardcoded in sp-ellipse (e. g. "abs(x - y) <= 1e-9" for "are_same") and I replaced them with the appropriate functions.
I'm not sure what the "right" value is, this depends on the magnitude of the errors that may appear in calculations.
1e-9 and 1e-18 seem to be common values, they appear rather often in Inkscape's source code, if you do a quick grep. Although I think the latter is too small.

[Bryce Harrington (bryce)]

Thanks for your work on this bug, Markus.

Have you had a chance to discuss the epsilon value change with the 2geom folks? Meanwhile, I think applying that to our copy of 2geom would be worthwhile to test if there are any further side effects to it.

The 2geom changes mentioned in comment #7 are now mainline in Inkscape. So I think once the epsilon change is resolved we can close this bug?

[Markus Engel (engelmarkus)]

Well, I wrote a mail about that some time ago:

===========
On 21-Oct-2013 17:06, Markus Engel wrote:
> Hi,
>
> in 2geom/coord.h there is an epsilon defined as 1e-5. This is used in
> many
> ways, e. g. in Geom::are_near, .
>
> Now I'm wondering why this value is that big.

Probably related to the precision limit of a small series of 32 bit
float operations being around 1e-6. 2geom may use all doubles (I did
not look), but probably there are applications that pass it 32 bit
floats that are promoted to doubles, which does not provide more
precision than the original value. For a little measure of safety to
handle precision loss in longish calculations they probably went up to
1e-5.

> Is there any reason why "1e-18" has been commented out?

Way too much precision for 32 bit float, would be my guess.

I ran into this issue a couple of decades ago in a simple problem: given
an arbitrarily oriented unit vector and a radius A is which side of a
plane defined by a point and another unit vector does A*vector lie? All
of the values were regular float, not double. By the time all the math
was done the final comparison was fuzzy to around 1e-6.

Regards,

David Mathog
===========

Actually, changing the epsilon seems to fix this problem, but I'm not sure that there are no other side effects.
We could simply give it a try and remember that change when we hit any bugs.

Sorry for my late reply but currently I'm quite busy.

[su_v (suv-lp)]

On 2014-01-17 23:37 +0100, Markus Engel wrote:
> (…) but I'm not sure that there are no other side effects.

It seems that I possibly ran into one last night when testing a larger patch for EMF support (reproduced with two different builds): see comments #9-10 and 15 in bug #1263242. The mentioned build which has a modified value for EPSILON isn't one I regularly use though, thus no other reports from me right now wrt to possible side-effects.

[Markus Engel (engelmarkus)]

It seems this bug has been fixed in r13327.
<http://bazaar.launchpad.net/~inkscape.dev/inkscape/trunk/revision/13327>

Can you confirm this?

[su_v (suv-lp)]

On 2014-07-20 01:19 +0100, Markus Engel wrote:
> It seems this bug has been fixed in r13327.

The test file from comment #9 still renders unexpectedly:
- r13325 missing arc as described
- r13328, r13454 incorrect arcs (png export attached)

[Alvin Penner (apenner)]

>> The test file from comment #9 still renders unexpectedly

confirmed on Inkscape rev 13446.
the problem is somehow related to the size of rx, relative to ry. The original file has very small rx, as in the attached (edited) snippet.

<path
    id="path846"
    sodipodi:end="4.712389"
    sodipodi:start="1.5707963"
    sodipodi:open="true"
    sodipodi:type="arc"
    sodipodi:ry="150"
    sodipodi:rx="0.001" />

If I increase rx from .001 to .01, the problem disappears, as in the attached svg file.

[Alvin Penner (apenner)]

the problem in this case can be traced to the file 2geom\affine.cpp, in the routine Affine::inverse().
This routine is being fed the 2x2 matrix:

0.001 0.0
0.0 150.0

and it is refusing to calculate the inverse, returning the identity instead.
The line of code that is doing the rejecting is Line 372:

if (!rel_error_bound(determ, mx*mx)) {

If I override this line and force the determinant to be calculated regardless, then the rendering works normally.
A reduced test case is attached, to make testing easier.

Will post a question on the 2geom list, since this issue cannot be resolved here.

[Alvin Penner (apenner)]

a fix has been committed to rev 2215 of lib2geom. A slight change was made in the test for zero determinant when inverting a matrix.
The fix can be tested by copying the file Affine.cpp from lib2geom to Inkscape.
http://bazaar.launchpad.net/~lib2geom-hackers/lib2geom/trunk/revision/2215

[su_v (suv-lp)]

Fixed in trunk with the latest 2geom update in rev 14226.