Closed Bug 999943 Opened 5 years ago Closed 5 years ago

getPointAtLength returns the wrong point for some path segments

Categories

(Core :: SVG, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla32
Tracking Status
firefox28 --- wontfix
firefox29 --- wontfix
firefox30 - wontfix
firefox31 - verified
firefox32 --- verified
firefox-esr24 --- unaffected

People

(Reporter: birtles, Assigned: bas.schouten)

References

Details

(Keywords: regression)

Attachments

(2 files, 1 obsolete file)

Attached image point-at-length.html (obsolete) —
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.
Attached image point-at-length.html
Attachment #8410776 - Attachment is obsolete: true
OS: All → Windows 7
Hardware: All → x86_64
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
Bas, could you have a look to this bug? Thanks
Flags: needinfo?(bas)
I'll fix this today.
Assignee: nobody → bas
Status: NEW → ASSIGNED
Flags: needinfo?(bas)
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.
Attachment #8428440 - Flags: review?(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. :)
Attachment #8428440 - Flags: review?(jwatt) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/0acfd585d47c
https://hg.mozilla.org/mozilla-central/rev/b520d877a92e
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
(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))
Flags: needinfo?(bas)
(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 :).
Flags: needinfo?(bas)
Verified fixed 32.0a1 (2014-06-02), win 7 x64
Status: RESOLVED → VERIFIED
(maybe worth going for backport approval now?)
Flags: needinfo?(bas)
(It's release week, so we'll need to request Beta approval instead of Aurora approval now (or soon).)
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
Attachment #8428440 - Flags: approval-mozilla-beta?
Flags: needinfo?(bas)
Attachment #8428440 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Please verify this is fixed in Firefox 31.0b2.
Keywords: verifyme
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
No longer blocks: 1044449
You need to log in before you can comment on or make changes to this bug.