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

Added Image::load_svg_from_(buffer|string) #78248

Merged
merged 1 commit into from Jul 12, 2023
Merged

Added Image::load_svg_from_(buffer|string) #78248

merged 1 commit into from Jul 12, 2023

Conversation

ghost
Copy link

@ghost ghost commented Jun 14, 2023

Fixes #3422

This PR adds two methods:

  • Image::load_svg_from_buffer(buffer, scale);
  • Image::load_svg_from_string(svg_str, scale).

The methods are mostly compatible with 3.x for cherrypick, but the changes introduced by migrating from nanosvg to thorvg should be accounted for.

You can use the snippet bellow to test both methods:

Expand to show the code

The SVG at the variable test_svg is like this: image

extends Node

# 4w 4h, 2x2 RED, 2x2 BLUE, 2x2 GREEN, 2x2 BLACK
#|-------|--------|
#|--RED--|-GREEN--|
#|-------|--------|
#|-------|--------|
#|-BLUE--|-BLACK--|
#|-------|--------|
var test_svg := """<?xml version="1.0" encoding="UTF-8"?>
<svg width="4" height="4" version="1.1" viewBox="0 0 1 1" xmlns="http://www.w3.org/2000/svg">
 <g stroke-linecap="square" stroke-width=".4">
  <rect width=".5" height=".5" fill="#f00"/>
  <rect width=".5" height=".5" fill="#f00"/>
  <rect x=".5" width=".5" height=".5" fill="#0f0"/>
  <rect y=".5" width=".5" height=".5" fill="#00f"/>
  <rect x=".5" y=".5" width=".5" height=".5"/>
 </g>
</svg>
"""


func _ready() -> void:
  var can_load_from_string := ClassDB.class_has_method(&'Image', &'load_svg_from_string')
  var can_load_from_buffer := ClassDB.class_has_method(&'Image', &'load_svg_from_buffer')

  var image := Image.new()

  print('can_load_from_string = %s' % can_load_from_string)
  print()

  if (can_load_from_string):
    print('scale = 2.0 (8w 8h)')
    image.load_svg_from_string(test_svg, 2.0)
    print_image_content(image)
    print()

    print('scale = 1.0 (4w 4h)')
    image.load_svg_from_string(test_svg)
    print_image_content(image)
    print()

    print('scale = 0.5 (2w 2h)')
    image.load_svg_from_string(test_svg, 0.5)
    print_image_content(image)
    print()

  print('can_load_from_buffer = %s' % can_load_from_buffer)
  print()

  var buffer := test_svg.to_utf8_buffer()

  if (can_load_from_buffer):
    print('scale = 2.0 (8w 8h)')
    image.load_svg_from_buffer(buffer, 2.0)
    print_image_content(image)
    print()

    print('scale = 1.0 (4w 4h)')
    image.load_svg_from_buffer(buffer)
    print_image_content(image)
    print()

    print('scale = 0.5 (2w 2h)')
    image.load_svg_from_buffer(buffer, 0.5)
    print_image_content(image)


func print_image_content(image: Image) -> void:
  for y in image.get_height():
    var line := '|'

    for x in image.get_width():
      var color := image.get_pixel(x, y)
      line += '#%02x%02x%02x%02x|' % [color.r8, color.g8, color.b8, color.a8]

    print(line)
Expand to show an output example
can_load_from_string = true

scale = 2.0 (8w 8h)
|#ff0000ff|#ff0000ff|#ff0000ff|#ff0000ff|#00ff00ff|#00ff00ff|#00ff00ff|#00ff00ff|
|#ff0000ff|#ff0000ff|#ff0000ff|#ff0000ff|#00ff00ff|#00ff00ff|#00ff00ff|#00ff00ff|
|#ff0000ff|#ff0000ff|#ff0000ff|#ff0000ff|#00ff00ff|#00ff00ff|#00ff00ff|#00ff00ff|
|#ff0000ff|#ff0000ff|#ff0000ff|#ff0000ff|#00ff00ff|#00ff00ff|#00ff00ff|#00ff00ff|
|#0000ffff|#0000ffff|#0000ffff|#0000ffff|#000000ff|#000000ff|#000000ff|#000000ff|
|#0000ffff|#0000ffff|#0000ffff|#0000ffff|#000000ff|#000000ff|#000000ff|#000000ff|
|#0000ffff|#0000ffff|#0000ffff|#0000ffff|#000000ff|#000000ff|#000000ff|#000000ff|
|#0000ffff|#0000ffff|#0000ffff|#0000ffff|#000000ff|#000000ff|#000000ff|#000000ff|

scale = 1.0 (4w 4h)
|#ff0000ff|#ff0000ff|#00ff00ff|#00ff00ff|
|#ff0000ff|#ff0000ff|#00ff00ff|#00ff00ff|
|#0000ffff|#0000ffff|#000000ff|#000000ff|
|#0000ffff|#0000ffff|#000000ff|#000000ff|

scale = 0.5 (2w 2h)
|#ff0000ff|#00ff00ff|
|#0000ffff|#000000ff|

can_load_from_buffer = true

scale = 2.0 (8w 8h)
|#ff0000ff|#ff0000ff|#ff0000ff|#ff0000ff|#00ff00ff|#00ff00ff|#00ff00ff|#00ff00ff|
|#ff0000ff|#ff0000ff|#ff0000ff|#ff0000ff|#00ff00ff|#00ff00ff|#00ff00ff|#00ff00ff|
|#ff0000ff|#ff0000ff|#ff0000ff|#ff0000ff|#00ff00ff|#00ff00ff|#00ff00ff|#00ff00ff|
|#ff0000ff|#ff0000ff|#ff0000ff|#ff0000ff|#00ff00ff|#00ff00ff|#00ff00ff|#00ff00ff|
|#0000ffff|#0000ffff|#0000ffff|#0000ffff|#000000ff|#000000ff|#000000ff|#000000ff|
|#0000ffff|#0000ffff|#0000ffff|#0000ffff|#000000ff|#000000ff|#000000ff|#000000ff|
|#0000ffff|#0000ffff|#0000ffff|#0000ffff|#000000ff|#000000ff|#000000ff|#000000ff|
|#0000ffff|#0000ffff|#0000ffff|#0000ffff|#000000ff|#000000ff|#000000ff|#000000ff|

scale = 1.0 (4w 4h)
|#ff0000ff|#ff0000ff|#00ff00ff|#00ff00ff|
|#ff0000ff|#ff0000ff|#00ff00ff|#00ff00ff|
|#0000ffff|#0000ffff|#000000ff|#000000ff|
|#0000ffff|#0000ffff|#000000ff|#000000ff|

scale = 0.5 (2w 2h)
|#ff0000ff|#00ff00ff|
|#0000ffff|#000000ff|

@ghost ghost requested review from a team as code owners June 14, 2023 23:33
@ghost ghost changed the title Added Image's load_svg_from_(buffer|string) Added Image::load_svg_from_(buffer|string) Jun 14, 2023
@Calinou Calinou added this to the 4.x milestone Jun 14, 2023
@Calinou

This comment was marked as resolved.

@ghost

This comment was marked as resolved.

doc/classes/Image.xml Outdated Show resolved Hide resolved
doc/classes/Image.xml Outdated Show resolved Hide resolved
doc/classes/Image.xml Outdated Show resolved Hide resolved
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally, it works. I can confirm this works in a release export template build too, as the SVG module in 4.0 and later is also enabled in non-editor builds.

The exposed API looks good to me.

@fire
Copy link
Member

fire commented Jun 15, 2023

This is pretty cool! I imagined this when we got svg in.

@akien-mga akien-mga modified the milestones: 4.x, 4.2 Jun 15, 2023
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks mostly good, but note that it creates a dependency in core on a module. We like to avoid core depending on modules as much as possible, especially for niche features.

Instead this function should be implemented the same as load_tga_from_buffer(). In that case Image exposes a function pointer that is assigned in the module.

@ghost
Copy link
Author

ghost commented Jun 15, 2023

@clayjohn

This looks mostly good, but note that it creates a dependency in core on a module. We like to avoid core depending on modules as much as possible, especially for niche features.

Instead this function should be implemented the same as load_tga_from_buffer(). In that case Image exposes a function pointer that is assigned in the module.

I started doing as you said (following the other load_* implementations) but I ended up doing it this way to not change too many files.

Agree, that's an interesting consideration about the core and modules, I'll change it soon.

Thanks.

@ghost ghost requested review from a team as code owners June 18, 2023 21:22
@ghost ghost requested a review from clayjohn June 18, 2023 22:40
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation looks good to me. The only thing I am unsure about (and I invite comment on from other reviewers) is whether we need to expose both load_from_string and load_from_buffer. Looking at the other formats we only expose load_from_buffer. Additionally since to_utf8_buffer() is exposed to users, users can trivially use load_from_buffer whether they have the svg in binary or in a string.

I don't have a strong opinion either way as it is only a few lines of code and this is not my area of expertise.

@fire
Copy link
Member

fire commented Jun 20, 2023

Looking at the other formats we only expose load_from_buffer.

This is enough justification for me to only expose load_from_buffer. I support this because string is svg's native format unlike png or jpg as an ease of use.

@ghost
Copy link
Author

ghost commented Jun 21, 2023

I've added Image::load_svg_from_string as a way of ease of use, it feels natural to use the String representation of SVGs.

I don't have a strong opinion on this either, I'm fine with just exposing Image::load_svg_from_buffer and keeping the previous patterns.

No core dependency to the svg module.
@YuriSizov
Copy link
Contributor

YuriSizov commented Jul 7, 2023

I think it's correct to offer a *_string variant. While it's trivial to convert a string to a buffer, it also feels unnecessary from the user perspective. Unlike binary image formats that we support right now, SVGs are naturally stored and passed around in a text form, so the string variant is likely the one that is going to be used most of the time.

@YuriSizov YuriSizov merged commit 6960a1d into godotengine:master Jul 12, 2023
@YuriSizov
Copy link
Contributor

Thanks!

@jordanlis
Copy link

Hi, just to understand. Does this allow to load SVG inside godot, and scale it as an SVG dynamically in the game ? Or should it be done in another proposal / PR ?

@Calinou
Copy link
Member

Calinou commented Aug 10, 2023

Hi, just to understand. Does this allow to load SVG inside godot, and scale it as an SVG dynamically in the game ? Or should it be done in another proposal / PR ?

Indeed, this allows loading SVGs in exported projects even if they weren't imported beforehand. They are still rasterized to an image (at the specified scale) though. This process is performed on the CPU and is relatively slow, so you don't want it to happen every frame when camera zoom changes.

@jordanlis
Copy link

Hi, just to understand. Does this allow to load SVG inside godot, and scale it as an SVG dynamically in the game ? Or should it be done in another proposal / PR ?

Indeed, this allows loading SVGs in exported projects even if they weren't imported beforehand. They are still rasterized to an image (at the specified scale) though. This process is performed on the CPU and is relatively slow, so you don't want it to happen every frame when camera zoom changes.

OK I see so it is no handled by the graphic card. So we won't be able to use svg everywhere instead of textures and it will still be rasterized.

@Calinou
Copy link
Member

Calinou commented Oct 23, 2023

Could this be used to rasterize SVG at runtime at different resolutions?

Yes, but you'll have to do it manually using the newly exposed method. Rasterizing during gameplay can be slow and cause stutters, so I recommend doing it at load-time.

Mipmaps don't seem to generate as good quality, often resulting in blurry textures

Mipmaps in Godot are generated with a box filter, which isn't ideal but this isn't the biggest issue for using mipmaps in 2D. There are arguably better filters out there, but they also come with their downsides.

The more prominent issue is that trilinear filtering is used by default, which isn't ideal for most mipmap usage in 2D. You can switch to bilinear filtering in the Project Settings (Use Nearest Mipmap Filter), but this will also affect 3D rendering which may be undesired. In a 2D-only project, you should be able to safely enable that setting. See godotengine/godot-proposals#7411.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a way to load SVG files from buffer at an adjustable scale
8 participants