Device orientation support #712
Conversation
Adds a LocationProvider that enhances another LocationProvider with the bearing values of the device rotation sensor. It provides accurate bearing values when the default location provider is unable to do so (commonly due to lack of movement).
kodebach
left a comment
kodebach
left a comment
There was a problem hiding this comment.
The bearing from LocationManager and the heading/orientation from SensorManager are to different things. The bearing is based on direction of travel, while orientation is based on compass heading.
I chose to use bearing partly because it is available through LocationManager, but also because it seems more relevant to our use case. If we want to support using compass heading, the logic should probably be integrated into the normal LocationProvider with an option to switch between bearing and heading. On iOS both are provided by CLLocationManager and for the GMS module there is FusedOrientationProviderClient.
I'll leave the decision on how to handle this to @sargunv
...rc/androidMain/kotlin/org/maplibre/compose/location/AndroidSensorEnhancedLocationProvider.kt
Outdated
Show resolved
Hide resolved
...rc/androidMain/kotlin/org/maplibre/compose/location/AndroidSensorEnhancedLocationProvider.kt
Outdated
Show resolved
Hide resolved
Indeed, they are different things, but you don't get useful readings from location while standing still and you don't get useful readings from compass while moving. This combines them in a way that makes sense for most applications - but perhaps not all. We could add this a separate |
I haven't looked into the code yet, but I'd love to understand (1) what's different, and (2) in what use cases one might prefer one over the other. My assumptions, may be wrong: one is the direction I'm traveling, the other is the direction the phone is facing. If that's correct, I definitely see use cases for both.
Very curious how other maps combine these |
That is my understanding too. Navigation apps will generally assume that your phone is facing in the direction you're traveling, so they're merging these in similar ways (Ride with GPS and Cyclers are two examples that do this). But some apps may want to implement their own logic for handling them, so the current approach isn't flexible enough How about a |
|
As I explain in #714 (comment), I think we need to support both heading and bearing and make it configurable. There are definitely use cases for both. The native MapLibre Android SDK also allows switching the
Yes, I agree I would suggest: public interface LocationProvider {
public val headingEnabled: Boolean
public val location: StateFlow<Location?>
public fun enableHeading(enabled: Boolean)
}By default the heading is disabled. Calling On the public class UserLocationState internal constructor(
locationState: State<Location?>,
headingEnabledState: MutableState<Boolean>,
) {
public val location: Location? by locationState
public var headingEnabled: Boolean? by headingEnabledState
}
@Composable
public fun rememberUserLocationState(
locationProvider: LocationProvider,
headingEnabled: Boolean = false,
lifecycleOwner: LifecycleOwner = LocalLifecycleOwner.current,
minActiveState: Lifecycle.State = Lifecycle.State.STARTED,
coroutineContext: CoroutineContext = EmptyCoroutineContext,
): UserLocationState {
val headingEnabledState = mutableStateOf(headingEnabled)
val locationState =
locationProvider.location.collectAsStateWithLifecycle(
lifecycleOwner = lifecycleOwner,
minActiveState = minActiveState,
context = coroutineContext,
)
LaunchedEffect(headingEnabledState.value) {
locationProvider.enableHeading(headingEnabledState.value)
}
return remember(locationState) { UserLocationState(locationState, enableHeadingState) }
}(Note: It feels a little odd to have the The |
|
Yea I agree, a pluggable public fun enableHeading(enabled: Boolean)Is there a use case where you'd need to toggle this frequently? I would've just keyed this so compose makes a new provider on change |
|
As far as terminology, I'd avoid calling one "bearing" and the other "heading"; it's not clear at all what it means to "enable heading". A bearing is a direction on the surface of the world. That bearing could the the direction the device is moving, or it could be the direction the device is facing. I'd recommend designing the API around three discrete concepts:
This could translate to something like: // using types from spatial-k
class PositionMeasurement {
val position: Position
val inaccuracy: Length?
}
class BearingMeasurement {
val bearing: Bearing
val inaccuracy: Rotation?
}
class SpeedMeasurement {
val distancePerSecond: Length
val inaccuracy: Length?
}
class DeviceLocationState {
val position: PositionMeasurement?
val movementDirection: BearingMeasurement?
val movementSpeed: SpeedMeasurement?
val facingDirection: BearingMeasurement?
}And then the LocationPuck accepts any PositionMeasurement and BearingMeasurement: val l = rememberDeviceLocationState(
// ...
requestPosition = true,
requestMovementDirection = true,
requestMovementSpeed = true,
requestFacingDirection = true,
)
// ^ or perhaps even split those up into their own remember functions and state flows
// all consuming the same provider
LocationPuck(
position = l.position,
bearing = l.movementDirection // or l.facingDirection
)And then it's up to the user if/how they want to fuse facing and movement direction. It might be based on the mode in the app (navigating vs browsing), or relative accuracy, or any other app-specific needs fun minByInaccuracy(a: BearingMeasurement, b: BearingMeasurement) =
if (a.inaccuracy <= b.inaccuracy) a else b
LocationPuck(
position = l.position,
bearing = minByInaccuracy(l.movementDirection, l.facingDirection)
) |
|
That looks good, but I think we can shorten the property names a bit. Movement direction on iOS is aptly named class DeviceLocationState {
val position: PositionMeasurement?
val course: BearingMeasurement?
val speed: SpeedMeasurement?
val facing: BearingMeasurement?
}Also, is |
IIUC
val location by rememberUserLocationState(...)or use it like this val locationState = rememberUserLocationState(...)
SomeComposable(location = locationState.value)with the val locationState = rememberUserLocationState(...)
SomeComposable(location = locationState.location)and crucially we abstract away the implementation detail of The only thing I don't like about the current |
|
Ah yeah, my mistake with the state naming I'd call the data class something like LocationUpdate perhaps? It may also make sense to separate location providers (position, course, speed) from orientation providers, as the underlying platform API is totally separate on all the platforms I checked (iOS/macOS, Android, Web, Windows). Though maybe it's better for us to combine them into one abstraction? Not sure here |
|
Did a first draft of this, will be away for the weekend so I'll finish it next week.
|
But sure, we could separate them if we wanted to, I don't really have an opinion on that tbh 😄 |
jgillich
changed the title
I think that's an unnecessary complication. Apart from Android (and maybe desktop if we ever support it) all platforms will probably only ever have a single implementation for location and orientation providers. Even on Android with the GMS special case, I'd don't think there would be big use case in combining platform location with GMS orientation or vice versa. If someone really needs it, they can always implement their own I think separating them just introduces unnecessary combining logic, unless we completely separate the two and only combine them at the public class UserLocationState internal constructor(
locationState: State<Location?>,
orientationState: State<Orientation?>,
) {
public val location: Location? by locationState
public var orientation: Orientation? by orientationState
}
@Composable
public fun rememberUserLocationState(
locationProvider: LocationProvider,
orientationProvider: OrientationProvider? = null,
headingEnabled: Boolean = false,
lifecycleOwner: LifecycleOwner = LocalLifecycleOwner.current,
minActiveState: Lifecycle.State = Lifecycle.State.STARTED,
coroutineContext: CoroutineContext = EmptyCoroutineContext,
): UserLocationState {
val orientationState =
orientationProvider?.orientation?.collectAsStateWithLifecycle(
lifecycleOwner = lifecycleOwner,
minActiveState = minActiveState,
context = coroutineContext,
)
val locationState =
locationProvider.location.collectAsStateWithLifecycle(
lifecycleOwner = lifecycleOwner,
minActiveState = minActiveState,
context = coroutineContext,
)
return remember(locationState, orientationState) {
UserLocationState(locationState, orientationState ?: mutableStateOf(null))
}
}That has pros and cons. On the pro side, it makes it clear that these are separate things that might update at different rates. On the con side, it also means that |
kodebach
left a comment
kodebach
left a comment
There was a problem hiding this comment.
I don't like the nullable position in Location. A location without a position doesn't make sense IMO. Same goes for toggling speed and course separately from position in LocationRequest. AFAIK these come from the same API on all platforms, i.e. we get all of them for the same cost. No reason to control them separately.
The API otherwise LGTM except for a few minor details. What I forgot to mention before is, that UserLocationState also exists to ensure we can pass around a remembered value that doesn't change with ever location/orientation update, which let's us avoid recomposition.
| * instead of e.g. [kotlin.time.Instant], to allow calculating how old a location is, even if the | ||
| * system clock changes. | ||
| */ | ||
| @Serializable |
There was a problem hiding this comment.
I guess making it @Serializable is neat, but is the serialization plugin even applied? AFAIK if it's not applied to the module containing the class @Serializable is useless
There was a problem hiding this comment.
Should be, it's in library-conventions
| @Serializable public data class BearingMeasurement(val bearing: Bearing, val inaccuracy: Rotation?) | ||
|
|
||
| @Serializable | ||
| public data class SpeedMeasurement(val distancePerSecond: Length, val inaccuracy: Length?) |
There was a problem hiding this comment.
Does spatial-k not have a speed unit? That should probably be added upstream.
Then we could also use a single generic Measurement class
public data class Measurement<T, A>(val value: T, val inaccuracy: A?)which would turn the slightly silly locationState.location.position.position and locationState.location.speed.distancePerSecond into simpler locationState.location.position.value and locationState.location.speed.value.
IMO this generic Measurement could also be a case for upstreaming, because it could be extended with functionality for doing calculations with measurements while correctly propagating the inaccuracies.
| id = "$idPrefix-bearing", | ||
| source = locationSource, | ||
| visible = showBearing && locationState.location?.bearing != null, | ||
| visible = bearing != null, |
There was a problem hiding this comment.
Might make sense to put the whole SymbolLayer behind an if. There is no reason to add an always invisible layer, if bearing is disabled.
| @Composable | ||
| public fun LocationTrackingEffect( | ||
| locationState: UserLocationState, | ||
| location: Location?, |
There was a problem hiding this comment.
This doesn't work for two reasons:
- I'm pretty sure it would mean the
LocationTrackingEffectrecomposes every time the location changes - The
snapshotFlowwould never emit a new value, even if it wasn't constantly recomposed, because its lambda doesn't contain any state reads.
There was a problem hiding this comment.
Yea I was going to revisit that file later, just had Claude do a quick fix to make it build
| public fun LocationPuck( | ||
| idPrefix: String, | ||
| locationState: UserLocationState, | ||
| location: Location?, |
There was a problem hiding this comment.
| location: Location?, | |
| position: PositionMeasurement?, |
Using Location might be slightly nicer, but (1) it hides the decision between course and orientation and (2) means LocationPuck will recompose if only the location.speed changed, even though we're not using it. (With the state class this wasn't the case because UserLocationState is always remembered)
| /** update camera orientation based on location course (direction of movement) */ | ||
| TRACK_COURSE, | ||
|
|
||
| /** update camera orientation based on device orientation (heading) */ | ||
| TRACK_ORIENTATION, | ||
|
|
||
| /** update camera orientation based on course if available, otherwise orientation */ | ||
| TRACK_LOCATION, |
There was a problem hiding this comment.
| /** update camera orientation based on location course (direction of movement) */ | |
| TRACK_COURSE, | |
| /** update camera orientation based on device orientation (heading) */ | |
| TRACK_ORIENTATION, | |
| /** update camera orientation based on course if available, otherwise orientation */ | |
| TRACK_LOCATION, | |
| /** update camera rotation based on location course (direction of movement) */ | |
| TRACK_COURSE, | |
| /** update camera rotation based on device orientation (heading) */ | |
| TRACK_ORIENTATION, | |
| /** update camera rotation based on course if available, otherwise orientation */ | |
| TRACK_AUTOMATIC, |
| if (!request.orientation) { | ||
| return@let flow | ||
| } | ||
| flow.zip(orientation) { location, (orientation, orientationAccuracy) -> |
There was a problem hiding this comment.
zip is not the right operator here. If we never get a orientation, we should still emit a location without it, so we should use combine(orientation.sample(1.seconds)) to slow down orientation to the level of the location flow, but still emit if orientation never emits or is slower.
There was a problem hiding this comment.
combine also doesn't emit until all flows have emitted at least once. Is it even possible to not get any orientation values? Other than faulty hardware?
Guess it doesn't matter if we split orientation into its own provider
There was a problem hiding this comment.
True, but you can force the orientation flow to emit at least once, by calling send(null) similar to how the location flow calls send(lastLocation). But yeah, doesn't matter, if we split it
| if (!request.orientation) { | ||
| return@let flow | ||
| } | ||
| flow.zip(orientation) { location, facing -> |
Devices without GPS receivers may want to use some network geolocation provider, and this would have no relevance to orientation Desktop will probably have three platform providers (macOS Core Location, Windows.Devices.Geolocation, Geoclue), maybe encapsulated into one universal "platform" provider for convenience.
This would be the strongest justification to separate them I think. I don't have a solid opinion on this yet, will have to play with it a bit. I'll also be out this weekend, so can take a look next week |
Is it ever possible to receive speed information (say, from an accelerometer) without a position? If location isn't available (missing hardware, missing permission, no gps signal), what should we return? If we combine the two providers, then it should be possible to request orientation without location/movement.
Agreed, but in the naming sense, which is why I'd name it LocationUpdate (or even DeviceProprioceptionUpdate if we really want to be precise and combine them) |
After looking at the draft and considering the cases above, I'm convinced a separate It solves quite a few edge cases. There's got to be a reason after all, why GMS also provides separate location and orientation. With
Technically I guess you could try to estimate speed from an accelerometer, but it would get quite inaccurate quickly. On Android Auto (compose isn't really supported their, but still) and possibly some frankenstein JVM thing you could get technically also get speed data from an actual speedometer. That said I don't think for the purposes of this library that's relevant. In the end the goal is to support displaying of user locations on a map. That use case doesn't make sense without a position. |
That's what we're doing with the LocationPuck, but I'd expect the location update provider to be more agnostic to exactly how the data is used. Maybe an app is showing some speed overlay next to the map. And even if there's nothing that can be done without a position, it's possible the position isn't available due to permissions, hardware, or the environment. The app needs to be able to handle these cases, and making the property nullable is a way to communicate that. |
I get what you want to achieve here, but I'm not sure it's the right approach. Again location (even "location update") without position makes no sense. If you really see a use case for a provider that provides speed and/or travel direction without position data, then I would suggest: public data class Location(val position: Position, val inaccuracy: Length?)
public data class Speed(val distancePerSecond: Length, val direction: Rotation?, val directionInaccuracy: Rotation?)
public data class Orientation(val heading: Bearing, val inaccuracy: Rotation?)
public interface LocationProvider {
public val location: StateFlow<Location?>
}
public interface SpeedProvider {
public val speed: StateFlow<Speed?>
}
public interface OrientationProvider {
public val orientation: StateFlow<Orientation?>
}
public class UserLocationState(
locationState: State<Location?>,
speedState: State<Speed?>,
orientationState: State<Orientation?>,
) {
public val location: Location? by locationState
public val speed: Speed? by speedState
public val orientation: Orientation? by orientationState
}It then becomes possible to implement a pure On Android: public class AndroidLocationProvider : LocationProvider, SpeedProvider
public class AndroidOrientationProvider : OrientationProvider
public class FusedLocationProvider : LocationProvider, SpeedProvider
public class FusedOrientationProvider : OrientationProviderOn iOS: public class IosLocationProvider : LocationProvider, SpeedProvider, OrientationProvider |
|
I'm not talking about a provider that never provides position. I wouldn't design a model around that. The question was about nullable position, and so I'm talking about a provider that sometimes can't provide a position (like if permission isn't available, or hardware, or gps signal) |
IMO in the end we should keep in mind where With that in mind, I think we can agree that a The question then becomes: What happens, if the Your solution with In essence, my main argument boils down to the usability of the API we provide. Making the position nullable, means apps need to constantly deal with a technically possible null position everywhere and for no reason at all, since the |
|
What non-nullable value do you suggest we provide if we no longer have a GPS signal, or if we no longer have location permissions, or if the location services on the device were disabled, or if we no longer have the hardware to sense location? The point is, we can't guarantee a position result, the same as an HTTP client can't guarantee a 2xx response. Any app requesting a position needs to be handling the case where a position isn't available, in a manner suitable to that app. Maybe that's ceasing to display a position, and notifying the user position is not available. Maybe that's falling back to a different provider. Or maybe that's interpolating with last known position and course. It depends on the use case of the app. |
|
Okay, maybe we misunderstood each other. I'm not suggesting that a Let's clarify some assumptions, to make sure we're thinking about the same issue:
(*) platform APIs includes GMS, Geoclue and other third party libraries that we use as the basis of our Under my own assumptions, I must conclude that it is not worth it to significantly worsen the experience of using
|
|
Ah sorry, you're correct. For clarity, the states:
And separately, orientation data:
|
3 participants
Description
Adds a LocationProvider that enhances another LocationProvider with the bearing values of the device rotation sensor. It provides accurate bearing values when the fused location provider is unable to do so (commonly due to lack of movement).
iOS is still TODO
cc @kodebach in case you have some thoughts as the author of the location package
Test plan
Provider is enabled in demo app for testing
Have you tested the changes? On which platforms?