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

Bypassses #40 #51

Merged
merged 1 commit into from
Mar 18, 2018
Merged

Bypassses #40 #51

merged 1 commit into from
Mar 18, 2018

Conversation

disarticulate
Copy link
Contributor

I don't know what the other letters are for. But it resolves the error.

Might want to add a test.

I don't know what the other letters are for. But it resolves the error.

Might want to add a test.
@disarticulate disarticulate mentioned this pull request Mar 17, 2018
@jwass jwass merged commit a83d7b6 into jwass:master Mar 18, 2018
@Agnpalm
Copy link

Agnpalm commented Mar 22, 2018

This should not be merged IMO. The other letters are different SVG codes, you can find it here: https://developer.mozilla.org/en-US/docs/Web/SVG/Tutorial/Paths

There are three commands that draw lines. The most generic is the "Line To" command, called with L. L takes two parameters—x and y coordinates—and draws a line from the current position to a new position.

We can shorten the above path declaration a little bit by using the "Close Path" command, called with Z. This command draws a straight line from the current position back to the first point of the path. It is often placed at the end of a path node, although not always.

You can string together several Bezier curves to create extended, smooth shapes. Often, the control point on one side of a point will be a reflection of the control point used on the other side to keep the slope constant. In this case, you can use a shortcut version of the cubic Bezier, designated by the command S

Treating Z and S like L like this patch does is wrong and will lead to other problems.

@disarticulate
Copy link
Contributor Author

OK, so whats the solution

@Agnpalm
Copy link

Agnpalm commented Mar 26, 2018

Well, I'm not really familiar with the mplleaflet code or SVG, I just found it as one of few options to plot on maps in Python, and immediately ran into bug #40. The only thing I know about SVG is what I've read in the link to Mozilla I posted above.

However, the solution is actually written in the comment from four years ago:

# TODO: Do this smartly by finding when pathcodes changes value and do
# smart indexing on data, instead of iterating over each coordinate

Different path codes does different things and use a different amount of parameters. The current code treats all path codes the same and assumes that all of them takes two parameters, like the L command. I wrote a lengthy comment (#40 (comment)) about it when I did a quick nightly debugging session trying to make it work.

This patch is like wrapping everything in a try except block and ignoring the error.

I'm not saying the fault lies with you either, I know you were asked to make the patch by @jwass. I disagree with that decision, but then I'm just a new user that wants to be able to plot circles and rings on maps.

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.

3 participants