-
Notifications
You must be signed in to change notification settings - Fork 2.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add positionMarker option to Geolocate Control and improve watchPosition with an active lock and background mode #3781
add positionMarker option to Geolocate Control and improve watchPosition with an active lock and background mode #3781
Conversation
@andrewharvey amazing design documentation!
An additional state to consider adding is an orientation lock. This is a feature I use often while navigating in maps.me to get my bearings right. |
Amazing @andrewharvey! Let me know if I can help. Due to an upstream dependency change you'll need to rebase on master to get unit tests passing again. |
d49e759
to
ee275fc
Compare
I'm thinking that could work something like this. You could unlock those states of the control with I see this as working in tandem with another option Although I'm tempted to leave these out of this PR to get it in a review/mergeable state before adding new features. |
Beautiful @andrewharvey, been waiting for this in GL JS for quite a while. |
4dc0fdb
to
9718cb7
Compare
I'm still working on improvements, cleaning up the code, more manual testing and more unit tests, but I have:
I've decided that any action by the user which changes the camera should change to background state AND any map api call which changes the camera should also change to background state. The reasoning is that if you change the camera using either method it's because you want that area to be shown and it seems counter that if the camera is immediately brought back to the device location when using watchPosition. I've used the eventData option to skip this for the internal jumpTo camera change.
|
Looks good! Thanks @andrewharvey. Let us know when you're done working on this PR and we'll get to work merging it 😄 |
with an active lock and background mode
…sn't current, add shadow to marker
…tate, except camera changes internal to the geolocate control
55931d6
to
d444afa
Compare
* @param {Object} [options.positionOptions={enableHighAccuracy: false, timeout: 6000}] A Geolocation API [PositionOptions](https://developer.mozilla.org/en-US/docs/Web/API/PositionOptions) object. | ||
* @param {Object} [options.fitBoundsOptions={maxZoom: 18}] A [`fitBounds`](#Map#fitBounds) options object to use when the map is panned and zoomed to the device location. The default is to use a `maxZoom` of 18 to limit how far the map will zoom in for very accurate locations. | ||
* @param {Object} [options.watchPosition=false] If `true` the Geolocate Control becomes a toggle button and when active the map will receive updates to the device location as it changes. | ||
* @param {Object} [options.showMarker=true] By default a marker will be added to the map with the device's location. Set to `false` to disable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The word "Marker" already has a different specific meaning within GL JS. For consistency with GL Native, how about calling this option showUserLocation
and the preceding option trackUserLocation
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
* @param {Object} [options.showMarker=true] By default a marker will be added to the map with the device's location. Set to `false` to disable. | ||
* @param {Object} [options.markerPaintProperties={'circle-radius': 10, 'circle-color': '#33b5e5', 'circle-stroke-color': '#fff', 'circle-stroke-width': 2}] A [Circle Layer Paint Properties](https://www.mapbox.com/mapbox-gl-style-spec/#paint_circle) object to customize the device location marker. The default is a blue dot with a white stroke. | ||
* @param {Object} [options.markerShadowPaintProperties={ 'circle-radius': 14, 'circle-color': '#000', 'circle-opacity': 0.5, 'circle-blur': 0.4, 'circle-translate': [2, 2], 'circle-translate-anchor': 'viewport' }] A [Circle Layer Paint Properties](https://www.mapbox.com/mapbox-gl-style-spec/#paint_circle) object to customize the device location marker, used as a "shadow" layer. The default is a blurred semi-transparent black shadow. | ||
* @param {Object} [options.markerStalePaintProperties={'circle-color': '#a6d5e5', 'circle-opacity': 0.5, 'circle-stroke-opacity': 0.8}] A [Circle Layer Paint Properties](https://www.mapbox.com/mapbox-gl-style-spec/#paint_circle) object applied to the base markerPaintProperties to customize the device location marker in a stale state. The marker is stale when there was a Geolocation error so the previous reported location is used, which may no longer be current. The default is a faded blue dot with a white stroke. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think there are scenarios where injecting a new source & layers into the map's style could have unintended side effects? Are there any ways we could mark the user's location without needing to mutate the style?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Certainly this isn't the cleanest approach.
-
Style/Layer IDs could conflict, I've tried to reduce the likelihood of prefixing with a _ and using longer more specific names. I believe gl-draw has the same issue.
-
The layers get inserted as top layers but if the user adds more layers to the map later, by default the user layers will go on top of the geolocation marker layer.
-
If the application does a setStyle and replaces the style it would mess this control up.
I think the solution to these issues is to use a private style separate to the public one such that name conflicts won't occur, it can be set to always go on top of the public style, and it would be unaffected by the public setStyle method. Unfortunately that's a deeper change and I would be looking to you for advice on what approach makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GL Native's implementation works well. In that implementation we draw the user location dot atop the rendered stylesheet.
In GL JS this might look like
- calls to the existing
drawCircle
module - calls to a new shader
- the use of an HTML overlay
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay.
Could you point me to the drawCircle module? I can't find it. found it as drawCircles
Obviously the current approach is the simplest to implement, but I understand the concerns. In light of this, I think the best alternative is for this control to create a new style layer "userLocation" to create something like
Hopefully then then it would have paint properties to customize the design. What do you think? If we went this way it would make sense to share that same new style layer with native.
The new style layer is also probably the hardest to implement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the usefulness of having this in GL JS, outweighs some of the immediate downsides to the current approach. mapbox-gl-draw suffers the same issues, it injects Layers and Sources into the style just like this PR.
I'm certainly leaning towards a new UserLocation Layer in the style spec as a feature enhancement, this lets the designer control the UserLocation dot together with the map style, shared across platforms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On iOS, the user location annotation view was originally implemented as a custom view atop the map view independent of anything else. The analogy in GL JS would’ve been a custom HTML element sitting atop the map.
At the time, we had the option of implementing it as a point annotation, which would take advantage of a gl-native feature that unconditionally injects a layer into every style on load. However, we opted for a custom view because it was much more straightforward (and more efficient) to implement the pulsing halo effect implicitly (declaratively) with Core Animation than to drive the animation with a timer.
When we finally implemented view-backed annotations (analogous to Marker
s in GL JS) as views tied to annotations with transparent spacer images, we migrated the user location annotation view to be an annotation view, taking advantage of both the platform infrastructure around animation and the mbgl infrastructure around annotations.
This document outlines the pros and cons of each of the three methods for displaying a point geometry on the map.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to prototype how this would look using an HTML Marker as the user location dot. However at the moment it suffers from the following due to #3770 🤔
-
Marker's only show up on world 0 which can lead to the userLocation dot not appearing when in another world. I believe By default, snap back to "w=0" world after panning operation (i.e. "worldCopyJump") #2071 would resolve this issue.
-
This also means on zoomed out maps you'd expect the userLocation dot to show up multiple times in each visible world but the way Marker's are implemented currently they can only appear in a single world.
Although by default the map will zoom into to show the userLocation, the userLocation dot still shows up when zooming out.
I'd still like to prototype another approach which keeps the user location dot as part of the WebGL canvas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I've got my prototype of the HTML Marker implementation at https://github.com/andrewharvey/mapbox-gl-js/tree/geolocate-track-show-user-location-html-marker
Haven't updated any tests though, would appreciate some feedback on if this approach is likely to be acceptable or not before I spend time on tests.
Thanks to @tristen for the userLocation Marker CSS from https://github.com/osmlab/census/blob/gh-pages/css/site.css
It breaks down when flyTo crosses the anti-meridian putting the view in world -1. I think the only solution is to implement #2071 (comment)
I've noticed the Marker wobbles a few pixels when zooming in.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Directions plugin also injects the style, which suffers the same issues. Would it be better if the Geolocate control was a plugin too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@andrewharvey Yes, I'd be much more comfortable with style injection if this were a plugin 😄
* @param {Object} [options.positionOptions={enableHighAccuracy: false, timeout: 6000}] A [PositionOptions](https://developer.mozilla.org/en-US/docs/Web/API/PositionOptions) object. | ||
* @param {Object} [options.watchPosition=false] If `true` the map will reposition each time the position of the device changes and the control becomes a toggle. | ||
* @param {Object} [options.positionOptions={enableHighAccuracy: false, timeout: 6000}] A Geolocation API [PositionOptions](https://developer.mozilla.org/en-US/docs/Web/API/PositionOptions) object. | ||
* @param {Object} [options.fitBoundsOptions={maxZoom: 18}] A [`fitBounds`](#Map#fitBounds) options object to use when the map is panned and zoomed to the device location. The default is to use a `maxZoom` of 18 to limit how far the map will zoom in for very accurate locations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The relationship between the GeolocationControl
and fitBounds
might not be intuitive to a non-expert user. Additionally, we may remove the fitBounds
method in the near future #2801. Would you be open to refactoring to expose seperate animationOptions
and maxZoom
properties?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fitBoundsOptions is optional so non-expert users can ignore it but it's inclusion lets users override how the camera moves to the users location which is desirable.
I'm excited by #2801, but I thought it was still a proposal and I don't want to unnecessarily make it a dependency before being able to merge this. My intention has always been to get this merged, then when #2801 happens I'll refactor. That's my preference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The concept of the AnimationOptions
interface exists outside of #2801. Using the name animationOptions
now creates a naming convention that focuses on behaviour rather than implementation and resilient to change when #2801 lands (in progress at #3583)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking through the new camera api (at least based on the current API docs in #3583) the new API call would be:
map.fitBounds(bounds, cameraOptions, animationOptions)
It would make sense to have the GeolocateControl have and pass through options.cameraOptions
and options.animationOptions
. (and have cameraOptions support a maxZoom)
However at the moment fitBounds
currently supports a few extra options which the AnimationOptions
interface doesn't. linear
, padding
, maxZoom
. So would you suggest I have an options.animationOptions
and options.linear
and options.padding
and options.maxZoom
? And then once the new camera api lands change the API to bring linear
into AnimationOptions as type
and padding
,maxZoom
into options.cameraOptions
?
Either way there will be a public API change, but the new camera API already does that, so I think it's simpler to leave as is at the moment and then refactor for the new camera api.
/** | ||
* Fired when the Geolocate Control changes to the active_lock state, which happens either when we first obtain a successful Geolocation API position for the device (a geolocate event will follow), or the user clicks the geolocate button when in the background state which uses the last known position to recenter the map and enter active_lock state (no geolocate event will follow unless the device's location changes). | ||
* | ||
* @event active_lock |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use camelCase
for all event names
"npm-run-all": "^3.0.0", | ||
"nyc": "^8.3.0", | ||
"object.map": "^0.2.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We're working really hard to pare down our external dependencies. We have an unmaintainable number of them, contributing to very slow installs, complexity, and occasional upstream bugs. Would you mind rewriting this functionality in yourself in our util
module or doing the object map using built-in primitives? I apologize -- I know this is a bummer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem, I've replaced it.
* use showUserLocation and trackUserLocation instead of showMarker and watchPosition * use camelCase for activeLock * use "user" location rather than "device"location * use dot to refer to the marker/symbol on the map representing the user's location
Cutting from a thread above at #3781 (comment) the HTML Marker approach breaks down when flyTo crosses the anti-meridian putting the view in world -1. I think the only solution is to implement #2071 (comment). @lucaswoj Would you ship an HTML Marker version of this PR even with that major bug? Or is this blocked by it? |
@andrewharvey This PR is blocked until we can figure out a way to draw the user location dot without any major caveats. It needs to work across wrapped worlds, persist across style changes, and not modify the user's style. I would be comfortable with any of these caveats in a plugin but core requires a higher bar. |
I'm closing this PR since, the branch based on the HTML Marker over at #4479 is going to be the best way to get this landed in core. |
Summary
As of GL JS 0.28.0, the Geolocate Control has worked as a button that when clicked the map camera will change to the device's location from using the HTML5 Geolocation API. There is no state, users press the button and the camera updates. This works well for desktop where the device is generally fixed in location.
In this PR I'm proposing an optional secondary mode which opens up more mobile focused use cases where the device's location can change. This is a continuation of #3739, which in hindsight I requested prematurely and should have waited until I had a more complete design which I'm putting forward here.
In this PR the Geolocate Control has the option
watchPosition
defaulting tofalse
(traditional desktop mode), which when set totrue
will allow the map camera to update when the device's location changes and optionally show a marker pin pointing the device's location. (#3739 suffers a usability issue where the user lacks freedom to pan the map as as soon as the device's location updates it overrides their pan operation.)Design considerations:
Control Icon Design
Color
grey
represents the control is dissabled and you can't interact with itblack
represents the control is off/inactive (not watching for location updates)blue
represents the control is on/active (is watching for location updates)red
represents the control is on/active, but there was an error getting the device's locationAnimation
spinning
represents the control is working, such as waiting for the Geolocation API to get a GPS signal or get a response from a location service.Icon
locked
(dot in centre) represents that the control is locked onto a location. If the devices location changes then the map's camera will be updated to follow. IfshowMarker
is enabled, then the location marker will be updated.not locked
(so dot in centre) the user has manually changed the map's camera so if the device's location changes then the map's camera will not be updated to follow. IfshowMarker
is enabled, then the location marker will be updated.State Diagram
Launch Checklist
geolocate
anderror
events?Future
circle-radius
is specified in pixels we need to update thecircle-radius
every time the position's latitude changes or the camera's zoom changes. An alternative is to vectorise a circle as a polygon. Ultimately it might be better to wait for Provide a way to specify function outputs in real-world units #2525 to specify thecircle-radius
inmeters
and use a common code path to update the size in pixels for the latitude and zoom.