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

Issue #366 - Add support for transforms on clipPath elements #367

Conversation

blayzen-w
Copy link

@blayzen-w blayzen-w commented Dec 9, 2022

See Issue #366 for details.

Let me know if you would like anything changed.

I want to validate this on a few more larger files before I move it out of draft mode.

@blayzen-w
Copy link
Author

The only issue that I have found so far is that scale transforms don't seem to be sandboxed correctly (translate works correctly). At this point I'm not sure if this is an issue with reportlab or svglib. I'll take a look at this some more on Monday but I'll put my findings below.

Take the following svg:

<svg namespace="http://www.w3.org/XML/1998/namespace" 
     xmlns="http://www.w3.org/2000/svg" 
     xmlns:xlink="http://www.w3.org/1999/xlink" version="1.1" 
     viewBox="0 0 100 100">
     
    <defs>
        <clipPath id="clip-1">
            <path transform="scale(2,1)" d="M 50, 50 m -50, 0 a 50,50 0 1,0 100,0 a 50,50 0 1,0 -100,0"/>
        </clipPath>
    </defs>
    <g transform="scale(1,3)">
        <g clip-path="url(#clip-1)">
            <path d="M50,50H0V0h50V50z" fill="red"/>
        </g>
    </g>
</svg>

The square should be 50x150, however its being generated at 100x150. It almost seems that the clip path scale it being applied to the square that its clipping.

f.seek(0)
return f

tar_data = opener(join(TEST_ROOT, "samples", archive_path)).read()
Copy link
Collaborator

Choose a reason for hiding this comment

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

May I ask you to put this part in a separate PR, please?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I may work on this now, in fact, wait a bit.

Copy link
Collaborator

Choose a reason for hiding this comment

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

See #368. You can drop this part of the patch.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, no problem. I just wanted to make sure the test cases were passing.

@blayzen-w
Copy link
Author

It looks like that issue that I'm running into is a bug with reportlab.

From what I can tell, the transformation is being applied when rendering the clipPath element, however when it's finished the state is never unapplied. So the transformation is then applied to the next element that is rendered. This can be seen in the drawNode function where it doesn't save and restore the canvas state if it's a path with clipPath set (if not (isinstance(node, Path) and node.isClipPath)).

    def drawNode(self, node):
        """This is the recursive method called for each node
        in the tree"""
        #print "pdf:drawNode", self
        #if node.__class__ is Wedge: stop
        if not (isinstance(node, Path) and node.isClipPath):
            self._canvas.saveState()

        #apply state changes
        deltas = getStateDelta(node)
        self._tracker.push(deltas)
        self.applyStateChanges(deltas, {})

        #draw the object, or recurse
        self.drawNodeDispatcher(node)

        self._tracker.pop()
        if not (isinstance(node, Path) and node.isClipPath):
            self._canvas.restoreState()

When the check is removed and the canvas state is saved and restored, the element is sized correctly but the clip path is not applied. They don't seem to host their code publicly so hopefully they are receptive to changes/bug reports from the public. Otherwise maybe we might be able to monkey patch it but I don't care for those too much.

@claudep
Copy link
Collaborator

claudep commented Dec 12, 2022

You can submit patches to reportlab with attaching the patch to a message to the [email protected] mailing list.

The official repo is https://hg.reportlab.com/hg-public/reportlab, and you can find a Git mirror on https://github.com/MrBitBucket/reportlab-mirror

@@ -895,7 +909,7 @@
class Svg2RlgShapeConverter(SvgShapeConverter):
"""Converter from SVG shapes to RLG (ReportLab Graphics) shapes."""

def convertShape(self, name, node, clipping=None):
def convertShape(self, name, node, clipping=None, transform_shape=False):

Check notice

Code scanning / CodeQL

Explicit returns mixed with implicit (fall through) returns

Mixing implicit and explicit returns may indicate an error as implicit returns always return None.
@blayzen-w
Copy link
Author

blayzen-w commented Jan 9, 2023

I haven't heard anything from reportlab so I did a bit of digging in the PDF ISO 32000-1 spec and found this interesting note.

There is no way to enlarge the current clipping path or to set a new clipping path without reference to the current one. However since the clipping path is part of the graphics state, its effect can be localized to specific objects by enclosing the modification of the clipping path and the painting of those objects between a pair of q and Q operators. - page 138.

So if I'm understanding this correctly, trying to apply a transform on the clipping path may be a dead end, since the clipping path transform needs to not depend on the current graphical state because it's referenced. I have almost no experience working with pdfs so I could be misunderstanding things, and I don't have access to ISO 32000-2 to see if anything has changed since 2008.

I was hoping this would be an easy fix since Illustrator seems to import svgs just fine, but I guess not. I'm not sure when I will have time to continue working on this. If you want I can close the draft and open a new one once I find a better solution.

@claudep
Copy link
Collaborator

claudep commented Jan 10, 2023

Thanks a lot for your explorations. Feel free to close or to let open depending on your future plans.

@blayzen-w
Copy link
Author

No problem. Thanks for all of your work on this project, I really appreciate it.

I'll close this draft but leave the issue open so other people can reference it later. I hope to be able to work on it again sometime later this year but we have a few larger projects that need to get done first.

@blayzen-w blayzen-w closed this Jan 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants