memononen / nanosvg

Simple stupid SVG parser
zlib License
1.71k stars 363 forks source link

Regression from commit e7f5981 breaks many hitherto valid SVG icons #188

Closed akien-mga closed 4 years ago

akien-mga commented 4 years ago

We use nanosvg in https://github.com/godotengine/godot, and after syncing with the current master branch, most of our SVG icons end up broken (downstream issue).

Screenshot_20201118_120142

Screenshot_20201118_115850

I bisected the regression to:

e7f5981b1efef8cb5db6f62915ca4e25482b1e5b is the first bad commit
commit e7f5981b1efef8cb5db6f62915ca4e25482b1e5b
Author: Mikko Mononen <memononen@gmail.com>
Date:   Mon Sep 28 10:35:41 2020 +0300

    Fix for #178

    - make sure nsvg__addPath() hands only valid number of pointts (1+N*3)
    - require moveTo path command before handling other commands
    - require (sign+)digit for a valid path command coordinate
    - allow to add bezier segment only after there’s at leat one point (now
    also consistent with nsvg__lineTo)

 src/nanosvg.h | 49 +++++++++++++++++++++++++++++++++++++------------
 1 file changed, 37 insertions(+), 12 deletions(-)

CC @memononen

I haven't investigated further yet to see if it's actually most Godot icons which are invalid, or if it's indeed a wrong change in e7f5981b1efef8cb5db6f62915ca4e25482b1e5b.

For the reference, here's the relevant Godot code that uses nanosvg to import SVG files as textures: https://github.com/godotengine/godot/blob/master/modules/svg/image_loader_svg.cpp

akien-mga commented 4 years ago

Here's one of those icons which are now broken with e7f5981: ToolMove.zip

The full icon set is in https://github.com/godotengine/godot/tree/master/editor/icons

I tried this ToolMove.svg icon with the example code in https://github.com/memononen/nanosvg/tree/master/example (by replacing 23.svg and nano.svg, here's the output in e7f5981: Screenshot_20201118_121424 Screenshot_20201118_121550 (Second screenshot upscaled by 1600%.)

Here's how it looks like with e7f5981 reverted: Screenshot_20201118_121652 Screenshot_20201118_121740

(All these icons are MIT licensed, you're welcome to use any of them as regression test material.)

fvogelnew1 commented 4 years ago

The following patch fixes the regression:

Change line 1477 of nanosvg.h from:

To:

Valid floating point numbers may start by a dot.

With this the icon displays as expected.

fvogelnew1 commented 4 years ago

And of course the comment at the previous line may be updated.

fvogelnew1 commented 4 years ago

Fix pushed to Tk: https://core.tcl-lang.org/tk/info/2b07cb2b9e86e2c4

oehhar commented 4 years ago

Great work. Also pushed to https://github.com/oehhar/tksvg

Thank you, Francois, that you care so well !

Thanks, Harald

akien-mga commented 4 years ago

@fvogelnew1 Do you want to make a PR to this repo so that @memononen can merge it when they get to it? (And for other downstream users to have a clearly visible PR to cherry-pick in the meantime.)

memononen commented 4 years ago

@akien-mga Thanks for debugging it, and @fvogelnew1 thanks for the PR. Merged.