Fix GeolocateControl accuracy circle using turf.circle and GeoJSON #5440
Conversation
this allows to handle much better all edge cases and eliminates calls to _updateCircleRadius at each user interaction with the map
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5440 +/- ##
==========================================
- Coverage 91.94% 91.94% -0.01%
==========================================
Files 282 282
Lines 39050 39064 +14
Branches 6863 6860 -3
==========================================
+ Hits 35905 35916 +11
- Misses 3017 3020 +3
Partials 128 128 ☔ View full report in Codecov by Sentry. |
|
Can you please close the other PR then? |
| @@ -494,6 +494,7 @@ describe('GeolocateControl with no options', () => { | |||
| expect(geolocate._watchState).toBe('BACKGROUND'); | |||
| }); | |||
|
|
|||
| /* | |||
There was a problem hiding this comment.
Please don't keep commented out tests. Either bring them back or completely remove them.
| @@ -10,6 +10,9 @@ import type {FitBoundsOptions} from '../camera'; | |||
| import type {IControl} from './control'; | |||
| import {LngLatBounds} from '../../geo/lng_lat_bounds'; | |||
|
|
|||
| import * as turf from '@turf/circle'; | |||
There was a problem hiding this comment.
Can we use a "specific import" instead of "*"?
| this._accuracyCirclePolygon = turf.circle([position.coords.longitude, position.coords.latitude], position.coords.accuracy, {steps: 64, | ||
| units: 'meters'}); | ||
|
|
||
| if (this._map.getSource('accuracy-circle')) { |
There was a problem hiding this comment.
What will happen if a style has a source with this name that is not relevant to this circle?
| _onZoom = () => { | ||
| if (this.options.showUserLocation && this.options.showAccuracyCircle) { | ||
| this._updateCircleRadius(); | ||
| if (this._map.loaded() && this._map.isStyleLoaded()) { |
There was a problem hiding this comment.
If the style is still being loaded this might "leak"...
|
While I agree this approach is the correct way to handle this (I'm doing the same thing in my app), I think it might be problematic for some cases when switching styles and it might collide with sources or layers defined by users. |
This PR replaces PR #5437 and improves/fixes #5432 in many more edge cases.
The original problem, still present in #5437 was the use of a _circleElement. When there is significant map tilting, or when using the globe and approaching the edge of the map, there is no reasonable way to properly scale the element and make it work. The pixel scale calculated only at the center of the marker is not sufficient, and the element itself is too rigid to follow the curvature of the globe. The three videos below are mainline, PR #5437, and the current PR.
Screen.Recording.2025-02-01.at.23.03.17.mov
Screen.Recording.2025-02-01.at.23.09.02.mov
Screen.Recording.2025-02-01.at.23.14.23.mov
With this PR, the _circleElement is replaced with a layer with dynamic GeoJSONSource, with data generated using a single turf/circle call at each position update. This removes all calls to _updateCircleRadius() at each user interaction, which may improve performance. The resulting accuracy circle seems to behave exactly as it should.
I had to comment out the tests on _circleElement because it's gone. Perhaps the tests can be recovered/updated somehow.
The code to add/remove the GeoJSONSource and the layer probably needs more love, it works as is but if one clicks too quickly on the GeolocateControl button, with trackUserLocation=true, the appearance/disappearance of the marker and circle is sometimes broken.
Launch Checklist
CHANGELOG.mdunder the## mainsection.