Possible fix for #2025, handle zero-area polygons better

[lukasappelhans]

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

[martgnz]

@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.

[mbostock]

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.

[mbostock]

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.

[lukasappelhans]

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

[mbostock]

Can you also post how you made municipios.json?

[mbostock]

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.

[lukasappelhans]

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.

[martgnz]

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

[mbostock]

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.

[mbostock]

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.

[mbostock]

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).

[mbostock]

So yeah, this is definitely the same as #2025. This is an example of a dynamically simplified polygon I extracted from your dataset:

var feature = {
  type: "Polygon",
  coordinates: [[
    [-13.487800541155952, 29.40322484792305],
    [-13.487125861717008, 29.40322484792305],
    [-13.486900968570694, 29.40322484792305],
    [-13.487800541155952, 29.40322484792305]
  ]]
};

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.)

[mbostock]

Okay, I’ve tested your change and it looks good! Will release shortly.

[lukasappelhans]

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?

[mbostock]

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.

[mbostock]

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.