ISSUE-6566: Fix SVGs for special Arc lines #6571

gloriousjob · 2020-09-04T18:33:36Z

#6566 noticed an issue with SVGs not loading properly. Chasing this down revealed that the 'a' instruction apparently allows for a numbers like '00' but expects two numbers from that. The normal regex did not handle this case, resulting in incorrect arc handling.
I pulled some code from #6023 and modified it to be es5 compliant to help with this. Thanks @asturur for pointing out a clue may be there!

gloriousjob · 2020-09-04T18:34:24Z

I'm not sure what to do for a unit test. The eStore svg in the issue returned an array of length 40 when loaded broken but returns length 41 when good. Should that be the test?

asturur · 2020-09-04T18:36:28Z

yes definitely we should test that arcs get parsed correctly.
I m also sure we can get away with less cose changes because some of those regex are already available in the codebase

gloriousjob · 2020-09-04T19:56:56Z

I pulled out one variable that seemed easy enough. Not sure I saw anything for the rest though. I added a test that failed with the old code.

asturur · 2020-09-04T20:51:28Z

i ll find some time to help you with this.

gloriousjob · 2020-09-04T21:08:32Z

Please do. It seems some visual tests are failing after changing that variable. I'd also appreciate help on my other issue as well.

fubuk-i · 2020-09-05T06:06:23Z

I'm not sure what to do for a unit test. The eStore svg in the issue returned an array of length 40 when loaded broken but returns length 41 when good. Should that be the test?

If you need more svg files for testing I can provide that @gloriousjob

asturur · 2020-09-05T06:14:48Z

is enough to write a simple path string 'A 001...' in al combinationations without spaces and check it gets parsed correctly. so one normal one with 2 space missing, another with just one... i do not know exactly which are the spaces that can be omitted.

gloriousjob · 2020-09-05T16:39:38Z

So that's the annoying thing. The first three are normal values, the middle two are these special flags that may or may not have spacing, and the last two are normal values. What's breaking us is when the flags don't have spacing (fine when they do with current code).

…

On September 5, 2020 2:15:01 AM Andrea Bogazzi ***@***.***> wrote: is enough to write a simple path string 'A 001...' in al combinationations without spaces and check it gets parsed correctly. so one normal one with 2 space missing, another with just one... i do not know exactly which are the spaces that can be omitted. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

gloriousjob · 2020-09-06T01:32:50Z

I don't know if this helps but the grammar is here:
https://svgwg.org/svg2-draft/paths.html#PathDataBNF
https://www.w3.org/TR/SVG11/paths.html#PathDataBNF

I don't know if we're striving for a particular version of svg... but anyways, that's where you can see in elliptical_arc_argument that before the first flag, there must be comma and/or space but after each flag, it's commawSp?. What's being thrown off in the normal regex is it's expecting the numbers to be separated, which is why we're getting 6 or 5 values instead of 7 (6 if the number after the flags is negative, 5 if positive without a sign).

src/util/path.js

asturur · 2020-09-06T10:00:18Z

src/util/path.js

+            coords.push(args[j]);
+          }
+        }
+      }


can be this written like:

if (command.toLowerCase() === 'a') { // arcs have special flags that apparently don't require spaces so handle special coords = regArcArgumentSequence.exec(coordsStr); } else { coords = []; while ((match = re.exec(coordsStr))) { coords.push(match[0]); } }

would it work?

line 613
https://github.com/fabricjs/fabric.js/pull/6571/files#diff-b4a17ab08e7f28e3bf83badb9236d01cR613
could go away

and so the pre declaration here would be just
https://github.com/fabricjs/fabric.js/pull/6571/files#diff-b4a17ab08e7f28e3bf83badb9236d01cR589

var coords,

thre is no point to reuse the same array this is not hot code to optimize.

This failed unit tests when I tried it. An arc can have multiple strings of numbers behind it so there needs to be a loop. Also, the exec returns an array that has [0] with a list of matches and then [1..n] with the matches (hence why j starts at 1). Maybe it can be reworked but I'm not sure there's a gain there.

Are you saying you don't like 'coords.length = 0' and would rather have 'coords = []' at 613? I didn't care for the former. Or are you saying you want 613 to be 'var coords = [];' and remove the top declaration? Or do you have other concerns as well?

asturur · 2020-09-06T10:07:09Z

src/util/path.js

+        rFlagCommaWsp = '([01])' + rCommaWsp + '?',
+        rCoordinatePair = '(' + fabric.reNum + ')' + rCommaWsp + '?(' + fabric.reNum + ')',
+        rArcSeq = rNumberCommaWsp + '?' + rNumberCommaWsp + '?' + rNumberCommaWsp + rFlagCommaWsp + rFlagCommaWsp +
+          rCoordinatePair,


isn't rCoordinatePair just another 2 rNumberCommaWsp?

is rArcSeq just:

rArcSeq = rNumberCommaWsp + '?' + rNumberCommaWsp + '?' + rNumberCommaWsp + rFlagCommaWsp + rNumberCommaWsp + '?' + rNumberCommaWsp;

or am i over simplifying?

Also, what the ? is doing? i see is between everything but not after the third rNumberCommaWsp

I'll change up the code. rCoordinatePair is rNumberCommaWsp + '?' + rNumber (no comma/spaces after). We don't need the abstraction unless we want to be clear we're matching the grammar with variable names.

The grammar claims the first two have optional commaspaces, which is what that '?' is handling (we'd need a different variable if '?' handled the whole rNumberCommaWsp but we don't want that). I question how accurate that is but matching the grammar is probably best here.

asturur · 2020-09-06T10:12:32Z

test/unit/path_utils.js

+  QUnit.test('fabric.util.parsePath test special arcs', function(assert) {
+    assert.ok(typeof fabric.util.parsePath === 'function');
+    assert.ok(typeof fabric.util.makePathSimpler === 'function');
+    // eslint-disable-next-line max-len
+    var pathWithWeirdArc = 'p m19.801 17.771h1.457v-1.84c0-1.918.247-3.442.73-4.57.49-1.134 1.313-1.954 2.474-2.473 1.16-.512 2.75-.774 4.773-.774 3.578 0 5.367.875 5.367 2.629 0 .566-.187 1.055-.562 1.457-.371.406-.817.61-1.325.61-.238 0-.652-.047-1.234-.137a10.56 10.56 0 00-1.484-.133c-1.11 0-1.82.324-2.133.976-.316.653-.473 1.583-.473 2.797v1.457h1.504c2.336 0 3.504.707 3.504 2.114 0 1.004-.309 1.64-.93 1.91-.62.27-1.48.402-2.574.402h-1.504v16.238c0 1.215-.289 2.141-.863 2.778-.578.633-1.324.953-2.234.953-.871 0-1.594-.32-2.168-.953-.578-.637-.868-1.563-.868-2.778v-16.238h-1.683c-.914 0-1.617-.207-2.11-.613-.496-.414-.742-.95-.742-1.61 0-1.468 1.024-2.203 3.078-2.203';
+    var parsed = fabric.util.parsePath(pathWithWeirdArc);
+    var atLeastOneArc = false;
+    parsed.forEach(function(command) {
+      if (command.length > 1 && (command[0] === 'a' || command[0] === 'A')) {
+        assert.deepEqual(command.length, 8, 'Arc in SVG should always have size 8 but had less. Did not parse correctly.');
+        atLeastOneArc = true;
+      }
+    });
+    assert.ok(atLeastOneArc, 'No arcs found from SVG but at least one should have been parsed.');
+  });


a fine test is:

Suggested change

QUnit.test('fabric.util.parsePath test special arcs', function(assert) {

assert.ok(typeof fabric.util.parsePath === 'function');

assert.ok(typeof fabric.util.makePathSimpler === 'function');

// eslint-disable-next-line max-len

var pathWithWeirdArc = 'p m19.801 17.771h1.457v-1.84c0-1.918.247-3.442.73-4.57.49-1.134 1.313-1.954 2.474-2.473 1.16-.512 2.75-.774 4.773-.774 3.578 0 5.367.875 5.367 2.629 0 .566-.187 1.055-.562 1.457-.371.406-.817.61-1.325.61-.238 0-.652-.047-1.234-.137a10.56 10.56 0 00-1.484-.133c-1.11 0-1.82.324-2.133.976-.316.653-.473 1.583-.473 2.797v1.457h1.504c2.336 0 3.504.707 3.504 2.114 0 1.004-.309 1.64-.93 1.91-.62.27-1.48.402-2.574.402h-1.504v16.238c0 1.215-.289 2.141-.863 2.778-.578.633-1.324.953-2.234.953-.871 0-1.594-.32-2.168-.953-.578-.637-.868-1.563-.868-2.778v-16.238h-1.683c-.914 0-1.617-.207-2.11-.613-.496-.414-.742-.95-.742-1.61 0-1.468 1.024-2.203 3.078-2.203';

var parsed = fabric.util.parsePath(pathWithWeirdArc);

var atLeastOneArc = false;

parsed.forEach(function(command) {

if (command.length > 1 && (command[0] === 'a' || command[0] === 'A')) {

assert.deepEqual(command.length, 8, 'Arc in SVG should always have size 8 but had less. Did not parse correctly.');

atLeastOneArc = true;

}

});

assert.ok(atLeastOneArc, 'No arcs found from SVG but at least one should have been parsed.');

});

QUnit.test('fabric.util.parsePath can parse arcs correctly when no spaces between flags', function(assert) {

// eslint-disable-next-line max-len

var pathWithWeirdArc = 'a10.56 10.56 0 00-1.484-.133';

var expected = ['a', 10.56, 10.56, 0, 0, 0, -1.484, -0.133];

var parsed = fabric.util.parsePath(pathWithWeirdArc);

var command = parsed[0];

assert.deepEqual(command, expected, 'Arc should be parsed correctly.');

});

Be careful i did not run that test, so it can be wrong.

Other cases to tests:

path = 'a10.56 10.56 0 001-0.133'

~~(gonna have a look if they work)~~

This extra case should work and we should test it gets parsed

This failed when I tried it. It appears that the problem is fabric.reNum doesn't match rNumber is the parsePath method. I reverted to rNumber and the test passed. I think this is also the cause of the visual test failures. I'm checking in the code.

asturur · 2020-09-06T10:16:52Z

test/fixtures/f_blue.svg

+<?xml version="1.0" encoding="UTF-8"?>
+<svg width="50mm" height="50mm" version="1.1" viewBox="0 0 50 50" xmlns="http://www.w3.org/2000/svg" xmlns:cc="http://creativecommons.org/ns#" xmlns:dc="http://purl.org/dc/elements/1.1/" xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#">
+	<path d="m19.801 17.771h1.457v-1.84c0-1.918.247-3.442.73-4.57.49-1.134 1.313-1.954 2.474-2.473 1.16-.512 2.75-.774 4.773-.774 3.578 0 5.367.875 5.367 2.629 0 .566-.187 1.055-.562 1.457-.371.406-.817.61-1.325.61-.238 0-.652-.047-1.234-.137a10.56 10.56 0 00-1.484-.133c-1.11 0-1.82.324-2.133.976-.316.653-.473 1.583-.473 2.797v1.457h1.504c2.336 0 3.504.707 3.504 2.114 0 1.004-.309 1.64-.93 1.91-.62.27-1.48.402-2.574.402h-1.504v16.238c0 1.215-.289 2.141-.863 2.778-.578.633-1.324.953-2.234.953-.871 0-1.594-.32-2.168-.953-.578-.637-.868-1.563-.868-2.778v-16.238h-1.683c-.914 0-1.617-.207-2.11-.613-.496-.414-.742-.95-.742-1.61 0-1.468 1.024-2.203 3.078-2.203" fill="#0cc0f4"/>
+</svg>


is this file still needed?

It isn't now. I can remove it. Don't know if we'd want to place it in a visual test.

asturur · 2020-09-06T10:24:48Z

All in all this is an improvement, regradless if between svg 1.1 and 2.0 there are small differences.
Let me know if you can do the extra clean up.

asturur · 2020-09-06T10:30:21Z

in your branch anyway those are failing:
https://github.com/fabricjs/fabric.js/blob/master/test/visual/golden/arc1.png
https://github.com/fabricjs/fabric.js/blob/master/test/visual/golden/arc2.png
https://github.com/fabricjs/fabric.js/blob/master/test/visual/golden/arc3.png
(they have the matching SVG in the test/visual/assets folder).
Something is changing in how they are rendered and is not good

- Also had to revert rNumber

asturur · 2020-09-07T16:17:12Z

I think we can merge, old test pass, i ll make a visual test with one of your failing SVGs and i ll try my personal battle with that loop for arcs. Thanks this was a great PR we kept changes super tidy and we reused some code we had around.

…s#6571)

ISSUE-6566: Fix SVGs so for special Arc lines

6964c17

gloriousjob changed the title ~~ISSUE-6566: Fix SVGs so for special Arc lines~~ ISSUE-6566: Fix SVGs for special Arc lines Sep 4, 2020

gloriousjob added 4 commits September 4, 2020 14:42

ISSUE-6566: Fix lint issues

ee48d93

ISSUE-6566: Reduce variable

bd681d9

ISSUE-6566: Add test for weird arc values in SVG

7baf58a

ISSUE-6566: Fix lint

59985ed

asturur reviewed Sep 6, 2020

View reviewed changes

src/util/path.js Outdated Show resolved Hide resolved

asturur reviewed Sep 6, 2020

View reviewed changes

gloriousjob added 3 commits September 7, 2020 10:14

Add vscode to gitignore list

b69f9c5

Consolidate commaWsp

40447f3

Simplify unit test

173e270

- Also had to revert rNumber

asturur approved these changes Sep 7, 2020

View reviewed changes

asturur merged commit 014b91b into fabricjs:master Sep 7, 2020

gloriousjob deleted the ISSUE-6566-Fix-svg-render-arc-wrong branch September 7, 2020 18:22

asturur mentioned this pull request Sep 26, 2020

Version 4.2.0 #6618

Merged

shanicerae pushed a commit to shanicerae/fabric.js that referenced this pull request Jan 16, 2021

fix(fabric.utils): ISSUE-6566 Fix SVGs for special Arc lines (fabricj…

e32bac0

…s#6571)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ISSUE-6566: Fix SVGs for special Arc lines #6571

ISSUE-6566: Fix SVGs for special Arc lines #6571

gloriousjob commented Sep 4, 2020

gloriousjob commented Sep 4, 2020

asturur commented Sep 4, 2020

gloriousjob commented Sep 4, 2020

asturur commented Sep 4, 2020

gloriousjob commented Sep 4, 2020

fubuk-i commented Sep 5, 2020

asturur commented Sep 5, 2020

gloriousjob commented Sep 5, 2020 via email

gloriousjob commented Sep 6, 2020 •

edited

Loading

asturur Sep 6, 2020

asturur Sep 6, 2020

gloriousjob Sep 7, 2020 •

edited

Loading

asturur Sep 6, 2020

gloriousjob Sep 7, 2020

asturur Sep 6, 2020

asturur Sep 6, 2020 •

edited

Loading

gloriousjob Sep 7, 2020 •

edited

Loading

asturur Sep 6, 2020

gloriousjob Sep 7, 2020

asturur commented Sep 6, 2020

asturur commented Sep 6, 2020

asturur commented Sep 7, 2020

ISSUE-6566: Fix SVGs for special Arc lines #6571

ISSUE-6566: Fix SVGs for special Arc lines #6571

Conversation

gloriousjob commented Sep 4, 2020

gloriousjob commented Sep 4, 2020

asturur commented Sep 4, 2020

gloriousjob commented Sep 4, 2020

asturur commented Sep 4, 2020

gloriousjob commented Sep 4, 2020

fubuk-i commented Sep 5, 2020

asturur commented Sep 5, 2020

gloriousjob commented Sep 5, 2020 via email

gloriousjob commented Sep 6, 2020 • edited Loading

asturur Sep 6, 2020

Choose a reason for hiding this comment

asturur Sep 6, 2020

Choose a reason for hiding this comment

gloriousjob Sep 7, 2020 • edited Loading

Choose a reason for hiding this comment

asturur Sep 6, 2020

Choose a reason for hiding this comment

gloriousjob Sep 7, 2020

Choose a reason for hiding this comment

asturur Sep 6, 2020

Choose a reason for hiding this comment

asturur Sep 6, 2020 • edited Loading

Choose a reason for hiding this comment

gloriousjob Sep 7, 2020 • edited Loading

Choose a reason for hiding this comment

asturur Sep 6, 2020

Choose a reason for hiding this comment

gloriousjob Sep 7, 2020

Choose a reason for hiding this comment

asturur commented Sep 6, 2020

asturur commented Sep 6, 2020

asturur commented Sep 7, 2020

gloriousjob commented Sep 6, 2020 •

edited

Loading

gloriousjob Sep 7, 2020 •

edited

Loading

asturur Sep 6, 2020 •

edited

Loading

gloriousjob Sep 7, 2020 •

edited

Loading