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 geolocate bearing #10817

Merged
merged 21 commits into from
Jul 8, 2021
Merged

add geolocate bearing #10817

merged 21 commits into from
Jul 8, 2021

Conversation

tsuz
Copy link
Contributor

@tsuz tsuz commented Jul 2, 2021

Launch Checklist

Fixes #8329 #8034

  • briefly describe the changes in this PR

This PR adds a heading to the current location if the data is available via Geolocate.

trimmed.mov

include before/after visuals or gifs if this PR includes visual changes

Before After
Screen Shot 2021-07-03 at 1 28 56 Screen Shot 2021-07-03 at 1 29 21

Which browser is this compatible with?

This event listener is compatible with all modern browsers. I was only able to test in Chrome's simulated mode, so I cannot guarantee this would work in other browsers (TODO create a debug page that can simulate these events)

https://developer.mozilla.org/en-US/docs/Web/API/Window/deviceorientation_event

Does this support landscape?

This supports portrait mode only, and that is because there is no way to confidently know that the device is in landscape mode. If there are good solutions to this, I am open to including it in this or subsequent PRs.

What to include in the changelog

I was not sure where to add as the changelog seems to be generated at releases, but I'd like to add this to the changelog.

Add user location tracking capability to `GeolocateControl` #4479, 
  * New option `showUserHeading` to draw a "hat" around the `Marker` "dot" on the map at the user's location
  • write tests for all new functionality
  • document any changes to public APIs -> Should I update the changelog now?
  • post benchmark scores
  • manually test the debug page
  • tagged @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes
  • tagged @mapbox/gl-native if this PR includes shader changes or needs a native port
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
  • add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog></changelog>

cc: @mapbox/map-design-team @mapbox/static-apis

@tsuz tsuz marked this pull request as draft July 2, 2021 14:06
@tsuz tsuz changed the title Taku/geolocate bearing add geolocate bearing Jul 2, 2021
@tsuz tsuz marked this pull request as ready for review July 2, 2021 16:42
@tsuz tsuz requested review from ryanhamley and mourner July 2, 2021 16:44
Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

Generally this looks great! Added just a few nits.

src/css/mapbox-gl.css Show resolved Hide resolved
src/ui/control/geolocate_control.js Outdated Show resolved Hide resolved
this._userLocationDotMarker.setRotation(this._heading);
this._dotElement.classList.add('mapboxgl-user-location-show-heading');
} else {
this._dotElement.classList.remove('mapboxgl-user-location-show-heading');
Copy link
Member

Choose a reason for hiding this comment

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

Should we move manipulating classList in the update method from here to the trigger method which hosts all other lines like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mourner I thought about that. The trigger would add an event listener for devicrorientation but it should not show the heading until the information is available in _updateMarkerRotation.

], this);

this._updateMarkerRotationThrottled = throttle(this._updateMarkerRotation, 20);
Copy link
Member

Choose a reason for hiding this comment

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

Is the deviceorientation event called much more often than 20ms to necessitate throttling? Just curious.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mourner The motion sensor frequency can be uncapped due to this bug https://w3c.github.io/deviceorientation/spec-source-orientation.html#security-and-privacy

Some say it's suggested to 60 Hz (equalling once every 16.6ms) but I couldn't find the source of this suggestion.

@tsuz tsuz requested a review from mourner July 5, 2021 01:34
Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

👍

@tsuz
Copy link
Contributor Author

tsuz commented Jul 7, 2021

@mourner

By testing it on a real device, this is what I know:

Safari Chrome Firefox Edge
iOS 14.3
Android (ver 10) n/a

Just wanted to let you know there is a limitation with Firefox x Android combination.

Feel free to test this it here
https://bl.ocks.org/tsuz/raw/9279b49384a0e8b18e729863c3739f3f/?raw=true

@tsuz tsuz merged commit ced0fc8 into main Jul 8, 2021
@tsuz tsuz deleted the taku/geolocate-bearing branch July 8, 2021 14:22
@andrewharvey
Copy link
Collaborator

Thanks for finishing this off @tsuz !

deviceOrientationEvent.webkitCompassHeading || deviceOrientationEvent.alpha

I haven't looked into this in a while, but from memory the alpha property might not be an absolute bearing relative to north, it might just be a local measurement to detect device orientation.

Which I why I originally had the code

    if (!deviceOrientationEvent.absolute && !deviceOrientationEvent.webkitCompassHeading) {
       // an absolute orientation or a webkitCompassHeading is required
       return;

To ensure we didn't use a non-absolute or non-compass bearing.

Did you look into this further and find that we don't need to handle this case?

@tsuz
Copy link
Contributor Author

tsuz commented Jul 9, 2021

@andrewharvey

To ensure we didn't use a non-absolute or non-compass bearing.
Did you look into this further and find that we don't need to handle this case?

absolute would be required to get this right, from my understanding according to here. This is tested on iOS/Android devices using this demo; feel free to test in your environment also!

absolute parameter is implemented here https://github.com/mapbox/mapbox-gl-js/pull/10817/files#diff-84599242d1f597ef20c09a671c8ab00fcaa80d4679997ffb2ca018f6a2ee38d1R487-R496

@andrewharvey
Copy link
Collaborator

absolute parameter is implemented here https://github.com/mapbox/mapbox-gl-js/pull/10817/files#diff-84599242d1f597ef20c09a671c8ab00fcaa80d4679997ffb2ca018f6a2ee38d1R487-R496

Oh sorry I was just looking at your first commit, when you removed it, I should have checked the whole PR to see you added this back in again. All good.

SnailBones pushed a commit to SnailBones/mapbox-gl-js that referenced this pull request Jul 15, 2021
* cherry-picking showuserheading1

* cherry-picking showuserheading2

* make lint happy

* add device orientation event

* remove unintended merge

* add tests, fix src

* update tests

* fix tests

* fix lint errors

* fix rotation * -1

* remove unused line

* update css

* remove deviceorientationabsolute note

* fix example position of trigger

* add appropriate listener reference for window

* fix lint error

* refactoring css

* support iOS device

* fix lint errors

* fixing issue on ios device

* fix flow errors

Co-authored-by: Andrew Harvey <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add heading to GeolocateControl
3 participants