Conversation
* New orientation provider returns device orientation * rememberUserLocationState will return orientation if orientationProvider is provided * Updated demos to display orientation * Renamed location bearing to course * Added measurement types (value+accuracy) for location members * LocationPuck accepts BearingMeasurement to support both course and orientation * Use spatialk/units for location provider minDistance
|
Thanks again for working on this! Will hold off on reviewing as it's in draft mode, but feel free to @ me anytime when you'd like a review |
| object : LocationCallback() { | ||
| override fun onLocationResult(result: LocationResult) { | ||
| result.locations.forEach { trySendBlocking(it.asMapLibreLocation()).getOrThrow() } | ||
| result.locations.forEach { trySend(it.asMapLibreLocation()) } |
There was a problem hiding this comment.
This had the potential to throw a CancellationException (ForgottenCoroutineScopeException) if values are sent after cancellation. As far as I know, there are only two failure scenarios, either the channel is closed or the channel is not being collected and the buffer exceeds capacity, neither of which justify throwing.
| public fun rememberUserLocationState( | ||
| locationProvider: LocationProvider, | ||
| orientationProvider: OrientationProvider? = null, | ||
| samplePeriod: Duration? = 1.seconds, |
There was a problem hiding this comment.
I added a sampling period here to avoid doubling recompositions when orientationProvider is used. It is up to the user to sync up all the periods, potential pitfall if they miss this one. Not sure if it would be better to remove the default value entirely
There was a problem hiding this comment.
I think a better solution would be to add a
val samplePeriod: Durationto the *Provider interfaces. The you can just use the max of those for the flow.sample call (or the code I suggest below).
But after thinking about it, I'm actually not sure what you're trying to avoid here. The trivial solution (a second collectAsStateWithLifecycle) shouldn't lead to unnecessary recompositions, assuming you want a recomposition every time either location or orientation changes. That recomposition should also just affect the composables that read location or orientation from the returned UserLocationState, i.e. normally just LocationPuck.
There was a problem hiding this comment.
If you have location at 1s and orientation at 1s, your UI will recompose every 0.5s on average. That is unnecessary for most apps. In my navigation app, I have a bunch of effects that respond to location changes, and keeping energy usage as low as possible is important to me. Although it is possible that this ordeal of combine/sample uses more energy than not doing it at all, so I'll see about optimizing it
There was a problem hiding this comment.
If you really want to reduce energy consumption, I would tune the parameters of the location/orientation providers so they emit fewer or less accurate updates in the first place. The most expensive part (in terms of energy) is definitely reading and processing the sensor values.
lib/maplibre-compose/src/commonMain/kotlin/org/maplibre/compose/location/OrientationProvider.kt
Show resolved
Hide resolved
kodebach
left a comment
There was a problem hiding this comment.
I think making UserLocationState a parameter of LocationPuck was actually a mistake and we should fix that. The new bearing parameter would defeat the point of passing location as a state object anyway.
| object : LocationCallback() { | ||
| override fun onLocationResult(result: LocationResult) { | ||
| result.locations.forEach { trySendBlocking(it.asMapLibreLocation()).getOrThrow() } | ||
| result.locations.forEach { trySend(it.asMapLibreLocation()) } |
lib/maplibre-compose/src/commonMain/kotlin/org/maplibre/compose/location/LocationPuck.kt
Outdated
Show resolved
Hide resolved
| accuracyThreshold: Float = 50f, | ||
| colors: LocationPuckColors = LocationPuckColors(), | ||
| sizes: LocationPuckSizes = LocationPuckSizes(), | ||
| showBearing: Boolean = true, |
There was a problem hiding this comment.
We don't need showBearing anymore, if bearing is a separate parameter. Instead of showBearing = false you can just pass bearing = null now.
There was a problem hiding this comment.
Two reasons for this:
- We also have
showBearingAccuracy: Boolean, for consistency's sake it would be odd to turn off one vianulland the other viafalse. We could remove both of these flags and only show them unless null, but: - We may, at some point in the future, want to indicate that bearing is unknown rather than disabled
...plibre-compose/src/commonMain/kotlin/org/maplibre/compose/location/LocationTrackingEffect.kt
Outdated
Show resolved
Hide resolved
...plibre-compose/src/commonMain/kotlin/org/maplibre/compose/location/LocationTrackingEffect.kt
Outdated
Show resolved
Hide resolved
lib/maplibre-compose/src/commonMain/kotlin/org/maplibre/compose/location/OrientationProvider.kt
Show resolved
Hide resolved
| public actual fun rememberDefaultOrientationProvider( | ||
| updateInterval: Duration | ||
| ): OrientationProvider { | ||
| TODO() |
There was a problem hiding this comment.
On iOS (compass) orientation is also provided through CLLocationManager (https://developer.apple.com/documentation/corelocation/getting-heading-and-course-information).
I think we can just make IosLocationProvider implement both LocationProvider and OrientationProvider and add enableLocation/enableOrientation constructor parameters. Alternatively, you can also duplicate the whole class.
kodebach
left a comment
There was a problem hiding this comment.
This time for the correct commit
lib/maplibre-compose/src/commonMain/kotlin/org/maplibre/compose/location/LocationPuck.kt
Show resolved
Hide resolved
lib/maplibre-compose/src/commonMain/kotlin/org/maplibre/compose/location/LocationPuck.kt
Outdated
Show resolved
Hide resolved
...plibre-compose/src/commonMain/kotlin/org/maplibre/compose/location/LocationTrackingEffect.kt
Outdated
Show resolved
Hide resolved
| public fun rememberUserLocationState( | ||
| locationProvider: LocationProvider, | ||
| orientationProvider: OrientationProvider? = null, | ||
| samplePeriod: Duration? = 1.seconds, |
There was a problem hiding this comment.
I think a better solution would be to add a
val samplePeriod: Durationto the *Provider interfaces. The you can just use the max of those for the flow.sample call (or the code I suggest below).
But after thinking about it, I'm actually not sure what you're trying to avoid here. The trivial solution (a second collectAsStateWithLifecycle) shouldn't lead to unnecessary recompositions, assuming you want a recomposition every time either location or orientation changes. That recomposition should also just affect the composables that read location or orientation from the returned UserLocationState, i.e. normally just LocationPuck.
lib/maplibre-compose/src/commonMain/kotlin/org/maplibre/compose/location/UserLocationState.kt
Outdated
Show resolved
Hide resolved
…e/location/LocationPuck.kt Co-authored-by: Klemens Böswirth <23529132+kodebach@users.noreply.github.com>
Co-authored-by: Klemens Böswirth <23529132+kodebach@users.noreply.github.com>
lib/maplibre-compose/src/commonMain/kotlin/org/maplibre/compose/location/UserLocationState.kt
Outdated
Show resolved
Hide resolved
|
Actions is broken again. Dammit Github, get it together 🤦♂️ |
Continuation of #712
TODO:
GMS Demo crashes when you hit closeFusedOrientationProvider emits much more frequently than the providedMinUpdateInterval- should we.sample? I wonder ifFusedLocationProviderdoes this too