347437 - Marker elements sometimes point backwards

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Alex Menzies]

Marker elements are sometimes rendered pointing 180 degrees away from the correct direction.  In the attached test case, the center marker of polylines A and B is drawn pointing the wrong way.

Comment 1

[Alex Menzies]

Attachment: testcase

Comment 2

[Alex Menzies]

Attachment: Patch

Patch corrects  nsSVGUtils::AngleBisect for the case when the difference between the two angles is less than -pi.

Comment 3

[Alex Menzies]

Of course, this can still gets into trouble if one of the input angles is less than PI, such as in the case:
a2 = -3 * PI - 1;
a1 = -2;

This isn't a problem in nsSVGPolyline but it might cause problems elsewhere.  Maybe this could be fixed by performing something analogous to a modulus operation of the difference of a2 and a1.

Comment 4

[Alex Menzies]

Attachment: fix

This patch corrects AngleBisect by normalizing the angles before computing their difference.

Comment 5

[tor]

Comment on attachment 232616 [details] [diff] [review]
fix

>+/*
>+ *  Returns an angle equivalent to ang but within the range [0, 2PI]
>+ */
>+float
>+NormalizeAngle(float ang)
>+{
>+  return ang - (floor(ang / (M_PI * 2)) * M_PI * 2);
>+}

Would fmod() do what you need?

>+
> float
> nsSVGUtils::AngleBisect(float a1, float a2)
> {
>-  if (a2 - a1 < M_PI)
>+  a1 = NormalizeAngle(a1);
>+  a2 = NormalizeAngle(a2);
>+  if (fabs(a2 - a1) < M_PI) {
>     return (a1+a2)/2;
>-  else
>+  } else {
>     return M_PI + (a1+a2)/2;
>+  }

Any reason for adding braces?

Comment 6

[Alex Menzies]

(In reply to comment #5)
 
> Would fmod() do what you need?
Yes.  Originally I thought there would be wraparound issues when subtracting a negative a1 but this turns out not to be a problem.

> Any reason for adding braces?
Yes.  I received this comment on one of my patches:
“Content style should be to wrap even one line in braces, at least for new code and code that's being touched anyway.”

Comment 7

[Alex Menzies]

Attachment: Patch using fmod

Comment 8

[tor]

Comment on attachment 232771 [details] [diff] [review]
Patch using fmod

> float
> nsSVGUtils::AngleBisect(float a1, float a2)
> {
>-  if (a2 - a1 < M_PI)
>+  a1 = fmod(a1, NS_STATIC_CAST(float, M_PI * 2));
>+  a2 = fmod(a2, NS_STATIC_CAST(float, M_PI * 2));

Do you really need the casts?

>+  if (fabs(a2 - a1) < M_PI) {
>     return (a1+a2)/2;
>-  else
>+  } else {
>     return M_PI + (a1+a2)/2;
>+  }

The prevailing style in nsSVGUtils for the most part is no braces on single statement blocks.

Comment 9

[Alex Menzies]

Attachment: Patch without braces

Comment 10

[tor]

(In reply to comment #9)
> Created an attachment (id=232928) [edit]
> Patch without braces

And my question about the casting?

Comment 11

[Alex Menzies]

(In reply to comment #10)
> (In reply to comment #9)
> > Created an attachment (id=232928) [edit]
> > Patch without braces
> 
> And my question about the casting?
> 

Oops, sorry, didn't see the question.  Without the cast, the compiler doesn't know if it should use fmod(float, float) or fmod(double, double)

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't think this is correct in all cases. Suppose a1 is +M_PI*7/4, a2 is -M_PI*7/4. This code will return M_PI, but I think the result should be zero.

How about this?

float delta = fmod(a2 - a1, 2*M_PI);
if (delta < 0) {
  delta += 2*M_PI;
}
/* delta is now the angle from a1 around to a2, in the range [0, 2*M_PI) */
float r = a1 + delta/2;
if (delta >= M_PI) {
  /* the arc from a2 to a1 is smaller, so use the ray on that side */
  r += M_PI;
}
return r;

Comment 13

[Alex Menzies]

Attachment: Patch with roc's code

You are correct, my method returns M_PI when it should return zero.  Your version looks good to me and passes all of my tests.  I am attaching your code as a patch with one minor change; I cast 2*M_PI to a float in the call to fmod.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 235752 [details] [diff] [review]
Patch with roc's code

Sure.

Comment 15

[tor]

Checked in for Alex.