svglib fails to render properly

[MrBitBucket]

With svglib 1.5.1 I see that the pdf generated for https://flagicons.lipis.dev/flags/4x3/gb.svg is not as rendered in a browser. Something seems to be wrong with the middle of the union flag.

I tested with svglib 1.4.1 & 1.5.1 and reportlab 3.6.12 & 4.0.4.

I see a similar issue with this version https://freesvg.org/download/147712

but not with this one https://upload.wikimedia.org/wikipedia/commons/8/83/Flag_of_the_United_Kingdom_(3-5).svg

Is there some common error in the failures that browsers fix easily?

[github-actions]

Thank you for raising your first issue! Your help to improve svglib is much appreciated!

[deeplook]

Without running svglib on it, but looking only at this, likely more official, source... I see already some differences in the centre (British spelling, sic...). Have you tried that? https://en.wikipedia.org/wiki/File:Flag_of_the_United_Kingdom.svg

[MrBitBucket]

I find that svglib has a problem with passing fills into reportlab. Probably this was because we didn't have a fillMode attribute for most shapes and paths. So svglib tries a monkey patch fix which seems to be wrong. I used these examples

my patch tried to handle the path patching and attempts to copy _fillRule into SolidShapes. I'm a bit out of date with svglib so I don't know if this is correct. It does seem to impove the test svgs and the flag issue.

diff -cr orig/svglib/svglib.py patched/svglib//svglib.py
*** orig/svglib/svglib.py	2023-06-23 08:39:23.678689574 +0100
--- patched/svglib/svglib.py	2023-06-23 08:28:00.755261136 +0100
***************
*** 967,974 ****
              return None
  
          nudge_points(points)
-         polyline = PolyLine(points)
-         self.applyStyleOnShape(polyline, node)
          has_fill = self.attrConverter.findAttr(node, 'fill') not in ('', 'none')
  
          if has_fill:
--- 967,972 ----
***************
*** 982,987 ****
--- 980,988 ----
              group.add(polyline)
              return group
  
+         polyline = PolyLine(points)
+         self.applyStyleOnShape(polyline, node)
+ 
          return polyline
  
      def convertPolygon(self, node):
***************
*** 1390,1395 ****
--- 1391,1401 ----
              # is not recommended.
              shape.strokeColor = None
  
+         if isinstance(shape,SolidShape):
+             fillRule = getattr(shape,'_fillRule',None)
+             if fillRule:
+                 shape.fillMode = fillRule
+ 
  
  def svg2rlg(path, resolve_entities=False, **kwargs):
      """
***************
*** 1530,1546 ****
          return original_renderPath(path, drawFuncs, **kwargs)
      shapes._renderPath = patchedRenderPath
  
!     original_drawPath = Canvas.drawPath
  
!     def patchedDrawPath(self, path, **kwargs):
!         current = self._fillMode
!         if hasattr(path, 'fillMode'):
!             self._fillMode = path.fillMode
!         else:
!             self._fillMode = FILL_NON_ZERO
!         original_drawPath(self, path, **kwargs)
!         self._fillMode = current
!     Canvas.drawPath = patchedDrawPath
  
  
  monkeypatch_reportlab()
--- 1536,1552 ----
          return original_renderPath(path, drawFuncs, **kwargs)
      shapes._renderPath = patchedRenderPath
  
!     #original_drawPath = Canvas.drawPath
  
!     #def patchedDrawPath(self, path, **kwargs):
!     #   current = self._fillMode
!     #   if hasattr(path, 'fillMode'):
!     #       self._fillMode = path.fillMode
!     #   else:
!     #       self._fillMode = FILL_NON_ZERO
!     #   original_drawPath(self, path, **kwargs)
!     #   self._fillMode = current
!     #Canvas.drawPath = patchedDrawPath
  
  
  monkeypatch_reportlab()

[MrBitBucket]

In fact it seems easier to drop the monkeypatch cmpletely and change the rl name in the mapping for fill-rule from _fillRule to fillMode. It seems reportlab has supported fillMode for most things since 2017 so perhaps the _fillRule hacks can be dropped now.

[claudep]

I'm fine with this proposal, would you mind suggesting a patch?

[deeplook]

In fact it seems easier to drop the monkeypatch cmpletely and change the rl name in the mapping for fill-rule from _fillRule to fillMode. It seems reportlab has supported fillMode for most things since 2017 so perhaps the _fillRule hacks can be dropped now.

Removing code, especially in form or ugly hacks, while not changing functionality is always a good idea.

[MrBitBucket]

I will try and create a pull over the next few days. My git skills are lacking so it might be a learning exercise. For some reason I could not get the pytest(s) to run. Probably something wrong with the way I'm running it..

I did some testing and find that svg doesn't seem to mind at all when fill type attrubutes are used on non-fillables eg a line. In svglib these seem to end in a log message (because reportlab objects to say fillMode being set on a non SolidShape), but the conversion proceeds. I might add some more info into logger.debug("Exception during applyStyleOnShape")

[MrBitBucket]

I have created a pull request #381 which I think worls, but github is objecting to something.

[claudep]

We should fix #382 first, which will solve the test issue.

[MrBitBucket]

I'll resubmit the pull when you want.

[MrBitBucket]

I am amazed that [pre-commit.ci] has changed some of the python code and removed some white space. Why can't the robots actually fix the code logic? :)

[Danillooost]

@MrBitBucket I was still able to produce the issue besides your monkeypatch_reportlab()

Seems like its caused by missing arguments in convertPath

def convertPath(self, node):
    d = node.get('d')
    if not d:
        return None
    normPath = normalise_svg_path(d)
    path = Path()
...
etc

Path is being called but Path takes arguments which aren't filled in when using convertPath

fillMode will default to FILL_EVEN_ODD

class Path(SolidShape):

_attrMap = AttrMap(BASE=SolidShape,
    points = AttrMapValue(isListOfNumbers),
    operators = AttrMapValue(isListOfNumbers),
    isClipPath = AttrMapValue(isBoolean),
    autoclose = AttrMapValue(NoneOr(OneOf('svg','pdf'))),
    fillMode = AttrMapValue(OneOf(FILL_EVEN_ODD,FILL_NON_ZERO)),
    )

def __init__(self, points=None, operators=None, isClipPath=0, autoclose=None, fillMode=FILL_EVEN_ODD, **kw):

Maybe it's good to fix this. Im just new here but had a hell of a time today getting my broken signatures to fit in my pdf xD
Don't like it when my signatures misses some pieces because a few lines cross.

@deeplook