Skip to content
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

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

Closed
akien-mga opened this issue Nov 18, 2020 · 7 comments · Fixed by #189
Closed

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

akien-mga opened this issue Nov 18, 2020 · 7 comments · Fixed by #189

Comments

@akien-mga
Copy link

akien-mga commented Nov 18, 2020

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 e7f5981.

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
Copy link
Author

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.)

akien-mga added a commit to godotengine/godot that referenced this issue Nov 18, 2020
This reverts commit f697e78.

Part of the update introduced a regression:
memononen/nanosvg#188.

We could include a local revert of the problematic commit but let's just do a
full revert to our previous version, and I'll re-update once the regression
is fixed upstream.

Fixes #43641.
@fvogelnew1
Copy link
Contributor

The following patch fixes the regression:

Change line 1477 of nanosvg.h from:

  • return nsvg__isdigit(*s);

To:

  • return (nsvg__isdigit(*s) || *s == '.');

Valid floating point numbers may start by a dot.

With this the icon displays as expected.

@fvogelnew1
Copy link
Contributor

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

@fvogelnew1
Copy link
Contributor

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

GryphonClaw pushed a commit to GryphonClaw/godot that referenced this issue Nov 19, 2020
This reverts commit f697e78.

Part of the update introduced a regression:
memononen/nanosvg#188.

We could include a local revert of the problematic commit but let's just do a
full revert to our previous version, and I'll re-update once the regression
is fixed upstream.

Fixes godotengine#43641.
andreas-kupries pushed a commit to tcltk/tk that referenced this issue Nov 19, 2020
oehhar added a commit to oehhar/tksvg that referenced this issue Nov 19, 2020
…to valid SVG icons #188', see memononen/nanosvg#188

From Tk commit https://core.tcl-lang.org/tk/info/2b07cb2b9e86e2c4
Bump Version to 0.7. Still waiting for big files bug decission in Tk for a release.
@oehhar
Copy link
Contributor

oehhar commented Nov 19, 2020

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

Thank you, Francois, that you care so well !

Thanks,
Harald

@akien-mga
Copy link
Author

@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.)

fvogelnew1 added a commit to fvogelnew1/nanosvg that referenced this issue Nov 19, 2020
@memononen
Copy link
Owner

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

Marirossibn added a commit to Marirossibn/Orca-deps-nanosvg that referenced this issue Jul 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants