wagtail / Willow

A wrapper that combines the functionality of multiple Python image libraries into one API
https://willow.wagtail.org/
BSD 3-Clause "New" or "Revised" License
273 stars 53 forks source link

SVGs viewbox are skewed when dimensions contain decimal point value #112

Closed bva-adamjakab closed 1 year ago

bva-adamjakab commented 1 year ago

Issue Summary

SVGs that contain a decimal point value in the height or width attribute of the svg tag the viewbox values change when wagtail (Willow?) processes it and saves it to disk. The difference is small sometimes but in the example below the viewbox changes from 6600 64071.3625 which is far away from it's original values.

Steps to Reproduce

  1. Upload the following SVG file to the image gallery:
    <svg xmlns="http://www.w3.org/2000/svg" version="1.1" height="800px" width="1732.4px" viewBox="6600 3500 12750 8050" xmlns:xlink="http://www.w3.org/1999/xlink">
    <rect x="676" y="36" width="386" height="924" fill="#72BDD5"/>
    </svg>
  2. Inspect the uploaded image original source:
    <svg xmlns="http://www.w3.org/2000/svg" version="1.1" height="800.0" width="1732.4" viewBox="64071.3625 35218.75 17438.3125 8050.0">
    <rect x="676" y="36" width="386" height="924" fill="#72BDD5" />
    </svg>

Any other relevant information. For example, why do you consider this a bug and what did you expect to happen instead?

Technical details

jams2 commented 1 year ago

Thanks for the reproduction steps @bva-adamjakab

Confirmed this is occuring when generating the original rendition - it's happening during the crop operation. The rect being passed to willow to crop to is Rect(left: 0, top: 0, right: 1733, bottom: 800) in this case - Wagtail rounds the rect values. Interestingly I get a similar result when using the same SVG but with width="1732px", so I'm not convinced this has to to with fractional size attributes. Looking into it further.

Scotchester commented 1 year ago

Related issue in the Wagtail repo: https://github.com/wagtail/wagtail/issues/8042

zerolab commented 1 year ago

Reading https://github.com/wagtail/wagtail/issues/8042, this might be a Wagtail issue after all 🤔

jams2 commented 1 year ago

This is a Willow issue, the transformation applied to the new viewBox is incorrect when the SVG's initial viewport aspect ratio is not the same as the initial viewBox aspect ratio.

diff --git a/tests/test_svg_image.py b/tests/test_svg_image.py
index e62590a..51e66e5 100644
--- a/tests/test_svg_image.py
+++ b/tests/test_svg_image.py
@@ -331,6 +331,36 @@ class SvgImageTestCase(SvgWrapperTestCase):
         svg = SvgImage(self.get_svg_wrapper())
         self.assertEqual(svg.get_frame_count(), 1)

+    def test_crop_original_dimensions(self):
+        params = [
+            # viewport and viewBox with same aspect ratio, landscape
+            (80, 40, "0 0 200 100"),
+            # viewport and viewBox with different aspect ratios, landscape
+            (80, 40, "0 0 150 100"),
+            # viewport and viewBox with same aspect ratio, portrait
+            (40, 80, "0 0 100 200"),
+            # viewport and viewBox with different aspect ratios, portrait
+            (40, 80, "0 0 100 150"),
+            # viewport landscape, viewBox portrait
+            (80, 40, "0 0 100 200"),
+            # viewport portrait, viewBox landscape
+            (40, 80, "0 0 200 100"),
+        ]
+        for width, height, view_box in params:
+            with self.subTest(width=width, height=height, view_box=view_box):
+                svg = SvgImage(
+                    self.get_svg_wrapper(
+                        width=width,
+                        height=height,
+                        view_box=view_box,
+                    )
+                )
+                cropped = svg.crop((0, 0, width, height))
+                self.assertEqual(
+                    cropped.image.view_box,
+                    ViewBox(*(int(i) for i in view_box.split())),
+                )
+
     def test_crop_preserves_original_image(self):
         """
         Cropping should create a new image, leaving the original untouched.
adamjkb commented 1 year ago

Thanks @jams2, you are right, it is not related to fractional values. I manage to create a more succint reproduction:

width=100
height=100
viewBox=1 1 200 200

Basically whenever the min_x and min_y is not zero and the viewbox_width and viewbox_height is not equal to width and height, the min_x and min_y is getting scalled. Which will produce the following:

#input rect: Rect(left=0, top=0, right=100, bottom=100)
ViewBox(min_x=2.0, min_y=2.0, width=200.0, height=200.0)

#expected: ViewBox(min_x=1.0, min_y=1.0, width=200.0, height=200.0)

So, I think there are might be two issues at play.

  1. min_x and min_y getting scalled down. Proportional to the viewbox_width:width ratio https://github.com/wagtail/Willow/blob/35983f85af7181318f3e48c11b746c0dac64cd0c/willow/svg.py#L68
  2. And the above mentioned incorrect transition applied to viewbox when the aspect ratio doesn't match the svg image aspect ratio

Unfortunately, I cannot seem to get the project up and running so I might be missing something...

jams2 commented 1 year ago

My working hypothesis is that the cause is:

  1. using both scale_x and scale_y to transform the cropping rect (here and here), when we should use the factor that was chosen as a result of the preserveAspectRatio value (in the case of xMidYMid meet, the default, it's min(scale_x, scale_y); and
  2. applying translation to the cropping rect twice (here and here).

I'm working on more test cases to validate this, but in the case of performing the "original" crop on SVGs with a viewport/viewBox aspect ratio mismatch and preserveAspectRatio="xMidYMid meet", it holds out.


I think it might be helpful to explain why performing an "original" crop won't always yield an SVG with the same height, width and viewBox attributes as the original image (and consequently why the testcases in my previous comment will fail).

It comes down to the discrepancy between the viewport coordinate space, and the internal SVG coordinate space - which is referred to, somewhat confusingly in this context, as the "user coordinate space" (Coordinate Systems, Transformations and Units SVG 1.1, SVG 2).

Any crop operation takes as input a bounding rect in the viewport coordinate space. To perform a crop on the SVG, we need to transform the input bounding rect into the user coordinate space, and then update the SVG's width, height, and viewBox attributes correspondingly.

The following cases are important:

  1. the viewBox width and height match the SVG width and height attributes - there is a 1:1 mapping between viewport units and user units;
  2. the viewBox width and height don't match the SVG width and height attributes, but the aspect ratio is the same - viewport units are scaled from user units; and
  3. the viewBox and viewport have different aspect ratios - viewport units may or may not be scaled, depending on the value of preserveAspectRatio.

In all three cases, there may also be a translation along the x and/or y axes between viewport and user space.

Consider the following examples:

<svg width="600" height="400" viewBox="0 0 400 400">
    <!-- This rect is the same size as the viewBox -->
    <rect x="0" y="0" width="400" height="400" fill="yellow" stroke="red"></rect>
</svg>

translated-x-no-scale

<svg width="600" height="400" viewBox="-100 0 600 400">
    <rect x="0" y="0" width="400" height="400" fill="yellow" stroke="red"></rect>
</svg>

translated-x-no-scale-cropped

<svg width="600" height="400" viewBox="0 0 200 200">
    <!-- This rect is the same size as the viewBox -->
    <rect x="0" y="0" width="200" height="200" fill="yellow" stroke="red"></rect>
</svg>

translated-x-2x-scale

<svg width="600" height="400" viewBox="-50 0 300 200">
    <rect x="0" y="0" width="200" height="200" fill="yellow" stroke="red"></rect>
</svg>

translated-x-2x-scale-cropped

Note that all four SVGs are visually identical, but (0, 0) in each viewport space is distinct from (0, 0) in the corresponding user space, and distinct from each of the other SVGs.

The "cropped" versions above (the 2nd and 4th images above, the "original" renditions, as it were) do not have the same attributes as their uncropped versions, but present the same. The cropped versions have the nice property that the viewBox maps directly onto the viewport, however, which makes further cropping simpler in a sense.

All this is to say that I think I haven't done a great job of documenting the internals of SVG processing in Willow, which makes it difficult to work on. I would like to add some technical documentation that describes the process of cropping SVGs in particular.

zerolab commented 1 year ago

@adamjkb any chance you can give #113 a try and see if it addresses your use cases?

bva-adamjakab commented 1 year ago

@adamjkb any chance you can give #113 a try and see if it addresses your use cases?

Yes, this will address my use case, thank you!

zerolab commented 1 year ago

Version 1.5.1 is out now with the fixes from #113. Thank you all