-
Notifications
You must be signed in to change notification settings - Fork 48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Arc Parameterization, Length & Error #47
Comments
There's also some issues with parameterizing the arcs with regard to matrix values and svg values. The SVG standard says we increase the size of our radius if there is no solution that can work. Okay. Done. But, your tests specifically generation for 'M 600,350 L 650,325 A 25,25 -30 0,1 700,300 L 750,275', says I should return 25,25 again when distance from (650,325) to (700,300) is ~55.9 which needs to scale up. to ~27.95 and if I do that. My actual points are that far apart. The code seems to demand I store the too small radius again to reproduce the same code rather than code that will correctly radius check, corresponds to the same shape. Unlike the other shapes which store all the information needed to use and manipulate them. Arcs store the svg parameters in order to regurgitate them. |
Actually that's not really your test it's actually code.
Which you can tell from the svg spec is actually correct. However svg.path causes:
To correct this, the last line in
Which will use the scaled radii. However this will cause several other checks to fail. Namely the svg parsing back and forth of the above code and a length check which was checked against non-scaled radii lengths. https://www.w3.org/TR/SVG/implnote.html#ArcImplementationNotes
Trying to make my library which has a bunch of different code still backwards compatible and bug checking with your awesome test code. It's found a few things that are important and a couple that aren't as much. But, this one kept popping out one way when I fixed it and the other when I broke it again. Traced it down to your test code is broken because the code it's testing is also kinda off spec. |
Beyond the implementation flaw there's also problem with the parameterization with regard to scaling. My pull request #46 to fix #36 #16 etc, generally had serious problems with the arc parameterization. You take in and save the SVG parameters and convert to circle based parameterization. However, neither of these are able to be transformed. They are static values. I could code up a functional matrix that could work with complex numbers but you'd still hit a wall at Arc. Even when I break the real and complex parts apart, and then perform the matrix multiplication stuff, applying that to Arc would still be broken because what a matrix does to the parameters differs greatly by the parameters and the operation invoked. Without arc, you could just multiply all the points by a real number and that would natively scale it. However, it would not properly scale the arcs. It would give them correct start and end points, but the ellipse used between the points would be unscaled. And the parameters would be screwy. |
You are talking about a whole bunch of different issues here, and in none of the cases do I really follow what the actual problem is. I think you are saying in the first comments that the length calculations of arcs isn't accurate enough? Your second comment is entirely beyond me. I can't find any place the SVG specs talk about increasing any radius. And: "Unlike the other shapes which store all the information needed to use and manipulate them. Arcs store the svg parameters in order to regurgitate them." Arcs also store the information needed to use them, which is the parameterization used to calculate the length(). I fail to see how it is a problem that the results of the parameterization is stored so it doesn't have to be redone for each length() or point() calculation. In your third comment you seem to claim that "You take in and save the SVG parameters and convert to circle based parameterization. However, neither of these are able to be transformed." If it was true, transforming arcs would be entirely impossible, which of course, is not the case, so yet again I have no idea what you are trying to say. |
Although shortcutting the arc calculations when it's actually a circle is a good idea, I might do that. |
TL;DR versions.
It's true.
|
"But, I basically have to store all the properties as points and recalculate them when queried." Queried? Nothing is being queried... |
For 3 you can just replace the value with the scaled up value, it's what the SVG 2.0 documentation now says should be done. And you calculated them already in the _parameterize function. Just overwrite the values, it's two lines.
The problem here is there's a bunch of values attached to the arc which are now invalid. It might have switched the sweep boolean or changed the sweep angle, changed the rx or ry and even made previously equal rx and ry non-uniform etc. The concrete suggestion I'd have on that is to simply store the internal values for the arc as points to begin with. Having rx and ry a real position in euclidean space (which you pretend is complex space). Then rather than calling them as variables you make them @Property functions that recalculate that. For rx and ry you're adding in a euclidean distance function (though with complex numbers you can do that with abs()) The other option is only allow methods that directly call the Arc convert those properties to points, transform them, and then recalculate the properties in question. Though for this case you'll need to calculate the methods for both stored parameterizations which was my original problem there, a lot of those parameters are redundant information since if you have one you can calculate the other. And you'll refigure a lot of data that you might not need. This is why in Should it do the transforms in a 3rd parameterization and then convert that into the other two parameterizations. Or should it just use 1 parameterization and then calculate relevant data about those parameterizations on the fly? Or should any attempt at transforms be skipped to avoid messing up a reasonable enough project with features that might require fundamental changes. I mean I already needed to solve this stuff for svg.elements and with a bit more work it could be pretty robust. PS. best thing I ever did was merge the parser with the Geometric objects. |
"Just overwrite the values, it's two lines." No, that defeats the purpose of the library. "The problem here is there's a bunch of values attached to the arc which are now invalid." Which is easily solved by running the parameterizing again, but anyway, that's an entirely moot point as the transform should return a new Arc object, not change the existing one. "The concrete suggestion I'd have on that is to simply store the internal values for the arc as points to begin with." But that's what I do!? "which you pretend is complex space" There is no pretending. "Then rather than calling them as variables you make them @Property functions that recalculate that." Recalculate what? I'm sorry, but this conversation isn't going any further. I can't see that any of the issues you point out are real issues. |
@regebro Surely the fact that failing to expand the radii causes the length calculation to be wrong is an issue. You calculate the length of an arc with a tiny radius but it should have a much larger one. |
@regebro Surely the fact that failing to expand the radii causes the length calculation to be wrong is an issue. You calculate the length of an arc with a tiny radius but it should have a much larger one.
Well, you didn't say that, so if that is an issue, you did not point
that out before. Also, I quote: "And you calculated them already in
the _parameterize function." In that case, the radius IS expanded. But
I haven't verified that. I'll look into it.
|
The two lines I'm talking about are setting the values in the bit in _parameterize() which is called ` # Correct out of range radii`` -- Which you check against but you never actually save. It updates the rx ry value as passed to the function, but then loses those at the end.
Though getting lost is pretty easy since the topic has like 6 topics and like 4 don't really matter. |
OK, I need to store the radius scale as well and use that in the point calculations then. |
Some of these errors are pretty large and all of them are negative since number of straight lines being used to approximate a circle will always be inside the circle. And the error check it was supposed to use was 1e-12 there. But, the factor there isn't whether the value is known to less than that value, you can do that but it takes subscribing and superscribing the arc.
However this method for approximating a circle has diminishing returns on the order of the square of the number of points. It's really hard and prohibitively expensive to eek out more correctness through this means.
Side note, in my test there, (feel free to use it), I'm pointlessly rotating the circle because that's where my code is breaking in my parameterizations.
Note that the point value there has itself error up to the 6th order. And you use the point value to get get the position at some value
pos
so even if your lengths were somehow perfect we'd still get 6th order error. Maybe there's something about the function itself that is introducing a lot more error than it needs. Like floats work better if you keep them between 0 and 1. Since you get denormal calculations. But, asking for 12th order error when you can only have 6th order possible seems like it's a big time suck for no results.Also, you might want to strongly consider cheating for the special case of circular. It's downright common that we actually have circles, and the equations are easy given tau is already calculated for us.
The text was updated successfully, but these errors were encountered: