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

Added keepRepeatCommandNames option to .toString() method #11

Closed
wants to merge 1 commit into from
Closed

Added keepRepeatCommandNames option to .toString() method #11

wants to merge 1 commit into from

Conversation

danburzo
Copy link

@danburzo danburzo commented Dec 7, 2015

With this PR I've added the ability to drop the optimization regarding repeat command names in the string output.

I also wanted to start a discussion on whether this is the right approach here. As per the SVG 1.2 spec (SVG 2 is similar):

If a 'moveto' is followed by multiple pairs of coordinates, the subsequent pairs shall be treated as implicit 'lineto' commands.

So it's not correct to automatically transform M 10 10 M 10 100 M 100 100 M 100 10 Z to M 10 10 10 100 100 100 100 10 Z, as seen in this example (notice the difference in rendering): http://jsfiddle.net/u0eym6d7/

While it would be better to treat all the specific cases where removing repeat command names does not result in equivalent paths, this is a quick fix to disable that optimization.

Edit: I fixed it through the extra parameter so that existing behavior is not changed. I've checked the SVG spec and the M seems to be the only command affected by this, so the alternative can be changing the condition to:
skipCmd = i > 0 && (this.segments[i][0] === this.segments[i - 1][0] && this.segments[i][0].toLowerCase() !== 'm');

Waiting for your thoughts on this :-)

@puzrin
Copy link
Member

puzrin commented Dec 7, 2015

Thanks for clear example.

Is this fix for bug workaround only or you need unclamped notation for something else? If workaround only, i could try to write a direct fix (not clamp multiple M to one).

@danburzo
Copy link
Author

danburzo commented Dec 7, 2015

At the moment it's only to work around the bug, and on second thought a direct fix would work better I think. 👍 I was reluctant to alter existing behavior, and assumed there might be other use cases for disabling the clamping.

@emiltamas
Copy link

Also related: svg/svgo#189

@danburzo danburzo closed this Dec 7, 2015
@danburzo
Copy link
Author

danburzo commented Dec 7, 2015

I've closed the PR since the more I think about it the more this is the wrong approach :-)
We can move the relevant discussion to an issue I guess.

@puzrin
Copy link
Member

puzrin commented Dec 7, 2015

@danburzo this param looks very specific. I could understand, if it disabled all opts, not single one. My suggestion is:

  • fix M&m clamping directly
  • add param to disable all opts OR postpone that until more problem happens

@emiltamas thanks for ref.

puzrin pushed a commit that referenced this pull request Dec 7, 2015
@puzrin
Copy link
Member

puzrin commented Dec 7, 2015

Try 2.1.1, should fix that bug.

Thanks for your help!

@danburzo
Copy link
Author

danburzo commented Dec 7, 2015

Looks good, thank you!

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.

None yet

3 participants