Convert library to KotlinMultiplatform#40
Conversation
|
Ready for review and discuss now @louwers and any other who is working on this project! |
louwers
left a comment
There was a problem hiding this comment.
Thanks, this is a very welcomed contribution!
I could use some help from maybe @sargunv and @westnordost to review this.
Did you already test this with MapLibre Android? I was thinking, maybe we make a pre-release of both this library and MapLibre Android, so we can test it?
Are there any breaking APIs needed in MapLibre Android you are aware of?
|
( commenting here, to keep in the loop ) |
|
From my side, this is ready to merge now (except the naming). I improved the models already based on the reviews. And the Turf functions are now extension functions on the models. The turf module was only used in the navigation project and the native Android test app. The native project itself is using only the models. And we need to discuss about the naming. Should we keep the |
|
I put the review of this on my todo, since it is a lot, and I will review on the side bit by bit, it will take some time. |
|
Since these are dependencies of MapLibre Android, I would like to have a migration plan and probably a draft PR with the needed changes. |
There was a problem hiding this comment.
I've read through the data classes and started reading the serializers. Love how clean it is, I found mostly typos.
For the latter, I noticed that this is a lot of boilerplate code as pretty much every single data class has a custom serializer, because of the coordinates field.
I recognize that the coordinates field is a different data structure for the different geometry types - a double array for the Point, an array of double arrays for LineString and MultiPoint, an array of arrays of double arrays for Polygon, MultiLineString and and array of arrays of arrays of double arrays of MultiPolygon so I guess maybe it can't be helped?
Anyway, two ideas to reduce copypasta / boilerplate:
- abstract superclass or delegate class for the shared code (optional bbox, boilerplate serializer code)
- serializers for linestring + multipoint as well as polygon+multilinestring should be the same, no?
| def xcf = new XCFrameworkConfig(project) | ||
| [iosX64(), iosArm64(), iosSimulatorArm64()].forEach { target -> | ||
| target.binaries.framework { framework -> | ||
| baseName = "maplibre-geojson" | ||
| xcf.add(framework) | ||
| } | ||
| } |
There was a problem hiding this comment.
What does this do? In other multiplatform libraries, I just see somethign like
| def xcf = new XCFrameworkConfig(project) | |
| [iosX64(), iosArm64(), iosSimulatorArm64()].forEach { target -> | |
| target.binaries.framework { framework -> | |
| baseName = "maplibre-geojson" | |
| xcf.add(framework) | |
| } | |
| } | |
| iosX64() | |
| iosArm64() | |
| iosSimulatorArm64() |
Also, what about
linuxX64()
linuxArm64()
macosX64()
macosArm64()
?
| //TODO(fabi755): can we use here something generic (not from kotlinx) instead? | ||
| // any idea? | ||
| /** | ||
| * Additional custom properties for this feature. | ||
| */ | ||
| val properties: JsonObject? = null, |
There was a problem hiding this comment.
Map<String, Any?>?
I think JsonObject? is the best possible fit, because it can not have Any? values, just what is allowed in JSON, i.e. certain primitives, JsonArrays and JsonObjects.
There was a problem hiding this comment.
Yes correct, but this is the only part where we force the developer using the serialization library (kotlinx.serialization). And exposing this library. The other parts are all internal and not relevant to the public API. But for now I also think, it's the best solution
services-geojson/src/commonMain/kotlin/org/maplibre/geojson/model/FeatureCollection.kt
Show resolved
Hide resolved
services-geojson/src/commonMain/kotlin/org/maplibre/geojson/model/FeatureCollection.kt
Outdated
Show resolved
Hide resolved
services-geojson/src/commonMain/kotlin/org/maplibre/geojson/model/LineString.kt
Outdated
Show resolved
Hide resolved
| /** | ||
| * The [BoundingBox] of this point. | ||
| */ | ||
| override val boundingBox: BoundingBox? = null, |
There was a problem hiding this comment.
Well, I guess this is where the GeoJson spec doesn't make much sense. (A bounding box for a point? Lol)
However, the GeoJson spec distinguishes between Positions and Points while this PR does not.
A Point is a Geometry (hence having a bbox is allowed) while positions (or coordinates) are not.
It's not a big issue, but since positions/coordinates are used all over (in Polygons, LineStrings, ...), that means that the (in memory) data carries a lot of superfluous null bboxes. Anyway, does anything speak against keeping closer to the GeoJson spec? (distinguish between Position and Point)
There was a problem hiding this comment.
It's not a big issue, but since positions/coordinates are used all over (in Polygons, LineStrings, ...), that means that the (in memory) data carries a lot of superfluous null bboxes.
Very good hint! I don't see this part. Make sense to add also a Position model like in the spec 👍
Thanks!
services-geojson/src/commonMain/kotlin/org/maplibre/geojson/model/Polygon.kt
Outdated
Show resolved
Hide resolved
| private fun LineString.requireLinearRing() { | ||
| require(points.size >= 4) { | ||
| "LinearString for Polygon rings need to be made up of 4 or more Points." | ||
| } | ||
|
|
||
| require(points.first() == points.last()) { | ||
| "LinearString for Polygon rings require first and last Point to be identical." | ||
| } |
There was a problem hiding this comment.
For more informative error message (i.e. which ring exactly is wrong):
| private fun LineString.requireLinearRing() { | |
| require(points.size >= 4) { | |
| "LinearString for Polygon rings need to be made up of 4 or more Points." | |
| } | |
| require(points.first() == points.last()) { | |
| "LinearString for Polygon rings require first and last Point to be identical." | |
| } | |
| private fun LineString.requireLinearRing(name: String, index: Int?) { | |
| require(points.size >= 4) { | |
| "${getRingName}: LineString for Polygon rings need to be made up of 4 or more Points." | |
| } | |
| require(points.first() == points.last()) { | |
| "${getRingName}: LineString for Polygon rings require first and last Point to be identical." | |
| } | |
| private fun getRingName(name: String, index: Int?): String { | |
| val indexString = index?.let { " at index $it" } ?: "" | |
| return "$name$indexString" | |
| } |
(also, typo: "LinearString" -> "LineString")
| /** | ||
| * The outer [LineString] ring of this Polygon. | ||
| */ | ||
| val outerLineStringRing: LineString, | ||
|
|
||
| /** | ||
| * The inner [LineString] rings of this Polygon. Or empty list if no inner holes exists. | ||
| */ | ||
| val holeLineStringRings: List<LineString> = emptyList(), |
There was a problem hiding this comment.
| /** | |
| * The outer [LineString] ring of this Polygon. | |
| */ | |
| val outerLineStringRing: LineString, | |
| /** | |
| * The inner [LineString] rings of this Polygon. Or empty list if no inner holes exists. | |
| */ | |
| val holeLineStringRings: List<LineString> = emptyList(), | |
| /** | |
| * The outer [LineString] ring of this Polygon. | |
| */ | |
| val outer: LineString, | |
| /** | |
| * The inner [LineString] rings of this Polygon. Or empty list if no inner holes exists. | |
| */ | |
| val inner: List<LineString> = emptyList(), |
I think this would be more consistent.
| classDiscriminatorMode = ClassDiscriminatorMode.ALL_JSON_OBJECTS | ||
|
|
||
| // Encode | ||
| encodeDefaults = true |
There was a problem hiding this comment.
wouldn't that encode e.g. null for every Point (and other Geometry) for bbox?
There was a problem hiding this comment.
I didn't look at the tests because I think you just converted them to Kotlin and made them work after any interface changes you did, right?
Regarding the geodesy, it looks like the antimeridian is not taken into account at all. I expect this could lead to all kind of problems, but anyway, if it should not be implemented, then it should at least be very clearly documented at all affected places.
I also noticed that quite a bit of functionality that was available earlier seems to be missing now, such as
- GeometryCollection.combine
- GeometryCollection.explode
- MultiPolygon.toMultiLineString
- Polygon.toLineString
- Polygon.contains
- MultiPolygon.contains
- Geometry.bbox
- GeometryCollection.bbox
- BoundingBox.toPolygon
- Polygon.area
- MultiPolygon.area
- Geometry.center
- GeometryCollection.center
I don't know if all of these are really necessary, but some look pretty necessary (point in polygon for example).
Edit: I see you stated earlier that you removed unused ones, ok.
| return drop(1) | ||
| .mapIndexed { index, point -> | ||
| // Using unmodified index for previous point is working, | ||
| // because we drop the first point | ||
| get(index).distanceTo(point, unit) | ||
| } | ||
| .sum() |
There was a problem hiding this comment.
Clever. But also, a bit difficult to understand.
How about
/** Return a sequence that iterates through the given list of points in pairs */
internal fun Iterable<Point>.asLineSequence(): Sequence<Pair<Point, Point>> = sequence {
val it = iterator()
if (!it.hasNext()) return@sequence
var p1 = it.next()
while (it.hasNext()) {
val p2 = it.next()
yield(p1 to p2)
p1 = p2
}
}and then
| return drop(1) | |
| .mapIndexed { index, point -> | |
| // Using unmodified index for previous point is working, | |
| // because we drop the first point | |
| get(index).distanceTo(point, unit) | |
| } | |
| .sum() | |
| return asLineSequence().sumOf { (p1, p2) -> p1.distanceTo(p2, unit) } |
?
Actually, this convenience function would also make implementation of List<Point>.along slightly more straightforward.
| fun Point.findMidpoint(to: Point): Point { | ||
| val dist = this.distanceTo(to, MeasureUnit.MILES) | ||
| val heading = this.bearingTo(to) | ||
| return this.destination(dist / 2, heading, MeasureUnit.MILES) | ||
| } |
There was a problem hiding this comment.
Hmm, other functions are not named something with find... (findLength, findBearingTo, ...), but of course, poin1.midpoint(point2) reads odd, true. I think a non-extension-function would be more expressive: midpoint(point1, point2)
| fun Point.findMidpoint(to: Point): Point { | |
| val dist = this.distanceTo(to, MeasureUnit.MILES) | |
| val heading = this.bearingTo(to) | |
| return this.destination(dist / 2, heading, MeasureUnit.MILES) | |
| } | |
| fun midpoint(p1: Point, p2: Point): Point { | |
| val dist = p1.distanceTo(p2, MeasureUnit.RADIANS) | |
| val heading = p1.bearingTo(p2) | |
| return p1.destination(dist / 2, heading, MeasureUnit.RADIANS) | |
| } |
| /** | ||
| * Enum class representing various units of measurement and their corresponding factors. | ||
| * These units are used for geographical and mathematical calculations, particularly | ||
| * in the context of distance and angular measurements. | ||
| */ | ||
| @Suppress("unused") | ||
| enum class MeasureUnit(val factor: Double) { |
There was a problem hiding this comment.
Is this from Turf, or from the old Java API?
I find a bit strange:
-
CENTIMETERSand alsoCENTIMETRES,METERSandMETRES,KILOMETRESandKILOMETERS, why?? -
why even have anything else than METERS in SI measurement system? Users will I think be able to divide by 1000 for kilometers etc. themselves
-
An API like that requires the use of the MeasureUnit in every single function that deals with distances. why not always return in some standard unit, e.g. meters or probably rather degrees (because not dependent on hardcoded earth circumference) and either let the user do whatever conversion he needs after or provide convenience functions for that?
MeasureUnithardcodes the values for the circumference of Earth (except degrees, radians)
| KILOMETRES(factor = 6373.0); | ||
|
|
||
| companion object { | ||
| val DEFAULT = KILOMETERS |
There was a problem hiding this comment.
What is this for? This makes it opaque what DEFAULT actually is wherever this is set as a default parameter.
There was a problem hiding this comment.
Copied from before. But true, using this directly make it clear 👍
| val difLat = degreesToRadians((point.latitude - this.latitude)) | ||
| val difLon = degreesToRadians((point.longitude - this.longitude)) | ||
| val lat1 = degreesToRadians(this.latitude) | ||
| val lat2 = degreesToRadians(point.latitude) | ||
|
|
||
| val value = sin(difLat / 2).pow(2.0) + sin(difLon / 2).pow(2.0) * cos(lat1) * cos(lat2) | ||
|
|
||
| return radiansToLength( | ||
| 2 * atan2(sqrt(value), sqrt(1 - value)), unit | ||
| ) |
There was a problem hiding this comment.
This will not yield correct results if the line between point A and point B crosses the antimeridian.
| * @param unit The unit of measurement. | ||
| * @return A [Feature] containing the closest point and properties for index and distance. | ||
| */ | ||
| fun LineString.closestPoint(point: Point, unit: MeasureUnit = MeasureUnit.DEFAULT): Feature { |
There was a problem hiding this comment.
Why not return a DistancePoint here? Returning some Feature with properties with hardcoded keys seems unnecessary (and requires additional documentation to make sense of for lib users)
| * @param points The list of points. | ||
| * @return The nearest point in the list. | ||
| */ | ||
| fun Point.closestPoint(points: List<Point>): Point { |
There was a problem hiding this comment.
That point1.closestPoint(emptyList()) returns point1 sounds odd. Not sure if this function is necessary at all, after all,
points.minBy { it.distanceTo(point) }is hardly longer than
point.closestPoint(points)but if it is necessary, I'd find it perfectly fine if it threw NoSuchElementException when points is empty.
| val longitude1 = degreesToRadians(longitude) | ||
| val latitude1 = degreesToRadians(latitude) | ||
| val bearingRad = degreesToRadians(bearing) | ||
|
|
||
| val radians = lengthToRadians(distance, unit) | ||
|
|
||
| val latitude2 = asin( | ||
| sin(latitude1) * cos(radians) + cos(latitude1) * sin(radians) * cos(bearingRad) | ||
| ) | ||
| val longitude2 = longitude1 + atan2( | ||
| sin(bearingRad) * sin(radians) * cos(latitude1), | ||
| cos(radians) - sin(latitude1) * sin(latitude2) | ||
| ) | ||
|
|
||
| return Point( | ||
| radiansToDegrees(longitude2), | ||
| radiansToDegrees(latitude2) | ||
| ) |
There was a problem hiding this comment.
If the resulting point crosses the antimeridian, the resulting point will have a longitude outside of the expected bounds of -180 and +180. Not sure if valid lat+lon are required/enforced? But it could very well be that software that uses the Point data structure assumes that the points are valid lat lon coordinates.
| * @property onLine1 True if the intersection is on the first line segment. | ||
| * @property onLine2 True if the intersection is on the second line segment. | ||
| */ | ||
| data class LineIntersects( |
There was a problem hiding this comment.
| data class LineIntersects( | |
| private data class LineIntersects( |
doesn't need to be exposed as long as it is only used internally
| * @return A [Polygon] representing the circle. | ||
| * @throws IllegalArgumentException if steps is less than 1. | ||
| */ | ||
| fun Point.circle( |
There was a problem hiding this comment.
or circle(point: Point, radius: Double, ...)? Or Point.createCircle?
or |
|
Have we considered forking spatialk under the maplibre org? It's already used by maplibre-compose and it's probably(?) more idiomatic. I'd be happy to help maintain a fork since I also have several open PRs that have gone unanswered |
|
I don't have any strong opinion on whether to use this or fork Spatial-K, but will note that:
MapLibre Compose uses Spatial-K right now. Long term, I'll most likely use whichever is maintained under the MapLibre org, whether that's a fork of Spatial-K or this KMP port. |
|
@jgillich Here is the process for onboarding a new repo: https://github.com/maplibre/maplibre/issues/new?template=new-repo-checklist.md |
|
@Fabi755 Do you want to try updating MapLibre Android in the maplibre-native repo with one of the pre-releases? |
|
Yes I can do this as next step. I'm back from my holiday now, and will try this out next 👍 |
|
I have submitted a proposal for forking spatialk ^, feel free to share your thoughts |
This PR is converting the whole code to Kotlin and also use converts to use Multiplatform Framework.
The goal is to use this library in more parts than the Android one. As we also converting the maplibre-navigation-android one to Kotlin Multiplatform and @sargunv is working on a Multiplatform Compose variant of MapLibre.
Converting the Java stuff to Kotlin, and removing the Android and JVM specific stuff was straight forward work. More complicated was to avoid a breaking change release, and keep the support with MapLibre Native for Android.
To use the new models, we need to convert it back to JVM with
toJvm()extension. But this should only a temporary solution, as I would create a PR on MapLibre Native to use the newer variant of this library.The idea is that I create also an PR for including this library to the iOS platform code.
We have starting using this PR changes already in our PR that convert the navigation core. There is also a small example usage of navigation with this GeoJSON lib.
@sotomski is also implemented this early state of this repository with navigation one in a private repository for Kurviger.
Changes
As mentioned, the project is now a KMP one. So we having a new code folder structure:
commonMain(generic Kotlin native)commonJvm(JVM target code. Here located the old method schemes)commonTest(tests for generic Kotlin native)jvmTest(tests for JVM target. Only used for test utils setup yet)appleTest(tests for Apple target. Only used for test utils setup yet)To avoid a breaking change release, I added all old function patterns to the JVM module. These are marked as deprecated and are working as proxy. The logic is removed and all calls are forwareded to the generic functions in Multiplatform
commonMainpackage.Follow up
Finally we need to decide how we handle the naming of this repository & project. The current
maplibre-javais not matching anymore. I suggest something likemaplibre-geojson.