Closed
Bug 984796
Opened 7 years ago
Closed 7 years ago
SVG path getTotalLength returns large incorrect number
Categories
(Core :: SVG, defect)
Tracking
()
VERIFIED
FIXED
mozilla31
People
(Reporter: jake, Assigned: bas.schouten)
References
Details
(Keywords: regression)
Attachments
(2 files)
416 bytes,
image/svg+xml
|
Details | |
2.94 KB,
patch
|
jwatt
:
review+
Sylvestre
:
approval-mozilla-aurora+
Sylvestre
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:28.0) Gecko/20100101 Firefox/28.0 (Beta/Release) Build ID: 20140313141126 Steps to reproduce: Run attached SVG, the path is a curved line. Actual results: getTotalLength returns a large incorrect number in version 28+. Expected results: In Firefox 27 reported length is 138.80595, in 28 it is reported as 379391.00000. Firefox 27: 138.80595 Firefox 28: 379391.00000 Firefox nightly (2014-03-15): 379392.21875 Chromium 32: 138.84326 Chrome 33: 138.84326 IE 11: 138.79509
Reporter | ||
Updated•7 years ago
|
Keywords: regression,
regressionwindow-wanted
![]() |
||
Comment 1•7 years ago
|
||
Regression window Good: http://hg.mozilla.org/integration/mozilla-inbound/rev/595fd5dfbb5a Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140109063242 Bad: http://hg.mozilla.org/integration/mozilla-inbound/rev/22c11c35d1a3 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 ID:20140109080444 Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=595fd5dfbb5a&tochange=22c11c35d1a3 Regressed by 22c11c35d1a3 Bas Schouten — Bug 941585: Better dealing with degenerate beziers. r=jrmuizel
Blocks: 941585
Status: UNCONFIRMED → NEW
status-firefox28:
--- → affected
status-firefox29:
--- → affected
status-firefox30:
--- → affected
status-firefox31:
--- → affected
status-firefox-esr24:
--- → unaffected
tracking-firefox29:
--- → ?
tracking-firefox30:
--- → ?
tracking-firefox31:
--- → ?
Ever confirmed: true
Keywords: regressionwindow-wanted
OS: Linux → All
Version: Trunk → 28 Branch
Assignee | ||
Comment 2•7 years ago
|
||
This fixes two small issues in the path handling code, one of which was actually responsible for this bug (splitting the bezier curve at a negative value when t1 was smaller than 0), and the other which turns out to be a regression I introduced fixing an older bug. Which occurs due to inaccuracies in pow(x,1/3) when x is negative.
![]() |
||
Comment 3•7 years ago
|
||
Hmm, there are three cube roots of a negative number, one of which is the negative of the cube root of the absolute value of the number. (For example, one of the cube roots of cbrt(-8) is -cbrt(8).) There are two other cube roots that are complex numbers though, which is presumably why pow() doesn't accept negative exponents. So I'm not really sure what the affect of converting cbrt(-x) to -cbrt(x) and ignoring the complex roots is. Does the fact that we're getting into that situation mean that the algorithm that we're implementing results in degeneracies that we should be handling in a different way, or is using the one non-imaginary root exactly what we want? I'm really not sure. Can someone provide me with a copy of the paper that describes the algorithm?
Comment 4•7 years ago
|
||
There are also errors with getPointAtLength. I have done some research and the result is here: Firefox 27: path length: 95.00848388671875 point at index 0: 180, 30 point at index 1: 182.32949829101562, 39.207218170166016 point at index 2: 185.4349822998047, 48.17625427246094 point at index 3: 191.12010192871094, 55.4254150390625 point at index 4: 197.1878662109375, 48.99620056152344 point at index 5: 200.015869140625, 39.9317741394043 point at index 6: 202.03709411621094, 30.65072250366211 point at index 7: 202.22813415527344, 21.28634262084961 point at index 8: 193.62380981445312, 18.317861557006836 point at index 9: 190.57847595214844, 9.474796295166016 point at index 10: 190, 0.000003157313130941475 Firefox 28: path length: 1189.25537109375 point at index 0: 180, 30 point at index 1: 177.79641723632812, 107.91682434082031 point at index 2: 140.84165954589844, 220.95492553710938 point at index 3: 103.88700103759766, 333.99267578125 point at index 4: 66.93222045898438, 447.0308532714844 point at index 5: 30.022491455078125, 559.9312133789062 point at index 6: 66.97498321533203, 446.89227294921875 point at index 7: 103.9274673461914, 333.85333251953125 point at index 8: 140.8799591064453, 220.81442260742188 point at index 9: 177.8329620361328, 107.77392578125 point at index 10: 190, 0 This is the fiddle I created: http://jsfiddle.net/g3cKa/
Assignee | ||
Comment 5•7 years ago
|
||
(In reply to Jonathan Watt [:jwatt] from comment #3) > Hmm, there are three cube roots of a negative number, one of which is the > negative of the cube root of the absolute value of the number. (For example, > one of the cube roots of cbrt(-8) is -cbrt(8).) There are two other cube > roots that are complex numbers though, which is presumably why pow() doesn't > accept negative exponents. > > So I'm not really sure what the affect of converting cbrt(-x) to -cbrt(x) > and ignoring the complex roots is. Does the fact that we're getting into > that situation mean that the algorithm that we're implementing results in > degeneracies that we should be handling in a different way, or is using the > one non-imaginary root exactly what we want? I'm really not sure. Can > someone provide me with a copy of the paper that describes the algorithm? I developed this particular section myself. Suffice to say since the solutions to the equation here are not real numbers, but in the imaginary space, they are not solutions that concern us (i.e. they don't describe intervals of t which we care about since we never draw into the imaginary plane).
Updated•7 years ago
|
![]() |
||
Updated•7 years ago
|
Attachment #8393000 -
Flags: review?(jwatt) → review+
Assignee | ||
Comment 6•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/87e39d7d851a
Comment 7•7 years ago
|
||
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/91e63f57c896 - I'm guessing you didn't actually intend to push those printfs that ran the log size up over the maximum with thousands of lines of "INFO - XXX - Bas - Wanting to draw client side!XXX - Bas - Starting to draw client side!XXX - Bas - Finished validation!XXX - Bas - Forwarded transaction!XXX - Bas - Finished drawing client side!XXX - Bas - Starting to draw host side!XXX - Bas - Finished compositing host side!XXX - Bas - Starting to draw host side!XXX - Bas - Finished compositing host side!" but if you did actually want to? Unwant to.
Assignee | ||
Comment 8•7 years ago
|
||
(In reply to Phil Ringnalda (:philor) from comment #7) > Backed out in > https://hg.mozilla.org/integration/mozilla-inbound/rev/91e63f57c896 - I'm > guessing you didn't actually intend to push those printfs that ran the log > size up over the maximum with thousands of lines of "INFO - XXX - Bas - > Wanting to draw client side!XXX - Bas - Starting to draw client side!XXX - > Bas - Finished validation!XXX - Bas - Forwarded transaction!XXX - Bas - > Finished drawing client side!XXX - Bas - Starting to draw host side!XXX - > Bas - Finished compositing host side!XXX - Bas - Starting to draw host > side!XXX - Bas - Finished compositing host side!" but if you did actually > want to? Unwant to. Erm, indeed I didn't :) I think I understand how this went wrong too, sorry about that.
Assignee | ||
Comment 9•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/56e6067c9514
Comment 10•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/56e6067c9514
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Updated•7 years ago
|
![]() |
||
Comment 11•7 years ago
|
||
Comment on attachment 8393000 [details] [diff] [review] Fix some errors in path flattening code [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 941585 User impact if declined: SVG textPath and scrip involving path length broken Testing completed (on m-c, etc.): been on m-c for days Risk to taking this patch (and alternatives if risky): low - this Moz2D functionality is only used by SVG path code, currently String or IDL/UUID changes made by this patch: none
Attachment #8393000 -
Flags: approval-mozilla-beta?
Attachment #8393000 -
Flags: approval-mozilla-aurora?
Updated•7 years ago
|
Attachment #8393000 -
Flags: approval-mozilla-beta?
Attachment #8393000 -
Flags: approval-mozilla-beta+
Attachment #8393000 -
Flags: approval-mozilla-aurora?
Attachment #8393000 -
Flags: approval-mozilla-aurora+
Comment 12•7 years ago
|
||
https://hg.mozilla.org/releases/mozilla-aurora/rev/a31b22a5e8a4 https://hg.mozilla.org/releases/mozilla-beta/rev/2548eb09ff87
Updated•7 years ago
|
Whiteboard: [good first verify]
Comment 13•7 years ago
|
||
I download the beta version 29, but still fails. Is added the patch in this beta version download? Please revise
Comment 14•7 years ago
|
||
This still seems quite broken in Firefox Beta (29.0) and Firefox Aurora (30.0a2). Here’s a simple test case using getPointAtLength: http://bl.ocks.org/mbostock/1705868 In Firefox Beta, the point goes way too fast and flies off the line (probably because getTotalLength is way too big, as described above). In Firefox Aurora, the total length looks fine but getPointAtLength behaves erratically / inaccurately and the point separates from the line.
Comment 15•7 years ago
|
||
It does seem fixed in Nightly (31.0a1), however.
Comment 16•7 years ago
|
||
Yeh, they are not part of the beta4 [1]. It is going to be in the beta5 (available on Friday). [1] http://sylvestre.ledru.info/blog/2014/04/02/changes-firefox-29-beta3-to-beta4
Comment 17•7 years ago
|
||
Reproduced on Nightly from 2014-02-03 with the URL from comment 14 and the attachment. Verified as fixed with Firefox 29 beta 5 (Build ID: 20140403132807) on Windows 7 x64, Ubuntu 13.04 x64 and Mac OS X 10.9.2: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:29.0) Gecko/20100101 Firefox/29.0 Mozilla/5.0 (X11; Linux x86_64; rv:29.0) Gecko/20100101 Firefox/29.0
Comment 18•7 years ago
|
||
Good morning, is fixed (the point not goes way too fast and flies off the line), but now there is a NEW PROBLEM. I this test case: http://bl.ocks.org/duopixel/3824661 When hover in the point is not fluid in version Firefox 29 beta5. It make some steps caused by obtain getPointAtLength in a PATH of svg. In firefox 27 is OK, and previews versions. Please revise. Thanks.
![]() |
||
Comment 19•7 years ago
|
||
(In reply to berenice from comment #18) > Good morning, is fixed (the point not goes way too fast and flies off the > line), but now there is a NEW PROBLEM. > > I this test case: http://bl.ocks.org/duopixel/3824661 > > When hover in the point is not fluid in version Firefox 29 beta5. It make > some steps caused by obtain getPointAtLength in a PATH of svg. In firefox > 27 is OK, and previews versions. > > Please revise. > Thanks. Please file a new bug.
![]() |
||
Updated•7 years ago
|
Flags: needinfo?(veronica_blanco_rodriguez)
Comment 20•7 years ago
|
||
Good morning, I open a new bug: https://bugzilla.mozilla.org/show_bug.cgi?id=992850
Flags: needinfo?(veronica_blanco_rodriguez)
Comment 21•7 years ago
|
||
Verified fixed 30.0a2 (2014-04-14), 31.0a1 (2014-04-14), Win 7 x64.
You need to log in
before you can comment on or make changes to this bug.
Description
•