Closed Bug 984796 Opened 6 years ago Closed 6 years ago

SVG path getTotalLength returns large incorrect number

Categories

(Core :: SVG, defect)

28 Branch
x86_64
All
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla31
Tracking Status
firefox28 --- wontfix
firefox29 + verified
firefox30 + verified
firefox31 + verified
firefox-esr24 --- unaffected

People

(Reporter: jake, Assigned: bas.schouten)

References

Details

(Keywords: regression)

Attachments

(2 files)

Attached image path.svg
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
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
Ever confirmed: true
OS: Linux → All
Version: Trunk → 28 Branch
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.
Assignee: nobody → bas
Status: NEW → ASSIGNED
Attachment #8393000 - Flags: review?(jwatt)
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?
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/
(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).
Attachment #8393000 - Flags: review?(jwatt) → review+
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.
(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.
https://hg.mozilla.org/mozilla-central/rev/56e6067c9514
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
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?
Attachment #8393000 - Flags: approval-mozilla-beta?
Attachment #8393000 - Flags: approval-mozilla-beta+
Attachment #8393000 - Flags: approval-mozilla-aurora?
Attachment #8393000 - Flags: approval-mozilla-aurora+
Whiteboard: [good first verify]
I download the beta version 29, but still fails. Is added the patch in this beta version download?

Please revise
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.
It does seem fixed in Nightly (31.0a1), however.
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
Keywords: verifyme
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
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.
(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.
Flags: needinfo?(veronica_blanco_rodriguez)
Good morning, 
I open a new bug:

 https://bugzilla.mozilla.org/show_bug.cgi?id=992850
Flags: needinfo?(veronica_blanco_rodriguez)
Depends on: 992850
No longer depends on: 992850
Verified fixed 30.0a2 (2014-04-14), 31.0a1 (2014-04-14), Win 7 x64.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.