diff --git a/js/id/renderer/features.js b/js/id/renderer/features.js index 115374eb4..bc6c1f3ff 100644 --- a/js/id/renderer/features.js +++ b/js/id/renderer/features.js @@ -281,46 +281,44 @@ iD.Features = function(context) { features.getMatches = function(entity, resolver, geometry) { if (geometry === 'vertex' || geometry === 'relation') return {}; - var ent = iD.Entity.key(entity), - matches = {}, - hasMatch = false; - + var ent = iD.Entity.key(entity); if (!_cache[ent]) { _cache[ent] = {}; } + if (!_cache[ent].matches) { - if (_.isEmpty(entity.tags)) { - matches.others = true; - } - else { - for (var i = 0; i < _keys.length; i++) { - if (_keys[i] === 'others') { - if (hasMatch) continue; + var matches = {}, + hasMatch = false; - // // If the entity is a way that has not matched any other - // // feature type, see if it has a parent relation, and if so, - // // match whatever feature types the parent has matched. - // // (The way is a member of a multipolygon.) - // // - // // IMPORTANT: - // // For this to work, getMatches must be called on relations before ways. - // // - // if (entity.type === 'way') { - // var parents = features.getParents(entity, resolver, geometry); - // if (parents.length === 1) { - // var pkey = iD.Entity.key(parents[0]); - // if (_cache[pkey] && _cache[pkey].matches) { - // matches = _.clone(_cache[pkey].matches); - // continue; - // } - // } - // } - } + for (var i = 0; i < _keys.length; i++) { + if (_keys[i] === 'others') { + if (hasMatch) continue; - if (_features[_keys[i]].filter(entity, resolver, geometry)) { - matches[_keys[i]] = hasMatch = true; + // Multipolygon members: + // If an entity... + // 1. is a way that hasn't matched other "interesting" feature rules, + // 2. and it belongs to a single parent multipolygon relation + // ...then match whatever feature rules the parent multipolygon has matched. + // see #2548, #2887 + // + // IMPORTANT: + // For this to work, getMatches must be called on relations before ways. + // + if (entity.type === 'way') { + var parents = features.getParents(entity, resolver, geometry); + if (parents.length === 1 && parents[0].isMultipolygon()) { + var pkey = iD.Entity.key(parents[0]); + if (_cache[pkey] && _cache[pkey].matches) { + matches = _.clone(_cache[pkey].matches); + continue; + } + } } } + + if (_features[_keys[i]].filter(entity, resolver, geometry)) { + matches[_keys[i]] = hasMatch = true; + } } _cache[ent].matches = matches; } @@ -329,20 +327,19 @@ iD.Features = function(context) { }; features.getParents = function(entity, resolver, geometry) { - var ent = iD.Entity.key(entity); + if (geometry === 'point') return []; + var ent = iD.Entity.key(entity); if (!_cache[ent]) { _cache[ent] = {}; } + if (!_cache[ent].parents) { var parents = []; - - if (geometry !== 'point') { - if (geometry === 'vertex') { - parents = resolver.parentWays(entity); - } else { // 'line', 'area', 'relation' - parents = resolver.parentRelations(entity); - } + if (geometry === 'vertex') { + parents = resolver.parentWays(entity); + } else { // 'line', 'area', 'relation' + parents = resolver.parentRelations(entity); } _cache[ent].parents = parents; } @@ -350,22 +347,23 @@ iD.Features = function(context) { }; features.isHiddenFeature = function(entity, resolver, geometry) { + if (!_hidden.length) return false; if (!entity.version) return false; var matches = features.getMatches(entity, resolver, geometry); for (var i = 0; i < _hidden.length; i++) { - if (matches[_hidden[i]]) { return true; } + if (matches[_hidden[i]]) return true; } return false; }; features.isHiddenChild = function(entity, resolver, geometry) { - if (!entity.version || geometry === 'point') { return false; } + if (!_hidden.length) return false; + if (!entity.version || geometry === 'point') return false; var parents = features.getParents(entity, resolver, geometry); - - if (!parents.length) { return false; } + if (!parents.length) return false; for (var i = 0; i < parents.length; i++) { if (!features.isHidden(parents[i], resolver, parents[i].geometry(resolver))) { @@ -376,6 +374,7 @@ iD.Features = function(context) { }; features.hasHiddenConnections = function(entity, resolver) { + if (!_hidden.length) return false; var childNodes, connections; if (entity.type === 'midpoint') { @@ -397,20 +396,15 @@ iD.Features = function(context) { }; features.isHidden = function(entity, resolver, geometry) { + if (!_hidden.length) return false; if (!entity.version) return false; - if (geometry === 'vertex') - return features.isHiddenChild(entity, resolver, geometry); - if (geometry === 'point') - return features.isHiddenFeature(entity, resolver, geometry); - - return features.isHiddenFeature(entity, resolver, geometry) || - features.isHiddenChild(entity, resolver, geometry); + var fn = (geometry === 'vertex' ? features.isHiddenChild : features.isHiddenFeature); + return fn(entity, resolver, geometry); }; features.filter = function(d, resolver) { - if (!_hidden.length) - return d; + if (!_hidden.length) return d; var result = []; for (var i = 0; i < d.length; i++) { diff --git a/test/spec/renderer/features.js b/test/spec/renderer/features.js index 39e72e21f..3cd2fc647 100644 --- a/test/spec/renderer/features.js +++ b/test/spec/renderer/features.js @@ -137,13 +137,17 @@ describe('iD.Features', function() { iD.Way({id: 'industrial', tags: {area: 'yes', landuse: 'industrial'}, version: 1}), iD.Way({id: 'parkinglot', tags: {area: 'yes', amenity: 'parking', parking: 'surface'}, version: 1}), - // Landuse with hole - iD.Way({id: 'inner', version: 1}), + // Landuse Multipolygon iD.Way({id: 'outer', version: 1}), + iD.Way({id: 'inner1', version: 1}), + iD.Way({id: 'inner2', tags: {barrier: 'fence'}, version: 1}), + iD.Way({id: 'inner3', tags: {highway: 'residential'}, version: 1}), iD.Relation({id: 'retail', tags: {landuse: 'retail', type: 'multipolygon'}, members: [ {id: 'outer', role: 'outer', type: 'way'}, - {id: 'inner', role: 'inner', type: 'way'} + {id: 'inner1', role: 'inner', type: 'way'}, + {id: 'inner2', role: 'inner', type: 'way'}, + {id: 'inner3', role: 'inner', type: 'way'} ], version: 1 }), @@ -234,7 +238,7 @@ describe('iD.Features', function() { doMatch([ 'motorway', 'motorway_link', 'trunk', 'trunk_link', 'primary', 'primary_link', 'secondary', 'secondary_link', - 'tertiary', 'tertiary_link', 'residential' + 'tertiary', 'tertiary_link', 'residential', 'inner3' ]); dontMatch([ @@ -301,13 +305,15 @@ describe('iD.Features', function() { doMatch([ 'forest', 'scrub', 'industrial', 'parkinglot', 'building_no', - 'rail_landuse', 'landuse_construction', 'retail', 'inner', 'outer' + 'rail_landuse', 'landuse_construction', 'retail', + 'outer', 'inner1', 'inner2' // non-interesting members of landuse multipolygon ]); dontMatch([ 'point_bar', 'motorway', 'service', 'path', 'building_yes', 'boundary', 'water', 'railway', 'power_line', - 'motorway_construction', 'fence' + 'motorway_construction', 'fence', + 'inner3' // member of landuse multipolygon, but tagged as highway ]); }); @@ -407,7 +413,7 @@ describe('iD.Features', function() { dontMatch([ 'point_bar', 'motorway', 'service', 'path', 'building_yes', 'forest', 'boundary', 'water', 'railway', 'power_line', - 'motorway_construction', 'retail', 'inner', 'outer' + 'motorway_construction', 'retail', 'outer', 'inner1', 'inner2', 'inner3' ]); }); }); @@ -426,73 +432,37 @@ describe('iD.Features', function() { features.gatherStats(all, graph, dimensions); expect(features.isHiddenChild(a, graph, geometry)).to.be.true; + expect(features.isHiddenChild(b, graph, geometry)).to.be.true; expect(features.isHidden(a, graph, geometry)).to.be.true; + expect(features.isHidden(b, graph, geometry)).to.be.true; }); - it('hides child "others" (e.g. untagged or barrier) ways on a hidden multipolygon relation', function() { - var a = iD.Node({id: 'a', version: 1}), - b = iD.Node({id: 'b', version: 1}), - c = iD.Node({id: 'c', version: 1}), - d = iD.Node({id: 'd', version: 1}), - e = iD.Node({id: 'e', version: 1}), - f = iD.Node({id: 'f', version: 1}), - g = iD.Node({id: 'g', version: 1}), - h = iD.Node({id: 'h', version: 1}), - i = iD.Node({id: 'i', version: 1}), - outer = iD.Way({id: 'outer', nodes: [a.id, b.id, c.id, a.id], tags: {area: 'yes', natural: 'wood'}, version: 1}), - inner1 = iD.Way({id: 'inner1', nodes: [d.id, e.id, f.id, d.id], tags: {barrier: 'fence'}, version: 1}), - inner2 = iD.Way({id: 'inner2', nodes: [g.id, h.id, i.id, g.id], version: 1}), + it('hides uninteresting (e.g. untagged or "other") member ways on a hidden multipolygon relation', function() { + var outer = iD.Way({id: 'outer', tags: {area: 'yes', natural: 'wood'}, version: 1}), + inner1 = iD.Way({id: 'inner1', tags: {barrier: 'fence'}, version: 1}), + inner2 = iD.Way({id: 'inner2', version: 1}), + inner3 = iD.Way({id: 'inner3', tags: {highway: 'residential'}, version: 1}), r = iD.Relation({ id: 'r', tags: {type: 'multipolygon'}, members: [ {id: outer.id, role: 'outer', type: 'way'}, {id: inner1.id, role: 'inner', type: 'way'}, - {id: inner2.id, role: 'inner', type: 'way'} + {id: inner2.id, role: 'inner', type: 'way'}, + {id: inner3.id, role: 'inner', type: 'way'} ], version: 1 }), - graph = iD.Graph([a, b, c, d, e, f, g, h, i, outer, inner1, inner2, r]), - geometry1 = inner1.geometry(graph), - geometry2 = inner2.geometry(graph), + graph = iD.Graph([outer, inner1, inner2, inner3, r]), all = _.values(graph.base().entities); features.disable('landuse'); features.gatherStats(all, graph, dimensions); - expect(features.isHiddenChild(inner1, graph, geometry1)).to.be.true; - expect(features.isHiddenChild(inner2, graph, geometry2)).to.be.true; - expect(features.isHidden(inner1, graph, geometry1)).to.be.true; - expect(features.isHidden(inner2, graph, geometry2)).to.be.true; - }); - - it('does not hide child "non-others" (e.g. highway) ways on a hidden multipolygon relation', function() { - var a = iD.Node({id: 'a', version: 1}), - b = iD.Node({id: 'b', version: 1}), - c = iD.Node({id: 'c', version: 1}), - d = iD.Node({id: 'd', version: 1}), - e = iD.Node({id: 'e', version: 1}), - f = iD.Node({id: 'f', version: 1}), - outer = iD.Way({id: 'outer', nodes: [a.id, b.id, c.id, a.id], tags: {area: 'yes', natural: 'wood'}, version: 1}), - inner = iD.Way({id: 'inner', nodes: [d.id, e.id, f.id, d.id], tags: {highway: 'residential'}, version: 1}), - r = iD.Relation({ - id: 'r', - tags: {type: 'multipolygon'}, - members: [ - {id: outer.id, role: 'outer', type: 'way'}, - {id: inner.id, role: 'inner', type: 'way'} - ], - version: 1 - }), - graph = iD.Graph([a, b, c, d, e, f, outer, inner, r]), - geometry = inner.geometry(graph), - all = _.values(graph.base().entities); - - features.disable('landuse'); - features.gatherStats(all, graph, dimensions); - - expect(features.isHiddenChild(inner, graph, geometry)).to.be.false; - expect(features.isHidden(inner, graph, geometry)).to.be.false; + expect(features.isHidden(outer, graph, outer.geometry(graph))).to.be.true; // #2548 + expect(features.isHidden(inner1, graph, inner1.geometry(graph))).to.be.true; // #2548 + expect(features.isHidden(inner2, graph, inner2.geometry(graph))).to.be.true; // #2548 + expect(features.isHidden(inner3, graph, inner3.geometry(graph))).to.be.false; // #2887 }); it('hides only versioned entities', function() { @@ -515,7 +485,7 @@ describe('iD.Features', function() { maxPoints = 200, all, hidden, autoHidden, i, msg; - for(i = 0; i < maxPoints; i++) { + for (i = 0; i < maxPoints; i++) { graph.rebase([iD.Node({version: 1})], [graph]); } @@ -546,7 +516,7 @@ describe('iD.Features', function() { dimensions = [2000, 1000], all, hidden, autoHidden, i, msg; - for(i = 0; i < maxPoints; i++) { + for (i = 0; i < maxPoints; i++) { graph.rebase([iD.Node({version: 1})], [graph]); }