Use ThorVG instead of NanoSVG for importing SVGs #49645

fire · 2021-06-16T05:07:36Z

ThorVG is a platform-independent portable library for drawing vector-based scene and animation.

Alternative to #49601.

Resolves the light to dark for fill, stop colors and strokes.

Fixes: #49205
Fixes: #16180 (Due to increased support we are able to resolve SVG issues better).
Fixes: #33641 (Changing SVG libraries replaces old bugs with new bugs.)
Fixes: #41339 (Via thorvg/thorvg#530)
Fixes: #51763
Fixes: godotengine/godot-proposals#2912

modules/thorvg/README.md

fire · 2021-06-16T15:46:39Z

There's a high velocity in the thorvg project. I posted broken test cases and the team responded the day after.

thorvg/thorvg#444

fire · 2021-06-22T15:39:55Z

@qarmin You may be able to test the svgs using this now in batch style.

fire · 2022-01-13T19:32:36Z

At the computer now, will look at the new commits and optimize the color conversion (it is slow).

fire · 2022-01-13T21:45:10Z

Is there a way in Godot Engine to take an array of string searches and an array of string replacements and does the substitution once rather than per number of searches? Used to convert the svg themes.

fire · 2022-01-14T01:01:01Z

@akien-mga I think we're ready and that it would be appropriate for this pr to be merge and wait for a follow-up but tracked in an issue..

For Light Theme users a startup time of 11 s when it used to be less than 9 s is a big difference.

Calinou · 2022-01-14T01:20:46Z

Benchmark with target=release_debug use_lto=no editor build:

OS: Fedora 34 (KDE, X11)
CPU: Intel Core i7-6700K
GPU: GeForce GTX 1080

godot includes this PR rebased against d13c3c9. godot.vanilla is d13c3c9 and doesn't include this PR.

Dark theme

❯ hyperfine -w1 -i "godot /tmp/4/project.godot --quit" "godot.vanilla /tmp/4/project.godot --quit"
Benchmark #1: godot /tmp/4/project.godot --quit
  Time (mean ± σ):      3.617 s ±  0.355 s    [User: 1.852 s, System: 0.245 s]
  Range (min … max):    3.092 s …  3.868 s    10 runs
 
Benchmark #2: godot.vanilla /tmp/4/project.godot --quit
  Time (mean ± σ):      3.275 s ±  0.357 s    [User: 1.821 s, System: 0.249 s]
  Range (min … max):    2.916 s …  3.816 s    10 runs
 
Summary
  'godot.vanilla /tmp/4/project.godot --quit' ran
    1.10 ± 0.16 times faster than 'godot /tmp/4/project.godot --quit'

Light theme

❯ hyperfine -w1 -i "godot /tmp/4/project.godot --quit" "godot.vanilla /tmp/4/project.godot --quit"
Benchmark #1: godot /tmp/4/project.godot --quit
  Time (mean ± σ):      3.923 s ±  0.404 s    [User: 2.268 s, System: 0.244 s]
  Range (min … max):    3.332 s …  4.264 s    10 runs
 
Benchmark #2: godot.vanilla /tmp/4/project.godot --quit
  Time (mean ± σ):      3.202 s ±  0.313 s    [User: 1.804 s, System: 0.253 s]
  Range (min … max):    2.927 s …  3.787 s    10 runs
 
Summary
  'godot.vanilla /tmp/4/project.godot --quit' ran
    1.23 ± 0.17 times faster than 'godot /tmp/4/project.godot --quit'

fire · 2022-01-14T03:05:25Z

Modified with a new code for replacing colors. The code subjectively feels 2 s versus 1.5 s in master to change the theme.

Thanks @akien-mga. Merged your changes.

akien-mga · 2022-01-14T10:16:51Z

modules/svg/image_loader_svg.cpp

-				Color new_color = n_c;
-				replace_colors.old_colors.push_back(old_color.to_abgr32());
-				replace_colors.new_colors.push_back(new_color.to_abgr32());
+		::Color c = ::Color::html(temp_color);


Does your IDE add this scoping? I don't see why it's needed here.

I'll fix it while doing some testing, I think the color replacement method could still be simplified, we have plenty of methods in String to do search and replace, doing it char by char seems pretty convoluted.

My clang compile requires the "::" scoping.

What clang is that? Works fine for me and on CI (macOS + iOS + Android all use clang).

We use Color everywhere in the codebase. What does it conflict with here?

You were also scoping Image in places but not consistently so it doesn't seem actually needed.

akien-mga · 2022-01-14T11:08:17Z

Just noting down some LTO warnings from ThorVG to report upstream (after checking that they're still reproducible in their master branch):

thirdparty/thorvg/src/loaders/external_png/tvgPngLoader.h:27:7: warning: type 'struct PngLoader' violates the C++ One Definition Rule [-Wodr]
   27 | class PngLoader : public LoadModule
      |       ^
thirdparty/thorvg/src/loaders/png/tvgPngLoader.h:29:7: note: a type with the same name but different number of polymorphic bases is defined in another translation unit
   29 | class PngLoader : public LoadModule, public Task
      |       ^
thirdparty/thorvg/src/lib/tvgTaskScheduler.h:42:8: note: the extra base is defined here
   42 | struct Task
      |        ^
thirdparty/thorvg/src/loaders/png/tvgLodePng.h:131:8: warning: type 'struct LodePNGInfo' violates the C++ One Definition Rule [-Wodr]
  131 | struct LodePNGInfo
      |        ^
thirdparty/basis_universal/encoder/lodepng.h:412: note: a different type is defined in another translation unit
  412 | typedef struct LodePNGInfo {
      | 
thirdparty/basis_universal/encoder/lodepng.h:443: note: the first difference of corresponding definitions is field 'background_defined'
  443 |   unsigned background_defined; /*is a suggested background color given?*/
      | 
thirdparty/basis_universal/encoder/lodepng.h:412: note: a type with different number of fields is defined in another translation unit
  412 | typedef struct LodePNGInfo {
      | 
thirdparty/thorvg/src/loaders/png/tvgLodePng.h:70:8: warning: type 'struct LodePNGDecompressSettings' violates the C++ One Definition Rule [-Wodr]
   70 | struct LodePNGDecompressSettings
      |        ^
thirdparty/basis_universal/encoder/lodepng.h:262: note: a different type is defined in another translation unit
  262 | struct LodePNGDecompressSettings {
      | 
thirdparty/thorvg/src/loaders/png/tvgLodePng.h:74:14: note: the first difference of corresponding definitions is field 'ignore_nlen'
   74 |     unsigned ignore_nlen; /*ignore complement of len checksum in uncompressed blocks*/
      |              ^
thirdparty/basis_universal/encoder/lodepng.h:267: note: a field with different name is defined in another translation unit
  267 |   unsigned (*custom_zlib)(unsigned char**, size_t*,
      | 
thirdparty/thorvg/src/loaders/png/tvgLodePng.h:144:8: warning: type 'struct LodePNGDecoderSettings' violates the C++ One Definition Rule [-Wodr]
  144 | struct LodePNGDecoderSettings
      |        ^
thirdparty/basis_universal/encoder/lodepng.h:618: note: a different type is defined in another translation unit
  618 | typedef struct LodePNGDecoderSettings {
      | 
thirdparty/thorvg/src/loaders/png/tvgLodePng.h:146:31: note: the first difference of corresponding definitions is field 'zlibsettings'
  146 |     LodePNGDecompressSettings zlibsettings; /*in here is the setting to ignore Adler32 checksums*/
      |                               ^
thirdparty/basis_universal/encoder/lodepng.h:619: note: a field of same name but different type is defined in another translation unit
  619 |   LodePNGDecompressSettings zlibsettings; /*in here is the setting to ignore Adler32 checksums*/
      | 
thirdparty/thorvg/src/loaders/png/tvgLodePng.h:70:8: note: type 'struct LodePNGDecompressSettings' itself violates the C++ One Definition Rule
   70 | struct LodePNGDecompressSettings
      |        ^
thirdparty/basis_universal/encoder/lodepng.h:730: warning: type 'struct LodePNGState' violates the C++ One Definition Rule [-Wodr]
  730 | typedef struct LodePNGState {
      | 
thirdparty/thorvg/src/loaders/png/tvgLodePng.h:161:8: note: a type with different virtual table pointers is defined in another translation unit
  161 | struct LodePNGState
      |        ^
thirdparty/thorvg/src/loaders/png/tvgPngLoader.h:41:5: warning: type of '__ct_comp ' does not match original declaration [-Wlto-type-mismatch]
   41 |     PngLoader();
      |     ^
thirdparty/thorvg/src/loaders/external_png/tvgPngLoader.cpp:45:1: note: '__ct_comp ' was previously declared here
   45 | PngLoader::PngLoader()
      | ^
thirdparty/thorvg/src/loaders/png/tvgLodePng.h:54: warning: type 'LodePNGColorType' violates the C++ One Definition Rule [-Wodr]
   54 | enum LodePNGColorType
      | 
thirdparty/basis_universal/encoder/lodepng.h:99: note: an enum with different number of values is defined in another translation unit
   99 | typedef enum LodePNGColorType {
      |

Some are internal redefinitions in ThorVG itself, and others are conflicts between the vendored LodePNG versions from Basis Universal and ThorVG.

akien-mga · 2022-01-14T11:55:31Z

Tested current PR with scons platform=linuxbsd tools=yes target=release_debug use_lto=yes and the results are pretty good.

Dark theme

$ hyperfine -iw1 "bin/godot.linuxbsd.opt.tools.64.vanilla ~/tmp/godot/test/NewTest/project.godot --quit" "bin/godot.linuxbsd.opt.tools.64.thorvg ~/tmp/godot/test/NewTest/project.godot --quit"
Benchmark #1: bin/godot.linuxbsd.opt.tools.64.vanilla ~/tmp/godot/test/NewTest/project.godot --quit
  Time (mean ± σ):      3.889 s ±  0.412 s    [User: 2.368 s, System: 0.289 s]
  Range (min … max):    3.424 s …  4.480 s    10 runs
 
  Warning: Ignoring non-zero exit code.
 
Benchmark #2: bin/godot.linuxbsd.opt.tools.64.thorvg ~/tmp/godot/test/NewTest/project.godot --quit
  Time (mean ± σ):      3.866 s ±  0.494 s    [User: 2.360 s, System: 0.288 s]
  Range (min … max):    3.116 s …  4.431 s    10 runs
 
  Warning: Ignoring non-zero exit code.
 
Summary
  'bin/godot.linuxbsd.opt.tools.64.thorvg ~/tmp/godot/test/NewTest/project.godot --quit' ran
    1.01 ± 0.17 times faster than 'bin/godot.linuxbsd.opt.tools.64.vanilla ~/tmp/godot/test/NewTest/project.godot --quit'

Light theme

$ hyperfine -iw1 "bin/godot.linuxbsd.opt.tools.64.vanilla ~/tmp/godot/test/NewTest/project.godot --quit" "bin/godot.linuxbsd.opt.tools.64.thorvg ~/tmp/godot/test/NewTest/project.godot --quit"
Benchmark #1: bin/godot.linuxbsd.opt.tools.64.vanilla ~/tmp/godot/test/NewTest/project.godot --quit
  Time (mean ± σ):      4.009 s ±  0.343 s    [User: 2.375 s, System: 0.287 s]
  Range (min … max):    3.514 s …  4.430 s    10 runs
 
  Warning: Ignoring non-zero exit code.
  Warning: The first benchmarking run for this command was significantly slower than the rest (4.430 s). This could be caused by (filesystem) caches that were not filled until after the first run. You should consider using the '--warmup' option to fill those caches before the actual benchmark. Alternatively, use the '--prepare' option to clear the caches before each timing run.
 
Benchmark #2: bin/godot.linuxbsd.opt.tools.64.thorvg ~/tmp/godot/test/NewTest/project.godot --quit
  Time (mean ± σ):      3.978 s ±  0.424 s    [User: 2.360 s, System: 0.286 s]
  Range (min … max):    3.492 s …  4.446 s    10 runs
 
  Warning: Ignoring non-zero exit code.
 
Summary
  'bin/godot.linuxbsd.opt.tools.64.thorvg ~/tmp/godot/test/NewTest/project.godot --quit' ran
    1.01 ± 0.14 times faster than 'bin/godot.linuxbsd.opt.tools.64.vanilla ~/tmp/godot/test/NewTest/project.godot --quit'

So it's pretty much the same for both dark and light theme with both current master (9b3535a) and this PR.

akien-mga · 2022-01-14T13:27:36Z

I pushed an update that simplifies the color conversion code by using existing String methods to do substring replacement, and by storing HTML code as strings in the conversion dictionary instead of doing an unnecessary HTML -> Color -> HTML conversion.

Despite the theoretical micro optimization, that seemed to make no difference at all in color conversion speed. But the code is nicer IMO :)

akien-mga · 2022-01-14T13:32:08Z

I need to do some more testing, I noticed that some assumptions don't work, as some files contain 3-letter color codes like fill="#fff".

I restored the previous version for now until I figure this out. I think going through String -> Color -> String conversion does actually help support this, as Color::html() supports abbreviated color codes, so I'll likely keep it.

ThorVG is a platform-independent portable library for drawing vector-based scene and animation. Co-authored-by: Rémi Verschelde <rverschelde@gmail.com>

akien-mga · 2022-01-14T14:53:39Z

Alright, I think it's ready.

I tweaked the color replacement code to make it handle the whole quoted string instead of hardcoding support for only the "#abcdef format. So now it should support:

Color codes without # (assuming they're supported in the spec, didn't check)
3-letter color codes (#abc)
Color codes with alpha (4 and 8 letter)
Named colors supported by Godot (black, blue, etc.)

While developing, I made it print colors for which it didn't find a conversion, so we can review and assess if some of those are meant to be converted:

Couldn't find color '#000' for property 'stroke="'.
Couldn't find color '#003e7a' for property 'fill="'.
Couldn't find color '#00f' for property 'fill="'.
Couldn't find color '#044b94' for property 'fill="'.
Couldn't find color '#0f0' for property 'fill="'.
Couldn't find color '#1aab1a' for property 'fill="'.
Couldn't find color '#288027' for property 'stop-color="'.
Couldn't find color '#2998ff' for property 'fill="'.
Couldn't find color '#3552b1' for property 'fill="'.
Couldn't find color '#404040' for property 'stroke="'.
Couldn't find color '#414042' for property 'fill="'.
Couldn't find color '#41562e' for property 'stroke="'.
Couldn't find color '#424242' for property 'stroke="'.
Couldn't find color '#4646ff' for property 'fill="'.
Couldn't find color '#46ff46' for property 'fill="'.
Couldn't find color '#474747' for property 'fill="'.
Couldn't find color '#478cbf' for property 'fill="'.
Couldn't find color '#4b4b4b' for property 'fill="'.
Couldn't find color '#62aeff' for property 'stop-color="'.
Couldn't find color '#66ff9e' for property 'stop-color="'.
Couldn't find color '#68b6ff' for property 'fill="'.
Couldn't find color '#68b6ff' for property 'stop-color="'.
Couldn't find color '#68b6ff' for property 'stroke="'.
Couldn't find color '#68d0ea' for property 'fill="'.
Couldn't find color '#699ce8' for property 'fill="'.
Couldn't find color '#69ec9e' for property 'fill="'.
Couldn't find color '#69f' for property 'fill="'.
Couldn't find color '#6e6e6e' for property 'fill="'.
Couldn't find color '#70bfff' for property 'fill="'.
Couldn't find color '#75d1e6' for property 'stop-color="'.
Couldn't find color '#77ce57' for property 'fill="'.
Couldn't find color '#7aff70' for property 'fill="'.
Couldn't find color '#808080' for property 'fill="'.
Couldn't find color '#84ffee' for property 'stop-color="'.
Couldn't find color '#919191' for property 'fill="'.
Couldn't find color '#999' for property 'fill="'.
Couldn't find color '#a2d2ff' for property 'fill="'.
Couldn't find color '#a2d2ff' for property 'stop-color="'.
Couldn't find color '#a3b6f3' for property 'fill="'.
Couldn't find color '#a5b7f3' for property 'fill="'.
Couldn't find color '#a5b7f4' for property 'fill="'.
Couldn't find color '#a5b8f3' for property 'fill="'.
Couldn't find color '#afafaf' for property 'fill="'.
Couldn't find color '#b3b3b3' for property 'fill="'.
Couldn't find color '#b8ea68' for property 'fill="'.
Couldn't find color '#b8ea68' for property 'stop-color="'.
Couldn't find color '#b8ea68' for property 'stroke="'.
Couldn't find color '#ccc' for property 'fill="'.
Couldn't find color '#ccc' for property 'stop-color="'.
Couldn't find color '#cf68ea' for property 'fill="'.
Couldn't find color '#cf68ea' for property 'stop-color="'.
Couldn't find color '#d5d5d5' for property 'fill="'.
Couldn't find color '#d6d6d6' for property 'fill="'.
Couldn't find color '#d9d9d9' for property 'fill="'.
Couldn't find color '#dbee15' for property 'stop-color="'.
Couldn't find color '#e1e1e1' for property 'fill="'.
Couldn't find color '#e6e6e6' for property 'fill="'.
Couldn't find color '#ea686c' for property 'fill="'.
Couldn't find color '#ea686c' for property 'stop-color="'.
Couldn't find color '#ea686c' for property 'stroke="'.
Couldn't find color '#eac968' for property 'fill="'.
Couldn't find color '#eae068' for property 'fill="'.
Couldn't find color '#eec315' for property 'stop-color="'.
Couldn't find color '#f2f2f2' for property 'fill="'.
Couldn't find color '#f5f5f5' for property 'fill="'.
Couldn't find color '#f5f5f5' for property 'stroke="'.
Couldn't find color '#f6f6f6' for property 'fill="'.
Couldn't find color '#f6f6f6' for property 'stroke="'.
Couldn't find color '#f70000' for property 'stop-color="'.
Couldn't find color '#f7f5cf' for property 'fill="'.
Couldn't find color '#fefefe' for property 'fill="'.
Couldn't find color '#fefeff' for property 'fill="'.
Couldn't find color '#ff4646' for property 'fill="'.
Couldn't find color '#ff5d5d' for property 'fill="'.
Couldn't find color '#ff7070' for property 'fill="'.
Couldn't find color '#ff7a7a' for property 'stop-color="'.
Couldn't find color '#ff8585' for property 'fill="'.
Couldn't find color '#ffa62a' for property 'fill="'.

No noticeable impact on speed compared to the previous version.

akien-mga · 2022-01-14T15:38:58Z

Thanks!

The wrongly claimed support for it was removed in godotengine#49645. See also godotengine#56862.

fire requested review from a team as code owners June 16, 2021 05:07

Calinou added enhancement topic:import labels Jun 16, 2021

Calinou added this to the 4.0 milestone Jun 16, 2021

fire force-pushed the thorvg branch 4 times, most recently from 4e3e2af to 3b43544 Compare June 16, 2021 06:06

fire marked this pull request as draft June 16, 2021 06:47

fire force-pushed the thorvg branch 2 times, most recently from 07d606d to 21bac89 Compare June 16, 2021 06:50

Xrayez reviewed Jun 16, 2021

View reviewed changes

modules/thorvg/README.md Outdated Show resolved Hide resolved

fire force-pushed the thorvg branch 10 times, most recently from f8cd613 to 3a0b665 Compare June 22, 2021 15:34

fire mentioned this pull request Jun 22, 2021

MSVC Compatibility thorvg/thorvg#447

Closed

fire force-pushed the thorvg branch 4 times, most recently from 7bf27bf to 523e0af Compare June 23, 2021 13:48

fire force-pushed the thorvg branch 2 times, most recently from a40ac25 to 033f08a Compare January 14, 2022 03:04

akien-mga reviewed Jan 14, 2022

View reviewed changes

akien-mga force-pushed the thorvg branch from 033f08a to 37506e6 Compare January 14, 2022 13:25

akien-mga force-pushed the thorvg branch 2 times, most recently from c4f8657 to a33ac17 Compare January 14, 2022 14:49

Use ThorVG instead of NanoSVG for importing SVGs

8d02759

ThorVG is a platform-independent portable library for drawing vector-based scene and animation. Co-authored-by: Rémi Verschelde <rverschelde@gmail.com>

akien-mga force-pushed the thorvg branch from a33ac17 to 8d02759 Compare January 14, 2022 14:49

akien-mga approved these changes Jan 14, 2022

View reviewed changes

akien-mga merged commit 8866c36 into godotengine:master Jan 14, 2022

fire deleted the thorvg branch January 14, 2022 15:39

This was referenced Jan 14, 2022

Editor icon colors which do not get converted for Light theme #56790

Open

Hide SVG scale for non-SVG textures #56850

Merged

akien-mga added a commit to akien-mga/godot that referenced this pull request Jan 17, 2022

Remove property hints referencing unsupported svgz extension

7176a43

The wrongly claimed support for it was removed in godotengine#49645. See also godotengine#56862.

akien-mga mentioned this pull request Jan 17, 2022

Remove property hints referencing unsupported svgz extension #56863

Merged

KoBeWi mentioned this pull request Jan 26, 2022

[4.0] Certain icons are shrunken (SVG scale not taken into account) #57129

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use ThorVG instead of NanoSVG for importing SVGs #49645

Use ThorVG instead of NanoSVG for importing SVGs #49645

fire commented Jun 16, 2021 •

edited

Loading

fire commented Jun 16, 2021

fire commented Jun 22, 2021

fire commented Jan 13, 2022

fire commented Jan 13, 2022

fire commented Jan 14, 2022

Calinou commented Jan 14, 2022 •

edited

Loading

fire commented Jan 14, 2022 •

edited

Loading

akien-mga Jan 14, 2022

akien-mga Jan 14, 2022

fire Jan 14, 2022

akien-mga Jan 14, 2022 •

edited

Loading

akien-mga commented Jan 14, 2022

akien-mga commented Jan 14, 2022

akien-mga commented Jan 14, 2022

akien-mga commented Jan 14, 2022 •

edited

Loading

akien-mga commented Jan 14, 2022 •

edited

Loading

akien-mga commented Jan 14, 2022

Use ThorVG instead of NanoSVG for importing SVGs #49645

Use ThorVG instead of NanoSVG for importing SVGs #49645

Conversation

fire commented Jun 16, 2021 • edited Loading

fire commented Jun 16, 2021

fire commented Jun 22, 2021

fire commented Jan 13, 2022

fire commented Jan 13, 2022

fire commented Jan 14, 2022

Calinou commented Jan 14, 2022 • edited Loading

Dark theme

Light theme

fire commented Jan 14, 2022 • edited Loading

akien-mga Jan 14, 2022

Choose a reason for hiding this comment

akien-mga Jan 14, 2022

Choose a reason for hiding this comment

fire Jan 14, 2022

Choose a reason for hiding this comment

akien-mga Jan 14, 2022 • edited Loading

Choose a reason for hiding this comment

akien-mga commented Jan 14, 2022

akien-mga commented Jan 14, 2022

Dark theme

Light theme

akien-mga commented Jan 14, 2022

akien-mga commented Jan 14, 2022 • edited Loading

akien-mga commented Jan 14, 2022 • edited Loading

akien-mga commented Jan 14, 2022

fire commented Jun 16, 2021 •

edited

Loading

Calinou commented Jan 14, 2022 •

edited

Loading

fire commented Jan 14, 2022 •

edited

Loading

akien-mga Jan 14, 2022 •

edited

Loading

akien-mga commented Jan 14, 2022 •

edited

Loading

akien-mga commented Jan 14, 2022 •

edited

Loading