googlefonts / picosvg

Helps simplify SVG files. Intended for use as part of a font build.
Apache License 2.0
137 stars 12 forks source link

consider not removing translate except when coords or translate suspicious #158

Closed anthrotype closed 3 years ago

anthrotype commented 3 years ago

currently picosvg always strip the translation part of a gradientTransform's affine2x3 matrix (preapplying the translate onto the radialGradient's coordinates), thus leaving only an 2x2 matrix. It was originally meant to solve an issue in some noto-emojis with gradient coordinates with huge values that can't be encoded in COLRv1 because they exceed Fixed16.16 bounds: https://github.com/googlefonts/picosvg/issues/127

However this is done unconditionally on all radialGradients, not just the problematic ones with off-bound coords, and may introduce other issues due to rounding and float precision loss. Perhaps it would be safer to only do it when required.

anthrotype commented 3 years ago

I just found an instance where this transformation leads to a gradient that's rendered incorrectly.

Take the MOSQUITO U+1F99F 🦟

After #153, the wings (that use fill-opacity despite having stroke="none") are converted correctly by picosvg, and it look like this (same as the original noto-emoji/svg/emoji_u1f99f.svg):

However, after nanoemoji creates a COLRv1 font from it, the bloody red part of the body is displaying incorrectly seemingly with no gradients.

anthrotype commented 3 years ago

(sorry, I pressed "Comment" green button before finishing.. bear with me, this is complicated to explain)

anthrotype commented 3 years ago

After nanoemoji compiles the picosvg shown in my previous comment, the shade of the mosquito's abdomen appears incorrect compared to the original:

image

Similarly, if I convert the COLRv1 font back to SVG (with tools/colr2svg.py script), the resulting SVG looks incorrect (the belly's color should transition from grey to red, but it appeas all flat red):

However, if I comment-out the line in topicosvg() where the gradient translation is applied, and rebuild with nanoemoji, the mosquito's belly appears correct. This is the skia png:

image

And this is the colr2svg:

So, something is going on with the translated gradient in the COLRv1 font that makes it no longer display correctly. I'll keep investigating after lunch.

anthrotype commented 3 years ago

I isolated the problematic radialGradient from the mosquito here: https://codepen.io/anthrotype/pen/jOMbzbo

The original gradient (without picosvg's translation applied, and before extracting back the floats from COLRv1 fixed-point numbers) should look like this:

After picosvg translation, the gradient still looks the same. Similarly, when encoding to COLRv1 the original un-translated gradient, it still looks fine.

It's only when encoding to COLR the translated gradient that it looks wrong:

anthrotype commented 3 years ago

The problem is caused by the rounding to integer of the radial gradient coordinates (the circle centres) when the SVG is converted to COLRv1 paint. That's where we are losing precision.

In fact, it goes away if I increase the UPEM (e.g. below with 4096 instead of the default 1024):

image

anthrotype commented 3 years ago

reminder to self: try to move the call to SVG.round_floats inside topicosvg method after (instead of before) it _apply_gradient_translation to get more precision, see if that fixes the issue

anthrotype commented 3 years ago
diff --git a/src/picosvg/svg.py b/src/picosvg/svg.py
index 7482ec9..1c7407c 100644
--- a/src/picosvg/svg.py
+++ b/src/picosvg/svg.py
@@ -961,11 +961,12 @@ class SVG:
         self.remove_unpainted_shapes(inplace=True)
         self.normalize_opacity(inplace=True)
         self.absolute(inplace=True)
-        self.round_floats(ndigits, inplace=True)

         self._apply_gradient_translation(inplace=True)
         self._collect_gradients(inplace=True)

+        self.round_floats(ndigits, inplace=True)
+
         nano_violations = self.checkpicosvg()
         if nano_violations:
             raise ValueError(

unfortunately moving the rounding after we apply gradient translation doesn't change the result in this particular case of the MOSQUITO emoji.

anthrotype commented 3 years ago

If I slightly modify the original emoji_u1f99f.svg like this:

diff --git a/svg/emoji_u1f99f.svg b/svg/emoji_u1f99f.svg
index 69bec4fb..baee32ea 100644
--- a/svg/emoji_u1f99f.svg
+++ b/svg/emoji_u1f99f.svg
@@ -17,7 +17,7 @@
        c0,0-0.35,3.57-4.17,5.58c-3.07,1.61-6.34,0.85-6.34,0.85s-5.93,4.53-8.9,6.54c-2.53,1.71-7.54,4.98-8.45,6.19s-3.5,8.1-4.43,10.61
        c-1.16,3.12-2.36,6.39-4.48,5.53c-2.41-0.98-1.27-4.09,0.35-8.95c2.16-6.49,3.07-9.66,5.33-11.62C21.57,86.75,35.5,77,35.5,77
        s-3.32-8.75,2.87-11.77c5.7-2.78,9.2,0.4,9.2,0.4s0.45-4.63,4.98-5.83S60.44,59.7,60.44,59.7z"/>
-<radialGradient id="SVGID_1_" cx="85.6768" cy="57.1157" r="32.5009" fx="53.4573" fy="61.3827" gradientTransform="matrix(0.9962 -0.0872 0.0488 0.558 -2.4623 32.7128)" gradientUnits="userSpaceOnUse">
+<radialGradient id="SVGID_1_" cx="85.6768" cy="57.1157" r="32.5009" fx="53.5" fy="61.3827" gradientTransform="matrix(0.9962 -0.0872 0.0488 0.558 -2.4623 32.7128)" gradientUnits="userSpaceOnUse">
        <stop  offset="5.280447e-03" style="stop-color:#745963;stop-opacity:0"/>
        <stop  offset="0.3639" style="stop-color:#894B51;stop-opacity:0.3605"/>
        <stop  offset="1" style="stop-color:#AA3535"/>

and rebuild with current nanoemoji, it works.. Basically we get lucky with integer rounding and the fx/fy circle centre happens to fall where it should be. But it does feel a little bit like cheating.

image

anthrotype commented 3 years ago

according to the SVG spec, "If the point defined by ‘fx’ and ‘fy’ lies outside the circle defined by ‘cx’, ‘cy’ and ‘r’, then the user agent shall set the focal point to the intersection of the line from (‘cx’, ‘cy’) to (‘fx’, ‘fy’) with the circle defined by ‘cx’, ‘cy’ and ‘r’." https://www.w3.org/TR/SVG11/pservers.html

According to this https://stackoverflow.com/a/31577151, IE and Firefox handle this correctly whereas Chrome "clips the gradient when focal point is outside the SVG gradient radius"

anthrotype commented 3 years ago

one more relevant link, which suggest this may be a SVG 1.1 vs SVG 2.0 difference:

https://bugs.chromium.org/p/chromium/issues/detail?id=322487#c20

anthrotype commented 3 years ago

https://www.w3.org/TR/SVG2/pservers.html#RadialGradientNotes confirms that the behavior of a focal point that lies outside the end circle has changed from SVG 1.1 to 2.0. In the latter, a cone is created that is touched by the two circles. It's still unclear to me how we end up with https://github.com/googlefonts/picosvg/issues/158#issuecomment-737368154

rsheeter commented 3 years ago

https://skia-review.googlesource.com/c/skia/+/300558/18/src/ports/SkFontHost_FreeType_common.cpp#646 suggests Skia side uses SkGradientShader::MakeTwoPointConical. That references http://dev.w3.org/html5/2dcontext/#dom-context-2d-createradialgradient, which doesn't exist. https://html.spec.whatwg.org/multipage/canvas.html#dom-context-2d-createradialgradient does exist though; in reading it I don't immediately see why we'd get the behavior we're seeing. @drott is this potentially an issue in Skia or how we are mapping constructs to Skia?

Edit: SkTwoPointConicalGradient.cpp refers the reader to https://skia.org/dev/design/conical.

anthrotype commented 3 years ago

The following SVG animation (which I got from this article explaining the differences between SVG1.1 and SVG2 radialGradient) clearly shows what happens to a conical radial gradient when the focal point is moved outside of the end circle.

It looks to me as if the orientation of a tridimensional cone were flipped along the imaginary Z axis as the focal point moves from being inside to outside of the end circle. When it's inside, the tip of the cone appears towards the viewer's eye; when the focal point is outside, it appears as if we are looking at the bottom of the 3D cone, with the tip pointing downwards through the page in the opposite direction.

Or depending on how you feel, it could also appear as if we are looking inside a cone's hollow interior shape when the focal point lies inside the end circle; and when it's outside, as if we are looking at the cone's exterior shape.

https://user-images.githubusercontent.com/6939968/103455866-b8b62300-4ce8-11eb-987a-57b6b99b2ab2.mp4

<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="300" height="600">
<defs>
<radialGradient id="grad" cx="50%" cy="50%" r="30%" fx="50%" fy="50%" fr="10%">
    <animate attributeName="fx" values="-2.5%;102.5%;-2.5%" dur="6s" repeatCount="indefinite" begin="0s"/>
    <stop stop-color="#FFDB80" offset="0%"/>
    <stop stop-color="#FF791F" offset="50%"/>
    <stop stop-color="#E09F00" offset="100%"/>
</radialGradient>
</defs>
<rect width="300" height="300" fill="url(#grad)"/>
</svg>
anthrotype commented 3 years ago

Note that the rendering above is consistent in all the browsers I have tested, not just Chrome so I don't think it's a bug in skia, rather it's a feature of SVG2 radialGradients and HMTL5 canvas.

In any case, given that the effect of shifting the focal point between inside/outside has such a big visual impact, we clearly need to avoid it when converting gradients from SVG to COLR.

To recap: the issue with this particular MOSQUITO emoji is that, in the original SVG the gradient's focal point lies exactly on the end circle (or slightly within, which produces the same visual effect), whereas after the radial gradient is converted to COLR (where coordinates must be rounded to int16 integers) the focal point happens to fall just outside the end circle and thus produces the unexpected rendering.

Increasing the font UPEM (and thus the precision of the integer grid) fixes this particular emoji, but increases the file size of the whole font. Note that the use of int16 for gradient coordinates in COLRv1 matches the type used for point coordinates in glyf table and other places in the OT spec that refers to font units, so we should probably treat the rounding of floats as a necessity.

One possible way to mitigate the distorting effects that the necessary rounding may have on radial gradients with focal point at the edges of the end circle could be to not pre-apply the translation part of the gradientTransform -- unless when it is necessary in order to avoid the overflow error that occurs compiling large numbers exceeding signed 16-bit bounds (the original issue that the pre-translation was intended to fix).

In the particular case of the MOSQUITO emoji, skipping the pre-translation step when converting the original SVG gradient to COLRv1 does prevent the focal-point-outside-circle issue, suggesting that the pre-translation may compound or exaggerate the effect of the rounding.

Although, it is conceivable that even without pre-applying the gradient translation, the mere effect of rounding combined with a particular UPEM/viewBox ratio may still produce the focal-point-outside-circle problem. But I think it would at least be less likely.

My suggestion is we only do the gradient translation when any of the gradient values scaled by UPEM/viewBox ratio would overflow MAX_INT16 bounds (-2^15 ... 2^15-1): e.g. viewBox width is 128 and UPEM is 1024, and the gradient's x0 or gradientTransform.dx are > 4095 or < -4096 ((MAX_INT16 * viewBox / UPEM)).

anthrotype commented 3 years ago

Another perhaps better solution could be this. We explicitly detect this situation where the focal point lies on or very close to the end circle perimeter (if the distance from the focal point to the end circle's centre is almost equal to the end circle radius). And if the focal point lies on the perimeter or immediately inside, then move it a little bit towards the centre along the circle radius so that it is guaranteed to stay inside even after the rounding to integer. Coversely, if it was just outside the circle perimeter, nudge it a bit further away to prevent it from slipping inside the circle after rounding.

rsheeter commented 3 years ago

Nudging to avoid moving in/out when compiling makes sense to me given the current behavior. However, I'm still unclear if the behavior is a bug.

Imagine a VF that moves the circles around, it'll look sane ... and then abruptly break.

anthrotype commented 3 years ago

still unclear if the behavior is a bug

given that all browsers seem to agree on this behavior, I don't believe it's a bug.

It's not that insane, just arguably a bit arbitrary to only see it like a hollow grandient cone that one looks from the inside and then abruptly from the outside, as opposed to a cone that is pointing at the viewer and is thus always "outside"

anthrotype commented 3 years ago

the algorithm described in section "5.7.11.1.2.3 Radial gradients" of https://github.com/googlefonts/colr-gradients-spec/blob/master/OFF_AMD2_WD.md also agrees with this definition of radial gradient. In particular see figure 5.15: image

The key part is (emphasized with italic by me):

For all values of ω where r(ω) > 0, starting with the value of ω nearest to positive infinity and ending with the value of ω nearest to negative infinity, draw the circumference of the ellipse resulting from translating a circle with radius r(ω) centered at position (x(ω), y(ω)), with the color at ω, but only painting on the parts of the bitmap that have not yet been painted on by earlier circles in this step for this rendering of the gradient.

Then later on, there's another example that makes it clear the abrupt break (from focal point being outside to being inside or viceversa) is by design:

When one circle is contained within the other, the extension of the gradient beyond the larger circle will fill the entire surface. Colors in the areas inside the inner circle and outside the outer circle are determined by the extend mode.

image

All this is consistent with HTML Canvas createRadialGradient spec, as well as with SVG 2 radialGradient as well with all implementation of the latter. So definitely not a bug.