topojson / topojson-client

Manipulate TopoJSON, such as to merge shapes, and convert it back to GeoJSON.
ISC License
213 stars 63 forks source link

topojson.mesh filter doesn’t support geometry with multiple neighbors #9

Closed evanl5 closed 7 years ago

evanl5 commented 7 years ago

If there are more than two geometries in a collection that share the same arcs, the mesh filter function will ignore some neighboring geometries.

For instance, given the following topology, the filter function will never be passed geometry with id 2.

var topology = {
  "type": "Topology",
  "objects": {
    "collection": {
      "type": "GeometryCollection",
      "geometries": [
        {"id": 1, "type": "LineString", "arcs": [0,1]},
        {"id": 2, "type": "LineString", "arcs": [0,1]},
        {"id": 3, "type": "LineString", "arcs": [0,1]}
      ]
    }
  },
  "arcs": [
    [[1, 0], [2, 0]],
    [[0, 0], [1, 0]]
  ]
};

topojson.mesh(topology, topology.objects.collection, function (geometry, neighbor) {
  return geometry.id === 2 || neighbor.id === 2;
});

For a more practical use case, in the screenshot below the bold line is created with a mesh filter intend to outline Uganda. However, since Uganda shares borders with both Kenya and Kenya’s states, the filter fails and the border is partially missing.

screen shot 2017-03-17 at 8 45 33 am

The issue appears to be in the extractArcs function (https://github.com/topojson/topojson-client/blob/master/src/mesh.js#L50) where only the first and last neighbors are sent to the filter function.

Here are some possible solutions that I could think of:

topojson.mesh(topology, object, filter, [apply = defaultApply])

function defaultApply(geometries, filterFn) {
  return geometries.reduce(function (accum, geom, i, geoms) {
    if (accum === false) return false;
    return filterFn(geoms[0], geom);
  }, true);
}

function (geoms) { 
  var apply = apply || defaultApply;
  if(apply(geoms.map(function(geom) { return geom.g; }), filter) arcs.push(geoms[0].i); 
});
mbostock commented 7 years ago

This is the expected behavior but there is an unstated assumption that the topology used to derive the mesh is valid, part of which is a requirement that the geometry used to derive the mesh is not self-overlapping. So in your example, you’ll need to decide whether to derive the mesh from Kenya or Kenya’s states, but you can’t include both in the source geometry; I recommend grouping your geometry by administrative level, as is done for example in us-atlas, which provides separate layers for the nation, states and counties.

evanl5 commented 7 years ago

My geometry actually is grouped by administrative level. I have four administrative levels and three desired mesh layers (one for disputed borders, one for non-disputed borders, and one for selected borders), creating them as separate layers would require 12 layers. In an effort to simplify the process of creating the mesh layers I decided to pass all visible administrative levels to the mesh function at once as nested GeometryCollections. This is how I discovered the problem.

Is there any chance you would consider a feature request to add this functionality?