invalid loop

[mick]

using this data: https://gist.github.com/mick/3439b673dfd7e5ffd59e and geojson-cover 0.1.2 I get:

/bench-s2/cardboard/node_modules/geojson-cover/index.js:134
    return s2.getCover(llRings, cover_options).map(function(cell) {
              ^
Error: invalid loop
    at multipolygonIndex (/bench-s2/cardboard/node_modules/geojson-cover/index.js:134:15)
    at Object.module.exports.geometryIndexes (/bench-s2/cardboard/node_modules/geojson-cover/index.js:60:20)
    at Cardboard.insert (/bench-s2/cardboard/index.js:41:32)
    at /bench-s2/cardboard/cardboard:32:27
    at Array.forEach (native)
    at /bench-s2/cardboard/cardboard:28:25
    at ConcatStream.<anonymous> (/bench-s2/cardboard/node_modules/concat-stream/index.js:32:43)
    at ConcatStream.EventEmitter.emit (events.js:117:20)
    at finishMaybe (/bench-s2/cardboard/node_modules/concat-stream/node_modules/readable-stream/lib/_stream_writable.js:455:14)
    at endWritable (/bench-s2/cardboard/node_modules/concat-stream/node_modules/readable-stream/lib/_stream_writable.js:464:3)

cc: @morganherlocker

[morganherlocker]

So, I found the culprit. Antarctica is technically an invalid polygon, since it contains a shared edge at 180.0, -90.0. Shifting this coordinate to 179.99999999, -90 causes the test to pass.

Soooooo... the question now is do we correct issues like this ✨automagically✨, or do we leave it to the data uploader to keep s2 validity? I do not think the geojson spec has anything to say about topological validity, but I normally go with its opinion.

[tmcw]

Soooooo... the question now is do we correct issues like this ✨automagically✨, or do we leave it to the data uploader to keep s2 validity?

I think we'll eventually need to support non-simple-features-compliant geometries. Since this is turning up early with common testing data, we should probably fix it now.

[morganherlocker]

I added code to test the entire world. What I found was that Antarctica, Russia, Fiji, and a couple other places cross over the -180 border. When they did, the geojson file split the actual geometry into two touching polygons in a single multipolygon.

I would call that invalid. Polygon Features touching is fine, but 1 MultiPolygon Feature with a shared boundary really does not make sense. I did a /s/-180.0,/-179.99999999999, and all countries pass.

[morganherlocker]

Just saw your comment @tmcw. In this case I am leaning toward putting this on the user. That said, there are probably other cases that we want to allow some fudging. Self intersection and general topology strike me as something that we should not worry about here though.