Skip to content
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 circle to GeolocateControl showing accuracy of position #9181

Closed
wants to merge 46 commits into from
Closed

add circle to GeolocateControl showing accuracy of position #9181

wants to merge 46 commits into from

Conversation

Meekohi
Copy link
Contributor

@Meekohi Meekohi commented Jan 14, 2020

Motivation

The default GeolocateControl shows the viewer's position as reported by navigator.geolocation.watchPosition immediately, but this often has a very high margin of error. The dot is often in the wrong location, which can be confusing or even dangerous depending on the application.

Implementation

Adds a transparent circle to GeolocateControl showing the accuracy of the position, as reported by navigator.geolocation.watchPosition.

Screen Shot 2020-01-14 at 10 30 00 AM

image

Launch Checklist

  • briefly describe the changes in this PR
  • include before/after visuals or gifs if this PR includes visual changes
  • write tests for all new functionality
  • document any changes to public APIs
  • manually test the debug page
  • tagged @mapbox/studio and/or @mapbox/map-design-team if this PR includes style spec or visual changes
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
  • Add a transparent circle to GeolocateControl that shows the reported location accuracy. Toggled by showAccuracy option which defaults to true.

@Meekohi
Copy link
Contributor Author

Meekohi commented Jan 14, 2020

Related to #9177

@ryanhamley @mourner @andrewharvey could you all give some feedback and direction for where to take this. Would especially like tips on what kind of tests would be appropriate and how to get started on those.

@Meekohi
Copy link
Contributor Author

Meekohi commented Jan 14, 2020

@mapbox/map-design-team this adds a transparent blue circle around the standard GeolocateControl marker. Currently using #1da1f233 (shown in png above).

Copy link
Collaborator

@andrewharvey andrewharvey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR @Meekohi it's looking very good. I've left a few comments. Not saying I'm right and you need to change it, just something to think about.

src/css/mapbox-gl.css Outdated Show resolved Hide resolved
@@ -78,6 +80,7 @@ let noTimeout = false;
* @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: 15}] A [`fitBounds`](#map#fitbounds) options object to use when the map is panned and zoomed to the user's location. The default is to use a `maxZoom` of 15 to limit how far the map will zoom in for very accurate locations.
* @param {Object} [options.trackUserLocation=false] If `true` the Geolocate Control becomes a toggle button and when active the map will receive updates to the user's location as it changes.
* @param {Object} [options.showAccuracyRadius=true] Show a transparent circle approximating the position accuracy as reported by `navigator.geolocation.watchPosition`. Set to `false` to disable.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about "By default a transparent circle will be drawn around the user location indicating the accuracy of the user's location"? What do you think?

I don't think we need to mention watchPosition,

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed that we don't need to explicitly mention watchPosition in this description. I think it might be helpful to note that if trackUserLocation=true, the accuracy will be refined over time.

The name showAccuracyRadius seems odd to me. It's not really a "radius", right? It's more like an area of uncertainty. Also, not sure how you feel @andrewharvey but I think this should probably be false by default.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this as just an extension of the showUserLocation option, should we support showUserLocation=false but still showing this accuracy circle? I'm not sure...

I'm on the fence with the default. After all we introduced showUserLocation=true as the default which changed behaviour when people upgraded. While I can understand we should try to avoid to unexpected changes in behaviour for users upgrading, this change is mostly cosmetic and if you're already showing the user location dot would you ever not want to show this accuracy circle?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think this is the core question -- are there use cases for disabling the circle entirely? I do think defaulting to it visible makes sense although it is a change. I added the flag anticipating someone might see the change and want to go back to the old behavior (especially if they have done something unusual like customized the dot icon or etc).

I do think there is some value in explaining "how is the size of this circle chosen" in the docs, but can leave it out if there's agreement it isn't needed.

We could name it something friendly: "circle-of-confusion" is common but a bit wordy... the radius shown specifically represents the 95% confidence level in the latitude/longitude.

Maybe in this context just showAccuracy is sufficient?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to

By default a transparent circle will be drawn around the user location indicating the accuracy (95% confidence level) of the user's location. Set to false to disable.

for now.

_geolocateButton: HTMLButtonElement;
_geolocationWatchID: number;
_timeoutId: ?TimeoutID;
_watchState: 'OFF' | 'ACTIVE_LOCK' | 'WAITING_ACTIVE' | 'ACTIVE_ERROR' | 'BACKGROUND' | 'BACKGROUND_ERROR';
_lastKnownPosition: any;
_userLocationDotMarker: Marker;
_accuracyCircleMarker: Marker;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given both markers will always have the same location, do you think it would be better to combine this with the _userLocationDotMarker to have a single Marker? Still having it optional and still using two separate classes for styling.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally I think the code will be more complicated trying to force both of these objects to use one Marker: that would introduce some container element that will need to be styled carefully to avoid breaking anything, some additional if/else around the flag, and I'd be worried that might introduce some other breaking change related to code I'm not familiar with, whereas I'm pretty confident the current design should be independent. (Obvious caveat I've only been looking at this codebase for 2 days so... let me know ;D)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably prefer a single Marker since their location is always going to be the same, but I can also see how two Markers is easier to implement and in some ways makes the code easier to manage (though single Marker also makes the Marker update code cleaner).

That said, I'm happy with either option.

src/ui/control/geolocate_control.js Outdated Show resolved Hide resolved
@@ -476,6 +498,28 @@ class GeolocateControl extends Evented {

export default GeolocateControl;

function getDistance(latlng1, latlng2) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this needs to be addressed within this PR, but eventually we should eliminate the duplication between this and

function getDistance(latlng1, latlng2) {
// Uses spherical law of cosines approximation.
const R = 6371000;
const rad = Math.PI / 180,
lat1 = latlng1.lat * rad,
lat2 = latlng2.lat * rad,
a = Math.sin(lat1) * Math.sin(lat2) +
Math.cos(lat1) * Math.cos(lat2) * Math.cos((latlng2.lng - latlng1.lng) * rad);
const maxMeters = R * Math.acos(Math.min(a, 1));
return maxMeters;
}
and in turn hopefully address #8777.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah agreed it's a copy/paste job -- good spot to refactor these into some helpers in the future.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll likely forget about it later so let's eliminate duplication now — I think the best place to put it is LngLat distanceTo(lngLat) method. This would follow the same convention as Leaflet.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Opened a separate PR for this: #9202

@Meekohi
Copy link
Contributor Author

Meekohi commented Jan 15, 2020

Been playing with this more on my iPhone and I think it really needs some CSS animation or something to smooth it out, it's a bit chaotic otherwise -- I'll look into adding this.

Nevermind, adding a css transition causes a lag when zoom changes -- not worth the complexity to split it out and manually animate I don't think.

@Meekohi
Copy link
Contributor Author

Meekohi commented Jan 15, 2020

  • Checklist asks that some tags be added to this PR, someone with permissions will have to handle that.

  • Also not quite sure how to handle the CHANGELOG -- should I add a changelog line to 1.6.1? Start a 1.6.2? Is that something someone else handles when/if the PR is merged? I can't tell in other PRs how this was handled, but see @ryanhamley in the history ;)

@Meekohi Meekohi changed the title [WIP] add circle to GeolocateControl showing accuracy of position add circle to GeolocateControl showing accuracy of position Jan 15, 2020
@Meekohi
Copy link
Contributor Author

Meekohi commented Jan 15, 2020

Trying to run the benchmark suite obliterated my entire computer and forced a restart 🤣. It doesn't look to me like any benchmarks actually exercise the GeolocateControl, and adding one DOM element shouldn't have any significant impact to performance, so I'm going to take it off the checklist.

@andrewharvey
Copy link
Collaborator

Also not quite sure how to handle the CHANGELOG -- should I add a changelog line to 1.6.1? Start a 1.6.2? Is that something someone else handles when/if the PR is merged? I can't tell in other PRs how this was handled, but see @ryanhamley in the history ;)

In that checklist you can just add an entry:

  • <changelog>Your changelog entry here</changelog>

This is the way to go over editing CHANGELOG directly because the merge conflicts would be a nightmare, so that get's updated when the release is done.

@andrewharvey
Copy link
Collaborator

I tested this out using Chrome DevTools Sensor option and cycling through different locations I notice that as the map smoothly zooms in the accuracy circle will smoothly grow in size as it should, but then near the end it starts to jump around. Is that something you noticed?

@andrewharvey
Copy link
Collaborator

Taking a step back I'm seeing the limitations of using a Marker element for this, even just manually zooming in and out results in jitter and panning the map around near the edge the circle it moves relative to the ground, which it shouldn't.

Normally for large circle the drawing out the circle as a polygon results in a much better experience, however the user location dot was intentionally done as a Marker to avoid mutating the users Style so to be honest I'm not sure the best way forward.

@Meekohi
Copy link
Contributor Author

Meekohi commented Jan 15, 2020

@andrewharvey huh interesting -- I'd been assuming any 'jumping' I saw was due to the actual reported accuracy changing (actually kind of amusing, moving the phone under my desk immediately decreases the accuracy by ~50% or so), but I can reproduce this with the sensors devtools (I didn't know about those!) and will investigate.

@pathmapper
Copy link
Contributor

pathmapper commented Jan 16, 2020

I'm seeing the limitations of using a Marker element for this, even just manually zooming in and out results in jitter

Depending on the browser/OS you're using, this could be related to #8614

* LOD support for tile coverage
@ryanhamley
Copy link
Contributor

showAccuracyArea? I'm ok with showAccuracyCircle as well.

I've been swayed by your arguments that default state should be on.

I'm agnostic on one or two markers. That's an implementation detail that could change in the future so I don't think it's a make or break decision.

@Meekohi
Copy link
Contributor Author

Meekohi commented Jan 31, 2020

Hey all, procedural question --
#9202 (refactoring distance calculations) is now merged into master. Typically, I would take this PR and git merge upstream/master... but I know you all prefer to rebase onto master and I'm worried of making a big mess. Should I rebase this work onto master now instead and clean up any merge difficulties? Is that going to make the diff totally unusable (usually my experience when rebasing)?

@ryanhamley
Copy link
Contributor

@Meekohi we do squash and merge when the PR is merged to master which automatically rebases if possible. It's probably ok to hold off merging/rebasing manually unless you need to. If you do, git rebase master is usually fine.

@Meekohi
Copy link
Contributor Author

Meekohi commented Jan 31, 2020

Okay rebased, merged, and updated to use the new distance refactor.

  1. Updated docs as suggested
  2. default = true -- it's a change I don't recommend lightly but it's very common in other mapping tools and a useful feature that I think most people will want on by default. As an aside, I've been playing with this more while I'm in Colombia this week (3G and not usually on wifi) and it's really nice to have the immediate feedback that you should wait a bit for the GPS signal to narrow down before you make a decision based on the map.
  3. Personally I feel best about 2 markers vs trying to merge it into one. I think there may be some unexpected CSS consequences of adding a container element that holds both.

@andrewharvey
Copy link
Collaborator

@Meekohi looks like something went wrong with the rebase, the diff now has a lot of other changes, which makes it hard to see what this PR is actually changing, could this be cleaned up? If you're not sure, maybe best to take a backup of your repo first just in case.

@Meekohi
Copy link
Contributor Author

Meekohi commented Feb 1, 2020

🙄This is why I thought it's very strange this repo recommends rebasing instead of merging and asked about it. Github shows "three dot diffs" not "two dot diffs" -- I'll just start a new PR.

@andrewharvey
Copy link
Collaborator

I'm interested if there's anything you think could be improved at https://github.com/mapbox/mapbox-gl-js/blob/master/CONTRIBUTING.md#version-control-conventions.

It should just be a matter of (on your-branch) git rebase master and then `git push --force origin your-branch.

@Meekohi
Copy link
Contributor Author

Meekohi commented Feb 1, 2020

Nah that is fine and makes sense, I wouldn't propose to decide for everyone what is best. My 2¢ personal opinion is that when using Github, merging is the best workflow since all the "compare" Github features are built with that assumption in place. Hacks like git push --force are ok for small one-off contributors, but a real mess for collaboration (e.g. two people working on a feature together). When you finally accept the commit you can still "Squash and Merge" which gets you a nice commit history.

@Meekohi Meekohi mentioned this pull request Feb 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.