godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
91.12k stars 21.18k forks source link

ImageLoaderSVG: Couldn't set target on ThorVG canvas. #72478

Closed Crystalwarrior closed 1 year ago

Crystalwarrior commented 1 year ago

Godot version

4.0.beta16

System information

Windows 10, Vulkan, NVIDIA GeForce GTX 1050

Issue description

Importing SVG's from Kenney.NL fails with the following message:

  ImageLoaderSVG: Couldn't set target on ThorVG canvas.
  core/io/image_loader.cpp:101 - Error loading image: res://assets/icons/skull.svg
  editor/editor_file_system.cpp:2004 - Error importing 'res://assets/icons/skull.svg'.

Steps to reproduce

  1. Grab icons from Kenney.NL here: https://www.kenney.nl/assets/board-game-icons
  2. Try importing an .svg from it
  3. The process will fail: image

Minimal reproduction project

N/A

fire commented 1 year ago

kenney_boardgameicons.zip

Image called skull.

skull

@mgrudzinska Here's a test case if you're interested.

image

mgrudzinska commented 1 year ago

@fire thanks for info :)

I see that the problem is that there's no viewbox attrib. We have pr solving this, but it has to be rebase/updated. Sorry for that. I'll take care of that today.

But still these svgs are a bit wired, because they are shifted and only part of the picture is visible - like the skull.svg you sent, that's not the whole skull... is that what you'd expect? I checked some other svg viewers and none of them showed the whole skull.

fire commented 1 year ago

I confirm that firefox shows the skull offset too.

capnm commented 1 year ago

:timer_clock: :timer_clock: :timer_clock: E.g. all the vector files from https://www.kenney.nl/assets/prototype-textures have the same issue. A quick »emergency« solution is to open the images in Inkscape, press sh+ctrl+r (resize page to selection) and overwrite the svg...

grafik

krita-menu

mgrudzinska commented 1 year ago

@capnm I'll try to make the pr ready asap - it is after rev and waiting for some changes.

mgrudzinska commented 1 year ago

https://github.com/thorvg/thorvg/pull/1318

@capnm please check the results - without the viewBox/viewPort info only a part of the pict is drawn. I've checked other svg viewers and haven't find any, that would draw the whole pict. Please comment on that.

capnm commented 1 year ago

@capnm please check the results - without the viewBox/viewPort info only a part of the pict is drawn. I've checked other svg viewers and haven't find any, that would draw the whole pict. Please comment on that.

@mgrudzinska Edit, edit, edit :) I think showing only the part is correct from the standard perspective if the image already has the width/height atribute. It could be an image bug, but you can use it on purpose.

e.g. in your PR test images:

test1.svg ```svg ```

When I recalculate the image width/height, it then brings in the app the access to hidden parts of the image. Maybe the lib could have (later) an option for this, Gogot could then have it in the export settings.

I built Godot together with thorvg tip and your PR, the Kenney images are now recognized,

Screenshot ![grafik](https://user-images.githubusercontent.com/4047289/224488953-d9b5be6f-05de-4336-b664-e07c111d1fe0.png)

but the clipping makes it unusable.

texture_01.svg ```svg ... ``` ![grafik](https://user-images.githubusercontent.com/4047289/224488820-93504456-b42d-42e5-aa61-514714aef5eb.png) Gnome Image viewer : ![grafik](https://user-images.githubusercontent.com/4047289/224490019-fe552268-127d-49a6-8e3d-b264ba3227b3.png)
mgrudzinska commented 1 year ago

@capnm I've updated the pr, but it's still under rev

capnm commented 1 year ago

@mgrudzinska I haven't had a chance to read through the code yet, had only a quick look at the SVG specs and Godot results with the last pr. The spec is, as you said, very css specific. Here, he also suggests, missing SVG size attributes should mean auto, so the whole image here, Godot now shows only 1x1 px image. Edit: or after a new copy randomly 1001x1001 px (+1px)

https://docs.google.com/presentation/d/1POUiroOBbLmXYlQKf0pIR8zVkHWH9jRVN-w8A4aNsIk/edit#slide=id.g1e19b0d66_285

prototype_textures_orig_vec.zip cc0

mgrudzinska commented 1 year ago

@mgrudzinska I haven't had a chance to read through the code yet, had only a quick look at the SVG specs and Godot results with the last pr. The spec is, as you said, very css specific. Here, he also suggests, missing SVG size attributes should mean auto, so the whole image here, Godot now shows only 1x1 px image.

I missed this, I'll solve this asap and let you know

capnm commented 1 year ago

Do you or someone know what should be done from the Godot side and/or what the intent is for ThorVG generated image, with zero svg or viewBox width/height attributes? If I'm reading the debugger backtraces correctly, Godot is currently getting an image with a width or height of zero, which results in error. In html/css context, which we don't have here, the intent is to not render the image. I think ThorVG should provide a transparent area in the image size or also as by auto the whole image in this case. :thinking:

https://www.w3.org/TR/SVG2/coords.html#ViewBoxAttribute tx_svg.zip

mgrudzinska commented 1 year ago

@capnm I didn't take a look in a godot code. the problem with incorrect size should be solved in the last commit. these are two separate cases:

capnm commented 1 year ago

@capnm I didn't take a look in a godot code. the problem with incorrect size should be solved in the last commit.

The current outcomes in the case of an empty svg tag -> image size + 1px:

Screenshot ![Bildschirmfoto von 2023-03-15 06-40-33](https://user-images.githubusercontent.com/4047289/225217712-29c8b631-e47a-4b1d-aca6-b50af5d1f9f8.png)
capnm commented 1 year ago

these are two separate cases:

* svgs with `viewbox="1 2 300 0"` (height=0) - nothing should be rendered - as I understand this is handled in godot correctly, right?

* svgs with `width="0"` or `height="0"` - there is a problem?
  If so, I'd like to handel the latter in the same way as the former - although it's not exactly the svg standard - I'll consult this with guys, just need to know whether it solves the problem

I'm just considering using Godot in our classes, so I'm looking at the errors now more from a philosophical side :)

E.g. Gnome image viewer shows by 0's in svg width or height and by viewbox(0 0 0 0) without svg's width and height - a 1x1 px image. In other cases, the full sized image.

I assume that Godot doesn't have access to the image data or the full size values. Guess: he is trying to create a zero size symbol here, giving out lots of internal errors and marking the image as broken. With zeros in the size, 1 px placeholder (+ documentation) could fix the issue... :thinking:

@fire ?

Screenshots ![Bildschirmfoto von 2023-03-15 05-38-59](https://user-images.githubusercontent.com/4047289/225246240-252ae4a8-b64d-4513-946c-a0edc325f53d.png) ![grafik](https://user-images.githubusercontent.com/4047289/225248884-8c047dfc-7b16-4a9d-a01e-93862804ebbc.png) [null.zip](https://github.com/godotengine/godot/files/10977607/null.zip)
fire commented 1 year ago

I think defaulting to a 1x1 image is better usability than fail looping, but I have not thought through the consequences.

capnm commented 1 year ago

The other option could be to remove everything from the SVG tag (this case works now, @mgrudzinska is almost there!) and render an equally sized transparent image (this would represent something similar to the "do not render" in the SVG standard) or the provided image (optionally?).

mgrudzinska commented 1 year ago

I'm quite sure, that for the t3.svg (viewBox="0 0 200 0") for the latest version of the corresponding pr (after changes pushed yesterday) you should get ERR_INVALID_DATA from the picture->load api. Let us discuss whether it's the best approach. we could render an empty scene and then both apis result in success.

for t8.svg (width="0") setting the canvas target won't return success, but you know the width and height values so you can set whatever values you want if some of them is zero. both as before - let us discuss this, because maybe we should handle this case exactly like the above...

capnm commented 1 year ago

@fire You wrote the bindings, please correct me if I got it wrong. The debugger has troubles stepping in the code (godot changes thorvg's pointers to internal api or multithreading issues).

1 Godot looks at the file extensions, loads the svg data itself and knows nothing else of the contents. 2 Sends it to thorvg's sw-renderer. 3 Gets the image data, width and height. 4 Attempts to change the color in the svg file and sends it to thorvg to create a file icon.

In cases the svg file contais swg's or viewBox's width or height (or both) 0, it gets zero width or height (or no success?) from the thorvg api. At this point we don't know the image size. The question is what to do next.

fire commented 1 year ago

Is zero size a failure case of import. It’s technically valid to zero width height image but I thought a zero area image is invalid.

capnm commented 1 year ago

In web context it's used to currently hide or not render the image.

capnm commented 1 year ago

@mgrudzinska quick test for latest godot tip + pr1318 godot_pr1318_diff.zip

Issues: svg without viewport/viewbox , \ (t8) and viewbox without viewport that contains 0s – get now marked as broken. Errors:

ERROR: ImageLoaderSVG: Couldn't set target on ThorVG canvas.
   at: create_image_from_utf8_buffer (modules/svg/image_loader_svg.cpp:103)
ERROR: Error loading image: res://prototype_textures_orig_vec/texture_02.svg
   at: load_image (core/io/image_loader.cpp:101)
ERROR: Error importing 'res://prototype_textures_orig_vec/texture_02.svg'.
   at: _reimport_file (editor/editor_file_system.cpp:2031)
ERROR: Error loading image: res://prototype_textures_orig_vec/texture_01.svg
   at: load_image (core/io/image_loader.cpp:101)
ERROR: Error importing 'res://prototype_textures_orig_vec/texture_01.svg'.
   at: _reimport_file (editor/editor_file_system.cpp:2031)
Screenshots ![230316](https://user-images.githubusercontent.com/4047289/225629862-dee27fd7-0c5a-42cd-9b25-5f8b755d8ab6.png) [vp_vb_tests.zip](https://github.com/godotengine/godot/files/10991257/vp_vb_tests.zip)
mgrudzinska commented 1 year ago

omg it's a huge diff :D but I don't see what did you change in godot in image_loader_svg.cpp. could you share?

mgrudzinska commented 1 year ago

I'll be available in a few hours and i run it myself

capnm commented 1 year ago

You were very hardworking :D _image_loadersvg.cpp hasn't been changed since 2022-12. You can clone the godot tip and run git apply pr1318.diff

mgrudzinska commented 1 year ago
  1. so, the viewbox error is now solved in #1318
  2. all svgs with the width or height value set to zero - in my opinion you have to handle the result in the image_loader_svg.cpp file. For now you'll get an error msg: ImageLoaderSVG: Couldn't set target on ThorVG canvas. because width and/or height are zero. you can change it to any value you want, like instead of zero use 1. it will be the canvas, but still nothing will be drawn, so maybe this is the sollution

I didn;t manage to install everything to run the code on my computer... butt the above should be helpfull

capnm commented 1 year ago

Here are the docs what you need for any OS, may help: https://docs.godotengine.org/en/latest/contributing/development/compiling/index.html I can take a look at it over the weekend.

capnm commented 1 year ago

@mgrudzinska After https://github.com/godotengine/godot/pull/75034 the svg import is now error free for all images we get with a 0 size!

The only small problem I can see is the case without viewport and viewbox, all images width/height is 1 px larger than it should be. Maybe some float rounding problems...

Details ```svg ``` ![grafik](https://user-images.githubusercontent.com/4047289/225991136-c494af50-d4c3-4d69-9062-dd704b0ad151.png) ![grafik](https://user-images.githubusercontent.com/4047289/226009376-78ffe6ff-3799-44fa-9966-beb78ae9335c.png) https://github.com/capnm/godot4/tree/update_thorvg_pr1318
mgrudzinska commented 1 year ago

@capnm good catch! I noticed this but I wanted to solve this after we agree with https://github.com/thorvg/thorvg/pull/1318

After https://github.com/godotengine/godot/pull/75034 the svg import is now error free for all images we get with a 0 size!

I hope that I've misunderstood - all of the images have a size equal 0? even if it shouldn;t be?

capnm commented 1 year ago

@mgrudzinska

I hope that I've misunderstood - all of the images have a size equal 0? even if it shouldn;t be?

Most images show a picture :+1: I didn't examine if they are correct cropped. The 0 size is no longer a bug after this patch. It renders 1 pixel images in this case.

From all test images, I posted: The 0 size cases are: \ (the Gnome Image Viewer: whole picture) -> t8.svg \ (giv: 1 pixel) \ (giv: 1 pixel) \ (giv: 1 pixel) \ (giv: 1 pixel) \ (giv: 1 pixel) same: \ \ This renders a transparent 200x200 sized images (giv: 200x200 cropped picture): \ \ \
mgrudzinska commented 1 year ago

great, so I'll take care now of this one extra pixel

mgrudzinska commented 1 year ago

https://github.com/thorvg/thorvg/pull/1333 should solve the extra pixel

but still we have https://github.com/thorvg/thorvg/issues/1331 and https://github.com/thorvg/thorvg/issues/1332 - both connected with svg without viewbox and width/height info. the problem described in the latter was mentioned in this issue.

mgrudzinska commented 1 year ago

@capnm please take a look: https://github.com/thorvg/thorvg/issues/1332#issuecomment-1478728411 do you have many such svgs that don;t have a viewbox/viewport info and are shifted so only a part of them is visible (like shield and skull on the very top of this issue)? I think that they are incorrect, so they should be cut and only partially visible, but I may be wrong. could you please comment?

mgrudzinska commented 1 year ago

@capnm please double check and let us know whether there is still some issue here

capnm commented 1 year ago

@mgrudzinska everything related to the image size and empty viewport/viewbox seems to be ok now. I have not yet received a response to #75034, I hope that one day it will be fixed too :)

image

mgrudzinska commented 1 year ago

@capnm @fire guys if there is still some issue regarding ThorVG please let me know. from my point of view it is solved already.

capnm commented 1 year ago

@mgrudzinska Currently I get ~90% crash by reading svg images on current godot + tvg tip https://github.com/thorvg/thorvg/issues/1370 The tests I looked at before were ok.

capnm commented 1 year ago

@mgrudzinska update: Both current godot 26fb911f79d7b16c46ca476923fe1f32ab5d27ed and thorvg 65116b87ada27689d79fa4db241cf777f3b268c5 github tip work together (the crash/dealock has been fixed). If the changes are in your next release, I don't see any issues.