999943 - getPointAtLength returns the wrong point for some path segments

Status: VERIFIED FIXED

(Core :: SVG, defect)

Description

[Brian Birtles (:birtles)]

Attachment: point-at-length.html

The attached test includes an SVG path. We call getPointAtLength at regular intervals along the path length and plot the returned point. The path created by the returned points deviates significantly from the drawn path.

If I change the first path command from:

  M17.635,27.137c0,0-17.466,36.073-3.425,45.662...

to, say:

  M17.635,27.137c0,1-17.466,36.073-3.425,45.662...

The problem is resolved.

Comment 1

[Brian Birtles (:birtles)]

Attachment: point-at-length.html

Comment 2

[Alice0775 White]

Regressed by 4f086025350f  Bug 930577 Convert much of the SVG code for calculating path lengths and position at an offset along a path to Moz2D

Comment 3

[Sylvestre Ledru [:Sylvestre]]

Bas, could you have a look to this bug? Thanks

Comment 4

[Bas Schouten (:bas.schouten)]

I'll fix this today.

Comment 5

[Bas Schouten (:bas.schouten)]

Attachment: Use abs to make sure min and max don't get swapped.

We need min to be the lowest point in the approximation range and max the highest. I accidentally forgot an abs call here which made this not always be true. This fixes that problem.

Comment 6

[Jonathan Watt [:jwatt]]

Comment on attachment 8428440 [details] [diff] [review]
Use abs to make sure min and max don't get swapped.

r=me, but maybe also consider storing |abs(aTolerance / (cp41.x - cp41.y))| in a variable unless you're absolutely sure all compilers will optimize it away so the division doesn't happen twice. It probably makes it clearer anyway.

Also please note in a comment why we use abs().

And I presume you'll add a test to the Moz2D suite. :)

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

We've already shipped this in 28/29 so I'm untracking for upcoming versions.  At this point we are too late in the FF30 cycle to take this on so a low risk uplift will have to be considered for later versions when ready.

Comment 8

[Bas Schouten (:bas.schouten)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0acfd585d47c
https://hg.mozilla.org/integration/mozilla-inbound/rev/b520d877a92e

Fwiw, this is extremely low risk to uplift.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0acfd585d47c
https://hg.mozilla.org/mozilla-central/rev/b520d877a92e

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to Bas Schouten (:bas.schouten) from comment #8)
> Fwiw, this is extremely low risk to uplift.

(Sounds like it might be worth requesting aurora approval, then? (though not beta, given comment 7))

Comment 11

[Bas Schouten (:bas.schouten)]

(In reply to Daniel Holbert [:dholbert] from comment #10)
> (In reply to Bas Schouten (:bas.schouten) from comment #8)
> > Fwiw, this is extremely low risk to uplift.
> 
> (Sounds like it might be worth requesting aurora approval, then? (though not
> beta, given comment 7))

I wanted to give it a day or 2 of nightly coverage :).

Comment 12

[Paul Silaghi, QA [:pauly]]

Verified fixed 32.0a1 (2014-06-02), win 7 x64

Comment 13

[Daniel Holbert [:dholbert]]

(maybe worth going for backport approval now?)

Comment 14

[Daniel Holbert [:dholbert]]

(It's release week, so we'll need to request Beta approval instead of Aurora approval now (or soon).)

Comment 15

[Bas Schouten (:bas.schouten)]

Comment on attachment 8428440 [details] [diff] [review]
Use abs to make sure min and max don't get swapped.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 930577
User impact if declined: Incorrect points reported for getPointAtLength
Testing completed (on m-c, etc.): nightly and some aurora testing
Risk to taking this patch (and alternatives if risky): Low
String or IDL/UUID changes made by this patch: None

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ed450cb3ad15
https://hg.mozilla.org/releases/mozilla-beta/rev/08300d929d50

Comment 17

[u279076]

Please verify this is fixed in Firefox 31.0b2.

Comment 18

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Reproduced the issue on Firefox Nightly from 2014-04-21.
Verified as fixed on Win 7 64bit, using Firefox 31 Beta 2:
- User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Firefox/31.0
- BuildID: 20140616143923