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.
Confirmed. Somebody probably forgot their trigonometry.
Created attachment 192655 [details] Testcase
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.
Created attachment 193015 [details] Correct bisector calculation for markers
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.
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?
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?
(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.
(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! :)
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.
(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.
(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.
Created attachment 193930 [details] Update test to test a wider range of values.
Comment on attachment 193930 [details] Update test to test a wider range of values. Clearing r flag. Let me update the test again.
Created attachment 193934 [details] Update test to test a wider range of values (2).
(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.
Comment on attachment 193934 [details] Update test to test a wider range of values (2). That's better. R=me.
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>
All reviewed patches have been landed. Closing bug.