From 4747ae253c97fa7f5ae671e5c1d1e8ca9bab4eb3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ky=E2=84=93e=20Hensel?= Date: Thu, 13 Feb 2025 00:22:09 +1100 Subject: [PATCH] refactor `isOneWay` to properly support bidirectional ways (#10730) --- modules/osm/index.js | 1 - modules/osm/tags.js | 31 ++++++++++++-- modules/osm/way.js | 51 +++++++++++++----------- modules/svg/lines.js | 15 +------ modules/ui/fields/check.js | 10 ++--- modules/util/index.js | 2 +- modules/util/object.js | 23 +++++++++++ modules/validations/impossible_oneway.js | 18 ++------- package-lock.json | 16 ++++++++ package.json | 1 + test/spec/osm/way.js | 1 - test/spec/util/object.js | 13 ++++++ 12 files changed, 117 insertions(+), 65 deletions(-) diff --git a/modules/osm/index.js b/modules/osm/index.js index 0046d72ca..8b5404ea0 100644 --- a/modules/osm/index.js +++ b/modules/osm/index.js @@ -29,7 +29,6 @@ export { osmVertexTags, osmSetVertexTags, osmNodeGeometriesForTags, - osmOneWayTags, osmPavedTags, osmIsInterestingTag, osmLifecyclePrefixes, diff --git a/modules/osm/tags.js b/modules/osm/tags.js index e31a1332c..4a4dadc44 100644 --- a/modules/osm/tags.js +++ b/modules/osm/tags.js @@ -1,3 +1,5 @@ +import { merge } from 'lodash-es'; + export function osmIsInterestingTag(key) { return key !== 'attribution' && key !== 'created_by' && @@ -124,7 +126,7 @@ export function osmNodeGeometriesForTags(nodeTags) { return geometries; } -export var osmOneWayTags = { +export const osmOneWayForwardTags = { 'aerialway': { 'chair_lift': true, 'drag_lift': true, @@ -138,8 +140,6 @@ export var osmOneWayTags = { }, 'conveying': { 'forward': true, - 'backward': true, - 'reversible': true, }, 'highway': { 'motorway': true @@ -152,6 +152,9 @@ export var osmOneWayTags = { 'goods_conveyor': true, 'piste:halfpipe': true }, + 'oneway': { + 'yes': true, + }, 'piste:type': { 'downhill': true, 'sled': true, @@ -176,6 +179,28 @@ export var osmOneWayTags = { 'tidal_channel': true } }; +export const osmOneWayBackwardTags = { + 'conveying': { + 'backward': true, + }, + 'oneway': { + '-1': true, + }, +}; +export const osmOneWayBiDirectionalTags = { + 'conveying': { + 'reversible': true, + }, + 'oneway': { + 'alternating': true, + 'reversible': true, + }, +}; +export const osmOneWayTags = merge( + osmOneWayForwardTags, + osmOneWayBackwardTags, + osmOneWayBiDirectionalTags, +); // solid and smooth surfaces akin to the assumed default road surface in OSM export var osmPavedTags = { diff --git a/modules/osm/way.js b/modules/osm/way.js index a8531e1fc..8c312d0a0 100644 --- a/modules/osm/way.js +++ b/modules/osm/way.js @@ -3,8 +3,8 @@ import { geoArea as d3_geoArea } from 'd3-geo'; import { geoExtent, geoVecCross } from '../geo'; import { osmEntity } from './entity'; import { osmLanes } from './lanes'; -import { osmTagSuggestingArea, osmOneWayTags, osmRightSideIsInsideTags, osmRemoveLifecyclePrefix } from './tags'; -import { utilArrayUniq } from '../util'; +import { osmTagSuggestingArea, osmRightSideIsInsideTags, osmRemoveLifecyclePrefix, osmOneWayBiDirectionalTags, osmOneWayBackwardTags, osmOneWayForwardTags, osmOneWayTags } from './tags'; +import { utilArrayUniq, utilCheckTagDictionary } from '../util'; export function osmWay() { @@ -138,29 +138,32 @@ Object.assign(osmWay.prototype, { }, - isOneWay: function() { - // explicit oneway tag.. - var values = { - 'yes': true, - '1': true, - '-1': true, - 'reversible': true, - 'alternating': true, - 'no': false, - '0': false - }; - if (values[this.tags.oneway] !== undefined) { - return values[this.tags.oneway]; - } + /** @returns {boolean} for example, if `oneway=yes` */ + isOneWayForwards() { + if (this.tags.oneway === 'no') return false; - // implied oneway tag.. - for (var key in this.tags) { - if (key in osmOneWayTags && - (this.tags[key] in osmOneWayTags[key])) { - return true; - } - } - return false; + return !!utilCheckTagDictionary(this.tags, osmOneWayForwardTags); + }, + + /** @returns {boolean} for example, if `oneway=-1` */ + isOneWayBackwards() { + if (this.tags.oneway === 'no') return false; + + return !!utilCheckTagDictionary(this.tags, osmOneWayBackwardTags); + }, + + /** @returns {boolean} for example, if `oneway=alternating` */ + isBiDirectional() { + if (this.tags.oneway === 'no') return false; + + return !!utilCheckTagDictionary(this.tags, osmOneWayBiDirectionalTags); + }, + + /** @returns {boolean} */ + isOneWay() { + if (this.tags.oneway === 'no') return false; + + return !!utilCheckTagDictionary(this.tags, osmOneWayTags); }, // Some identifier for tag that implies that this way is "sided", diff --git a/modules/svg/lines.js b/modules/svg/lines.js index 3e31f91b3..80f54950f 100644 --- a/modules/svg/lines.js +++ b/modules/svg/lines.js @@ -265,19 +265,8 @@ export function svgLines(projection, context) { var onewayArr = v.filter(function(d) { return d.isOneWay(); }); var onewaySegments = svgMarkerSegments( projection, graph, 35, - function shouldReverse(entity) { - return ( - entity.tags.oneway === '-1' - || entity.tags.conveying === 'backward' - ); - }, - function bothDirections(entity) { - return ( - entity.tags.oneway === 'alternating' - || entity.tags.oneway === 'reversible' - || entity.tags.conveying === 'reversible' - ); - } + entity => entity.isOneWayBackwards(), + entity => entity.isBiDirectional(), ); onewaydata[k] = utilArrayFlatten(onewayArr.map(onewaySegments)); diff --git a/modules/ui/fields/check.js b/modules/ui/fields/check.js index 40d905136..378b9f0eb 100644 --- a/modules/ui/fields/check.js +++ b/modules/ui/fields/check.js @@ -7,7 +7,6 @@ import { import { utilRebind } from '../../util/rebind'; import { t } from '../../core/localizer'; import { actionReverse } from '../../actions/reverse'; -import { osmOneWayTags } from '../../osm'; import { svgIcon } from '../../svg/icon'; export { uiFieldCheck as uiFieldDefaultCheck }; @@ -61,12 +60,9 @@ export function uiFieldCheck(field, context) { // where implied oneway tag exists (e.g. `junction=roundabout`) #2220, #1841 if (field.id === 'oneway') { var entity = context.entity(_entityIDs[0]); - for (var key in entity.tags) { - if (key in osmOneWayTags && (entity.tags[key] in osmOneWayTags[key])) { - _impliedYes = true; - texts[0] = t.html('_tagging.presets.fields.oneway_yes.options.undefined'); - break; - } + if (entity.type === 'way' && entity.isOneWay()) { + _impliedYes = true; + texts[0] = t.html('_tagging.presets.fields.oneway_yes.options.undefined'); } } } diff --git a/modules/util/index.js b/modules/util/index.js index 2f53a90e2..f88ba1c7f 100644 --- a/modules/util/index.js +++ b/modules/util/index.js @@ -33,7 +33,7 @@ export { utilHashcode } from './util'; export { utilHighlightEntities } from './util'; export { utilKeybinding } from './keybinding'; export { utilNoAuto } from './util'; -export { utilObjectOmit } from './object'; +export { utilObjectOmit, utilCheckTagDictionary } from './object'; export { utilCompareIDs } from './util'; export { utilOldestID } from './util'; export { utilPrefixCSSProperty } from './util'; diff --git a/modules/util/object.js b/modules/util/object.js index fc905e81d..dec54a6ff 100644 --- a/modules/util/object.js +++ b/modules/util/object.js @@ -7,3 +7,26 @@ export function utilObjectOmit(obj, omitKeys) { return result; }, {}); } + +/** + * @template T + * @typedef {{ [key: string]: { [value: string]: T } }} TagDictionary + */ + +/** + * searches a dictionary for a match, such as `osmOneWayForwardTags`, + * `osmAreaKeysExceptions`, etc. + * @template T + * @param {Tags} tags + * @param {TagDictionary} tagDictionary + * @returns {T | undefined} + */ +export function utilCheckTagDictionary(tags, tagDictionary) { + for (const key in tags) { + const value = tags[key]; + if (tagDictionary[key] && value in tagDictionary[key]) { + return tagDictionary[key][value]; + } + } + return undefined; +} diff --git a/modules/validations/impossible_oneway.js b/modules/validations/impossible_oneway.js index 25d15d84d..65022572b 100644 --- a/modules/validations/impossible_oneway.js +++ b/modules/validations/impossible_oneway.js @@ -2,7 +2,7 @@ import { t, localizer } from '../core/localizer'; import { modeDrawLine } from '../modes/draw_line'; import { actionReverse } from '../actions/reverse'; import { utilDisplayLabel } from '../util/utilDisplayLabel'; -import { osmFlowingWaterwayTagValues, osmOneWayTags, osmRoutableHighwayTagValues } from '../osm/tags'; +import { osmFlowingWaterwayTagValues, osmRoutableHighwayTagValues } from '../osm/tags'; import { validationIssue, validationIssueFix } from '../core/validation'; import { services } from '../services'; @@ -17,7 +17,7 @@ export function validationImpossibleOneway() { if (!typeForWay(entity)) return []; - if (!isOneway(entity)) return []; + if (!entity.isOneWay()) return []; var firstIssues = issuesForNode(entity, entity.first()); var lastIssues = issuesForNode(entity, entity.last()); @@ -32,18 +32,6 @@ export function validationImpossibleOneway() { return null; } - function isOneway(way) { - if (way.tags.oneway === 'yes') return true; - if (way.tags.oneway) return false; - - for (var key in way.tags) { - if (osmOneWayTags[key] && osmOneWayTags[key][way.tags[key]]) { - return true; - } - } - return false; - } - function nodeOccursMoreThanOnce(way, nodeID) { var occurrences = 0; for (var index in way.nodes) { @@ -129,7 +117,7 @@ export function validationImpossibleOneway() { if (wayType === 'waterway' && attachedWaysOfSameType.length === 0) return []; var attachedOneways = attachedWaysOfSameType.filter(function(attachedWay) { - return isOneway(attachedWay); + return attachedWay.isOneWay(); }); // ignore if the way is connected to some non-oneway features diff --git a/package-lock.json b/package-lock.json index caff02c0b..176230e89 100644 --- a/package-lock.json +++ b/package-lock.json @@ -50,6 +50,7 @@ "@types/chai": "^5.0.1", "@types/d3": "^7.4.3", "@types/happen": "^0.3.0", + "@types/lodash-es": "^4.17.12", "@types/sinon": "^17.0.3", "@types/sinon-chai": "^4.0.0", "autoprefixer": "^10.4.20", @@ -1941,6 +1942,21 @@ "integrity": "sha512-5+fP8P8MFNC+AyZCDxrB2pkZFPGzqQWUzpSeuuVLvm8VMcorNYavBqoFcxK8bQz4Qsbn4oUEEem4wDLfcysGHA==", "dev": true }, + "node_modules/@types/lodash": { + "version": "4.17.15", + "resolved": "https://registry.npmjs.org/@types/lodash/-/lodash-4.17.15.tgz", + "integrity": "sha512-w/P33JFeySuhN6JLkysYUK2gEmy9kHHFN7E8ro0tkfmlDOgxBDzWEZ/J8cWA+fHqFevpswDTFZnDx+R9lbL6xw==", + "dev": true + }, + "node_modules/@types/lodash-es": { + "version": "4.17.12", + "resolved": "https://registry.npmjs.org/@types/lodash-es/-/lodash-es-4.17.12.tgz", + "integrity": "sha512-0NgftHUcV4v34VhXm8QBSftKVXtbkBG3ViCjs6+eJ5a6y6Mi/jiFGPc1sC7QK+9BFhWrURE3EOggmWaSxL9OzQ==", + "dev": true, + "dependencies": { + "@types/lodash": "*" + } + }, "node_modules/@types/pako": { "version": "1.0.7", "resolved": "https://registry.npmjs.org/@types/pako/-/pako-1.0.7.tgz", diff --git a/package.json b/package.json index 60f823089..301a9c5b8 100644 --- a/package.json +++ b/package.json @@ -85,6 +85,7 @@ "@types/chai": "^5.0.1", "@types/d3": "^7.4.3", "@types/happen": "^0.3.0", + "@types/lodash-es": "^4.17.12", "@types/sinon": "^17.0.3", "@types/sinon-chai": "^4.0.0", "autoprefixer": "^10.4.20", diff --git a/test/spec/osm/way.js b/test/spec/osm/way.js index 89dc0bd30..2329f10ba 100644 --- a/test/spec/osm/way.js +++ b/test/spec/osm/way.js @@ -305,7 +305,6 @@ describe('iD.osmWay', function() { it('returns true when the way has tag oneway=yes', function() { expect(iD.osmWay({tags: { oneway: 'yes' }}).isOneWay(), 'oneway yes').to.be.true; - expect(iD.osmWay({tags: { oneway: '1' }}).isOneWay(), 'oneway 1').to.be.true; expect(iD.osmWay({tags: { oneway: '-1' }}).isOneWay(), 'oneway -1').to.be.true; }); diff --git a/test/spec/util/object.js b/test/spec/util/object.js index 2d577a8a6..cd81a7267 100644 --- a/test/spec/util/object.js +++ b/test/spec/util/object.js @@ -7,3 +7,16 @@ describe('iD.utilObjectOmit', function() { }); }); + +describe('iD.utilCheckTagDictionary', () => { + it('can search a standard tag-dictionary', () => { + expect(iD.utilCheckTagDictionary({}, iD.osmPavedTags)).toBeUndefined(); + expect(iD.utilCheckTagDictionary({ surface: 'asphalt' }, iD.osmPavedTags)).toBe(true); + }); + + it('works for falsy values', () => { + const dictionary = { surface: { paved: 0 } }; + expect(iD.utilCheckTagDictionary({}, dictionary)).toBeUndefined(); + expect(iD.utilCheckTagDictionary({ surface: 'paved' }, dictionary)).toBe(0); + }); +});