Skip to content

Conversation

aparshin
Copy link
Contributor

I've added support of holes to L.Polygon.setLalLngs().

But there are several more problems with holes in polygons:

  • no public method to get holes (getLatLngs() returns only external ring)
  • no functions to modify holes similar to addLatLng(), spliceLatLngs(), etc.
  • no information about restrictions on holes: should they be strictly inside external ring, can they intersect, include each other, etc.

mourner added a commit that referenced this pull request Oct 14, 2013
Add holes support to L.Polygon.setLatLngs(). Fixes #1518
@mourner mourner merged commit 0f2da30 into Leaflet:master Oct 14, 2013
@mourner
Copy link
Member

mourner commented Oct 14, 2013

Thanks!

Not sure how we should go about the hole getter — perhaps it would be better for getLatLngs to return both rings if the second is present rather than having something like getHoles — this way it would be consistent with setLatLngs and constructor arguments, but could potentially break existing users if someone relied on the current behavior. But I guess the latter is unlikely, so we could do that for a major version.

For modifiers, we could introduce addHoleLatLng or addInnerLatLng (and splice... too).

Regarding restrictions, technically (with SVG/Canvas) there are none, so you can do anything. And I don't think we would like to enforce any kind of validity check here.

@tmcw thoughts?

@aparshin
Copy link
Contributor Author

Change of getLatLngs() semantic is the simplest solution, if you don't worry about backward compatibility...

By the way, there is no separation to external and internal contours in used rendering engines (SVG/Canvas). They define array of subpathes and fill rule (can be changed only in SVG). From this point of view, it is not very natural to define separate functions for inner/outer subpathes...

Another variant of public interface - introduce methods to handle points in single ring and method to get a ring. For example:

polygon.rings[n].addLatLng(...);
polygon.rings[n].spliceLatLng(...);
polygon.addRing(latlngs);
...

For backward compatibility existing methods can become aliases for the first ring...

And one more note: getBounds() should take into account all the rings, not only the first one (since "holes" can be outside "outer" ring).

@mourner
Copy link
Member

mourner commented Oct 15, 2013

Yeah, something like this may be a good option. We should also have consistency with MultiPolygon/MultiPolyline set/getLatLngs, which obviously work with an array of arrays.

I'm going to refactor vector layers so that MultiPolygon/MultiPolyline are made of one path with subpaths instead of how it is done atm, and this will allow those methods to be shared between them and Polygon (with multiple rings).

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.

2 participants