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

Provide a camera system #2563

Merged
merged 13 commits into from Nov 23, 2018

Conversation

Projects
None yet
3 participants
@miracle2k
Copy link
Contributor

miracle2k commented Oct 28, 2018

Does any other open PR do the same thing?

No.

What issue is this PR fixing?

There is currently no way to control the camera via props.

How did you test this PR?

I tested the example that comes with the PR on Android and on iOS with MapKit + Google Maps. I further tested the examples that have been updated as part of the PR on those platforms.

Proposal

It is currently not possible to control things like the viewing angle, or the bearing, via props. In addition, the set of imperative methods to control the camera as they exist feel to me a bit as if they have grown over time, maybe just focussing on those parts the contributor needed to solve their problem at hand.

On the other hand, both the Apple Maps SDK as well as Google Maps support a flexible powerful camera system, which we can (and should) expose more directly.

One related problem is that the new animateToNavigation lacks the ability to set the zoom level (#2436). The challenge there is that iOS does not support a zoom level in the way Google Maps does - it uses a meter-based altitude measurement instead.

My suggestion, here (partially) implemented for Apple Maps, is as follows:

// The camera system is an alternative to the region system; it is based on  a center point that 
// we look at. 
type Camera = {
    center: {
       latitude: number,
       longitude: number,
   },
   pitch: number,
   bearing: number
   altitude: number.
   zoom: number
}
<MapView  initialCamera={} />
<MapView  camera={} />

// We can just set the bearing and the pitch, the others remain then at their current value.
mapView.setCamera({
    pitch: 10,
    bearing: 90
})

// Here we animate instead.
mapView.animateCamera({
    pitch: 10,
    bearing: 90
})

// Zoom (for Google) and altitude (for Apple) need to be set set separately.
mapView.animateCamera({
    zoom: 15
    altitude: 1000
})

// We can also allow the camera to be queried:
await mapView.getCamera();

This would also allow us to deprecate most of the existing animateTo* methods. Whatever you need to do, you can configure the camera to the desired combination of changes.

I would be willing to implement this for all platforms, but wanted to get some feedback first.

@techieyann @dorshay6 @rborn @christopherdro

Current state:

  • camera/initial Camera iOS
  • camera/initial Camera Android
  • camera/initial Camera GoogleMaps iOS
  • setCamera/animateCamera iOS
  • setCamera/animateCamera Android
  • setCamera/animateCamera GoogleMaps iOS
  • getCamera iOS
  • getCamera Android
  • getCamera GoogleMaps iOS
  • Example
  • Docs

When merged, I believe this would allow us to close #2436 and #2407.

@miracle2k miracle2k changed the title WIP: Add initialCamera and camera props. WIP: Provide a camera system (was: Add initialCamera and camera props) Oct 28, 2018

@rborn

This comment has been minimized.

Copy link
Collaborator

rborn commented Oct 29, 2018

@miracle2k ❤️

@alvelig this LGTM and the idea is good, maybe you could have a look and add feedback?

@miracle2k miracle2k force-pushed the miracle2k:camera branch 2 times, most recently from 7a3bb53 to 5b0f156 Nov 14, 2018

@miracle2k

This comment has been minimized.

Copy link
Contributor Author

miracle2k commented Nov 14, 2018

This is now fully implemented on all three platforms, in the form I suggested, along with an example screen.

I added a "deprecated" warning to all animateTo* methods which can be replaced by animateCamera().

What is missing is a documentation update, which I intend to add shortly.

If there are any other changes desired, please let me know.

@miracle2k miracle2k changed the title WIP: Provide a camera system (was: Add initialCamera and camera props) Provide a camera system Nov 15, 2018

@miracle2k

This comment has been minimized.

Copy link
Contributor Author

miracle2k commented Nov 15, 2018

I believe this is now ready to be merged. It includes updated documentation, updated examples (replace use of the deprecated methods, plus a dedicated example). Also, the typescript typings have been updated.

I tested all those changes on the Android and iOS emulators.

@alvelig hearted the original post, so I am hopeful he is in board as well, and I'd be thrilled for him or @rborn to have a look (or another maintainer). I'd also be curious about the opinion of @techieyann, whose PR this would replace.

@rborn

This comment has been minimized.

Copy link
Collaborator

rborn commented Nov 20, 2018

LGTM @alvelig 🐽

@miracle2k miracle2k referenced this pull request Nov 21, 2018

Closed

Set zoom level #920

@rborn

This comment has been minimized.

Copy link
Collaborator

rborn commented Nov 21, 2018

@miracle2k I tried to reply to your email but your spam filter keeps rejecting me 😕
Anyway, any help with the PRs or anything it's amazing, if you can review them an be sure they don't break the module me or @alvelig will merge them.

Related to this PR let's wait a little more for @alvelig and if he doesn't show not I'll merge 🤗
The fact is that he's more knowledgeable at the native side than me 😊

@rborn

This comment has been minimized.

Copy link
Collaborator

rborn commented Nov 22, 2018

@miracle2k could you sync please ?

@miracle2k miracle2k force-pushed the miracle2k:camera branch from 54b45e9 to 9dbee56 Nov 23, 2018

@miracle2k

This comment has been minimized.

Copy link
Contributor Author

miracle2k commented Nov 23, 2018

@rborn done.

@rborn rborn merged commit 2dc1953 into react-native-community:master Nov 23, 2018

1 check passed

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

This comment has been minimized.

Copy link
Contributor

techieyann commented Nov 27, 2018

This is great, thanks @miracle2k

It's working perfectly for me so far, except for when I update the heading while using mapPadding. The map rotates around the true center rather than the center I've defined with the padding. I vaguely recall this being an issue with Google Maps though, so I'll just keep using my < double height map (which makes the true center also near the bottom of the screen).

One other issue is that initialCamera requires both altitude and zoom, but it's such a trivial thing I'm not sure it's necessary to fix.

@miracle2k

This comment has been minimized.

Copy link
Contributor Author

miracle2k commented Nov 27, 2018

@techieyann Is there a way to make the padding issue work, natively speaking? Since both the padding and the camera are done inside Google Maps, is there something we can do to account for it?

Regarding the altitude + zoom thing: Are you referring to the fact that zoom is exclusive to Google, and altitude exclusive to MapKit, and you thus need to give both if, and only if, you want to support both platforms (which is by design), or do you have a different issue?

As for the "as designed" thing - I decided to go the easy route and just represent the two distinct systems as-is, but, I suppose we could implement a conversion: Somewhere I saw Google documenting the approximate altitude for each zoom level, so I think we could use that to do a conversion if one or the other is missing.

@techieyann

This comment has been minimized.

Copy link
Contributor

techieyann commented Nov 27, 2018

@miracle2k re: padding
The map rotation around true center has been a problem for at least 4 years. The only solution I've found is as I've described -- overflow the map offscreen as needed to move the true center.
This could be implemented in this package, replacing the Google map padding by adjusting absolute top/left positioning and height/width based on the given padding (and keeping the Google logo still on screen). But it inevitably loads more data than necessary with all the invisible tiles and is fundamentally Google's responsibility.

re: zoom/altitude
Conversion would be ideal, but it's a decent bit of maths when the either/or solution works just fine.
My issue was that when I only use Google maps, and the prop initialCamera still required an altitude var; I set it to 0 and moved on, but I figured it should only require altitude/zoom when the relevant map is being used. animateCamera() functions properly as it only updates the camera based on what property is being passed.

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
  ...
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.