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.

Sign up for GitHub

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

Jump to bottom

L.Ellipse #1993

Closed
wants to merge 1 commit into from
Closed

L.Ellipse #1993

wants to merge 1 commit into from

Conversation

patrickarlt
Copy link
Member

I've been working with Yuriy Dybskiy (@dybskiy) for the last few days to add Ellipses to Leaflet. He had a client that really needed to be able to draw ellipses (via Leaflet.draw) but since there is no native support for ellipses in Leaflet we needed to add it.

I have also been wondering why there is no support for ellipses which seems like a natural base class for L.Circle and L.CircleMarker.

This PR adds a new L.Ellipse class with a signature of L.Ellipse(latlng, [radiusX, radiusY], options).

I wanted to throw this up and see if its a good candidate for inclusion in the Leaflet core or would be better as a plugin.

@html5cat
Copy link

👍

@mourner
Copy link
Member

mourner commented Aug 25, 2013

Thanks Patrick, I'm currently on vacation but will take a closer look next week.

@jdfergason
Copy link
Contributor

I agree that the ellipse should be the base for circle. I don't see any options to affect the rotation of the ellipse though.
I added tilt, startAngle, and endAngle options in the commit jdfergason/Leaflet@b97caa435ad9466f4839b127427848f307b18d28

@mourner
Copy link
Member

mourner commented Dec 11, 2013

Reconsidering this after I finish the vector layers refactoring I'm doing right now #155.

@mourner mourner mentioned this pull request Dec 11, 2013
Closed
@patrickarlt
Copy link
Member Author

@mourner makes sense. I thinking this might look cleaner if the properties were radius and radiusY that way we can keep backward compatibility by having a radius property.

@jdfergason clearly you are much better at math then I am. I'm wondering if tilt should be a property in options though? The 4 arguments to a method seems a little much and tilt might be 0 quite often.

@mourner
Copy link
Member

mourner commented Jan 2, 2014

I've added low level ellipse rendering support in #2345 (to approximate projected circles with ellipses), check it out. Closing the pull as too much have changed in master after all the refactoring, but will be useful as a reference.

@jdfergason I'd love to see true ellipse implementation (with start/end and tilt) as a plugin.

@mourner mourner closed this Jan 2, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
undecided
Development

Successfully merging this pull request may close these issues.
4 participants
@patrickarlt @html5cat @mourner @jdfergason