WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
112054
SVG Marker orientation=auto has incorrect behavior around 0°
https://bugs.webkit.org/show_bug.cgi?id=112054
Summary
SVG Marker orientation=auto has incorrect behavior around 0°
Tom MacWright
Reported
2013-03-11 12:59:18 PDT
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.
Attachments
Testcase
(1.15 KB, text/html)
2013-03-12 00:25 PDT
,
Philip Rogers
no flags
Details
Correct bisector calulcation
(2.92 KB, patch)
2013-03-12 00:42 PDT
,
Philip Rogers
no flags
Details
Formatted Diff
Diff
Correct bisector calculation for markers
(79.21 KB, patch)
2013-03-13 15:59 PDT
,
Philip Rogers
no flags
Details
Formatted Diff
Diff
Correct bisector calculation for markers (2)
(79.63 KB, patch)
2013-03-13 20:20 PDT
,
Philip Rogers
schenney
: review-
Details
Formatted Diff
Diff
Update test to test a wider range of values.
(60.92 KB, patch)
2013-03-19 14:48 PDT
,
Philip Rogers
no flags
Details
Formatted Diff
Diff
Update test to test a wider range of values (2).
(72.56 KB, patch)
2013-03-19 15:20 PDT
,
Philip Rogers
no flags
Details
Formatted Diff
Diff
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Philip Rogers
Comment 1
2013-03-11 13:05:01 PDT
Confirmed. Somebody probably forgot their trigonometry.
Philip Rogers
Comment 2
2013-03-12 00:25:30 PDT
Created
attachment 192655
[details]
Testcase
Philip Rogers
Comment 3
2013-03-12 00:42:27 PDT
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
Comment 4
2013-03-13 15:59:38 PDT
Created
attachment 193015
[details]
Correct bisector calculation for markers
Philip Rogers
Comment 5
2013-03-13 20:20:30 PDT
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 6
2013-03-13 23:01:45 PDT
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 7
2013-03-13 23:03:42 PDT
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
Comment 8
2013-03-13 23:26:51 PDT
(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
Comment 9
2013-03-14 08:53:32 PDT
(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 10
2013-03-14 10:45:46 PDT
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
Comment 11
2013-03-15 14:51:02 PDT
(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
Comment 12
2013-03-15 16:58:48 PDT
(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
Comment 13
2013-03-19 14:48:45 PDT
Created
attachment 193930
[details]
Update test to test a wider range of values.
Philip Rogers
Comment 14
2013-03-19 14:49:41 PDT
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
Comment 15
2013-03-19 15:20:53 PDT
Created
attachment 193934
[details]
Update test to test a wider range of values (2).
Philip Rogers
Comment 16
2013-03-19 15:22:57 PDT
(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 17
2013-03-21 12:15:27 PDT
Comment on
attachment 193934
[details]
Update test to test a wider range of values (2). That's better. R=me.
WebKit Review Bot
Comment 18
2013-03-21 13:38:01 PDT
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
Comment 19
2013-03-21 13:38:05 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug