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 LatLngBounds.contains() method #7512

Closed
ryanhamley opened this issue Oct 30, 2018 · 4 comments · Fixed by #8200
Closed

Add LatLngBounds.contains() method #7512

ryanhamley opened this issue Oct 30, 2018 · 4 comments · Fixed by #8200
Assignees
Labels
api 📝 GL native → GL JS For feature parity with Mapbox Maps SDK on a native platform good first issue

Comments

@ryanhamley
Copy link
Contributor

Motivation

I'm splitting this request out from #7090 in which several different LatLng and LatLngBounds methods were brought up. GL Native has a LatLngBounds.contains utility method which is missing in JS. Adding this method to JS would increase parity between GL implementations and provide a useful method to users.

Design

I can see two possibilities.

  1. A static method on LatLngBounds
LatLngBounds.contains(bounds, point)
  1. An instance method
const bounds = new LatLngBounds([sw, ne]);
bounds.contains(point);

I'd say the instance method is preferable since all other methods on LatLngBounds are also instance methods. The static method would mostly be useful as a pure utility method in which you may want to check arbitrary bounds for a point, but does that have many use cases? I'm unsure.

contains should be able to check that a bounding box contains a point or a CanonicalTileID, as the Native implementation does. Native also checks unwrapped bounds, but that concept isn't well defined in JS and I think we can side step that implementation for now.

Implementation

We should be able to just port the C++ implementation
https://github.com/mapbox/mapbox-gl-native/blob/master/src/mbgl/util/tile_range.hpp#L40

@ryanhamley ryanhamley added good first issue GL native → GL JS For feature parity with Mapbox Maps SDK on a native platform api 📝 labels Oct 30, 2018
@FernandoKGA
Copy link

This topic has already been opened as an issue here: #4278, but I don't think they got it correctly or it isn't really doable. I had came across the same issue, probably you want check if it contains the point, one of the members, @mollymerp suggested using Turf.js, #4278 (comment).

But in my case, I want to check if the bounds contain a marker with a popup in the same position given it's coordinates, in order to block an insertion, and this should be done with an layer I do think, however, I don't know how to do it yet.

@ryanhamley
Copy link
Contributor Author

Thanks for pointing that ticket out. The lack of methods like this was mostly due to a conceptual way of thinking about Mapbox GL JS as a rendering library, not a geospatial query library. But given that there's user demand, it's straightforward to implement and it brings JS into parity with Native, I think it's ok to move forward with this. We'd welcome a PR if you're interested in adding this @FernandoKGA

@FernandoKGA
Copy link

I would love to @ryanhamley , however, I don't have so much time to develop a topic like this now as I'm studying, in addition, I do not have enough experience or practice with JS, yet. I'm using the API to develop a web project as a final test on one of my subjects at university. Probably by the time I will have finished the project, someone would already done it. (I think...)

@ryanhamley
Copy link
Contributor Author

The GL JS implementation of this will need to implement all 3 definitions of contains in the GL Native repo as seen in https://github.com/mapbox/mapbox-gl-native/blob/master/src/mbgl/util/geo.cpp#L37-L65

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api 📝 GL native → GL JS For feature parity with Mapbox Maps SDK on a native platform good first issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants