From b383bfec1f7daeaa1f8b158e65dce9930c94d666 Mon Sep 17 00:00:00 2001 From: John Firebaugh Date: Wed, 13 Nov 2013 17:41:01 -0800 Subject: [PATCH] Fix unclosed area rendering (fixes #1958) --- js/id/core/entity.js | 6 ----- js/id/core/relation.js | 6 +++++ js/id/core/way.js | 51 +++++++++++++++++++++++++--------------- js/id/svg.js | 2 +- test/spec/core/entity.js | 38 ------------------------------ test/spec/core/way.js | 45 +++++++++++++++++++++++++++++++---- 6 files changed, 80 insertions(+), 68 deletions(-) diff --git a/js/id/core/entity.js b/js/id/core/entity.js index 8f27599c9..3d68e776f 100644 --- a/js/id/core/entity.js +++ b/js/id/core/entity.js @@ -102,12 +102,6 @@ iD.Entity.prototype = { resolver.parentRelations(this).length > 0; }, - area: function(resolver) { - return resolver.transient(this, 'area', function() { - return d3.geo.area(this.asGeoJSON(resolver, true)); - }); - }, - hasInterestingTags: function() { return _.keys(this.tags).some(function(key) { return key !== 'attribution' && diff --git a/js/id/core/relation.js b/js/id/core/relation.js index b68666e76..8fbdd05b2 100644 --- a/js/id/core/relation.js +++ b/js/id/core/relation.js @@ -156,6 +156,12 @@ _.extend(iD.Relation.prototype, { }); }, + area: function(resolver) { + return resolver.transient(this, 'area', function() { + return d3.geo.area(this.asGeoJSON(resolver)); + }); + }, + isMultipolygon: function() { return this.tags.type === 'multipolygon'; }, diff --git a/js/id/core/way.js b/js/id/core/way.js index 966da007e..8f703049a 100644 --- a/js/id/core/way.js +++ b/js/id/core/way.js @@ -146,34 +146,47 @@ _.extend(iD.Way.prototype, { return r; }, - asGeoJSON: function(resolver, polygon) { + asGeoJSON: function(resolver) { return resolver.transient(this, 'GeoJSON', function() { - var nodes = resolver.childNodes(this); - - if (this.isArea() && polygon && nodes.length >= 4) { - if (!this.isClosed()) { - nodes = nodes.concat([nodes[0]]); - } - - var json = { + var coordinates = _.pluck(resolver.childNodes(this), 'loc'); + if (this.isArea() && this.isClosed()) { + return { type: 'Polygon', - coordinates: [_.pluck(nodes, 'loc')] + coordinates: [coordinates] }; - - // Heuristic for detecting counterclockwise winding order. Assumes - // that OpenStreetMap polygons are not hemisphere-spanning. - if (d3.geo.area(json) > 2 * Math.PI) { - json.coordinates[0] = json.coordinates[0].reverse(); - } - - return json; } else { return { type: 'LineString', - coordinates: _.pluck(nodes, 'loc') + coordinates: coordinates }; } }); + }, + + area: function(resolver) { + return resolver.transient(this, 'area', function() { + var nodes = resolver.childNodes(this); + + if (!this.isClosed() && nodes.length) { + nodes = nodes.concat([nodes[0]]); + } + + var json = { + type: 'Polygon', + coordinates: [_.pluck(nodes, 'loc')] + }; + + var area = d3.geo.area(json); + + // Heuristic for detecting counterclockwise winding order. Assumes + // that OpenStreetMap polygons are not hemisphere-spanning. + if (d3.geo.area(json) > 2 * Math.PI) { + json.coordinates[0] = json.coordinates[0].reverse(); + area = d3.geo.area(json); + } + + return isNaN(area) ? 0 : area; + }); } }); diff --git a/js/id/svg.js b/js/id/svg.js index c4dca3025..fc848e5de 100644 --- a/js/id/svg.js +++ b/js/id/svg.js @@ -31,7 +31,7 @@ iD.svg = { if (entity.id in cache) { return cache[entity.id]; } else { - return cache[entity.id] = path(entity.asGeoJSON(graph, polygon)); // jshint ignore:line + return cache[entity.id] = path(entity.asGeoJSON(graph)); // jshint ignore:line } }; }, diff --git a/test/spec/core/entity.js b/test/spec/core/entity.js index 79d1fb753..85dd41ec7 100644 --- a/test/spec/core/entity.js +++ b/test/spec/core/entity.js @@ -201,42 +201,4 @@ describe('iD.Entity', function () { expect(iD.Entity({tags: {'tiger:source': 'blah', 'tiger:foo': 'bar'}}).hasInterestingTags()).to.equal(false); }); }); - - describe("#area", function() { - it("returns a relative measure of area", function () { - var graph = iD.Graph([ - iD.Node({id: 'a', loc: [-0.0002, 0.0001]}), - iD.Node({id: 'b', loc: [ 0.0002, 0.0001]}), - iD.Node({id: 'c', loc: [ 0.0002, -0.0001]}), - iD.Node({id: 'd', loc: [-0.0002, -0.0001]}), - iD.Node({id: 'e', loc: [-0.0004, 0.0002]}), - iD.Node({id: 'f', loc: [ 0.0004, 0.0002]}), - iD.Node({id: 'g', loc: [ 0.0004, -0.0002]}), - iD.Node({id: 'h', loc: [-0.0004, -0.0002]}), - iD.Way({id: 's', tags: {area: 'yes'}, nodes: ['a', 'b', 'c', 'd', 'a']}), - iD.Way({id: 'l', tags: {area: 'yes'}, nodes: ['e', 'f', 'g', 'h', 'e']}) - ]); - - var s = Math.abs(graph.entity('s').area(graph)), - l = Math.abs(graph.entity('l').area(graph)); - - expect(s).to.be.lt(l); - }); - - it("returns 0 for degenerate areas", function () { - var graph = iD.Graph([ - iD.Node({id: 'a', loc: [-0.0002, 0.0001]}), - iD.Node({id: 'b', loc: [ 0.0002, 0.0001]}), - iD.Node({id: 'c', loc: [ 0.0002, -0.0001]}), - iD.Node({id: 'd', loc: [-0.0002, -0.0001]}), - iD.Way({id: '0', tags: {area: 'yes'}, nodes: []}), - iD.Way({id: '1', tags: {area: 'yes'}, nodes: ['a']}), - iD.Way({id: '2', tags: {area: 'yes'}, nodes: ['a', 'b']}) - ]); - - expect(graph.entity('0').area(graph)).to.equal(0); - expect(graph.entity('1').area(graph)).to.equal(0); - expect(graph.entity('2').area(graph)).to.equal(0); - }); - }); }); diff --git a/test/spec/core/way.js b/test/spec/core/way.js index d01cdf99d..47b092dd9 100644 --- a/test/spec/core/way.js +++ b/test/spec/core/way.js @@ -290,7 +290,7 @@ describe('iD.Way', function() { json = w.asGeoJSON(graph); expect(json.type).to.equal('LineString'); - expect(json.coordinates).to.eql([[1, 2], [3, 4]]); + expect(json.coordinates).to.eql([a.loc, b.loc]); }); it("converts an area to a GeoJSON Polygon geometry", function () { @@ -305,15 +305,52 @@ describe('iD.Way', function() { expect(json.coordinates).to.eql([[a.loc, b.loc, c.loc, a.loc]]); }); - it("forces clockwise polygon winding order", function () { + it("converts an unclosed area to a GeoJSON LineString geometry", function () { var a = iD.Node({loc: [1, 2]}), b = iD.Node({loc: [5, 6]}), c = iD.Node({loc: [3, 4]}), - w = iD.Way({tags: {area: 'yes'}, nodes: [a.id, c.id, b.id, a.id]}), + w = iD.Way({tags: {area: 'yes'}, nodes: [a.id, b.id, c.id]}), graph = iD.Graph([a, b, c, w]), json = w.asGeoJSON(graph, true); - expect(json.coordinates).to.eql([[a.loc, b.loc, c.loc, a.loc]]); + expect(json.type).to.equal('LineString'); + expect(json.coordinates).to.eql([a.loc, b.loc, c.loc]); + }); + }); + + describe("#area", function() { + it("returns a relative measure of area", function () { + var graph = iD.Graph([ + iD.Node({id: 'a', loc: [-0.0002, 0.0001]}), + iD.Node({id: 'b', loc: [ 0.0002, 0.0001]}), + iD.Node({id: 'c', loc: [ 0.0002, -0.0001]}), + iD.Node({id: 'd', loc: [-0.0002, -0.0001]}), + iD.Node({id: 'e', loc: [-0.0004, 0.0002]}), + iD.Node({id: 'f', loc: [ 0.0004, 0.0002]}), + iD.Node({id: 'g', loc: [ 0.0004, -0.0002]}), + iD.Node({id: 'h', loc: [-0.0004, -0.0002]}), + iD.Way({id: 's', tags: {area: 'yes'}, nodes: ['a', 'b', 'c', 'd', 'a']}), + iD.Way({id: 'l', tags: {area: 'yes'}, nodes: ['e', 'f', 'g', 'h', 'e']}) + ]); + + var s = Math.abs(graph.entity('s').area(graph)), + l = Math.abs(graph.entity('l').area(graph)); + + expect(s).to.be.lt(l); + }); + + it("returns 0 for degenerate areas", function () { + var graph = iD.Graph([ + iD.Node({id: 'a', loc: [-0.0002, 0.0001]}), + iD.Node({id: 'b', loc: [ 0.0002, 0.0001]}), + iD.Way({id: '0', tags: {area: 'yes'}, nodes: []}), + iD.Way({id: '1', tags: {area: 'yes'}, nodes: ['a']}), + iD.Way({id: '2', tags: {area: 'yes'}, nodes: ['a', 'b']}) + ]); + + expect(graph.entity('0').area(graph)).to.equal(0); + expect(graph.entity('1').area(graph)).to.equal(0); + expect(graph.entity('2').area(graph)).to.equal(0); }); }); });