diff --git a/CHANGELOG.md b/CHANGELOG.md index 80be3c79a..c12d118a7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -38,8 +38,10 @@ _Breaking developer changes, which may affect downstream projects or sites that # unreleased (v2.34.0-dev) #### :sparkles: Usability & Accessibility -* Show full relation label as hover-text in _membership editor_, disambiguate relations with duplicate labels by appending the relation id ([#10942]) +* Show full relation label as hover-text in _membership editor_, disambiguate relations with duplicate labels by appending the relation id ([#10492]) #### :scissors: Operations +* Preserve the sum of certain tags (`step_count`, `parking:*:capacity`) during _join_ operation ([#10492], thanks [@ChaitanyaKadu03]) +* Preserve total value of `parking:*:capacity` tags during _split_ operation by distributing it proportionally to the resulting ways ([#10492]) #### :camera: Street-Level * Keep photo viewer open when disabling Panoramax overlay ([#10966]) * Don't de-select map feature when clicking on a street level photo ([#10959]) @@ -57,11 +59,13 @@ _Breaking developer changes, which may affect downstream projects or sites that [#9873]: https://github.com/openstreetmap/iD/issues/9873 [#10104]: https://github.com/openstreetmap/iD/issues/10104 +[#10492]: https://github.com/openstreetmap/iD/issues/10492 [#10942]: https://github.com/openstreetmap/iD/pull/10942 [#10943]: https://github.com/openstreetmap/iD/pull/10943 [#10946]: https://github.com/openstreetmap/iD/issues/10946 [#10959]: https://github.com/openstreetmap/iD/issues/10959 [#10966]: https://github.com/openstreetmap/iD/issues/10966 +[@ChaitanyaKadu03]: https://github.com/ChaitanyaKadu03 # v2.33.0 diff --git a/modules/actions/join.js b/modules/actions/join.js index 1aee129a6..7bd9bd310 100644 --- a/modules/actions/join.js +++ b/modules/actions/join.js @@ -1,6 +1,6 @@ import { actionDeleteRelation } from './delete_relation'; import { actionDeleteWay } from './delete_way'; -import { osmIsInterestingTag } from '../osm/tags'; +import { osmIsInterestingTag, osmSummableTags } from '../osm/tags'; import { osmJoinWays } from '../osm/multipolygon'; import { geoPathIntersections } from '../geo'; import { utilArrayGroupBy, utilArrayIdentical, utilArrayIntersection, utilOldestID } from '../util'; @@ -61,7 +61,12 @@ export function actionJoin(ids) { graph = graph.replace(parent.replaceMember(way, survivor)); }); - survivor = survivor.mergeTags(way.tags); + const summedTags = {}; + for (const key in way.tags) { + if (!canSumTags(key, way.tags, survivor.tags)) continue; + summedTags[key] = (+way.tags[key] + +survivor.tags[key]).toString(); + } + survivor = survivor.mergeTags(way.tags, summedTags); graph = graph.replace(survivor); graph = actionDeleteWay(way.id)(graph); @@ -181,6 +186,8 @@ export function actionJoin(ids) { for (var k in way.tags) { if (!(k in tags)) { tags[k] = way.tags[k]; + } else if (canSumTags(k, tags, way.tags)) { + tags[k] = (+tags[k] + +way.tags[k]).toString(); } else if (tags[k] && osmIsInterestingTag(k) && tags[k] !== way.tags[k]) { conflicting = true; } @@ -196,6 +203,12 @@ export function actionJoin(ids) { } }; + function canSumTags(key, tagsA, tagsB) { + return osmSummableTags.has(key) && + isFinite(tagsA[key] && + isFinite(tagsB[key])); + } + return action; } diff --git a/modules/actions/split.js b/modules/actions/split.js index 65089874e..aa70ec0a4 100644 --- a/modules/actions/split.js +++ b/modules/actions/split.js @@ -2,6 +2,7 @@ import { geoSphericalDistance } from '../geo/geo'; import { osmRelation } from '../osm/relation'; import { osmWay } from '../osm/way'; import { utilArrayIntersection, utilWrap, utilArrayUniq } from '../util'; +import { osmSummableTags } from '../osm/tags'; // Split a way at the given node. @@ -136,32 +137,31 @@ export function actionSplit(nodeIds, newWayIds) { wayB = wayB.update({ nodes: nodesB }); } - if (wayA.tags.step_count) { - // divide up the the step count proportionally between the two ways + for (const key in wayA.tags) { + if (!osmSummableTags.has(key)) continue; - var stepCount = Number(wayA.tags.step_count); - if (stepCount && + // divide up the the e.g. step count proportionally between the two ways + var count = Number(wayA.tags[key]); + if (count && // ensure a number - isFinite(stepCount) && + isFinite(count) && // ensure positive - stepCount > 0 && + count > 0 && // ensure integer - Math.round(stepCount) === stepCount) { - + Math.round(count) === count) { var tagsA = Object.assign({}, wayA.tags); var tagsB = Object.assign({}, wayB.tags); var ratioA = lengthA / (lengthA + lengthB); - var countA = Math.round(stepCount * ratioA); - tagsA.step_count = countA.toString(); - tagsB.step_count = (stepCount - countA).toString(); + var countA = Math.round(count * ratioA); + tagsA[key] = countA.toString(); + tagsB[key] = (count - countA).toString(); wayA = wayA.update({ tags: tagsA }); wayB = wayB.update({ tags: tagsB }); } } - graph = graph.replace(wayA); graph = graph.replace(wayB); diff --git a/modules/osm/entity.js b/modules/osm/entity.js index 16025a67b..38033fed6 100644 --- a/modules/osm/entity.js +++ b/modules/osm/entity.js @@ -123,12 +123,20 @@ osmEntity.prototype = { }, - mergeTags: function(tags) { - var merged = Object.assign({}, this.tags); // shallow copy - var changed = false; - for (var k in tags) { - var t1 = merged[k]; - var t2 = tags[k]; + /** + * + * @param {Tags} tags tags to merge into this entity's tags + * @param {Tags} setTags (optional) a set of tags to overwrite in this entity's tags + * @returns {iD.OsmEntity} + */ + mergeTags: function(tags, setTags = {}) { + const merged = Object.assign({}, this.tags); // shallow copy + let changed = false; + + for (const k in tags) { + if (setTags.hasOwnProperty(k)) continue; + const t1 = this.tags[k]; + const t2 = tags[k]; if (!t1) { changed = true; merged[k] = t2; @@ -140,6 +148,13 @@ osmEntity.prototype = { ); } } + for (const k in setTags) { + if (this.tags[k] !== setTags[k]) { + changed = true; + merged[k] = setTags[k]; + } + } + return changed ? this.update({ tags: merged }) : this; }, diff --git a/modules/osm/tags.js b/modules/osm/tags.js index a5fa59e80..959618dce 100644 --- a/modules/osm/tags.js +++ b/modules/osm/tags.js @@ -322,3 +322,10 @@ export function osmShouldRenderDirection(vertexTags, wayTags) { if (vertexTags.cycleway === 'asl') return !!wayTags.highway; return true; } + +export var osmSummableTags = new Set([ + 'step_count', + 'parking:both:capacity', + 'parking:left:capacity', + 'parking:left:capacity' +]); diff --git a/test/spec/actions/join.js b/test/spec/actions/join.js index f75be5814..47dd9af41 100644 --- a/test/spec/actions/join.js +++ b/test/spec/actions/join.js @@ -598,6 +598,19 @@ describe('iD.actionJoin', function () { expect(graph.entity('-').tags).to.eql({ natural: 'cliff' }); }); + it('merges the number of steps', function () { + let graph = iD.coreGraph([ + iD.osmNode({ id: 'a', loc: [0, 0] }), + iD.osmNode({ id: 'b', loc: [1, 0] }), + iD.osmNode({ id: 'c', loc: [4, 0] }), + iD.osmWay({ id: '-', nodes: ['a', 'b'], tags: { highway: 'steps', step_count: '12' } }), + iD.osmWay({ id: '=', nodes: ['b', 'c'], tags: { highway: 'steps', step_count: '30' } }) + ]); + graph = iD.actionJoin(['-', '='])(graph); + // step count should be merged + expect(graph.entity('-').tags.step_count).to.equal('42'); + }); + it('merges relations', function () { var graph = iD.coreGraph([ diff --git a/test/spec/actions/split.js b/test/spec/actions/split.js index c2c9d186f..7656328df 100644 --- a/test/spec/actions/split.js +++ b/test/spec/actions/split.js @@ -531,6 +531,40 @@ describe('iD.actionSplit', function () { expect(g1.entity('-').nodes).to.eql(['b', 'c', 'd', 'a']); expect(g1.entity('=').nodes).to.eql(['a', 'b']); }); + + it('distributes the number of steps proportionally', () => { + const tags = { highway: 'steps', step_count: '40' }; + let graph = iD.coreGraph([ + iD.osmNode({ id: 'a', loc: [0, 0] }), + iD.osmNode({ id: 'b', loc: [1, 0] }), + iD.osmNode({ id: 'c', loc: [4, 0] }), + iD.osmWay({ id: '-', nodes: ['a', 'b', 'c'], tags: tags }) + ]); + + graph = iD.actionSplit('b', ['='])(graph); + + // step count should be distributed according the the resulting ways' + // segment lengths + expect(graph.entity('=').tags.step_count).to.equal('10'); + expect(graph.entity('-').tags.step_count).to.equal('30'); + }); + + it('preserves the total number of steps', () => { + const tags = { highway: 'steps', step_count: '42' }; + let graph = iD.coreGraph([ + iD.osmNode({ id: 'a', loc: [0, 0] }), + iD.osmNode({ id: 'b', loc: [1, 0] }), + iD.osmNode({ id: 'c', loc: [4, 0] }), + iD.osmWay({ id: '-', nodes: ['a', 'b', 'c'], tags: tags }) + ]); + + graph = iD.actionSplit('b', ['='])(graph); + + // the sum of the resulting step count should be preserved + // even when the intermediate values are rounded + expect(+graph.entity('=').tags.step_count + + +graph.entity('-').tags.step_count).to.equal(42); + }); }); diff --git a/test/spec/osm/entity.js b/test/spec/osm/entity.js index b6b68d26b..d3e096e59 100644 --- a/test/spec/osm/entity.js +++ b/test/spec/osm/entity.js @@ -168,6 +168,15 @@ describe('iD.osmEntity', function () { expect(a.mergeTags(b.tags).tags).to.eql({a: 'a;b'}); expect(b.mergeTags(a.tags).tags).to.eql({a: 'b;a'}); }); + + it('accepts override tags', function () { + const a = iD.osmEntity({tags: {a: 'a', c: '1'}}); + const b = iD.osmEntity({tags: {b: 'b', c: '2'}}); + + const merged = a.mergeTags(b.tags, { c: '3' }); + + expect(merged.tags.c).to.eql('3'); + }); }); describe('#osmId', function () {