From 86d3703aa9bcfcf74629ac0832c7d3a22191a140 Mon Sep 17 00:00:00 2001 From: Simon Greuter <6537188+greuters@users.noreply.github.com> Date: Sun, 24 Mar 2024 19:22:27 -0700 Subject: [PATCH] Refactored to use sequences and added tests Now testing with RobolectricTestRunner instead of mocking XmlPullParser --- app/build.gradle.kts | 1 + .../streetcomplete/data/import/GpxImport.kt | 346 +++++++++++------- .../data/import/GpxImportParse.kt | 76 ++-- .../screens/main/MainFragment.kt | 4 +- .../main/controls/GpxImportSettingsDialog.kt | 2 +- .../screens/main/map/MainMapFragment.kt | 2 +- .../components/ImportedTrackMapComponent.kt | 9 +- .../GpxImportAddInterpolatedPointsTest.kt | 105 ++++++ .../import/GpxImportDetermineBBoxesTest.kt | 102 ++++++ .../GpxImportDiscardRedundantPointsTest.kt | 96 +++++ .../GpxImportMapToCenteredSquaresTest.kt | 37 ++ .../data/import/GpxImportMergeBBoxesTest.kt | 70 ++++ .../data/import/GpxImportParseTest.kt | 194 ++++++++++ .../data/import/GpxImportTest.kt | 335 +++++++++++++++++ .../data/import/GpxSingleTrackParserTest.kt | 225 ------------ .../streetcomplete/data/import/TestHelpers.kt | 33 ++ 16 files changed, 1246 insertions(+), 391 deletions(-) create mode 100644 app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportAddInterpolatedPointsTest.kt create mode 100644 app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportDetermineBBoxesTest.kt create mode 100644 app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportDiscardRedundantPointsTest.kt create mode 100644 app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportMapToCenteredSquaresTest.kt create mode 100644 app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportMergeBBoxesTest.kt create mode 100644 app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportParseTest.kt create mode 100644 app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportTest.kt delete mode 100644 app/src/test/java/de/westnordost/streetcomplete/data/import/GpxSingleTrackParserTest.kt create mode 100644 app/src/test/java/de/westnordost/streetcomplete/data/import/TestHelpers.kt diff --git a/app/build.gradle.kts b/app/build.gradle.kts index 3a1850abbd0..bb4617e2262 100644 --- a/app/build.gradle.kts +++ b/app/build.gradle.kts @@ -116,6 +116,7 @@ dependencies { testImplementation("org.mockito:mockito-inline:$mockitoVersion") testImplementation("org.assertj:assertj-core:3.23.1") testImplementation(kotlin("test")) + testImplementation("org.robolectric:robolectric:4.12.2") androidTestImplementation("androidx.test:runner:1.5.2") androidTestImplementation("androidx.test:rules:1.5.0") diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/import/GpxImport.kt b/app/src/main/java/de/westnordost/streetcomplete/data/import/GpxImport.kt index 9f48597d4ae..7c6bac52fa3 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/data/import/GpxImport.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/data/import/GpxImport.kt @@ -23,173 +23,227 @@ private const val TAG = "GpxImport" data class GpxImportData( val displayTrack: Boolean, val downloadAlongTrack: Boolean, - val trackpoints: List, + val segments: List>, val downloadBBoxes: List, val areaToDownloadInSqkm: Double, ) -private class DecoratedBoundingBox(val polygon: Iterable) { +internal class DecoratedBoundingBox( + val polygon: Iterable, + requestedTiles: Set? = null, +) { val boundingBox = polygon.enclosingBoundingBox() - val area = boundingBox.area() val tiles = boundingBox.enclosingTilesRect(ApplicationConstants.DOWNLOAD_TILE_ZOOM) .asTilePosSequence() val numberOfTiles = tiles.count() + val requestedTiles = when (requestedTiles) { + null -> { + tiles.toHashSet() + } + + else -> { + requestedTiles + } + } } -// TODO sgr: refactor function signature when adapting UI /** - * @param inputStream GPX track to parse + * @param inputStream valid XML according to http://www.topografix.com/GPX/1/1 schema + * Note: the caller is responsible to close the inputStream as appropriate * @param displayTrack display the track on the map after import * @param findDownloadBBoxes if true, compute bounding boxes which need to be downloaded to cover the track - * @param minDownloadDistance in meters; points within minDownloadDistance along the track should be downloaded - * @param progressCallback a callback to set progress between 0 .. 100 + * @param minDownloadDistance in meters; all points within minDownloadDistance along the track should be downloaded */ suspend fun importGpx( inputStream: InputStream, displayTrack: Boolean, findDownloadBBoxes: Boolean, minDownloadDistance: Double, - progressCallback: suspend (progress: Int) -> Unit, ): Result = withContext(Dispatchers.Default) { require(minDownloadDistance in 10.0..500.0) { "minDownloadDistance needs to be of reasonable size" } + val trackSegments = parseGpxFile(inputStream) + + if (findDownloadBBoxes) { + return@withContext importWithBBoxes(trackSegments, displayTrack, minDownloadDistance) + } else { + return@withContext importTrackOnly(trackSegments, displayTrack) + } +} + +private fun importTrackOnly( + trackSegments: Sequence, + displayTrack: Boolean, +): Result { + return Result.success( + GpxImportData( + displayTrack, + false, + trackSegments + .map { it.toList() } + .toList(), + arrayListOf(), + 0.0 + ) + ) +} +private fun importWithBBoxes( + trackSegments: Sequence, + displayTrack: Boolean, + minDownloadDistance: Double, +): Result { /* Algorithm overview: - * - * Given that two resampled points A and B are at most 2 * minDownloadDistance away from each - * other and any track point between them is at most minDownloadDistance away from either A or B, - * an area that fully contains the track between A and B is given by a square S_track centered - * on the middle point between A and B, with side length 2 * minDownloadDistance and rotated - * such that two of its sides align with the vector from A to B. As we need to cover the area - * within minDownloadDistance of any track point (which might lie almost on the edge of S_track), - * a square S_min centered and rotated the same as S_track, but with - * side length = 4 * minDownloadDistance is a handy upper bound. - * - * If we download two non-rotated squares centered on A and B, they are guaranteed to contain - * S_min if their side length is at least 4 * minDownloadDistance / sqrt(2) - the worst case - * being were S_min is rotated 45 degrees with respect to the non-rotated squares. - */ + * Given that two resampled points A and B are at most 2 * minDownloadDistance away from each + * other and any track point between them is at most minDownloadDistance away from either A or B, + * an area that fully contains the track between A and B is given by a square S_track centered + * on the middle point between A and B, with side length 2 * minDownloadDistance and rotated + * such that two of its sides align with the vector from A to B. As we need to cover the area + * within minDownloadDistance of any track point (which might lie almost on the edge of S_track), + * a square S_min centered and rotated the same as S_track, but with + * side length = 4 * minDownloadDistance is a handy upper bound. + * + * If we download two north-south aligned squares centered on A and B, they are guaranteed to + * contain S_min if their side length is at least 4 * minDownloadDistance / sqrt(2) - the worst + * case being were S_min is rotated 45 degrees with respect to the aligned squares. + */ val maxSampleDistance = 2 * minDownloadDistance val coveringSquareHalfLength = 2 * minDownloadDistance / sqrt(2.0) - val originalTrackPoints = parseSingleGpxTrack(inputStream) - var progress = 0 - val mergedBBoxes = originalTrackPoints - .asSequence() - // TODO sgr: just a test how one could decorate sequences with a callback - // -> this approach would need orchestration at this level from UI code + // TODO sgr: find a good way of retrieving original track points and area to download. + // Up to this point, working on sequences seems clean to me and would be easy if we only need + // the bounding boxes. Retrieving additional info seems to be awkward though. + // - + // One workaround might be to parse the file twice (once only to retrieve the original track + // points) and omit estimating any download size. + val originalTrackPoints = arrayListOf>() + val mergedBBoxes = arrayListOf() + val areaToDownloadInSqkm = trackSegments + // TODO sgr: this seems to be an awkward way of smuggling out original track points .map { - progress++ - if (progress % 500 == 0) { - Log.d(TAG, "updating progress: ${progress / originalTrackPoints.size}") + originalTrackPoints.add(arrayListOf()) + it.onEach { trackPoint -> + originalTrackPoints.last().add(trackPoint) } - it } - .addInterpolatedPoints(maxSampleDistance) - .discardRedundantPoints(maxSampleDistance) - .mapToCenteredSquares(coveringSquareHalfLength) - .determineBBoxesToDownload() - .mergeBBoxesToDownload() + .map { trackSegment -> + trackSegment + .addInterpolatedPoints(maxSampleDistance) + .discardRedundantPoints(maxSampleDistance) + .mapToCenteredSquares(coveringSquareHalfLength) + .determineBBoxes() + } + .flatten() + .mergeBBoxes() + // TODO sgr: this seems to be an awkward way of smuggling out bounding boxes + .onEach { + mergedBBoxes.add(it.boundingBox) + } + .flatMap { it.tiles } + .distinct() + .sumOf { it.asBoundingBox(ApplicationConstants.DOWNLOAD_TILE_ZOOM).area() } / 1000000 - return@withContext Result.success( + return Result.success( GpxImportData( displayTrack, true, originalTrackPoints, - mergedBBoxes.map { it.boundingBox }.toList(), - mergedBBoxes - .flatMap { it.tiles } - .distinct() - .sumOf { it.asBoundingBox(ApplicationConstants.DOWNLOAD_TILE_ZOOM).area() } - / 1000000 + mergedBBoxes, + areaToDownloadInSqkm ) ) } /** - * TODO sgr: convert implementation to real sequence.. might be tricky - * Iteratively merge bounding boxes to save download calls in trade for a few more unique tiles - * downloaded + * Merge bounding boxes to save download calls in trade for a few more unique tiles + * downloaded. + * + * The algorithm merges adjacent boxes if the merged box still has a good enough ratio + * of actually requested vs total number of tiles downloaded. */ -private fun Sequence.mergeBBoxesToDownload(): Sequence { - var bBoxes = this.toList() - val mergedBBoxes = ArrayList() - while (mergedBBoxes.size < bBoxes.size) { - Log.d(TAG, "start a new round of bounding box merging") - var currentBBox: DecoratedBoundingBox? = null - for (bBox in bBoxes) { - if (currentBBox == null) { - currentBBox = bBox - continue +internal fun Sequence.mergeBBoxes(): Sequence { + val inputIterator = this.iterator() + if (!inputIterator.hasNext()) + return emptySequence() + return sequence { + var mergedBBox = inputIterator.next() + while(inputIterator.hasNext()) { + val bBox = inputIterator.next() + val candidateBBox = DecoratedBoundingBox( + mergedBBox.polygon + bBox.polygon, + mergedBBox.requestedTiles.plus(bBox.tiles) + ) + val requestedRatio = + candidateBBox.requestedTiles.size.toDouble() / candidateBBox.numberOfTiles + Log.d(TAG, "requestedRatio = $requestedRatio)") + // requestedRatio >= 0.75 is a good compromise, as this allows downloading three + // neighbouring tiles at zoom level x in a single call at level x-1 + mergedBBox = if (requestedRatio >= 0.75) { + candidateBBox + } else { + yield(mergedBBox) + bBox } - val mergedBBox = DecoratedBoundingBox(bBox.polygon + currentBBox.polygon) - // merge two adjacent boxes if at most one additional tile needs to be downloaded to save one call - currentBBox = - if (mergedBBox.numberOfTiles <= (currentBBox.tiles + bBox.tiles).toHashSet().size + 1) { - Log.d(TAG, "merge currentBBox with previous one") - mergedBBox - } else { - Log.d(TAG, "keep currentBBox separate from previous one") - mergedBBoxes.add(currentBBox) - bBox - } - } - currentBBox?.let { mergedBBoxes.add(it) } - if (mergedBBoxes.size < bBoxes.size) { - Log.d(TAG, "reduced bounding boxes from ${bBoxes.size} to ${mergedBBoxes.size}") - bBoxes = mergedBBoxes.toList() - mergedBBoxes.clear() - } else { - Log.d(TAG, "final number of bounding boxes: ${mergedBBoxes.size}") + } + yield(mergedBBox) } - return mergedBBoxes.asSequence() } -private fun Sequence.determineBBoxesToDownload(): Sequence { - var currentBBox: DecoratedBoundingBox? = null - val uniqueTilesToDownload = HashSet() +/** + * Reduce a sequence of bounding boxes by + * - dropping boxes which don't contribute additional tiles to download + * - merging adjacent boxes if no additional tiles are contained in the merged box + * + * the mapped boxes are also decorated with some cached data for future processing. + */ +internal fun Sequence.determineBBoxes(): Sequence { val inputIterator = this.map { DecoratedBoundingBox(it.toPolygon()) }.withIndex().iterator() + if (!inputIterator.hasNext()) { + return emptySequence() + } + + val uniqueTilesToDownload = HashSet() return sequence { - for ((index, newBBox) in inputIterator) { - if (currentBBox == null) { - currentBBox = newBBox - yield(newBBox) - continue - } + var currentBBox = inputIterator.next().value + uniqueTilesToDownload.addAll(currentBBox.tiles) - if (!newBBox.tiles.any { tilePos -> tilePos !in uniqueTilesToDownload }) { + for ((index, newBBox) in inputIterator) { + if (newBBox.tiles.all { it in uniqueTilesToDownload }) { Log.d(TAG, "omit bounding box #$index, all tiles already scheduled for download") continue } - val extendedBBox = DecoratedBoundingBox(currentBBox!!.polygon + newBBox.polygon) + val extendedBBox = DecoratedBoundingBox(currentBBox.polygon + newBBox.polygon) currentBBox = if ( - // no additional tile needed to extend the polygon and download newBBox together with currentBBox - extendedBBox.numberOfTiles <= (currentBBox!!.tiles + newBBox.tiles).toHashSet().size - || - // downloaded area is not increased by extending the current polygon instead of downloading separately - extendedBBox.area < currentBBox!!.area + newBBox.area + extendedBBox.numberOfTiles <= (currentBBox.tiles + newBBox.tiles).toHashSet().size ) { + // no additional tile needed to extend the polygon and download newBBox together with currentBBox Log.d(TAG, "extend currentBBox with bounding box #$index") extendedBBox } else { - Log.d(TAG, "schedule currentBBox, start new with bounding box #$index") - yield(currentBBox!!) - uniqueTilesToDownload.addAll(currentBBox!!.tiles) + Log.d(TAG, "retain currentBBox, start new with bounding box #$index") + yield(currentBBox) + uniqueTilesToDownload.addAll(currentBBox.tiles) newBBox } } - currentBBox?.let { yield(it) } + yield(currentBBox) } } /** - * Transform a sequence of points to a sequence of bounding boxes centered on the points. + * Transform a sequence of points to a sequence of north-south aligned bounding boxes centered on + * these points. + * + * @param halfSideLength > 0.0, in meters */ -private fun Sequence.mapToCenteredSquares(halfSideLength: Double): Sequence = - map { +internal fun Sequence.mapToCenteredSquares(halfSideLength: Double): Sequence { + require(halfSideLength > 0.0) { + "halfSideLength has to be positive" + } + return map { arrayListOf( it.translate(halfSideLength, 0.0), it.translate(halfSideLength, 90.0), @@ -197,6 +251,7 @@ private fun Sequence.mapToCenteredSquares(halfSideLength: Double): Seque it.translate(halfSideLength, 270.0) ).enclosingBoundingBox() } +} /** * Ensure points are at most samplingDistance away from each other. @@ -204,19 +259,28 @@ private fun Sequence.mapToCenteredSquares(halfSideLength: Double): Seque * Given two consecutive points A, B which are more than samplingDistance away from each other, * add intermediate points on the line from A to B, samplingDistance away from each other until the * last one is <= samplingDistance away from B. + * A and B are always retained, even if they are < samplingDistance away from each other. + * + * @param samplingDistance > 0.0, in meters */ -private fun Sequence.addInterpolatedPoints(samplingDistance: Double): Sequence { - var candidatePoint: LatLon? = null - val seq = this.flatMap { currentPoint -> - if (candidatePoint == null) { - candidatePoint = currentPoint - return@flatMap emptySequence() +internal fun Sequence.addInterpolatedPoints(samplingDistance: Double): Sequence { + require(samplingDistance > 0.0) { + "samplingDistance has to be positive" + } + + val inputIterator = this.iterator() + if (!inputIterator.hasNext()) { + return emptySequence() + } + var lastPoint = inputIterator.next() + return sequence { + while (inputIterator.hasNext()) { + val currentPoint = inputIterator.next() + this.yieldAll(interpolate(lastPoint, currentPoint, samplingDistance)) + lastPoint = currentPoint } - val interpolatedPoints = interpolate(candidatePoint!!, currentPoint, samplingDistance) - candidatePoint = currentPoint - return@flatMap interpolatedPoints + yield(lastPoint) } - return seq + sequenceOf(candidatePoint).mapNotNull { it } } /** @@ -224,44 +288,62 @@ private fun Sequence.addInterpolatedPoints(samplingDistance: Double): Se * * Returned points are samplingDistance away from each other and on the line between start and end. * The last returned point is <= samplingDistance away from end. + * + * @param samplingDistance > 0.0, in meters */ private fun interpolate(start: LatLon, end: LatLon, samplingDistance: Double): Sequence = sequence { - val bearing = start.initialBearingTo(end) + require(samplingDistance > 0.0) { + "samplingDistance has to be positive" + } + var intermediatePoint = start while (true) { yield(intermediatePoint) - intermediatePoint = intermediatePoint.translate(samplingDistance, bearing) if (intermediatePoint.distanceTo(end) <= samplingDistance) { break } + intermediatePoint = intermediatePoint.translate( + samplingDistance, + intermediatePoint.initialBearingTo(end) + ) } } /** - * Discard redundant points, such that no three remaining points A, B, C exist where B is less than + * Discard redundant points, such that no three adjacent points A, B, C remain where B is less than * samplingDistance away from both A and C + * + * @param samplingDistance > 0.0, in meters */ -private fun Sequence.discardRedundantPoints(samplingDistance: Double): Sequence { - var lastRetainedPoint: LatLon? = null - var candidatePoint: LatLon? = null - return this.flatMap { currentPoint -> - sequence { - if (candidatePoint == null) { - candidatePoint = currentPoint - } else if (lastRetainedPoint == null) { - lastRetainedPoint = candidatePoint - candidatePoint = currentPoint - } else if (lastRetainedPoint!!.distanceTo(candidatePoint!!) < samplingDistance - && candidatePoint!!.distanceTo(currentPoint) < samplingDistance - ) { - // discard candidatePoint - candidatePoint = currentPoint - } else { - lastRetainedPoint = candidatePoint - yield(lastRetainedPoint!!) +internal fun Sequence.discardRedundantPoints(samplingDistance: Double): Sequence { + require(samplingDistance > 0.0) { + "samplingDistance has to be positive" + } + + val inputIterator = this.iterator() + if (!inputIterator.hasNext()) { + return emptySequence() + } + return sequence { + var lastRetainedPoint = inputIterator.next() + yield(lastRetainedPoint) + + if (inputIterator.hasNext()) { + var candidatePoint = inputIterator.next() + while (inputIterator.hasNext()) { + val currentPoint = inputIterator.next() + if (lastRetainedPoint.distanceTo(candidatePoint) < samplingDistance + && candidatePoint.distanceTo(currentPoint) < samplingDistance + ) { + // discard candidatePoint + } else { + lastRetainedPoint = candidatePoint + yield(lastRetainedPoint) + } candidatePoint = currentPoint } + yield(candidatePoint) } - } + sequenceOf(candidatePoint).mapNotNull { it } + } } diff --git a/app/src/main/java/de/westnordost/streetcomplete/data/import/GpxImportParse.kt b/app/src/main/java/de/westnordost/streetcomplete/data/import/GpxImportParse.kt index 4c97453301a..a6741dab7e5 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/data/import/GpxImportParse.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/data/import/GpxImportParse.kt @@ -8,48 +8,72 @@ import java.io.IOException import java.io.InputStream private const val TRACK_POINT = "trkpt" -private const val TRACK = "trk" +private const val SEGMENT = "trkseg" + +typealias TrackSegment = Sequence /** - * Parses the track points of the first track from a GPX file. + * Parses all track points from a GPX file. * - * As the standard doesn't require multiple tracks to be connected, and this lightweight parser - * function is intended to provide a single consecutive track, further tracks are ignored. - * Multiple track segments within the first track are parsed however, assuming any gaps are - * negligible and can be interpolated meaningfully during subsequent processing. + * Yields consecutive segments. Track points within a TrackSegment can be interpolated meaningfully + * during subsequent processing, while track points from different TrackSegments cannot be assumed + * to be connected. * * @param inputStream valid XML according to http://www.topografix.com/GPX/1/1 schema - */ -fun parseSingleGpxTrack(inputStream: InputStream): List { - // TODO sgr: if this should return a sequence, would need to pass e.g. a Sequence from - // File.useLines to make sure the file is correctly closed; inputStream.use closes it - // immediately upon return, leading to an exception when requesting the next element of the sequence - inputStream.use { - val xpp = Xml.newPullParser() - xpp.setInput(inputStream, "UTF-8") - try { - return parseSingleGpxTrack(xpp).toList() - } catch (e: Exception) { - throw e; - } - } -} - -/** - * @param parser a pull parser with input already set, ready to call nextTag + * Note: the caller is responsible to close the inputStream as appropriate; + * calls to the resulting sequence after closing the inputStream may fail. + * @return a sequence of TrackSegments, i.e. sequences of track points logically connected. + * + * Note: The nested sequences returned work on a single input stream, thus make sure to exhaust + * any TrackSegment first before proceeding to the next one. A TrackSegment will not yield any + * more track points once you proceed to the next TrackSegment in the outer sequence, thus e.g. + * result.toList().first().toList() will not yield any element, while + * result.flatMap { it.toList() }.toList() will. */ @Throws(XmlPullParserException::class, IOException::class) -fun parseSingleGpxTrack(parser: XmlPullParser): Sequence = sequence { +fun parseGpxFile(inputStream: InputStream): Sequence { + val parser = Xml.newPullParser() + parser.setInput(inputStream, "UTF-8") + parser.setFeature(XmlPullParser.FEATURE_PROCESS_NAMESPACES, true) parser.nextTag() parser.require(XmlPullParser.START_TAG, "http://www.topografix.com/GPX/1/1", "gpx") + return sequence { + var depth = 1 + while (depth != 0) { + when (parser.next()) { + XmlPullParser.END_TAG -> { + depth-- + } + + XmlPullParser.START_TAG -> { + if (parser.name == SEGMENT) { + // segment is closed while parsing, thus depth remains the same + yield( + parseSegment(parser) + ) + } else { + depth++ + } + } + + XmlPullParser.END_DOCUMENT -> { + break + } + } + } + } +} + +@Throws(XmlPullParserException::class, IOException::class) +private fun parseSegment(parser: XmlPullParser): TrackSegment = sequence { var depth = 1 while (depth != 0) { when (parser.next()) { XmlPullParser.END_TAG -> { - if (parser.name == TRACK) { + if (parser.name == SEGMENT) { return@sequence } depth-- diff --git a/app/src/main/java/de/westnordost/streetcomplete/screens/main/MainFragment.kt b/app/src/main/java/de/westnordost/streetcomplete/screens/main/MainFragment.kt index 5f299515e5f..0322e5a04dc 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/screens/main/MainFragment.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/screens/main/MainFragment.kt @@ -1221,7 +1221,8 @@ class MainFragment : }.getOrNull() ?: return@launch if (importData.displayTrack) { - fragment.replaceImportedTrack(importData.trackpoints) + // TODO sgr: maybe want to get rid of showing the track, as it would mean storing all points which we are avoiding with sequences.. + fragment.replaceImportedTrack(importData.segments) } if (importData.downloadAlongTrack) { if (importData.areaToDownloadInSqkm > ApplicationConstants.MAX_DOWNLOADABLE_AREA_IN_SQKM * 25) { @@ -1234,7 +1235,6 @@ class MainFragment : lengthUnit ) { cont.resume(it) }.show() } - for (bBox in importData.downloadBBoxes) { downloadController.download(bBox) } diff --git a/app/src/main/java/de/westnordost/streetcomplete/screens/main/controls/GpxImportSettingsDialog.kt b/app/src/main/java/de/westnordost/streetcomplete/screens/main/controls/GpxImportSettingsDialog.kt index a575b8f3f9f..27189b62fbf 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/screens/main/controls/GpxImportSettingsDialog.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/screens/main/controls/GpxImportSettingsDialog.kt @@ -114,7 +114,7 @@ class GpxImportSettingsDialog( binding.displayTrackCheckBox.isChecked, binding.downloadCheckBox.isChecked, minDownloadDistanceInMeters() - ) { p -> withContext(Dispatchers.Main) { binding.importProgress.progress = p } } + ) } val importData = worker!!.await() diff --git a/app/src/main/java/de/westnordost/streetcomplete/screens/main/map/MainMapFragment.kt b/app/src/main/java/de/westnordost/streetcomplete/screens/main/map/MainMapFragment.kt index 6b40a92db6a..8478f524e5d 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/screens/main/map/MainMapFragment.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/screens/main/map/MainMapFragment.kt @@ -316,7 +316,7 @@ class MainMapFragment : LocationAwareMapFragment(), ShowsGeometryMarkers { } /* -------------------------------- Show imported tracks ------------------------------------ */ - fun replaceImportedTrack(trackpoints: List) { + fun replaceImportedTrack(trackpoints: List>) { importedTrackMapComponent?.replaceImportedTrack(trackpoints) } } diff --git a/app/src/main/java/de/westnordost/streetcomplete/screens/main/map/components/ImportedTrackMapComponent.kt b/app/src/main/java/de/westnordost/streetcomplete/screens/main/map/components/ImportedTrackMapComponent.kt index 992a1e0e8b0..98ae3ac491d 100644 --- a/app/src/main/java/de/westnordost/streetcomplete/screens/main/map/components/ImportedTrackMapComponent.kt +++ b/app/src/main/java/de/westnordost/streetcomplete/screens/main/map/components/ImportedTrackMapComponent.kt @@ -12,16 +12,17 @@ class ImportedTrackMapComponent(ctrl: KtMapController) : KoinComponent { private val trackLayer = ctrl.addDataLayer(IMPORTED_TRACK_LAYER) fun replaceImportedTrack( - trackpoints: List, + trackpoints: List>, ) { + trackLayer.clear() trackLayer.setFeatures( - listOf( + trackpoints.map { it -> Polyline( - trackpoints.map { it.toLngLat() }.toMutableList(), + it.map { it.toLngLat() }.toMutableList(), mutableMapOf("type" to "line") ) - ) + } ) } diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportAddInterpolatedPointsTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportAddInterpolatedPointsTest.kt new file mode 100644 index 00000000000..5574c85882e --- /dev/null +++ b/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportAddInterpolatedPointsTest.kt @@ -0,0 +1,105 @@ +package de.westnordost.streetcomplete.data.import + +import de.westnordost.streetcomplete.data.osm.mapdata.LatLon +import de.westnordost.streetcomplete.util.math.distanceTo +import org.junit.Test +import kotlin.test.assertContains +import kotlin.test.assertEquals +import kotlin.test.assertFails +import kotlin.test.assertTrue + +class GpxImportAddInterpolatedPointsTest { + @Test + fun `fails with bad parameters`() { + assertFails { + emptySequence().addInterpolatedPoints(-15.0).count() + } + } + + @Test + fun `gracefully handles small sequences`() { + assertEquals( + 0, + emptySequence().addInterpolatedPoints(1.0).count(), + "empty sequence invents points" + ) + assertEquals( + 1, + sequenceOf(LatLon(89.9, 27.1)).addInterpolatedPoints(77.0).count(), + "size 1 sequence invents points" + ) + } + + @Test + fun `does not add unnecessary points`() { + val originalPoints = listOf(LatLon(0.0, 0.0), LatLon(0.1, -0.5), LatLon(1.0, -1.0)) + val samplingDistance = + originalPoints.zipWithNext().maxOf { it.first.distanceTo(it.second) } + val interpolatedPoints = + originalPoints.asSequence().addInterpolatedPoints(samplingDistance).toList() + assertEquals(originalPoints.size, interpolatedPoints.size) + } + + @Test + fun `ensures promised sampling distance on single segment`() { + val p1 = LatLon(-11.1, 36.7) + val p2 = LatLon(-89.0, 61.0) + val numIntermediatePoints = 100 + val samplingDistance = p1.distanceTo(p2) / (numIntermediatePoints + 1) + val originalPoints = listOf(p1, p2) + val interpolatedPoints = + originalPoints.asSequence().addInterpolatedPoints(samplingDistance).toList() + assertEquals( + numIntermediatePoints + 2, + interpolatedPoints.size, + "wrong number of points created" + ) + assertCorrectSampling(originalPoints, interpolatedPoints, samplingDistance) + } + + @Test + fun `ensures promised sampling distance on multiple segments`() { + val originalPoints = + listOf(LatLon(0.0, 0.0), LatLon(1.0, 1.0), LatLon(2.1, 1.3), LatLon(0.0, 0.0)) + val samplingDistance = + originalPoints.zipWithNext().minOf { it.first.distanceTo(it.second) } / 100 + val interpolatedPoints = + originalPoints.asSequence().addInterpolatedPoints(samplingDistance).toList() + assertCorrectSampling(originalPoints, interpolatedPoints, samplingDistance) + } + + private fun assertCorrectSampling( + originalPoints: List, + interpolatedPoints: List, + samplingDistance: Double, + ) { + // some tolerance is needed due to rounding errors + val maxToleratedDistance = samplingDistance * 1.001 + val minToleratedDistance = samplingDistance * 0.999 + + val distances = interpolatedPoints.zipWithNext().map { it.first.distanceTo(it.second) } + distances.forEachIndexed { index, distance -> + assertTrue( + distance <= maxToleratedDistance, + "distance between consecutive points too big; $distance > $maxToleratedDistance" + ) + + // the only distance that may be smaller than samplingDistance is between the last + // interpolated point of one segment and the original point starting the next segment + if (distance < minToleratedDistance) { + assertContains( + originalPoints, interpolatedPoints[index + 1], + "distance between two interpolated points too small ($distance < $minToleratedDistance" + ) + } + + } + + originalPoints.forEach { + assertContains( + interpolatedPoints, it, + "original point $it is missing in interpolated points" + ) + } + } +} diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportDetermineBBoxesTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportDetermineBBoxesTest.kt new file mode 100644 index 00000000000..6a35b2181be --- /dev/null +++ b/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportDetermineBBoxesTest.kt @@ -0,0 +1,102 @@ +package de.westnordost.streetcomplete.data.import + +import de.westnordost.streetcomplete.data.osm.mapdata.BoundingBox +import de.westnordost.streetcomplete.data.osm.mapdata.LatLon +import de.westnordost.streetcomplete.util.math.area +import de.westnordost.streetcomplete.util.math.isCompletelyInside +import org.junit.Test +import kotlin.test.assertEquals +import kotlin.test.assertTrue + +class GpxImportDetermineBBoxesTest { + @Test + fun `gracefully handles empty sequence`() { + assertEquals( + 0, + emptySequence().determineBBoxes().count(), + "empty sequence not retained" + ) + } + @Test + fun `drops duplicates`() { + val inputBoxes = listOf( + BoundingBox(LatLon(-0.01, -0.01), LatLon(0.0, 0.0)), + BoundingBox(LatLon(-0.01, -0.01), LatLon(0.0, 0.0)), + ) + val downloadBoxes = inputBoxes.asSequence().determineBBoxes().toList() + assertEquals( + 1, + downloadBoxes.size, + "failed to merge all boxes into one" + ) + val downloadBBox = downloadBoxes[0].boundingBox + inputBoxes.forEach { + assertTrue( + it.isCompletelyInside(downloadBBox), + "input bounding box $it is not contained in download box $downloadBBox" + ) + } + assertEquals( + inputBoxes[0].area(), + downloadBBox.area(), + 1.0, + "area to download is not the same as area of one input box" + ) + } + + @Test + fun `merges adjacent boxes forming a rectangle`() { + val inputBoxes = listOf( + BoundingBox(LatLon(0.00, 0.0), LatLon(0.01, 0.01)), + BoundingBox(LatLon(0.01, 0.0), LatLon(0.02, 0.01)), + BoundingBox(LatLon(0.02, 0.0), LatLon(0.03, 0.01)), + ) + val downloadBoxes = inputBoxes.asSequence().determineBBoxes().toList() + assertEquals( + 1, + downloadBoxes.size, + "failed to merge all boxes into one" + ) + val downloadBBox = downloadBoxes[0].boundingBox + inputBoxes.forEach { + assertTrue( + it.isCompletelyInside(downloadBBox), + "input bounding box $it is not contained in download box $downloadBBox" + ) + } + assertEquals( + inputBoxes.sumOf { it.area() }, + downloadBBox.area(), + 1.0, + "area to download is not the same as area of input boxes" + ) + } + + @Test + fun `partially merges boxes in an L-shape`() { + val inputBoxes = listOf( + BoundingBox(LatLon(0.00, 0.00), LatLon(0.01, 0.01)), + BoundingBox(LatLon(0.01, 0.00), LatLon(0.02, 0.01)), + BoundingBox(LatLon(0.00, 0.01), LatLon(0.01, 0.06)), + ) + val downloadBoxes = inputBoxes.asSequence().determineBBoxes().toList() + assertEquals( + 2, + downloadBoxes.size, + "failed to merge one side of the L-shape" + ) + val downloadBBoxes = downloadBoxes.map { it.boundingBox } + inputBoxes.forEach { + assertTrue( + downloadBBoxes.any { downloadBBox -> it.isCompletelyInside(downloadBBox) }, + "input bounding box $it is not contained in any download box $downloadBBoxes" + ) + } + assertEquals( + inputBoxes.sumOf { it.area() }, + downloadBBoxes.sumOf { it.area() }, + 1.0, + "area to download is not the same as area of input boxes" + ) + } +} diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportDiscardRedundantPointsTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportDiscardRedundantPointsTest.kt new file mode 100644 index 00000000000..18fea84c02c --- /dev/null +++ b/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportDiscardRedundantPointsTest.kt @@ -0,0 +1,96 @@ +package de.westnordost.streetcomplete.data.import + +import de.westnordost.streetcomplete.data.osm.mapdata.LatLon +import de.westnordost.streetcomplete.util.math.distanceTo +import org.junit.Test +import kotlin.test.assertEquals +import kotlin.test.assertFails + +class GpxImportDiscardRedundantPointsTest { + @Test + fun `fails with bad parameters`() { + assertFails { + emptySequence().discardRedundantPoints(-15.0).count() + } + } + + @Test + fun `gracefully handles small sequences`() { + assertEquals( + 0, + emptySequence().discardRedundantPoints(1.0).count(), + "empty sequence not retained" + ) + assertEquals( + 1, + sequenceOf(LatLon(89.9, 27.1)).discardRedundantPoints(77.0).count(), + "size 1 sequence not retained" + ) + assertEquals( + 2, + sequenceOf(LatLon(-41.7, -39.8), LatLon(33.1, 78.8)).discardRedundantPoints(20.0) + .count(), + "size 2 sequence not retained" + ) + } + + @Test + fun `keeps non-redundant points`() { + val originalPoints = listOf(LatLon(10.10, -2.0), LatLon(19.0, -54.4), LatLon(51.04, -71.30)) + val samplingDistance = originalPoints.zipWithNext().minOf { it.first.distanceTo(it.second) } + val retainedPoints = + originalPoints.asSequence().discardRedundantPoints(samplingDistance).toList() + assertEquals( + originalPoints.size, + retainedPoints.size, + "dropping non-redundant points" + ) + } + + @Test + fun `discards single redundant point`() { + val originalPoints = listOf(LatLon(10.10, -2.0), LatLon(19.0, -54.4), LatLon(51.04, -71.30)) + val epsilon = 0.00001 + val samplingDistance = + originalPoints.zipWithNext().maxOf { it.first.distanceTo(it.second) } + epsilon + assertEquals( + originalPoints.size - 1, + originalPoints.asSequence().discardRedundantPoints(samplingDistance).count(), + "failed to drop redundant point" + ) + } + + @Test + fun `discards multiple adjacent redundant points`() { + val originalPoints = listOf( + LatLon(1.0, 0.0), + LatLon(2.0, 0.0), + LatLon(3.0, 0.0), + LatLon(4.0, 0.0) + ) + val samplingDistance = originalPoints.first().distanceTo(originalPoints.last()) + assertEquals( + 2, + originalPoints.asSequence().discardRedundantPoints(samplingDistance).count(), + "failed to drop redundant point" + ) + } + + @Test + fun `discards multiple non-adjacent redundant points`() { + val originalPoints = listOf( + LatLon(1.0, 0.0), + LatLon(2.0, 0.0), + LatLon(3.0, 0.0), + LatLon(7.0, 0.0), + LatLon(8.0, 0.0), + LatLon(9.0, 0.0), + ) + val samplingDistance = originalPoints[0].distanceTo(originalPoints[2]) + assertEquals( + 4, + originalPoints.asSequence().discardRedundantPoints(samplingDistance).count(), + "failed to drop redundant point" + ) + } +} diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportMapToCenteredSquaresTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportMapToCenteredSquaresTest.kt new file mode 100644 index 00000000000..d44ed346180 --- /dev/null +++ b/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportMapToCenteredSquaresTest.kt @@ -0,0 +1,37 @@ +package de.westnordost.streetcomplete.data.import + +import de.westnordost.streetcomplete.data.osm.mapdata.LatLon +import de.westnordost.streetcomplete.util.math.area +import de.westnordost.streetcomplete.util.math.contains +import org.junit.Test +import kotlin.math.pow +import kotlin.test.assertEquals +import kotlin.test.assertFails +import kotlin.test.assertTrue + +class GpxImportMapToCenteredSquaresTest { + @Test + fun `fails with bad parameters`() { + assertFails { + emptySequence().mapToCenteredSquares(-1.0).count() + } + } + + @Test + fun `maps correctly`() { + val point = LatLon(12.0, 37.1) + val halfSideLength = 100.0 + val squares = sequenceOf(point).mapToCenteredSquares(halfSideLength).toList() + assertEquals( + 1, + squares.size, + "expected a single bounding box, ${squares.size} returned" + ) + assertTrue(squares[0].contains(point), "center not contained in bounding box") + assertEquals( + (halfSideLength * 2).pow(2), + squares[0].area(), + 10.0, + "area not matching" + ) + }} diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportMergeBBoxesTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportMergeBBoxesTest.kt new file mode 100644 index 00000000000..32aacd7856d --- /dev/null +++ b/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportMergeBBoxesTest.kt @@ -0,0 +1,70 @@ +package de.westnordost.streetcomplete.data.import + +import de.westnordost.streetcomplete.data.osm.mapdata.BoundingBox +import de.westnordost.streetcomplete.data.osm.mapdata.LatLon +import de.westnordost.streetcomplete.data.osm.mapdata.toPolygon +import de.westnordost.streetcomplete.util.math.area +import de.westnordost.streetcomplete.util.math.isCompletelyInside +import org.junit.Test +import kotlin.test.assertEquals +import kotlin.test.assertTrue + +class GpxImportMergeBBoxesTest { + @Test + fun `gracefully handles empty sequence`() { + assertEquals( + 0, + emptySequence().mergeBBoxes().count(), + "empty sequence not retained" + ) + } + @Test + fun `merges same size squares in an L-shape`() { + val inputBoxes = listOf( + BoundingBox(LatLon(0.00, 0.00), LatLon(0.01, 0.01)), + BoundingBox(LatLon(0.01, 0.00), LatLon(0.02, 0.01)), + BoundingBox(LatLon(0.00, 0.01), LatLon(0.01, 0.02)), + ).map { DecoratedBoundingBox(it.toPolygon()) } + val downloadBoxes = inputBoxes.asSequence().mergeBBoxes().toList() + assertEquals( + 1, + downloadBoxes.size, + "failed to merge all boxes into one" + ) + val downloadBBox = downloadBoxes[0].boundingBox + inputBoxes.map { it.boundingBox }.forEach { + assertTrue( + it.isCompletelyInside(downloadBBox), + "input bounding box $it is not contained in download box $downloadBBox" + ) + } + } + + @Test + fun `partially merges L-shape with one long leg`() { + val inputBoxes = listOf( + BoundingBox(LatLon(0.00, 0.00), LatLon(0.01, 0.01)), + BoundingBox(LatLon(0.01, 0.00), LatLon(0.02, 0.01)), + BoundingBox(LatLon(0.00, 0.01), LatLon(0.01, 0.06)), + ).map { DecoratedBoundingBox(it.toPolygon()) } + val downloadBoxes = inputBoxes.asSequence().mergeBBoxes().toList() + assertEquals( + 2, + downloadBoxes.size, + "failed to merge one side of the L-shape" + ) + val downloadBBoxes = downloadBoxes.map { it.boundingBox } + inputBoxes.map { it.boundingBox }.forEach { + assertTrue( + downloadBBoxes.any { downloadBBox -> it.isCompletelyInside(downloadBBox) }, + "input bounding box $it is not contained in any download box $downloadBBoxes" + ) + } + assertEquals( + inputBoxes.sumOf { it.boundingBox.area() }, + downloadBBoxes.sumOf { it.area() }, + 1.0, + "area to download is not the same as area of input boxes" + ) + } +} diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportParseTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportParseTest.kt new file mode 100644 index 00000000000..d5e9f392403 --- /dev/null +++ b/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportParseTest.kt @@ -0,0 +1,194 @@ +package de.westnordost.streetcomplete.data.import + +import de.westnordost.streetcomplete.data.osm.mapdata.LatLon +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import kotlin.test.Test +import kotlin.test.assertEquals +import kotlin.test.assertFails + +@RunWith(RobolectricTestRunner::class) +class GpxImportParseTest { + @Test + fun `successfully parses minimal sample track`() { + val originalTrackPoints = arrayListOf( + TrackPoint("22.22", "172.3"), + TrackPoint("39.11111", "-179.999"), + TrackPoint("-25.312", "7"), + TrackPoint("57.0", "123"), + TrackPoint("-89.9999", "-12.02"), + TrackPoint("-72.0", "0.3"), + ) + + val inputGpx = minimalGpxBuilder(originalTrackPoints) + + assertSuccess(originalTrackPoints, parseGpx(inputGpx)) + } + + @Test + fun `concatenates multiple track segments`() { + val trackPointsSegment1 = arrayListOf( + TrackPoint("-56.0", "0.0"), + TrackPoint("57.57", "172.3") + ) + val trackPointsSegment2 = arrayListOf( + TrackPoint("-87.0", "-99.2"), + TrackPoint("12.67", "132.29") + ) + + val inputGpx = buildString { + append("") + append("") + append("") + trackPointsSegment1.forEach { + append("") + } + append("") + append("") + trackPointsSegment2.forEach { + append("") + } + append("") + append("") + append("") + } + + assertSuccess(trackPointsSegment1 + trackPointsSegment2, parseGpx(inputGpx)) + } + + @Test + fun `process multiple tracks and segments`() { + val trackPoints1 = arrayListOf( + TrackPoint("-12.33", "0.0"), + TrackPoint("74.1", "-122.34") + ) + val trackPoints2 = arrayListOf( + TrackPoint("-0.0", "-12"), + TrackPoint("-90.0", "180.0") + ) + val trackPoints3 = arrayListOf( + TrackPoint("11.1", "-92"), + TrackPoint("90", "-0.0") + ) + + val inputGpx = buildString { + append("") + append("") + append("") + trackPoints1.forEach { + append("") + } + append("") + append("") + trackPoints2.forEach { + append("") + } + append("") + append("") + append("") + append("") + trackPoints3.forEach { + append("") + } + append("") + append("") + append("") + } + + assertSuccess(trackPoints1 + trackPoints2 + trackPoints3, parseGpx(inputGpx)) + } + + @Test + fun `throws on invalid trackPoints`() { + assertFails { + parseGpx(minimalGpxBuilder(listOf(TrackPoint("99.0", "-12.1")))) + } + assertFails { + parseGpx(minimalGpxBuilder(listOf(TrackPoint("-11.5", "-181.0")))) + } + } + + @Test + fun `throws on non-gpx files`() { + val nonGpxXml = """ + + + """.trimIndent() + assertFails { + parseGpx(nonGpxXml) + } + } + + @Test + fun `Exhausting outer before inner sequence yields no elements`() { + val inputGpx = minimalGpxBuilder( + arrayListOf( + TrackPoint("-39.654", "180"), + TrackPoint("90.0", "-180") + ) + ) + + // exhausting outer first + val incorrectlyRetrievedSegments = parseGpxFile(inputGpx.byteInputStream()).toList(); + assertEquals( + 1, incorrectlyRetrievedSegments.size, + "Exhausting outer first fails to retrieve the track segment" + ) + assertEquals( + 0, incorrectlyRetrievedSegments.first().count(), + "Exhausting outer first unexpectedly yields track points" + ) + + // exhausting inner first + val correctlyRetrievedSegments = + parseGpxFile(inputGpx.byteInputStream()).flatMap { it.toList() } + assertEquals( + 2, correctlyRetrievedSegments.count(), + "Exhausting inner first fails to retrieve track points" + ) + } + + @Test + fun `handles additional data gracefully`() { + val originalTrackPoints = arrayListOf(TrackPoint("88", "-19")) + + val inputGpx = buildString { + append("") + append("") + append("Some GPS track") + append("") + append("") + append("") + originalTrackPoints.forEach { + append("") + } + append("") + append("") + append("") + } + + assertSuccess(originalTrackPoints, parseGpx(inputGpx)) + } +} + +private fun assertSuccess( + originalTrackPoints: List, + parseResult: List, +) { + assertEquals( + originalTrackPoints.size, parseResult.size, + "Not all trackPoints are retrieved" + ) + originalTrackPoints.map{it.toLatLon()}.zip(parseResult).forEach { pair -> + assertEquals( + expected = pair.component1().latitude, + actual = pair.component2().latitude, + "Latitudes don't match" + ) + assertEquals( + expected = pair.component1().longitude, + actual = pair.component2().longitude, + "Longitudes don't match" + ) + } +} diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportTest.kt new file mode 100644 index 00000000000..cc32914d37a --- /dev/null +++ b/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxImportTest.kt @@ -0,0 +1,335 @@ +package de.westnordost.streetcomplete.data.import + +import de.westnordost.streetcomplete.ApplicationConstants +import de.westnordost.streetcomplete.data.download.tiles.enclosingTilePos +import de.westnordost.streetcomplete.data.download.tiles.enclosingTilesRect +import de.westnordost.streetcomplete.data.osm.mapdata.LatLon +import de.westnordost.streetcomplete.util.math.contains +import de.westnordost.streetcomplete.util.math.initialBearingTo +import de.westnordost.streetcomplete.util.math.translate +import kotlinx.coroutines.runBlocking +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner +import kotlin.test.assertEquals +import kotlin.test.assertTrue + +@RunWith(RobolectricTestRunner::class) +class GpxImportTest { + @Test + fun `download works on single-segment track`() = runBlocking { + val originalTrackPoints = arrayListOf( + TrackPoint("0.0", "0.0"), + TrackPoint("1.3", "-0.3"), + TrackPoint("2", "-2"), + TrackPoint("2.4", "-2.2"), + TrackPoint("2.4", "-2.2"), + TrackPoint("2.6", "-3"), + ) + + val minDownloadDistance = 100.0 + val inputGpx = minimalGpxBuilder(originalTrackPoints) + val data = importGpx( + inputGpx.byteInputStream(), + displayTrack = true, + findDownloadBBoxes = true, + minDownloadDistance + ) + assertInvariants(originalTrackPoints, data.getOrThrow(), minDownloadDistance) + } + + @Test + fun `display-only import works on single-segment track`() = runBlocking { + val originalTrackPoints = arrayListOf( + TrackPoint("-36.1", "-143.0"), + TrackPoint("-40.2", "-179.999"), + TrackPoint("-42.0", "179"), + TrackPoint("-38.38", "171"), + ) + + val inputGpx = minimalGpxBuilder(originalTrackPoints) + val data = importGpx( + inputGpx.byteInputStream(), + displayTrack = true, + findDownloadBBoxes = false, + 250.0 + ) + + originalTrackPoints.forEach { trackPoint -> + assertTrue( + data.getOrThrow().segments.any { it.contains(trackPoint.toLatLon()) }, + "originalTrackPoint $trackPoint not contained in displayed segments" + ) + } + } + + @Test + fun `east-west line downloads necessary area`() = runBlocking { + // at zoom level 16, one tile is 0.005° latitude high, or ~500m high on equator + // the equator itself lies directly on a boundary between two tile rows + // LatLon(0.0025, ..) is in the middle of the row above the equator + require(ApplicationConstants.DOWNLOAD_TILE_ZOOM == 16) + val originalTrackPoints = arrayListOf( + TrackPoint("0.0025", "-70.0"), + TrackPoint("0.0025", "-71.0"), + ) + val inputGpx = minimalGpxBuilder(originalTrackPoints) + val centerRowY = + originalTrackPoints[0].toLatLon() + .enclosingTilePos(ApplicationConstants.DOWNLOAD_TILE_ZOOM).y + + // setting the minDownloadDistance well below half tile height should download only one row + val smallMinDownloadDistance = 100.0 + val smallData = importGpx( + inputGpx.byteInputStream(), + displayTrack = true, + findDownloadBBoxes = true, + smallMinDownloadDistance + ).getOrThrow() + assertInvariants(originalTrackPoints, smallData, smallMinDownloadDistance) + smallData.downloadBBoxes + .map { it.enclosingTilesRect(ApplicationConstants.DOWNLOAD_TILE_ZOOM) } + .forEach { + assertEquals( + centerRowY, + it.top, + "$it scheduled for download does not start on center row" + ) + assertEquals( + centerRowY, + it.bottom, + "$it scheduled for download does not end on center row" + ) + } + + // setting the minDownloadDistance at half tile width should download three rows of tiles + val bigDownloadDistance = 250.0 + val bigData = importGpx( + inputGpx.byteInputStream(), + displayTrack = true, + findDownloadBBoxes = true, + bigDownloadDistance + ).getOrThrow() + assertInvariants(originalTrackPoints, bigData, bigDownloadDistance) + bigData.downloadBBoxes + .map { it.enclosingTilesRect(ApplicationConstants.DOWNLOAD_TILE_ZOOM) } + .forEach { + assertEquals( + centerRowY - 1, + it.top, + "$it scheduled for download does not start one below center row" + ) + assertEquals( + centerRowY + 1, + it.bottom, + "$it scheduled for download does not start one above center row" + ) + } + } + + @Test + fun `north-south line downloads necessary area`() = runBlocking { + // at zoom level 16, one tile is 0.005° longitude wide, or ~250m at 60° latitude + // LatLon(.., 0.0025) is in the middle of the column to the east of the 0 meridian + require(ApplicationConstants.DOWNLOAD_TILE_ZOOM == 16) + val originalTrackPoints = arrayListOf( + TrackPoint("59.9", "0.0025"), + TrackPoint("60.1", "0.0025"), + ) + val inputGpx = minimalGpxBuilder(originalTrackPoints) + val centerTileX = + originalTrackPoints[0].toLatLon() + .enclosingTilePos(ApplicationConstants.DOWNLOAD_TILE_ZOOM).x + + // setting the minDownloadDistance well below half tile width should download only one column + val smallMinDownloadDistance = 50.0 + val smallData = importGpx( + inputGpx.byteInputStream(), + displayTrack = true, + findDownloadBBoxes = true, + smallMinDownloadDistance + ).getOrThrow() + assertInvariants(originalTrackPoints, smallData, smallMinDownloadDistance) + smallData.downloadBBoxes + .map { it.enclosingTilesRect(ApplicationConstants.DOWNLOAD_TILE_ZOOM) } + .forEach { + assertEquals( + centerTileX, + it.left, + "$it scheduled for download does not start on center column" + ) + assertEquals( + centerTileX, + it.right, + "$it scheduled for download does not end on center column" + ) + } + + // setting the minDownloadDistance at 1.5 times tile width should download five columns + val bigDownloadDistance = 375.0 + val bigData = importGpx( + inputGpx.byteInputStream(), + displayTrack = true, + findDownloadBBoxes = true, + bigDownloadDistance + ).getOrThrow() + assertInvariants(originalTrackPoints, bigData, bigDownloadDistance) + bigData.downloadBBoxes + .map { it.enclosingTilesRect(ApplicationConstants.DOWNLOAD_TILE_ZOOM) } + .forEach { + assertEquals( + centerTileX - 2, + it.left, + "$it scheduled for download does not start two left of center column" + ) + assertEquals( + centerTileX + 2, + it.right, + "$it scheduled for download does not end two right of center column" + ) + } + } + + @Test + fun `line close to corner downloads adjacent tile`() = runBlocking { + /* + a line barely not touching the corner of a tile should download all four tiles around + the corner - even if one of them is not touched by the line itself + + this test takes the four tiles around the origin LatLon(0.0, 0.0), named NW, NE, SE, SW + in clockwise direction + + a diagonal line from south west to north east just below the origin would cross the tiles + NE, SE and SW, but not NW + + if minDownloadDistance is greater than the distance of the line to the origin, NW should + still be downloaded; if not by some margin, it should be omitted + */ + + // at zoom level 16, one tile is 0.005° wide / high, or ~500m wide / high on equator + require(ApplicationConstants.DOWNLOAD_TILE_ZOOM == 16) + val lineOriginDistance = 100.0 + val startPoint = LatLon(-0.002, -0.002).translate(lineOriginDistance, 135.0) + val endPoint = LatLon(0.002, 0.002).translate(lineOriginDistance, 135.0) + val nWCenterPoint = LatLon(0.0025, -0.0025) + + val originalTrackPoints = arrayListOf( + startPoint.toTrackPoint(), + endPoint.toTrackPoint(), + ) + val inputGpx = minimalGpxBuilder(originalTrackPoints) + + // area around line should touch NW + val touchingMinDownloadDistance = lineOriginDistance + 0.001 + val touchingData = importGpx( + inputGpx.byteInputStream(), + displayTrack = true, + findDownloadBBoxes = true, + touchingMinDownloadDistance + ).getOrThrow() + assertInvariants(originalTrackPoints, touchingData, touchingMinDownloadDistance) + assertTrue( + touchingData.downloadBBoxes.any { it.contains(nWCenterPoint) }, + "north west center point not contained" + ) + + // area around line should not touch NW + val noTouchMinDownloadDistance = lineOriginDistance / 2 + val noTouchData = importGpx( + inputGpx.byteInputStream(), + displayTrack = true, + findDownloadBBoxes = true, + noTouchMinDownloadDistance + ).getOrThrow() + assertInvariants(originalTrackPoints, noTouchData, noTouchMinDownloadDistance) + assertTrue( + !noTouchData.downloadBBoxes.any { it.contains(nWCenterPoint) }, + "north west center point contained even if it should not" + ) + } + + @Test + fun `works around equator`() = runBlocking { + val originalTrackPoints = arrayListOf( + TrackPoint("0.0", "73.1"), + TrackPoint("-0.3", "74.2"), + ) + val minDownloadDistance = 100.0 + + val inputGpx = minimalGpxBuilder(originalTrackPoints) + val data = importGpx( + inputGpx.byteInputStream(), + displayTrack = true, + findDownloadBBoxes = true, + minDownloadDistance + ) + assertInvariants(originalTrackPoints, data.getOrThrow(), minDownloadDistance) + } + + @Test + fun `works with wrap-around longitude`() = runBlocking { + val originalTrackPoints = arrayListOf( + TrackPoint("-33.0", "164.9"), + TrackPoint("-35.1", "-170.3"), + ) + val minDownloadDistance = 100.0 + + val inputGpx = minimalGpxBuilder(originalTrackPoints) + val data = importGpx( + inputGpx.byteInputStream(), + displayTrack = true, + findDownloadBBoxes = true, + minDownloadDistance + ) + assertInvariants(originalTrackPoints, data.getOrThrow(), minDownloadDistance) + } + + @Test + fun `works around north pole`() = runBlocking { + // TODO sgr: would actually like to run test with lat: 90.0, but this fails + // latitude should be between -85.0511 and 85.0511 to be within OSM + // tiles apparently, see https://wiki.openstreetmap.org/wiki/Slippy_map_tilenames + // TilesRect.lat2tile gives negative numbers for latitude > 85.08, which leads to huge + // number of tiles => maybe LatLon.checkValidity should require latitude to be <= 85.0? + val originalTrackPoints = arrayListOf( + TrackPoint("83.0", "-44.1"), + TrackPoint("84.0", "178.2"), + ) + val minDownloadDistance = 500.0 + + val inputGpx = minimalGpxBuilder(originalTrackPoints) + val data = importGpx( + inputGpx.byteInputStream(), + displayTrack = false, + findDownloadBBoxes = true, + minDownloadDistance + ) + assertInvariants(originalTrackPoints, data.getOrThrow(), minDownloadDistance) + } + + private fun assertInvariants( + originalTrackPoints: List, + importData: GpxImportData, + minDownloadDistance: Double, + ) { + originalTrackPoints.forEach { trackPoint -> + assertTrue( + importData.segments.any { it.contains(trackPoint.toLatLon()) }, + "originalTrackPoint $trackPoint not contained in displayed segments" + ) + assertTrue( + importData.downloadBBoxes.any { it.contains(trackPoint.toLatLon()) }, + "originalTrackPoint $trackPoint not contained in area to download" + ) + for (testAngle in 0..360 step 10) { + val testPoint = + trackPoint.toLatLon().translate(minDownloadDistance, testAngle.toDouble()) + assertTrue( + importData.downloadBBoxes.any { it.contains(testPoint) }, + "$testPoint <= $minDownloadDistance away from $trackPoint not included in area to download" + ) + } + + } + } +} diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxSingleTrackParserTest.kt b/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxSingleTrackParserTest.kt deleted file mode 100644 index 81aa8dca065..00000000000 --- a/app/src/test/java/de/westnordost/streetcomplete/data/import/GpxSingleTrackParserTest.kt +++ /dev/null @@ -1,225 +0,0 @@ -package de.westnordost.streetcomplete.data.import - -import de.westnordost.streetcomplete.data.osm.mapdata.LatLon -import de.westnordost.streetcomplete.testutils.any -import de.westnordost.streetcomplete.testutils.mock -import de.westnordost.streetcomplete.testutils.on -import org.mockito.ArgumentMatchers.anyInt -import org.mockito.ArgumentMatchers.anyString -import org.xmlpull.v1.XmlPullParser -import org.xmlpull.v1.XmlPullParserException -import kotlin.test.Test -import kotlin.test.assertEquals -import kotlin.test.assertFails - -private const val TRACK = "trk" -private const val TRACK_SEGMENT = "trkseg" -private const val TRACK_POINT = "trkpt" - -class GpxSingleTrackParserTest { - @Test - fun `successfully parses minimal sample track`() { - val originalTrackPoints = arrayListOf( - createWaypoint("22.22", "172.3"), - createWaypoint("39.11111", "-179.999"), - createWaypoint("-25.312", "7"), - createWaypoint("57.0", "123"), - createWaypoint("-89.9999", "-12.02"), - createWaypoint("-72.0", "0.3"), - ) - assertSuccess(originalTrackPoints, parse(createGpxWithTrackPoints(originalTrackPoints))) - } - - @Test - fun `concatenates multiple track segments`() { - val trackPointsSegment1 = arrayListOf( - createWaypoint("-56.0", "0.0"), - createWaypoint("57.57", "172.3") - ) - val trackPointsSegment2 = arrayListOf( - createWaypoint("-87.0", "-99.2"), - createWaypoint("12.67", "132.29") - ) - val gpx = MockXmlElement( - "gpx", MockXmlElement( - TRACK, arrayListOf( - MockXmlElement(TRACK_SEGMENT, trackPointsSegment1), - MockXmlElement(TRACK_SEGMENT, trackPointsSegment2) - ) - ) - ) - assertSuccess(trackPointsSegment1 + trackPointsSegment2, parse(gpx)) - } - - @Test - fun `discards further tracks`() { - val trackPoints1 = arrayListOf( - createWaypoint("-12.33", "0.0"), - createWaypoint("74.1", "-122.34") - ) - val trackPoints2 = arrayListOf( - createWaypoint("-0.0", "-12"), - createWaypoint("-90.0", "180.0") - ) - val gpx = MockXmlElement( - "gpx", arrayListOf( - MockXmlElement(TRACK, MockXmlElement(TRACK_SEGMENT, trackPoints1)), - MockXmlElement(TRACK, MockXmlElement(TRACK_SEGMENT, trackPoints2)) - ) - ) - assertSuccess(trackPoints1, parse(gpx)) - } - - @Test - fun `throws on invalid trackPoints`() { - assertFails { - parse(createGpxWithTrackPoints(arrayListOf(createWaypoint("99.0", "-12.1")))).toList() - } - assertFails { - parse(createGpxWithTrackPoints(arrayListOf(createWaypoint("-11.5", "-181.0")))).toList() - } - } - - @Test - fun `throws on non-gpx files`() { - assertFails { parse(MockXmlElement("xml", arrayListOf())).toList() } - } - - @Test - fun `handles additional data gracefully`() { - val trackPoints = arrayListOf(createWaypoint("88", "-19")) - val gpx = MockXmlElement( - "gpx", - hashMapOf("version" to "1.1", "creator" to ""), - arrayListOf( - MockXmlElement("metadata", arrayListOf(MockXmlElement("desc", arrayListOf()))), - MockXmlElement(TRACK, MockXmlElement(TRACK_SEGMENT, trackPoints)) - ) - ) - - assertSuccess(trackPoints, parse(gpx)) - } -} - -/* helper functions */ -private fun createGpxWithTrackPoints(trackPoints: MutableList): MockXmlElement { - return MockXmlElement( - "gpx", - hashMapOf("version" to "1.1", "creator" to ""), - arrayListOf(MockXmlElement(TRACK, MockXmlElement(TRACK_SEGMENT, trackPoints))) - ) -} - -private fun createWaypoint(lat: String, lon: String): MockXmlElement { - return MockXmlElement(TRACK_POINT, mapOf("lat" to lat, "lon" to lon), ArrayList()) -} - -private fun parse(gpx: MockXmlElement): Sequence { - val xpp: XmlPullParser = mock() - on(xpp.next()).thenAnswer { gpx.next() } - on(xpp.nextTag()).thenAnswer { gpx.next() } - on(xpp.eventType).thenAnswer { gpx.getEventType() } - on(xpp.name).thenAnswer { gpx.getName() } - on(xpp.getAttributeValue(any(), anyString())) - .thenAnswer { i -> gpx.getAttributeValue(i.getArgument(1)) } - on(xpp.require(anyInt(), anyString(), anyString())) - .thenAnswer { i -> - val eventType: Int = i.getArgument(0) - val name: String? = i.getArgument(2) - if (eventType != gpx.getEventType() || (name != null && name != gpx.getName())) { - throw XmlPullParserException("wrong eventType or name") - } - } - return parseSingleGpxTrack(xpp) -} - -private fun assertSuccess( - originalTrackPoints: List, - parseResult: Sequence, -) { - val result = parseResult.toList() - assertEquals( - originalTrackPoints.size, result.size, - "All trackPoints are retrieved" - ) - originalTrackPoints.zip(result).forEach { pair -> - assertEquals( - expected = pair.component1().getAttributeValue("lat")?.toDouble(), - actual = pair.component2().latitude, - "Latitudes match" - ) - assertEquals( - expected = pair.component1().getAttributeValue("lon")?.toDouble(), - actual = pair.component2().longitude, - "Longitudes match" - ) - } -} - -private class MockXmlElement( - private val name: String, - private val attributes: Map, - private val children: MutableList, -) { - constructor(name: String, child: MockXmlElement) : this(name, hashMapOf(), arrayListOf(child)) - constructor(name: String, children: MutableList) - : this(name, hashMapOf(), children) - - private enum class TagState { BEFORE, ON_START_TAG, ON_CHILD_TAG, ON_END_TAG } - - private var tagState = TagState.BEFORE - private var currentChildIdx = 0 - - fun next(): Int { - when (tagState) { - TagState.BEFORE -> { - tagState = TagState.ON_START_TAG - return XmlPullParser.START_TAG - } - - TagState.ON_END_TAG -> { - return XmlPullParser.END_DOCUMENT - } - - else -> { - if (currentChildIdx < children.size) { - if (children[currentChildIdx].tagState == TagState.ON_END_TAG) { - currentChildIdx++ - } - } - return if (currentChildIdx < children.size) { - tagState = TagState.ON_CHILD_TAG - children[currentChildIdx].next() - } else { - tagState = TagState.ON_END_TAG - XmlPullParser.END_TAG - } - } - } - } - - private fun currentElement(): MockXmlElement { - return when (tagState) { - TagState.BEFORE -> throw IllegalStateException("call next first") - TagState.ON_START_TAG -> this - TagState.ON_CHILD_TAG -> children[currentChildIdx].currentElement() - TagState.ON_END_TAG -> this - } - } - - fun getEventType(): Int { - return when (currentElement().tagState) { - TagState.ON_START_TAG -> XmlPullParser.START_TAG - TagState.ON_END_TAG -> XmlPullParser.END_TAG - else -> throw IllegalStateException("current element should always be on START or END tag") - } - } - - fun getName(): String { - return currentElement().name - } - - fun getAttributeValue(name: String): String? { - return currentElement().attributes[name] - } -} diff --git a/app/src/test/java/de/westnordost/streetcomplete/data/import/TestHelpers.kt b/app/src/test/java/de/westnordost/streetcomplete/data/import/TestHelpers.kt new file mode 100644 index 00000000000..1e0cdecd4cf --- /dev/null +++ b/app/src/test/java/de/westnordost/streetcomplete/data/import/TestHelpers.kt @@ -0,0 +1,33 @@ +package de.westnordost.streetcomplete.data.import + +import de.westnordost.streetcomplete.data.osm.mapdata.LatLon + +data class TrackPoint( + val lat: String, + val lon: String, +) +internal fun LatLon.toTrackPoint() : TrackPoint { + return TrackPoint(this.latitude.toString(), this.longitude.toString()) +} +internal fun TrackPoint.toLatLon() : LatLon { + return LatLon(this.lat.toDouble(), this.lon.toDouble()) +} + +// explicitly not using LatLon to allow testing wrong and special string formatting +internal fun minimalGpxBuilder(trackPoints: List): String = buildString { + append("") + append("") + append("") + trackPoints.forEach { + append("") + } + append("") + append("") + append("") +} + +internal fun parseGpx(input: String): List { + // make sure sequences are exhausted in correct order + return parseGpxFile(input.byteInputStream()).flatMap { it.toList() }.toList() +} +