From 814f6f6debf7adc6c3639fb5a6c20d3b94240e56 Mon Sep 17 00:00:00 2001 From: Tobias Zwick Date: Mon, 13 Nov 2023 23:30:49 +0100 Subject: [PATCH] do not ask whether oneway road is really also a oneway for cyclists if the contra-flow side remains to be not defined (fixes #5367) --- .../streetcomplete/osm/cycleway/Cycleway.kt | 31 ++-- .../osm/cycleway/CyclewayCreator.kt | 7 +- .../osm/cycleway/CyclewayKtTest.kt | 135 ++++++++++++------ 3 files changed, 114 insertions(+), 59 deletions(-) diff --git a/app/src/main/java/de/westnordost/streetcomplete/osm/cycleway/Cycleway.kt b/app/src/main/java/de/westnordost/streetcomplete/osm/cycleway/Cycleway.kt index 4db7a71de4..c3150fabde 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/osm/cycleway/Cycleway.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/osm/cycleway/Cycleway.kt @@ -28,27 +28,36 @@ fun LeftAndRightCycleway.selectableOrNullValues(countryInfo: CountryInfo): LeftA fun LeftAndRightCycleway.wasNoOnewayForCyclistsButNowItIs(tags: Map, isLeftHandTraffic: Boolean): Boolean = isOneway(tags) && isNotOnewayForCyclists(tags, isLeftHandTraffic) - && !isNotOnewayForCyclistsNow(tags, isLeftHandTraffic) + && isNotOnewayForCyclistsNow(tags, isLeftHandTraffic) == false -fun LeftAndRightCycleway.isNotOnewayForCyclistsNow(tags: Map, isLeftHandTraffic: Boolean): Boolean { +/** Returns whether this is now not a oneway for cyclists. It returns null if the contra-flow side + * is null, i.e. no change is being made for the contra-flow side */ +fun LeftAndRightCycleway.isNotOnewayForCyclistsNow(tags: Map, isLeftHandTraffic: Boolean): Boolean? { val onewayDir = when { isForwardOneway(tags) -> FORWARD isReversedOneway(tags) -> BACKWARD else -> return true } - val previous = createCyclewaySides(tags, isLeftHandTraffic) - val l = (left ?: previous?.left) - val r = (right ?: previous?.right) /* "no cycleway" has no direction and should be ignored "separate" should also be ignored because if the cycleway is mapped separately, the existence of a separate way that enables cyclists to go in contra-flow-direction doesn't mean that they can do the same on the main way for the road too (see #4715) */ - val leftDir = l?.direction?.takeIf { l.cycleway != NONE && l.cycleway != SEPARATE } - val rightDir = r?.direction?.takeIf { r.cycleway != NONE && r.cycleway != SEPARATE } - - return leftDir == BOTH || rightDir == BOTH || - leftDir != null && leftDir != onewayDir || - rightDir != null && rightDir != onewayDir + val leftDir = left?.direction?.takeIf { left.cycleway != NONE && left.cycleway != SEPARATE } + val rightDir = right?.direction?.takeIf { right.cycleway != NONE && right.cycleway != SEPARATE } + + // any side goes in both directions + if (leftDir == BOTH || rightDir == BOTH) return true + // left side has contra-flow to oneway + if (leftDir != null && leftDir != onewayDir) return true + // right side has contra-flow to oneway + if (rightDir != null && rightDir != onewayDir) return true + + // if no cycleway is defined for the side that is usually contra to the oneway flow, we + // return that we do not know it + val contraFlowSide = if (isForwardOneway(tags) xor isLeftHandTraffic) left else right + if (contraFlowSide == null) return null + + return false } @Serializable diff --git a/app/src/main/java/de/westnordost/streetcomplete/osm/cycleway/CyclewayCreator.kt b/app/src/main/java/de/westnordost/streetcomplete/osm/cycleway/CyclewayCreator.kt index de54fe9013..9549d87b0e 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/osm/cycleway/CyclewayCreator.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/osm/cycleway/CyclewayCreator.kt @@ -83,12 +83,13 @@ private fun Tags.expandBareTagIntoSide(key: String, suffix: String = "", side: S } private fun LeftAndRightCycleway.applyOnewayNotForCyclists(tags: Tags, isLeftHandTraffic: Boolean) { - if (isOneway(tags) && isNotOnewayForCyclistsNow(tags, isLeftHandTraffic)) { - tags["oneway:bicycle"] = "no" - } else { + val isNotOnewayForCyclistsNow = isNotOnewayForCyclistsNow(tags, isLeftHandTraffic) + if (!isOneway(tags) || isNotOnewayForCyclistsNow == false) { if (tags["oneway:bicycle"] == "no") { tags.remove("oneway:bicycle") } + } else if(isNotOnewayForCyclistsNow == true) { + tags["oneway:bicycle"] = "no" } } diff --git a/app/src/test/java/de/westnordost/streetcomplete/osm/cycleway/CyclewayKtTest.kt b/app/src/test/java/de/westnordost/streetcomplete/osm/cycleway/CyclewayKtTest.kt index 598913a992..efa808f6bd 100644 --- a/app/src/test/java/de/westnordost/streetcomplete/osm/cycleway/CyclewayKtTest.kt +++ b/app/src/test/java/de/westnordost/streetcomplete/osm/cycleway/CyclewayKtTest.kt @@ -4,6 +4,7 @@ import de.westnordost.streetcomplete.osm.cycleway.Cycleway.* import de.westnordost.streetcomplete.osm.cycleway.Direction.* import kotlin.test.Test import kotlin.test.assertFalse +import kotlin.test.assertNull import kotlin.test.assertTrue class CyclewayKtTest { @@ -62,12 +63,45 @@ class CyclewayKtTest { ) } + @Test fun `not a oneway for cyclists is still not a oneway for cyclists when contra-flow side is not defined`() { + val noOnewayForCyclists = mapOf("oneway" to "yes", "oneway:bicycle" to "no") + val noOnewayForCyclistsReverse = mapOf("oneway" to "-1", "oneway:bicycle" to "no") + val forwardTrack = CyclewayAndDirection(TRACK, FORWARD) + val backwardTrack = CyclewayAndDirection(TRACK, BACKWARD) + + assertNull( + LeftAndRightCycleway(null, null) + .isNotOnewayForCyclistsNow(noOnewayForCyclists, false) + ) + assertNull( + LeftAndRightCycleway(null, forwardTrack) + .isNotOnewayForCyclistsNow(noOnewayForCyclists, false) + ) + assertNull( + LeftAndRightCycleway(backwardTrack, null) + .isNotOnewayForCyclistsNow(noOnewayForCyclistsReverse, false) + ) + + assertNull( + LeftAndRightCycleway(null, null) + .isNotOnewayForCyclistsNow(noOnewayForCyclists, true) + ) + assertNull( + LeftAndRightCycleway(forwardTrack, null) + .isNotOnewayForCyclistsNow(noOnewayForCyclists, true) + ) + assertNull( + LeftAndRightCycleway(null, backwardTrack) + .isNotOnewayForCyclistsNow(noOnewayForCyclistsReverse, true) + ) + } + @Test fun `not a oneway is no oneway for cyclists`() { assertTrue( LeftAndRightCycleway( CyclewayAndDirection(NONE, BACKWARD), CyclewayAndDirection(NONE, FORWARD) - ).isNotOnewayForCyclistsNow(mapOf(), false) + ).isNotOnewayForCyclistsNow(mapOf(), false)!! ) } @@ -76,7 +110,7 @@ class CyclewayKtTest { LeftAndRightCycleway( CyclewayAndDirection(NONE, BACKWARD), CyclewayAndDirection(NONE, FORWARD) - ).isNotOnewayForCyclistsNow(mapOf("oneway" to "yes"), false) + ).isNotOnewayForCyclistsNow(mapOf("oneway" to "yes"), false)!! ) } @@ -85,7 +119,7 @@ class CyclewayKtTest { LeftAndRightCycleway( CyclewayAndDirection(NONE, BACKWARD), CyclewayAndDirection(NONE, FORWARD) - ).isNotOnewayForCyclistsNow(mapOf("oneway" to "-1"), false) + ).isNotOnewayForCyclistsNow(mapOf("oneway" to "-1"), false)!! ) } @@ -94,70 +128,81 @@ class CyclewayKtTest { LeftAndRightCycleway( CyclewayAndDirection(NONE, BACKWARD), CyclewayAndDirection(TRACK, BOTH) - ).isNotOnewayForCyclistsNow(mapOf("oneway" to "yes"), false) + ).isNotOnewayForCyclistsNow(mapOf("oneway" to "yes"), false)!! ) assertTrue( LeftAndRightCycleway( CyclewayAndDirection(TRACK, BOTH), CyclewayAndDirection(NONE, FORWARD) - ).isNotOnewayForCyclistsNow(mapOf("oneway" to "-1"), false) + ).isNotOnewayForCyclistsNow(mapOf("oneway" to "-1"), false)!! ) } @Test fun `oneway is no oneway for cyclists when any cycleway goes in contra-flow direction`() { + val forwardTrack = CyclewayAndDirection(TRACK, FORWARD) + val backwardTrack = CyclewayAndDirection(TRACK, BACKWARD) + assertTrue( - LeftAndRightCycleway(CyclewayAndDirection(TRACK, BACKWARD), null) - .isNotOnewayForCyclistsNow(mapOf("oneway" to "yes"), false) + LeftAndRightCycleway(backwardTrack, null) + .isNotOnewayForCyclistsNow(mapOf("oneway" to "yes"), false)!! ) assertTrue( - LeftAndRightCycleway(null, CyclewayAndDirection(TRACK, BACKWARD)) - .isNotOnewayForCyclistsNow(mapOf("oneway" to "yes"), false) + LeftAndRightCycleway(null, backwardTrack) + .isNotOnewayForCyclistsNow(mapOf("oneway" to "yes"), false)!! ) assertTrue( - LeftAndRightCycleway(CyclewayAndDirection(TRACK, FORWARD), null) - .isNotOnewayForCyclistsNow(mapOf("oneway" to "-1"), false) + LeftAndRightCycleway(forwardTrack, null) + .isNotOnewayForCyclistsNow(mapOf("oneway" to "-1"), false)!! ) assertTrue( - LeftAndRightCycleway(null, CyclewayAndDirection(TRACK, FORWARD)) - .isNotOnewayForCyclistsNow(mapOf("oneway" to "-1"), false) + LeftAndRightCycleway(null, forwardTrack) + .isNotOnewayForCyclistsNow(mapOf("oneway" to "-1"), false)!! ) } @Test fun `oneway is a oneway for cyclists when separately mapped cycleway goes in contra-flow direction`() { - assertFalse( - LeftAndRightCycleway(CyclewayAndDirection(SEPARATE, BACKWARD), null) - .isNotOnewayForCyclistsNow(mapOf("oneway" to "yes"), false) - ) - assertFalse( - LeftAndRightCycleway(null, CyclewayAndDirection(SEPARATE, BACKWARD)) - .isNotOnewayForCyclistsNow(mapOf("oneway" to "yes"), false) - ) - assertFalse( - LeftAndRightCycleway(CyclewayAndDirection(SEPARATE, FORWARD), null) - .isNotOnewayForCyclistsNow(mapOf("oneway" to "-1"), false) - ) - assertFalse( - LeftAndRightCycleway(null, CyclewayAndDirection(SEPARATE, FORWARD)) - .isNotOnewayForCyclistsNow(mapOf("oneway" to "-1"), false) - ) + // ... because direction of separate cycleway and no cycleway is ignored + val separate = CyclewayAndDirection(SEPARATE, BOTH) + val none = CyclewayAndDirection(NONE, BOTH) + + for (isLeftHandTraffic in listOf(false, true)) { + for (oneway in listOf(mapOf("oneway" to "yes"), mapOf("oneway" to "-1"))) { + assertFalse( + LeftAndRightCycleway(separate, none) + .isNotOnewayForCyclistsNow(oneway, isLeftHandTraffic)!! + ) + assertFalse( + LeftAndRightCycleway(none, separate) + .isNotOnewayForCyclistsNow(oneway, isLeftHandTraffic)!! + ) + } + } } + @Test fun `oneway is oneway for cyclists when no cycleway goes in contra-flow direction`() { - assertFalse( - LeftAndRightCycleway(CyclewayAndDirection(TRACK, FORWARD), null) - .isNotOnewayForCyclistsNow(mapOf("oneway" to "yes"), false) - ) - assertFalse( - LeftAndRightCycleway(null, CyclewayAndDirection(TRACK, FORWARD)) - .isNotOnewayForCyclistsNow(mapOf("oneway" to "yes"), false) - ) - assertFalse( - LeftAndRightCycleway(CyclewayAndDirection(TRACK, BACKWARD), null) - .isNotOnewayForCyclistsNow(mapOf("oneway" to "-1"), false) - ) - assertFalse( - LeftAndRightCycleway(null, CyclewayAndDirection(TRACK, BACKWARD)) - .isNotOnewayForCyclistsNow(mapOf("oneway" to "-1"), false) - ) + val forwardTrack = CyclewayAndDirection(TRACK, FORWARD) + val backwardTrack = CyclewayAndDirection(TRACK, BACKWARD) + // direction does not matter for NONE, best way to test that is to set BOTH + val none = CyclewayAndDirection(NONE, BOTH) + + for (isLeftHandTraffic in listOf(false, true)) { + assertFalse( + LeftAndRightCycleway(forwardTrack, none) + .isNotOnewayForCyclistsNow(mapOf("oneway" to "yes"), isLeftHandTraffic)!! + ) + assertFalse( + LeftAndRightCycleway(none, forwardTrack) + .isNotOnewayForCyclistsNow(mapOf("oneway" to "yes"), isLeftHandTraffic)!! + ) + assertFalse( + LeftAndRightCycleway(backwardTrack, none) + .isNotOnewayForCyclistsNow(mapOf("oneway" to "-1"), isLeftHandTraffic)!! + ) + assertFalse( + LeftAndRightCycleway(none, backwardTrack) + .isNotOnewayForCyclistsNow(mapOf("oneway" to "-1"), isLeftHandTraffic)!! + ) + } } }