SVG Marker orientation=auto has incorrect behavior around 0°

• Component: SVG
• Status: RESOLVED FIXED

[Tom MacWright]

In the linked testcase, markers flip between correct and incorrect orientations when they go near zero degrees. This behavior is against the SVG spec and deviates from Firefox, Opera, and other SVG-supporting browsers.

[Philip Rogers]

Confirmed. Somebody probably forgot their trigonometry.

[Philip Rogers]

Created attachment 192655 [details]
Testcase

[Philip Rogers]

Created attachment 192658 [details]
Correct bisector calulcation

This bug turned out to be an interesting one: the average of two angles is not (angle a + angle b) / 2.

E.g., a=90deg, b=-180deg: The bisector should be 135deg, not -45deg.

This patch is not yet ready for review because I think we can do this faster.

[Philip Rogers]

Created attachment 193015 [details]
Correct bisector calculation for markers

[Philip Rogers]

Created attachment 193052 [details]
Correct bisector calculation for markers (2)

Updated patch without slow normalize() calls. Should be ready for review once it passes EWS.

[Eric Seidel (no email)]

Comment on attachment 193052 [details]
Correct bisector calculation for markers (2)

View in context: https://bugs.webkit.org/attachment.cgi?id=193052&action=review

> Source/WebCore/ChangeLog:12
> +        Angles cannot be averaged this way! Consider in=90deg and out=-180deg: the bisector should

Except this was using radians... Or?

[Eric Seidel (no email)]

Comment on attachment 193052 [details]
Correct bisector calculation for markers (2)

View in context: https://bugs.webkit.org/attachment.cgi?id=193052&action=review

>> Source/WebCore/ChangeLog:12
>> +        Angles cannot be averaged this way! Consider in=90deg and out=-180deg: the bisector should
> 
> Except this was using radians... Or?

I see, it's converted to degrees.  But degrees can always be expressed positively, mod 360.  I assume this bug affects all ports?

[Philip Rogers]

(In reply to comment #7)
> (From update of attachment 193052 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193052&action=review
> 
> >> Source/WebCore/ChangeLog:12
> >> +        Angles cannot be averaged this way! Consider in=90deg and out=-180deg: the bisector should
> > 
> > Except this was using radians... Or?
> 
> I see, it's converted to degrees.  But degrees can always be expressed positively, mod 360.  I assume this bug affects all ports?

Atan2 is special in that it returns values in the range -pi to pi which can be a useful property, and FloatPoint::slopeAngleRadians() is implemented using atan2. With the range comments, I just wanted to point out that we still honor the -pi to pi (or -180deg to 180deg) convention, even though it doesn't really matter here (i.e., these values can't be accessed from JS).

This may be easiest to review in person because I can draw a couple examples on a whiteboard.

This bug does affect all ports.

[Dirk Schulze]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 193052 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=193052&action=review
> > 
> > >> Source/WebCore/ChangeLog:12
> > >> +        Angles cannot be averaged this way! Consider in=90deg and out=-180deg: the bisector should
> > > 
> > > Except this was using radians... Or?
> > 
> > I see, it's converted to degrees.  But degrees can always be expressed positively, mod 360.  I assume this bug affects all ports?
> 
> Atan2 is special in that it returns values in the range -pi to pi which can be a useful property, and FloatPoint::slopeAngleRadians() is implemented using atan2. With the range comments, I just wanted to point out that we still honor the -pi to pi (or -180deg to 180deg) convention, even though it doesn't really matter here (i.e., these values can't be accessed from JS).
> 
> This may be easiest to review in person because I can draw a couple examples on a whiteboard.
> 
> This bug does affect all ports.

Does it mean I can not review it, because I am not sitting next to you? That is seriously going into the wrong direction! :)

[Stephen Chenney]

Comment on attachment 193052 [details]
Correct bisector calculation for markers (2)

I think the code change is right. I don't think the test has optimal coverage because the angle between the path segments in each case ranges within a relatively narrow band. The global orientation changes for any given marker, but the relative orientation of the in and out segments does not change anywhere near as much.

I think you need a test with a fixed in angle and all possible out angles (at 5 degree increments or something). I would probably try 8 sets, each with a different input angle: 0.1 deg, -0.1 deg, 89.9, 90.1, 179.9, -179.9, -89.9, -90.1 and the range of output angles.

[Philip Rogers]

(In reply to comment #10)
> (From update of attachment 193052 [details])
> I think the code change is right. I don't think the test has optimal coverage because the angle between the path segments in each case ranges within a relatively narrow band. The global orientation changes for any given marker, but the relative orientation of the in and out segments does not change anywhere near as much.
> 
> I think you need a test with a fixed in angle and all possible out angles (at 5 degree increments or something). I would probably try 8 sets, each with a different input angle: 0.1 deg, -0.1 deg, 89.9, 90.1, 179.9, -179.9, -89.9, -90.1 and the range of output angles.

What if we increased the amplitude of the "waves" around the circle? It already covers a pretty wide range, but by increasing it further we can cover whatever range we want.

I'm suggesting this because hand-writing an exhaustive test of input and output angles will be very tedious. Due to numeric precision issues, we cannot use a reftest anyway.

[Stephen Chenney]

(In reply to comment #11)
> (In reply to comment #10)
> > (From update of attachment 193052 [details] [details])
> > I think the code change is right. I don't think the test has optimal coverage because the angle between the path segments in each case ranges within a relatively narrow band. The global orientation changes for any given marker, but the relative orientation of the in and out segments does not change anywhere near as much.
> > 
> > I think you need a test with a fixed in angle and all possible out angles (at 5 degree increments or something). I would probably try 8 sets, each with a different input angle: 0.1 deg, -0.1 deg, 89.9, 90.1, 179.9, -179.9, -89.9, -90.1 and the range of output angles.
> 
> What if we increased the amplitude of the "waves" around the circle? It already covers a pretty wide range, but by increasing it further we can cover whatever range we want.
> 
> I'm suggesting this because hand-writing an exhaustive test of input and output angles will be very tedious. Due to numeric precision issues, we cannot use a reftest anyway.

Sharp angles are not tested, and making your test sharper will also make it slower and I think more redundant. One option is to make your star pointier and just do one quadrant - I think that might cover the cases.

Or, how about three rows of test points at varied spacing: *      *    *  * * *  *   *      *

The rows should be close together.

Then form all possible three-point combinations that take a point from the first row, then one from the second row, and one from the third row. And reverse those.

[Philip Rogers]

Created attachment 193930 [details]
Update test to test a wider range of values.

[Philip Rogers]

Comment on attachment 193930 [details]
Update test to test a wider range of values.

Clearing r flag. Let me update the test again.

[Philip Rogers]

Created attachment 193934 [details]
Update test to test a wider range of values (2).

[Philip Rogers]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > (From update of attachment 193052 [details] [details] [details])
> > > I think the code change is right. I don't think the test has optimal coverage because the angle between the path segments in each case ranges within a relatively narrow band. The global orientation changes for any given marker, but the relative orientation of the in and out segments does not change anywhere near as much.
> > > 
> > > I think you need a test with a fixed in angle and all possible out angles (at 5 degree increments or something). I would probably try 8 sets, each with a different input angle: 0.1 deg, -0.1 deg, 89.9, 90.1, 179.9, -179.9, -89.9, -90.1 and the range of output angles.
> > 
> > What if we increased the amplitude of the "waves" around the circle? It already covers a pretty wide range, but by increasing it further we can cover whatever range we want.
> > 
> > I'm suggesting this because hand-writing an exhaustive test of input and output angles will be very tedious. Due to numeric precision issues, we cannot use a reftest anyway.
> 
> Sharp angles are not tested, and making your test sharper will also make it slower and I think more redundant. One option is to make your star pointier and just do one quadrant - I think that might cover the cases.
> 
> Or, how about three rows of test points at varied spacing: *      *    *  * * *  *   *      *
> 
> The rows should be close together.
> 
> Then form all possible three-point combinations that take a point from the first row, then one from the second row, and one from the third row. And reverse those.

In the updated patch I've updated the star shapes to test both acute and obtuse angles, as well as reducing the number of markers to 160. If this doesn't look good to you, I'll go with the varied point spacing method.

[Stephen Chenney]

Comment on attachment 193934 [details]
Update test to test a wider range of values (2).

That's better. R=me.

[WebKit Review Bot]

Comment on attachment 193934 [details]
Update test to test a wider range of values (2).

Clearing flags on attachment: 193934

Committed r146510: <http://trac.webkit.org/changeset/146510>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.