fix geoproject for polygons with holes #146
Conversation
by way of consequence, geoproject and d3.geoProject() were creating garbage when applied to South Africa tests, etc @ https://beta.observablehq.com/d/47b2918ff37def88
|
This is not enough… the function fails with rings that cross the antimeridian or run around a pole. |
…gated to d3-geo's slower but correct functions
|
Also, this wouldn’t be backwards-compatible. The name “geoProject” is misleading, but I believe it was an intentional decision to make d3.geoProject assume planar input, so that it could be used for planar-to-planar transformations, too. If we want to make d3.geoProject assume spherical input, we’ll need a different name or an optional argument. |
|
Re: the first commit, I believe it's always been broken for polygons with holes — clockwise() was returning the opposite value of what we wanted in any case, spherical or planar. The second commit does indeed break planar coordinates if they extend beyond [-180, 180]. That use case is mentioned as a legal, if not typical, in the documentation. I'd prefer geoproject to consider spherical coords by default, but I understand if backwards-compatibility considerations would make the default planar, and request an explicit (The fun part is that I just wanted to use geoquantize — which has no reason to transform MultiPolygons into Polygons or vice-versa.) |
|
Do you by any chance have a copy of d3/d3#1558 (comment) (it got deleted at some point). It is referenced in d3/d3-geo#102 (comment) and in https://github.com/d3/d3-geo-projection/blob/master/src/project/index.js#L114 |
|
It's difficult for me to add an option to geoProject(), given how deep it is in the code. Will it work if we define it at the top of the closure? In the meantime I've coded a straightforward geoformat (or maybe geoprettyprint :-)) that works for my purposes, and might be a better pattern for geoquantize (?) |
|
"Clockwiseness" is evaluated on the projected object, so the question is not input being planar or spherical, but output. Which means the function is not broken, but should definitely not be used to project onto the sphere (i.e. #122, d3.geoIdentity applied on spherical coords, is the precise case that's broken). |
|
Here is another example where it would be nice to use d3.geoProject() in a sphere-to-sphere mode: But if you use remap2 (ie geoProject) instead of remap (ie just fiddling with line coordinates without using the geoStream), some polygons are in the wrong winding order and the map ends up all gray (color of land). |
Context: geoProject rewinds polygons to put them in the correct order as [outer ring, …holes], creating MultiPolygons as necessary. This is the desired (and semi-documented) behavior, however it precludes using geoProject with transforms that map spherical geometries to the sphere. This change allows to specify, with an optional *spherical* boolean flag, that the transform returns spherical coordinates. We can then call the relevant polygon winding algorithms: geoContains and a test on geoArea replace the planar contains and clockwise functions to determine is a ring is an outer ring or a hole. For clarity, the change is kept minimal in this commit, but before we merge I will reformat it (whitespace). An older tentative was at: #146
clockwise() was returning false for clockwise rings and vice-versa.
by way of consequence, geoproject and d3.geoProject() were creating garbage
when applied to South Africa
tests, etc @ https://beta.observablehq.com/d/47b2918ff37def88