-
Notifications
You must be signed in to change notification settings - Fork 22.9k
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
Possible fix for #2025, handle zero-area polygons better #2784
Conversation
@mbostock please can you take a look at this? We are going to release a Canvas-based map library and don't want to rely on forks. |
The math here is extremely nuanced and reviewing this change, albeit a tiny one, is deceptively difficult. I’d love @jasondavies input here, but I haven’t heard from him recently. I’d like to review this change but I have a lot of work already on my plate trying to get D3 4.0 released ASAP, so I’m not sure when I’ll be able to dive into it. |
Can you share the data that led to the winding-order problem? That would help me test this change. I just fixed a related bug #2024 which turned out to be in topojson.merge: it was selecting the wrong exterior arc after merging the polygons. That arc happened to be degenerate, with the wrong winding order. I’ve fixed topojson in 1.6.26 and I’ve pushed new data files to bl.ock 4090846 which is used by many of my D3 examples. |
Okay, unfortunately this doesn't seem to be the issue here. I updated this block to use the latest official topojson and d3 releases. You can see that the bounds are fine now, but at least one polygon "leaks". http://bl.ocks.org/lukasappelhans/967f744f80ff2209ad946570063210f4 |
Can you also post how you made municipios.json? |
Strangely, I’m not able to reproduce the bounding box or rendering problem with the data you provided. It seems to work fine? The bounding box returned by d3.geo.bounds is: [
[-18.161305014722235, 27.637839000751285],
[4.3277847236038625, 43.79237956913792]
] This seems reasonable. It looks like when rendered it’s not exactly capturing all of the Canary Islands, and it extends a bit too far to the North, so I suppose there could be a small bug in d3.geo.bounds. But it looks approximately correct. |
Yes, d3.geo.bounds is fine with the other PR merged. The rendering also works as long as I keep dynamic simplification disabled. Once I turn it on, it paints everything in one color. |
BTW, I just created a new TopoJSON using the latest version and still "leaks": http://bl.ocks.org/martgnz/7889428fcad5eb8b43eec7c71ddb90af I made this json with: topojson municipios.shp -o municipios.json You can download the original shapefile here: https://drive.google.com/file/d/0B_UyUs3UduSUNkZiMmdJRlVJTlU/view?usp=sharing |
Okay, I think I’ve figured out the discrepancy here: when reproducing your example, I removed the call to topojson.presimplify, so it seems like that’s still introducing artifacts. |
Well, more precisely, it isn’t that topojson.presimplify is introducing artifacts, it’s that this enables dynamic simplification in the client using your simplify transform (which is presumably based on my dynamic simplification example): var simplify = d3.geo.transform({
point: function(x, y, z) {
if (!z || z >= settings.area) {
this.stream.point(x, y);
}
}
}); So I assume what’s happening is that the dynamic simplification is introducing degenerate polygons, some of which are interpreted having the wrong winding order. |
The other thing is that dynamic simplification is really designed to operate on projected (planar) geometry, not spherical geometry. The idea is that you project the non-simplified geometry down to the plane, and then filter the planar geometry to the desired simplification. Your code does it in the opposite order: it filters the spherical geometry and then projects down to the plane. My guess is that’s more likely to introduce degenerate polygons like this (and it’s also a lot more expensive). |
So yeah, this is definitely the same as #2025. This is an example of a dynamically simplified polygon I extracted from your dataset:
The latitudes are identical, so this has obviously collapsed to a straight line. (Well, this is spherical coordinates, so it’s more appropriate to say great arc or geodesic.) |
Okay, I’ve tested your change and it looks good! Will release shortly. |
Awesome, thank you! Yes, I agree that projecting the topojson beforehand would be better and faster. However, there is (correct me if I'm wrong) no way to use custom projections (e.g. rveciana/d3-composite-projections) when using the cli client. The other problem I see is responsiveness, since we use different projection parameters for mobile vs desktop. Other than that, I should at least add support for projected topojsons. I wonder why the simplification produces a straight polygon though. Shouldn't this be avoided in any case? |
I recommend loading different pre-projected data if you need it. Projection
is especially slow on mobile devices—I can’t imagine a 12MB unprotected
dataset displaying, although maybe the latest devices are fast enough these
days.
Another consideration is that presimplify assumes planar geometry by
default: it uses planar triangles for Visvalingam simplification. If you
give it spherical geometry the math is wrong. You need to pass the
spherical triangle area function as the second argument rather than using
the default planar area. (This happens automatically on the command line.)
The difference may not be huge depending on the size of your dataset
relative to the size of the Earth though, but it just exacerbates the
problem of simplifying before projecting.
|
I’m pretty sure you can use custom projections from the command-line, but
you need to require them (and npm install them) first. The projection
definition is just JavaScript that gets evaluated and returns the
projection object, so anything that can be run in Node should be available
for you to use. You can even define the projection definition in a separate
JavaScript file and require that if escaping it through the command-line is
a pain.
That said this needs better documentation and the design of the
command-line tools needs work (specifically decoupling into smaller UNIX-y
tools). It’s on my long list of things to do after I finish D3 4.0.
|
Hi!
I was hit by what seems to be #2025 when developing a couple of canvas-based maps. This change fixes it for me.
I have no idea if this is correct or makes any sense in general, but I have not found any regressions so far.
Thank you,
Lukas