Fixed #16

[alphanoob1337]

Groups are now taken into account during parsing and transform attributes of groups are applied to the paths before they are returned.

[mathandy]

Hi @alphanoob1337 I take it you're somewhat new at contributing to open source projects -- that's ok, me too :) Also I want to say I'm psyched that you're contributing svgpathtools.

That said there are some major problems with this pull-request:

1. (OK this one is pretty minor) the fork you made this pull-request from is out of date. Thus if I merge your pull-request it will undo some recent work. Take a look at the diff.

2. It looks like you changed a lot more than necessary. There's nothing wrong with this, but how well tested is your code? Why should I trust your code is well tested?

You've chosen to add to a portion of svgpathtools that is currently very poorly covered by unittests (but well-tested by extensive use), that means any changes you make need to be carefully vetted.

Generally speaking, if the code you add/alter in a pull-request is more than a line or two, you should probably include thorough unittests to test the changes (or make sure there are unittests that already exist doing this). unittests are also a great way to explain to others working on a project what you think the expected behavior of a feature should be.

If you want to avoid adding unittests for existing functionality, then try to keep the impact your code potentially has to current features to a minimum.

3. svgpathtools adheres quite closesly to PEP 8 standards.
   Your code is far from PEP 8 standards. PyCharm does a great job flagging stylistic errors and makes renaming variables and functions quite simple.
   If you want I'm happy to do this part for you, but if you want the contribution credit (that listed automatically by GitHub), email me the code when you're ready to redo your pull-request and I'll e-mail it back fixed-up.

Thanks again for your contributing! It's awesome to have developers like you helping out.
-Andy

[alphanoob1337]

Hi Andy,

thanks for that answer! It is actually my first contribution to opensource software so I appreciate the guidance a lot. I will have a look at PyCharms and tests for the code on the weekend. I am not sure if I can publish the SVGs I had for testing, so I will create new ones. I think it is better if I learn the whole process of contributing for the future.

Cheers
Michael

[alphanoob1337]

Hi Andy,

Changes made as requested. Created an example SVG to test the transformations.

Cheers
Michael

[derVedro]

I think that this i a good begin for handling group transformation. But that doesn't handle concatinated transform attribute. To do this done you could try something like that:

def parse_trafo(transform_str):
"""
parses the whole g-transform-attribute string and computes the six values transformation matrix
"""

    # init matrix 
    matrix = [1., 0., 0., 1., 0., 0.]

    for some_transform in transform_str.lower().split(' '):
        trafo_str, _values = some_transform.split('(')
        # please don't hate me for that:
        values = list(map(float, _values.split(')')[0].split(',')))
        matrix = adjust_trafo(trafo_str, values, matrix)

    def adjust_trafo(trafo_str, values, matrix=[1., 0., 0., 1., 0., 0.]):
        if trafo_str == 'translate':
            x = values[0]
            y = values[1] if (len(values) > 1) else 0.      
            matrix[4] += x
            matrix[5] += y
            # same as: matrix[4:6] = map(float.__add__, matrix[4:6], (x, y))
        elif trafo_str == 'scale':
            x = values[0]
            y = values[1] if (len(values) > 1) else x
            matrix[0] *= x 
            matrix[3] *= y    
        elif trafo_str == 'rotate':
            if len(values) > 1:
                matrix = adjust_trafo("translate", values[1:], matrix)                
                matrix = adjust_trafo("rotate", [values[0]], matrix)
                matrix = adjust_trafo("translate", list(map(float.__neg__, values[1:])), matrix)
            else:
                a = values[0] * np.pi / 180.
                matrix[0] += np.cos(a)
                matrix[1] += np.sin(a)
                matrix[2] -= np.sin(a)
                matrix[3] += np.cos(a)
        elif trafo_str = 'skewX':
            a = values[0] * np.pi / 180.
            matrix[2] = np.tan(a)
        elif trafo_str == 'skewY':
            a = values[0] * np.pi / 180.
            matrix[1] =  np.tan(a)
        elif trafo_str == 'matrix':
            if len(values) == 6:
                matrix = values
            else:
                raise ValueError("matrix transform requieres 6 values")                
        return matrix

    return matrix

so you can just put it on each path element later. I don't tested the code, just write it down from my head. I am shure that this looks ugly and I think you have to update some other parts to. May be I get a working peace in few days.

[alphanoob1337]

Took me three minutes longer than you to fix it ;-) . I chose a very simmilar approach although I did not use recursion but put the parsing in a for loop.

[alphanoob1337]

Hi @mathandy,

Today I tried to address your first point (my fork not being up to date). When I tried to make a pull request on my fork with your master branch, I got

alphanoob1337:master is up to date with all commits from mathandy:master. Try switching the base for your comparison.

When I switch the base, only my changes are shown to me in the comparison. Can you please make a pull request on my fork with the changes it would undo when merging with yours? The 27 lines of yours that are missing in my fork are intentionally deleted by me.

Cheers
Michael

[mathandy]

These commented out lines, for example, were removed in a recent update.

[mathandy]

@alphanoob1337 sorry, I don't follow. I see your pull-requests, so what weren't you able to do? Were you unable to find the recent updates your code was missing? I pointed out one to help, but you should just download current zip and diff your svg2paths.py with the current master svg2paths.py. The unintended differences should be easy to spot.

I see you added some unittests, that's great!

A couple notes:

1. Your line length is too long. Please limit yourself to the 79 character PEP8 standard. It makes code easier to read (and easier to scroll through in many environments).
2. You still unnecessarily change a lot of working code. Please leave working functionality alone. Any alterations to functioning code should be minimal enough to be obviously not disruptive.

Why not just keep track of the transformations and which paths they affect and then apply the transformations after all the svg elements are converted to Path objects?
I never used groups much with SVGs, would it make sense to create a group_attributes dictionary for each group (or perhaps each path)?

Perhaps it actually makes more sense to include the group attributes in the attributes dictionary for the path, but that would require some verification of the rules (when group attributes override element attributes or vise-versa). This brings up some questions though...

1. Can a group be inside another group? If so, what if inner group and outer group have conflicting attributes (e.g. different strokes/colors)? What order would inner and outer group transformations be applied?
2. The same question for group attributes vs path (or other element) attributes. E.g. See below example.

Here's an example group attributes being overridden by path attributes and interacting transforms:

	<svg xmlns="http://www.w3.org/2000/svg"
	    xmlns:xlink="http://www.w3.org/1999/xlink">

		<g stroke="orange" 
			fill="white"
			stroke-width="5" 
			transform="rotate(45)">

			<path d='M 0,0 L 100,0 L 100,100 L 0,100z'
		          stroke="blue"
		          transform="translate(50,50)">
		    </path>
		</g>

		<g stroke="red" 
			fill="white" 
			stroke-width="5" 
			transform="translate(50,50)">

		    <path d='M 0,0 L 100,0 L 100,100 L 0,100z' 
		    	stroke='green' 
		    	transform="rotate(45)">
		    </path>
		</g>
	</svg>

[mathandy]

HI @alphanoob1337 , I'm going to close this pull request. If you're still interested in adding support for groups or group transform attributes in specific, let's have a conversation about how to best go about it under the issue #16 .