Pomax / svg-path-reverse

This is a JavaScript SVG path to "something" converter, turning a path into hookable graphics instructions for arbitrary conversion
59 stars 15 forks source link

Questions and issues #4

Closed thednp closed 4 years ago

thednp commented 4 years ago

First off: thanks for such a rare gem, I'm happy I found this despite it doesn't solve my problem.

Is it possible you will extend this functionality to support all path commands?

Pomax commented 4 years ago

I hadn't planned on it, but the missing ones are relatively straight forward to add, which one(s) are you missing?

thednp commented 4 years ago

Thanks for the quick reply.

My few test shapes included ARC and I ended up with some squares or triangles instead of the expected shapes; scaling to thousands of shapes I happen to work with, this solution won't do obviously.

I'm now playing with SvgPath and my own reverseCurve function, currently extending it to support all kinds of shapes.

Perhaps we can improve anyone's life with a good solution based on the above mentioned code base, I'm curious if you would have a good or better suggestion. :D

Pomax commented 4 years ago

right now the only supported codes are M, L, V, H, Q, S, C, and T (well and Z but that's really an afterthought), so adding A would completely the list, might as well grab a drink and add that in for you.

Pomax commented 4 years ago

v1.2.0 published with arc support.

thednp commented 4 years ago

Thanks for your input man, thumbs up!

thednp commented 4 years ago

Sorry to bother you, I'm back to testing your code. Some notes:

thednp commented 4 years ago

There seems to be some issue with your normalize function, here an example

var original = "M6.5 12a5.5 5.5 0 100-11 5.5 5.5 0 000 11zM13 6.5a6.5 6.5 0 11-13 0 6.5 6.5 0 0113 0z M10.344 11.742c.03.04.062.078.098.115l3.85 3.85a1 1 0 001.415-1.414l-3.85-3.85a1.007 1.007 0 00-.115-.1 6.538 6.538 0 01-1.398 1.4z M3 6.5a.5.5 0 01.5-.5h6a.5.5 0 010 1h-6a.5.5 0 01-.5-.5z";

var normalized = "M 6.5 12 A 5.5 5.5 0 100 -11 12 17.5 A 0 0 11 undefined undefined NaN NaN Z M 13 6.5 A 6.5 6.5 0 11 -13 13 13 A 6.5 0 113 0 undefined NaN NaN Z M 10.344 11.742 C 10.373999999999999 NaN NaN NaN NaN NaN L NaN NaN A 1 1 0 1.415 -1.414 NaN NaN L NaN NaN A 1.007 1.007 0 0 -0.115 NaN NaN A 6.538 0 1 -1.398 1.4 NaN NaN Z M 3 6.5 A 0.5 0 1.5 -0.5 undefined NaN NaN L NaN NaN A 0.5 0 10 1 undefined NaN NaN L NaN NaN A 0.5 0 1 -0.5 -0.5 NaN NaN Z"

var reversed = "M 6.5 12 A 5.5 5.5 0 100 -11 12 17.5 A 0 0 11 undefined undefined NaN NaN Z M NaN NaN A 6.5 0 113 0 0 13 13 A 6.5 6.5 0 11 0 13 6.5 Z M10.344 11.742c.03.04.062.078.098.115l3.85 3.85a1 1 0 001.415-1.414l-3.85-3.85a1.007 1.007 0 00-.115-.1 6.538 6.538 0 01-1.398 1.4z M 3 6.5 A 0.5 0 1.5 -0.5 undefined NaN NaN L NaN NaN A 0.5 0 10 1 undefined NaN NaN L NaN NaN A 0.5 0 1 -0.5 -0.5 NaN NaN Z"

I think this may be the main problem we're facing. Also some RegExp fixes here and there .replace(/([a-z])/gi," $1 ") then instructions = d.replace(/\s([a-z])\s/gi,"|$1").split("|").filter(f=>f), and for (i = 0; i < instructionLength; i++) { because we filtered the results.

Pomax commented 4 years ago

It's better to file a few new issues, although the first two can probably just be an on-github-without-ever-cloning-and-editing-offline PR. (that said, I've fixed the header comment, as that as a 2 second change)

Pathing wise: V and H are converted into L instructions during normalization, and reverse only reverses, so the resultant path will never contain V or H, and so it also doesn't need to look for those with the Regex. It might be nice to file a "feature request" issue to preserve V and H, since they're work in reverse just fine (I don't even rememeber why I originally needed them collapsed into L anymore! =)

As for that last one: what is a5.5 5.5 0 100-11 5.5 5.5 0 000 11z supposed to be? The arc operator takes seven arguments at a time (rx, ry, the x-axis rotation, a large-arc flag, the sweep direction flag, and then finally the x and y coordinates for where the arc should end), but your code only shows five, so that's not valid SVG. Did you mean to write 5.5 5.5 0 1 0 0-11 or 5.5 5.5 0 1,0,0-11 instead of 5.5 5.5 0 100-11? Because right now you've specified a "large arc" flag with value 100, which isn't a valid value for that flag (it should be either 0 or 1, but not one hundred =).

thednp commented 4 years ago

5.5 5.5 0 100-11 5.5 5.5 0 000 11 is 5.5, 5.5, 0, 100, -11, 5.5, 5.5, 0, 0, 0, 0, 11 but that's not a problem, the path is rendered fine, no browser issue.

Anyways, there are 2 main issues:

Optional:

Pomax commented 4 years ago

That was my point: even if this shows, for some bizarre reason, according to the spec on the Arc command that is not valid notation because Arc takes seven arguments, not five. So if you input an invalid-per-the-spec path, the conversion is essentially guaranteed to be wrong.

So:

  1. normalization: please use a proper path first, because if that works, the problem here is whatever created that path, not the normalize function.
  2. please file that as a new issue so it can be worked on.
  3. that's not this library's responsibility (plus, even at 3 decimals, a compound rounding error is very quickly accumulated). you probably want to look at something like svgo instead for that kind of optimization.

Or, do this at the outset: An <svg width="100" height="100" viewBox="0 0 100 100"...> with coordinates like "3.12655" is the same thing as <svg width="100" height="100" viewBox="0 0 1000000 1000000"...> with coordinates like "31265.5", because these are vector graphics. The actual on page presentation is the same, you just picked a better viewbox crop, with better coordinate scaling. So that's really something you should be doing either in the code that generates your SVG, or the design application you're using (scale up your SVG, and then ensure your document is set to only allow integer coordinates).

thednp commented 4 years ago

The path values are "fine" as exported by third party optimization scripting, I can make use of third party normalization to solve that.

Agreed with almost everything you say. My main concern is output size and faster markup parsing for just a couple icons we don't need 3+ decimals, but again, that can be worked on with third party scripts.

Will file a new issue for H and V and a PR with the suggested changes above.

Thanks for the "rapid fire".

Pomax commented 4 years ago

If they really are "fine", I kind of need you to point me to the part of the SVG spec that explains why that is, so that I can understand what the parsing requirements are and update the arc parsing based on that. Because as far as I can tell from the spec, officially they are not fine at all, and arc must have 7 arguments, not five.

Pomax commented 4 years ago

Specifically, https://www.w3.org/TR/SVG/paths.html says:


The syntax of path data is concise in order to allow for minimal file size and efficient downloads, since many SVG files will be dominated by their path data. Some of the ways that SVG attempts to minimize the size of path data are as follows:

  • All instructions are expressed as one character (e.g., a moveto is expressed as an M).
  • Superfluous white space and separators (such as commas) may be eliminated; for instance, the following contains unnecessary spaces:
    M 100 100 L 200 200
    It may be expressed more compactly as:
    M100 100L200 200
  • A command letter may be eliminated if an identical command letter would otherwise precede it; for instance, the following contains an unnecessary second "L" command:
    M 100 200 L 200 100 L -100 -200
    It may be expressed more compactly as:
    M 100 200 L 200 100 -100 -200
  • For most commands there are absolute and relative versions available (uppercase means absolute coordinates, lowercase means relative coordinates).
  • Alternate forms of lineto are available to optimize the special cases of horizontal and vertical lines (absolute and relative).
  • Alternate forms of curve are available to optimize the special cases where some of the control points on the current segment can be determined automatically from the control points on the previous segment.

The path data syntax is a prefix notation (i.e., commands followed by parameters). The only allowable decimal point is a Unicode U+0046 FULL STOP (".") character (also referred to in Unicode as PERIOD, dot and decimal point) and no other delimiter characters are allowed [UNICODE]. (For example, the following is an invalid numeric value in a path data stream: "13,000.56". Instead, say: "13000.56".)


So nowhere in there does it mention that it's legal to remove spaces between values: 1 0 0 is not the same as 100, and https://www.w3.org/TR/SVG/paths.html#PathDataEllipticalArcCommands very clearly indicates that A/a take exactly 7 arguments, with none of them being optional, so even if using collapsed values works: it works despite being wrong.

If you know where I can find the text that explains why the syntax you're showing is fine, and what it means, I'll be happy to update the parser =)

thednp commented 4 years ago

That's true, however the JavaScript engine implemented with SVGElement API can understand that " 000 " is not a real number, it's also illegal in a different context, but it's a string composed of 3 decimal numbers.

In a different context if you specify a different number literal type 000 can mean something different as well, but for path d values this is fine, we don't get any console error. See for yourself the zoom-out icon has the path I mentioned above.

Pomax commented 4 years ago

Looking at the technical grammar specification, https://www.w3.org/TR/SVG/paths.html#PathDataBNF:

The processing of the EBNF must consume as much of a given EBNF production as possible, stopping at the point when a character is encountered which no longer satisfies the production. Thus, in the string "M 100-200", the first coordinate for the "moveto" consumes the characters "100" and stops upon encountering the minus sign because the minus sign cannot follow a digit in the production of a "coordinate". The result is that the first coordinate will be "100" and the second coordinate will be "-200".

Similarly, for the string "M 0.6.5", the first coordinate of the "moveto" consumes the characters "0.6" and stops upon encountering the second decimal point because the production of a "coordinate" only allows one decimal point. The result is that the first coordinate will be "0.6" and the second coordinate will be ".5".

Note that the EBNF allows the path data string in the d property to be empty. This is not an error, instead it disables rendering of the path. Rendering is also disabled when the d property has the value none.

With the following grammar:

elliptical_arc::=
    ( "A" | "a" ) wsp* elliptical_arc_argument_sequence

elliptical_arc_argument_sequence::=
    elliptical_arc_argument
    | (elliptical_arc_argument comma_wsp? elliptical_arc_argument_sequence)

elliptical_arc_argument::=
    number comma_wsp? number comma_wsp? number comma_wsp
    flag comma_wsp? flag comma_wsp? coordinate_pair

...

sign::= "+"|"-"
number ::= ([0-9])+
flag::=("0"|"1")
comma_wsp::=(wsp+ ","? wsp*) | ("," wsp*)
wsp ::= (#x9 | #x20 | #xA | #xC | #xD)

So I think that's the part that explains why 100 is valid, as EBFN parsing will needs a value that fulfills a flag rule, which can only be 1 or 0 and so the next value does not require a comma or space. I will file a new issue to fix that.

thednp commented 4 years ago

I was about to say: "I'm not here to argue with you the circle is round", but you figured out the technicalities. The simple and string based normalize function you developed cannot manage complex situations, which is expected considering the size. For instance Raphael uses some complex sets of RegExp only for spaces, it never fails to parse path commands.

Anyways, I'm thinking of creating a simple tool, inspired by your script, Raphael and SVGPath just to have this specific set of tools:

Why not co-author such a tool, will you be interested in it?

Pomax commented 4 years ago

Probably not, not because it doesn't sound interesting but simply because I've not had a need for this tool for quite a few years now (I needed it in the context of font glyph work, which is something I've not done for a long time now).

But if you do make an ES6 library based on ES2020 (or later), may I put in the selfish request to not create a bundle, but have each part be importable on its own, too. One thing that's plagued Node projects over the years is that if you just needed one bit of functionality, you had to bundle in an entire library without being able to shake the tree. Now that we have actual ES6 support as of Node 14, bundling should not be necessary anymore (even if you have an index.js that just imports all the parts and then exports them as a single module without any real code in between).

thednp commented 4 years ago

Have a look at my repo, looks good enough?

Pomax commented 4 years ago

I won't be going code diving (mostly because as mentioned, this is not something I'm working with anymore), but it looks like quite encompassing, which is nice, but also delivered as a prebuilt ES5 bundle, which is understandable for a large project, but also a little disappointing. Then again, I'm not the audience for your library, so my opinion should not factor into your decision if your users rely on ES5 bundles.

thednp commented 4 years ago

This is getting frustrating. The more I'm testing the more frustration. SnapSVG, Raphael (both share identical codebase) and a couple more have the same problem: none process paths properly, but I'm going to have a good working code to do proper path reverse functionality. The idea is stupid simple, just the pathToAbsolute fails with every library.

https://jsfiddle.net/gLfydqo4/

Pomax commented 4 years ago

I assume you've filed issues for both, with links to the spec that explain that their implementation doesn't quite follow the spec yet?

Because that's why I kept asking you tell me where in the spec that was explained: unless you can show people WHY their code is not following the spec, just saying "look, this SVG works in so-and-so application, so your code should accept it, too" is literally meaningless: unless you can back up a claim with specs that explain it should work, the fact that it does work is not proof of a valid claim. I happened to get annoyed enough to keep looking, but you need to give people the information they need so they can justify fixing things. Their time is valuable, and "digging through dense specs to find proof of some person making a claim in an issue" is time they could have spent actually working on justified code changes =)

thednp commented 4 years ago

I think I understand that I was expecting all to follow the spec.

At first I was looking elsewhere, when in fact it's all about RegExp and character-by-character parsing of arcTo seems to be the adequate solution. I seem to have produced adverse effects and I'm really sorry. I was really hoping to team up with you for a good long lasting and modern tool, but I'm back to the oneManArmy formula.

I will keep my promise and provide a link to the final tool when done. It should be as flexible and modular as possible.

CHEERS