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

Feature request: Support for {lat, lon} objects in LngLat#convert #7090

Closed
bfrengley opened this issue Aug 7, 2018 · 6 comments
Closed

Feature request: Support for {lat, lon} objects in LngLat#convert #7090

bfrengley opened this issue Aug 7, 2018 · 6 comments

Comments

@bfrengley
Copy link
Contributor

Motivation

There are a number of JS libraries which deal with { lat: number; lon: number; } types rather than mapbox-gl-js' { lng: number; lat: number; }. Currently interoperating between these and mapbox-gl-js is slightly painful, with conversions required in both directions. Other mapping libraries such as Leaflet support converting from { lat: number; lon: number; } already.

Design

LngLat#convert already supports converting from { lng: number; lat: number } objects which are not instances of LngLat. If no lng is present in input, fall back to lon instead.

I am happy to make a pull request if this feature would be considered.

@jmlopez-rod
Copy link

Mapbox-gl-js: https://github.com/mapbox/mapbox-gl-js/blob/master/src/geo/lng_lat.js
Leaflet: https://github.com/Leaflet/Leaflet/blob/master/src/geo/LatLng.js

Would be nice to bring in the equals method and the utility method toLngLat.

No point in opening another issue. The same should be considered for LngLatBounds. Currently mapbox only supports extends. The contains method is a very nice utility as well as the quick toLngLatBounds.

Leaflet: https://github.com/Leaflet/Leaflet/blob/master/src/geo/LatLngBounds.js
Mapbox: https://github.com/mapbox/mapbox-gl-js/blob/master/src/geo/lng_lat_bounds.js

@bfrengley
Copy link
Contributor Author

@mourner @jfirebaugh before I make any changes, would a pull request adding support for this be considered?

@ryanhamley
Copy link
Contributor

I think this makes sense. It's a fairly small addition that improves compatibility between GL JS and other libraries.

As for the other methods mentioned: GL Native has a LngLatBounds#contains method so porting that to JS is probably a good idea. The LngLat and LngLatBounds convert methods (the JS one is here) seem like they're equivalent to toLngLat[Bounds]. An equals method is not present in Native or JS for LngLat or LngLatBounds. I'm agnostic on whether or not we should include it, but if anything I'd probably lean towards not since checking equality is easy enough to do without expanding the API.

TLDR on my two cents:
👍 to {lat, lon} support and LngLatBounds#contains if you want to include it
👎 to equals and toLngLat[Bounds]

@jmlopez-rod
Copy link

The only reason I mentioned equals is because the leaflet implementation allows us to provide a margin of error

equals: function (obj, maxMargin) {
		if (!obj) { return false; }

		obj = toLatLng(obj);

		var margin = Math.max(
		        Math.abs(this.lat - obj.lat),
		        Math.abs(this.lng - obj.lng));

		return margin <= (maxMargin === undefined ? 1.0E-9 : maxMargin);
	},

I'm guessing that was necessary since a lot of manipulations of LatLng objects leaves us with some rounding errors. Either way I'd be happy if I can remove my contains wrapper method and use the method from this library instead.

I did not realize that the toLngLat[Bounds] are already here in with the static methods convert.

@ansis
Copy link
Contributor

ansis commented Oct 8, 2018

@bfrengley I'm happy to accept a pull request adding support for lon to LngLat#convert

@ryanhamley
Copy link
Contributor

Fixed by #7507

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants