Flattening SVG groups and handling transforms

[mxgrey]

This is a follow-up to #16 . I apologize for how long it's taken to follow through with this, but I've been preoccupied the last two weeks with relocating myself to the other side of the world for my job.

I'd say this PR is still a work-in-progress, but I wanted to submit it to begin the review process so I can get a little bit of live feedback. The to-do list as I see it is:

• [x] Finalize the API
• [x] Write more unit tests (e.g. tests for parsing each type of SVG transform string)
• [x] Complete all the documentation

Feel free to add items to that if you think I'm overlooking anything.

Despite what I proposed in my comment, I did not create a Group class. Instead, while I was implementing everything, I came to the conclusion that it makes more sense to just leverage the already existing xml.etree.ElementTree.Element class instead of creating a new custom class.

The design in this PR pivots away from some of the intentions that @mathandy seemed to have for the Document class. In particular, from the existing skeleton code, I got the impression that Document objects were supposed to contain two copies of the document information: one in the form of a xml.etree.ElementTree and one in the form of a collection of Path objects, probably maintained in a tree of groups somehow.

I decided to veer away from this direction in favor of a single-source-of-truth pattern. Now the Document class only contains data in the form of an xml.etree.ElementTree object. It provides convenience functions that convert between the ElementTree representation and the Path representations, with functions that assist in creating <g> (group) elements and placing <path> elements into those groups. This has the following trade-offs:

• Advantages: Code is simpler, and there is no risk of two sources of truth falling out of sync with each other. There is also no wasted effort in processing changes to the tree.
• Disadvantages: If someone wants to modify the ElementTree and then retrieve the new resulting list of paths, the Document class must re-traverse the entire xml tree.

My assumption is that the most common use-cases for the Document class will be to either (1) extract information from a pre-existing SVG document, or (2) create/append information into an SVG document. I don't think it's likely that users will want to do one and then the other on the same Document object. If they really want to do that, it can still be done with the current implementation; it just won't be optimized for their case.

I think the most important aspect of the added API is the function

Document.flatten_all_paths(
    self,
    group_filter=lambda x: True,
    path_filter=lambda x: True,
    path_conversions=CONVERSIONS)

The basic idea behind this function is that it will traverse the xml tree of the Document and transform all the paths it comes across into the root frame. group_filter is used to select which group elements will be traversed (by default, all of them), path_filter is used to select which path elements will be included (by default, all of them), and path_conversions is used to specify which path-like elements will be considered for inclusion (e.g. <circle>, <line>, <rect>. Note that users are allowed to exclude <path> from this dictionary if they want to).

This function returns a list of named tuples. Each named tuple contains (path, element, transform) where path is the transformed Path object, element is a reference to the original xml.etree.ElementTree.Element object that the path was parsed from (useful for retrieving element attributes), and transform is a numpy.matrix that defines the matrix transformation which was applied to produce path (this might be useful for users who want to figure out how to skew/scale the path thickness attribute).

There are other functions that were designed to support Document.flatten_all_paths(). Those auxiliary functions can be leveraged by users who want some extra granularity in what they're doing.

I'll continue working on this to finish up the checklist at the top. In the meantime if @mathandy has a chance to glance this over, I'd appreciate any feedback that can be offered. This is my first time working in Python, so I'm afraid I don't have a good sense for what constitutes "pythonic" code. Please feel free to harshly scrutinize what you see.

A few final notes:

• Some of the changes are simply from merging with the latest master branch, so the diff of this PR can be reduced by updating the ElementTree branch locally
• I'm noticing two test failures which are unrelated to my changes. One of the test failures seems to occur on the current master branch. I can offer fixes for these, but I think that should be done separately.

[mathandy]

Hey @mxgrey looks like you forked the ElementTree branch. The ElementTree branch contained a few (somewhat significant) changes to current functionality that are unrelated to this issue, and was also a bit out of date. I updated the ElementTree branch so it only contains the relevant changes and is up to date with the master branch. Would you please update your fork and resubmit your pull-request? That will make it much easier to see what you're proposing.

[mxgrey]

I'm looking at the new version of svg2paths in your updated ElementTree branch, and I see that you've replaced the dictionary of conversion functions into a long list of boolean options in the svg2paths function.

I would strongly recommend the earlier way of handling conversions for the following reasons:

1. It's more extensible (users can seamlessly add support for their own custom element types outside of the SVG standard).
2. The code is simpler (you don't have to evaluate a long list of if-statements).
3. You get separation of concerns (the function svg2paths takes in a single object that dictates how elements get converted, so svg2paths doesn't need to be concerned about the details of what different elements might exist and what function to call to convert them).

[mxgrey]

Alternatively, if the list of boolean options makes for a more pythonic API, perhaps the function that takes in the boolean options could construct the conversion dictionary based on the boolean options and then pass the dictionary along to a lower-level version of svg2paths that takes in a conversion dictionary like the original way that the function worked.

[mathandy]

I agree. The problem is that, the way I had it, it would break people's code. I.e. scripts written in the old syntax would no longer work.

The new Document class will offer that functionality anyways.

-Andy

On Mon, May 28, 2018 at 9:09 PM, Michael X. Grey notifications@github.com wrote:

Alternatively, if the list of boolean options makes for a more pythonic API, perhaps the function that takes in the boolean options could construct the conversion dictionary based on the boolean options and then pass the dictionary along to a lower-level version of svg2paths that takes in a conversion dictionary like the original way that the function worked.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mathandy/svgpathtools/pull/55#issuecomment-392647337, or mute the thread https://github.com/notifications/unsubscribe-auth/ADB0PQWCzR0QXD6rI-_QAGU3OgTtT9J8ks5t3MoJgaJpZM4UNwj1 .

[mxgrey]

Ah, I see. That works for me, then.

I've merged your latest ElementTree branch, so the diff should look better now.

[mathandy]

This looks good. I merged your fork into the ElementTree branch -- I'll figure out how to give you push access to that branch when I have a chance.

[manuelvolk]

Hi, is there something I can do, so the pull request can finally be merged?

[mathandy]

I'd love that. The ideal fix would be to finish the ElementTree branch, but I'll merge in any pull-request that

1. fixes issue #16 (i.e. allows svgpathtools to work with inherited transforms), and
2. doesn't interfere with current functionality

Assuming it works, there was a workaround (satisfying condition 1 but not 2) included in a pull-request around when issue #16 was originally opened. If you want to investigate, please do so -- it should be easy to find, feel free to ask if you have any trouble. For the sake of finally having a workaround that satisfies condition 2, it'd be acceptable to simply give the modified functions a new -- or even a new submodule if that makes more sense.

@mxgrey, I haven't heard from you in some time now -- are you still working on this? Any progress to share?

[mxgrey]

Sorry for the delays. My priorities at work got shifted around a few months ago, but I'll have time this week to wrap things up here.

[mathandy]

OK, great! @mxgrey

[mxgrey]

Sorry for all the delays. I believe this PR is ready for a final review now. All the new functions are documented and accompanied by unit tests. I've also been using this code for several months now in my own projects.

Please let me know if I've missed anything.

[mxgrey]

It seems that CI tests are failing after a module was renamed, so I'll patch that soon.

[mathandy]

Looking good @mxgrey!