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

Get visible map bounding box #2571

Merged
merged 11 commits into from Nov 22, 2018

Conversation

Projects
None yet
5 participants
@radeno
Copy link
Contributor

radeno commented Nov 3, 2018

It is still in progress. Do not merge yet. I can't build code on Android platform.

Does any other open PR do the same thing?

No

What issue is this PR fixing?

#356

How did you test this PR?

Examples:

iOS: Apple Maps iOS: Google Maps Android: Google Maps
simulator screen shot - iphone 6 - 2018-11-03 at 18 30 13 simulator screen shot - iphone 6 - 2018-11-03 at 18 30 49 screenshot_1541585289

@radeno radeno changed the title [WIP] Add getting visible map bounding box [WIP] Get visible map bounding box Nov 3, 2018

@radeno radeno changed the title [WIP] Get visible map bounding box WIP: Get visible map bounding box Nov 3, 2018

@radeno

This comment has been minimized.

Copy link
Contributor Author

radeno commented Nov 5, 2018

Guys, if you someone can guide me how to make Android version functional i will be helpful.
There is problem that all commands are called through wrapper function. But that function is defined as void function. So there should not be any return value.
Any idea how to make it work without high code pollution?

@rborn

This comment has been minimized.

Copy link
Collaborator

rborn commented Nov 5, 2018

@alvelig any idea? 🤗

@radeno

This comment has been minimized.

Copy link
Contributor Author

radeno commented Nov 7, 2018

@rborn @alvelig got it. It works as you can see in first comment.
Would be nice to get some code review response.

@Peretz30

This comment has been minimized.

Copy link

Peretz30 commented Nov 14, 2018

@radeno man, awesome job! I had tested your fork, works brilliant!

@rborn

This comment has been minimized.

Copy link
Collaborator

rborn commented Nov 14, 2018

@Peretz30 tested both ios and android? ❤️

@radeno radeno changed the title WIP: Get visible map bounding box Get visible map bounding box Nov 14, 2018

@Peretz30

This comment has been minimized.

Copy link

Peretz30 commented Nov 14, 2018

@rborn tested on Android device. iOS not tested.

@radeno

This comment has been minimized.

Copy link
Contributor Author

radeno commented Nov 20, 2018

Merged with upstream. Hope will be merged. Want to add getZoom (need to investigate how to do that properly) or something like that. Than it will be easier to get visible all markers and so on.

@rborn

This comment has been minimized.

Copy link
Collaborator

rborn commented Nov 20, 2018

@alvelig 🐽

@rborn
Copy link
Collaborator

rborn left a comment

LGTM , one question though: why does it have to be a promise? (I really don't know so I want to understand 😊)

@radeno

This comment has been minimized.

Copy link
Contributor Author

radeno commented Nov 20, 2018

@rborn if i understand it correct, then there are two solutions how to return a value:

  1. callbacks https://facebook.github.io/react-native/docs/native-modules-ios#callbacks or
  2. promises https://facebook.github.io/react-native/docs/native-modules-ios#promises
    Edit: for Android: https://facebook.github.io/react-native/docs/native-modules-android#promises and https://facebook.github.io/react-native/docs/native-modules-android#promises

Callbacks are "hell", anyways is tagged as experimental.

So returned promise is awaited in javascript async function. It is transparent on JS side.

@miracle2k

This comment has been minimized.

Copy link
Contributor

miracle2k commented Nov 21, 2018

On Google Maps, more values are available: https://developers.google.com/android/reference/com/google/android/gms/maps/model/VisibleRegion - the farLeft, farRight etc. properties. Are these different values than are contained in the latLngBoundsobject? I can't quite tell if there is different information in those, or not.

If so, is it worth it to return them as well?

I am a little bit obsessive about exposing as much of the native map helpers as possible, and wouldn't like to miss out on available data which we later cannot include for backwards-compatibility reasons, thus requiring us to add yet another helper.

@radeno

This comment has been minimized.

Copy link
Contributor Author

radeno commented Nov 21, 2018

@miracle2k if have MapKit same functionality then no problem to return it. But if it's only for Google Maps then it is not multiplatform.

@miracle2k

This comment has been minimized.

Copy link
Contributor

miracle2k commented Nov 21, 2018

@radeno MapKit does not expose those fields directly, but it might be possible to calculate them ourselves. It might also be ok to only return those fields on Android.

Still, I am not even sure if those fields contain any new information; the whole thing might be nothing. Can anyone currently running this PR check?

@radeno

This comment has been minimized.

Copy link
Contributor Author

radeno commented Nov 21, 2018

@miracle2k ok, let me check what google maps returns. But as naming says i except it should be for camera view, when map is skewed (not 90° angle).

@Peretz30

This comment has been minimized.

Copy link

Peretz30 commented Nov 22, 2018

@radeno can you say, please, what is the difference between your solution and this?

const getBoundingBox = (region) => ([
  region.longitude - region.longitudeDelta/2, // westLng - min lng
  region.latitude - region.latitudeDelta/2, // southLat - min lat
  region.longitude + region.longitudeDelta/2, // eastLng - max lng
  region.latitude + region.latitudeDelta/2 // northLat - max lat
])
@radeno

This comment has been minimized.

Copy link
Contributor Author

radeno commented Nov 22, 2018

@Peretz30
looks like not much different
Top is native calculated, bottom by region. So depends on Zoom, and just latitude coordinate is different. Third decimal place is 110m and Fifth just 1.1m. I don't make any deep investigation how internal SDK calculates it neither performance difference.

1 2
simulator screen shot - iphone 6 - 2018-11-22 at 09 42 29 simulator screen shot - iphone 6 - 2018-11-22 at 09 44 40
@radeno

This comment has been minimized.

Copy link
Contributor Author

radeno commented Nov 22, 2018

@Peretz30 @rborn
It's look like computing bounding box from region is much cheaper and performant than native. Because everytime is regionChanged https://github.com/react-community/react-native-maps/blob/c7a55f5e2b3b7854e248d35baf8dbd716a95350c/lib/ios/AirMaps/AIRMapManager.m#L837 then callback Region contains precalculated delta and then sum-up 3 numbers on JS side with simple operators is ultra-cheap. Almost 0 miliseconds.

On the other hand native calculations through my PR should takes in average 250ms on MapKit and average 150ms on Google Maps. But should be more precise.

I think we should also implement helper method which takes region as argument and returns bounding box. Something like coordinateForPoint method. But without promise. Because coordinateForPoint calls native method.
Eg:

  /**
   * Get bounding box from region
   *
   * @param region Region
   *
   * @return Object Object bounding box ({ northEast: <LatLng>, southWest: <LatLng> })
   */
  mapBoundariesForRegion(region) {
    return {
      northEast: {
        latitude: region.latitude + region.latitudeDelta / 2,
        longitude: region.longitude + region.longitudeDelta / 2,
      },
      southWest: {
        latitude: region.latitude - region.latitudeDelta / 2,
        longitude: region.longitude - region.longitudeDelta / 2,
      },
    };
  }

What do you think about it?

@Peretz30

This comment has been minimized.

Copy link

Peretz30 commented Nov 22, 2018

@radeno I agree, but I think name of the function should be getMapBoundaries, because MapView has setMapBoundaries

@miracle2k

This comment has been minimized.

Copy link
Contributor

miracle2k commented Nov 22, 2018

Note there is currently no way to get the region from the map, except via the event. I do think an imperative way to access the region, getRegion() or getMapBoundaries() as in this PR is useful.

@rborn

This comment has been minimized.

Copy link
Collaborator

rborn commented Nov 22, 2018

@miracle2k you DO think or DON'T ?

@miracle2k

This comment has been minimized.

Copy link
Contributor

miracle2k commented Nov 22, 2018

Right now, if I ever want to use the region in calculations, I have to setup a handler for onRegionChangeComplete or onRegionChange, then copy the region to local state (component state, or an instance member).

I think in many cases, where I only need to know the region in rare cases, for example when a user presses a "find button", I would think I would opt for calling this.refs.map.getMapBoundaries() at the time when I need that data, instead.

@rborn

This comment has been minimized.

Copy link
Collaborator

rborn commented Nov 22, 2018

@miracle2k ok
@radeno I think @Peretz30 's suggestion for the name is valid, maybe you agree?

@radeno

This comment has been minimized.

Copy link
Contributor Author

radeno commented Nov 22, 2018

@miracle2k you are right about getting bounding box without region change.
@rborn @Peretz30 hard to say what name should be better. Based on documentation and implementation https://github.com/react-community/react-native-maps/blob/e0c61eddebb2bfde5058338ce15e04148d812809/lib/components/MapView.js#L603 setMapBoudaries accepts two arguments and passing to native side https://github.com/react-community/react-native-maps/blob/1aee143302129f3811f1753155600bd041b5214d/lib/ios/AirGoogleMaps/AIRGoogleMapManager.m#L394
On the other hand you want to pass region to method getMapBoundaries, which i think is very confusion. Why region? Or why pass any argument? Region is something "low level" how native maps works. As @miracle2k says, we don't have region in every event. So when we are forcing to pass region everytime, then it will have very limited usage.

I think adding helper method like mapBoundariesForRegion is very self-descriptive. When i pass region i get map boundaries. Same idea as pointForCoordinate or coordinateForPoint But without promise. No need to be async function.

@Peretz30

This comment has been minimized.

Copy link

Peretz30 commented Nov 22, 2018

@radeno , thanks for explanation. Now I see, your logic seems to be right and your function name suits better.

@rborn rborn merged commit df9f735 into react-native-community:master Nov 22, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@radeno radeno deleted the radeno:master branch Dec 6, 2018

ryanbourneuk pushed a commit to 3sidedcube/react-native-maps that referenced this pull request Dec 10, 2018

Ryan Bourne
Merge remote-tracking branch 'upstream/master'
* upstream/master: (28 commits)
  Calculate bounding box from region (react-native-community#2615)
  [iOS GoogleMap] Fix animateCamera (react-native-community#2608)
  Fix type definition error (react-native-community#2607)
  [Android] Fix app crash in Android if building found but cannot getActiveLevelIndex (react-native-community#2598)
  Provide a camera system (react-native-community#2563)
  Get visible map bounding box (react-native-community#2571)
  [0.22.1] Release (react-native-community#2574)
  Move dev only deps to devDependencies. (react-native-community#2548)
  Specify how to use Google Maps (react-native-community#2550)
  r2507: remove marker: Attempt to invoke virtual method 'void com.google.android.gms.maps.model.setIcon(com.google.android.gms.maps.model.BitmapDescription)' on a null object reference #: remove marker: Attempt to invoke virtual method 'void com.google.android.gms.maps.model.setIcon(com.google.android.gms.maps.model.BitmapDescription)' on a null object reference (react-native-community#2555)
  update to clarify cacheEnabled is apple maps only
  [0.22.0] Release (react-native-community#2535)
  Fix for “The specified child already has a parent”
  Improve installation docs (react-native-community#2541)
  fix fitToSuppliedMarkers function (react-native-community#2524)
  Performance improvements for tracksViewChanges (react-native-community#2487)
  fix spelling mistakes
  Added flag to make sure that there has an Observer of view.
  hotfix PR react-native-community#2478
  Fix a peer dependencies warning
  ...
@sebackend

This comment has been minimized.

Copy link

sebackend commented Jan 22, 2019

Hi, thanks for the solution...Everything was working fine (i put some markers to test both coordinates, northEast & southWest), but when i rotated the map, the markers are showed in another position...How can this be fixed? there is no support for rotation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.